Re: bk commit into 5.1 tree (istruewing:1.2576) BUG#35807

[Sven Sandberg]

Hi Ingo,

Thanks for your fix! The source patch is good, but I have some remarks 
on the test case.

/Sven

Ingo Struewing wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of istruewing.  When istruewing does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-04-03 18:07:19+02:00, istruewing@stripped +5 -0
>   Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>   
>   INSTALL PLUGIN and UNINSTALL PLUGIN worked with statement-based and
>   mixed-mode replication only, but not with row-based replication.
>   
>   There is no statement-based replication of these statements.
>   But there was row-based replication of the inserts and deletes
>   to and from the mysql.plugin table.
>   
>   The fix is to suppress binlogging during insert and delete to
>   and from the mysql.plugin table.
> 
>   mysql-test/suite/rpl/r/rpl_plugin_load.result@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +17 -0
>     Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>     New test result.
> 
>   mysql-test/suite/rpl/r/rpl_plugin_load.result@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +0 -0
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load-master.opt@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +1 -0
>     Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>     New master options.
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load-master.opt@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +0 -0
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load-slave.opt@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +1 -0
>     Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>     New slave options.
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load-slave.opt@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +0 -0
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load.test@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +51 -0
>     Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>     New test case.
> 
>   mysql-test/suite/rpl/t/rpl_plugin_load.test@stripped, 2008-04-03 18:07:18+02:00,
> istruewing@stripped +0 -0
> 
>   sql/sql_plugin.cc@stripped, 2008-04-03 18:07:18+02:00, istruewing@stripped +16 -1
>     Bug#35807 - INSTALL PLUGIN replicates row-based, but not stmt-based
>     Suppress binlogging during insert and delete to/from the
>     mysql.plugin table.
> 
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_plugin_load.result
> b/mysql-test/suite/rpl/r/rpl_plugin_load.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/r/rpl_plugin_load.result	2008-04-03 18:07:18 +02:00
> @@ -0,0 +1,17 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +connection master
> +INSTALL PLUGIN example SONAME 'ha_example.so';
> +connection slave
> +INSTALL PLUGIN example SONAME 'ha_example.so';
> +connection master
> +End of 5.1 tests
> +connection master
> +UNINSTALL PLUGIN example;
> +connection slave
> +UNINSTALL PLUGIN example;
> +connection default
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_plugin_load-master.opt
> b/mysql-test/suite/rpl/t/rpl_plugin_load-master.opt
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_plugin_load-master.opt	2008-04-03 18:07:18 +02:00
> @@ -0,0 +1 @@
> +$EXAMPLE_PLUGIN_OPT
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_plugin_load-slave.opt
> b/mysql-test/suite/rpl/t/rpl_plugin_load-slave.opt
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_plugin_load-slave.opt	2008-04-03 18:07:18 +02:00
> @@ -0,0 +1 @@
> +$EXAMPLE_PLUGIN_OPT
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_plugin_load.test
> b/mysql-test/suite/rpl/t/rpl_plugin_load.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_plugin_load.test	2008-04-03 18:07:18 +02:00
> @@ -0,0 +1,51 @@

Please include the following as comments:

  - A high-level description of the *purpose* of the test case (e.g., 
"Verify that (UN)INSTALL PLUGIN works with replication.").

  - A high-level description of *how the test works* (e.g., "Try to 
install and uninstall a plugin on master, and verify that it does not 
affect the slave and that it does not add anything to the binlog")

  - The bug number.

> +--source include/not_embedded.inc
> +--source include/have_log_bin.inc
> +# Dynamic loading of Example does not work on Windows currently.
> +--source include/not_windows.inc
> +
> +#
> +# Check if server has support for loading udf's
> +# i.e it will support dlopen
> +#
> +--require r/have_dynamic_loading.require
> +disable_query_log;
> +show variables like "have_dynamic_loading";
> +enable_query_log;
> +
> +--require r/true.require
> +disable_query_log;
> +select LENGTH("$EXAMPLE_PLUGIN") > 0 as `TRUE`;
> +enable_query_log;

Why do you test that $EXAMPLE_PLUGIN != ""? It does not seem to be 
needed in the remainder of the test case...

> +
> +
> +source ./include/master-slave.inc;
> +
> +echo connection master;
> +connection master;

No need to connect to master: include/master-slave.inc does that.

> +INSTALL PLUGIN example SONAME 'ha_example.so';
> +#
> +    echo connection slave;
> +    connection slave;
> +    INSTALL PLUGIN example SONAME 'ha_example.so';

I don't think you need to install a plugin on slave. You just need to 
check that installation on master does not affect the slave. Moreover, 
you need to check that uninstallation on master does not affect slave. I 
think the test should do something like this:


--echo Check that INSTALL PLUGIN does not get replicated
INSTALL PLUGIN example SONAME 'ha_example.so';
sync_slave_with_master;

--echo Check that UNINSTALL PLUGIN does not get replicated
connection master;
UNINSTALL PLUGIN example SONAME 'ha_example.so';
sync_slave_with_master;

--echo Double-check that (UN)INSTALL PLUGIN did not modify the binlog
connection master;
SHOW BINLOG EVENTS;



> +#
> +echo connection master;
> +connection master;
> +
> +# We have to sync with master, to ensure slave had time to start properly
> +# before we stop it. If not, we get errors about UNIX_TIMESTAMP() in the log.
> +connection master;
> +sync_slave_with_master;
> +
> +
> +--echo End of 5.1 tests
> +
> +echo connection master;
> +connection master;
> +UNINSTALL PLUGIN example;
> +
> +echo connection slave;
> +connection slave;
> +UNINSTALL PLUGIN example;
> +
> +echo connection default;
> +connection default;

Please just do source include/master-slave-end.inc to clean up the test.

> diff -Nrup a/sql/sql_plugin.cc b/sql/sql_plugin.cc
> --- a/sql/sql_plugin.cc	2008-03-14 18:50:33 +01:00
> +++ b/sql/sql_plugin.cc	2008-04-03 18:07:18 +02:00
> @@ -1666,11 +1666,18 @@ bool mysql_install_plugin(THD *thd, cons
>      goto deinit;
>    }
>  
> +  /*
> +    We do not replicate the INSTALL PLUGIN statement. Disable binlogging
> +    of the insert into the plugin table, so that it is not replicated in
> +    row based mode.
> +  */
> +  tmp_disable_binlog(thd);
>    table->use_all_columns();
>    restore_record(table, s->default_values);
>    table->field[0]->store(name->str, name->length, system_charset_info);
>    table->field[1]->store(dl->str, dl->length, files_charset_info);
>    error= table->file->ha_write_row(table->record[0]);
> +  reenable_binlog(thd);
>    if (error)
>    {
>      table->file->print_error(error, MYF(0));
> @@ -1735,7 +1742,15 @@ bool mysql_uninstall_plugin(THD *thd, co
>                                          HA_READ_KEY_EXACT))
>    {
>      int error;
> -    if ((error= table->file->ha_delete_row(table->record[0])))
> +    /*
> +      We do not replicate the UNINSTALL PLUGIN statement. Disable binlogging
> +      of the delete from the plugin table, so that it is not replicated in
> +      row based mode.
> +    */
> +    tmp_disable_binlog(thd);
> +    error= table->file->ha_delete_row(table->record[0]);
> +    reenable_binlog(thd);
> +    if (error)
>      {
>        table->file->print_error(error, MYF(0));
>        DBUG_RETURN(TRUE);
> 

-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com