List:Commits« Previous MessageNext Message »
From:Luís Soares Date:January 21 2010 12:35pm
Subject:Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)
Bug#50157
View as plain text  
Hi Libing,

  First of all, I could not find your commit in my 
inbox!!! Only on the list archives!

  Nice Work. 
  Please find my review comments below.

STATUS
------
  Approved (pending clarification on R1 which is not a Show stopper).

REQUIRED CHANGES
----------------
 n/a

REQUESTS
--------
  
  R1. It seems to me that after this patch, each statement that
      is written to the binlog will request an ACK from the
      slave. This is OK, as transactional cache is flushed on
      commit, thence only one OK from the slave is needed for
      transactions. This is OK for non-transactional statements
      as well. However, there may be some ambiguous behavior for
      MIXED statements (tt - transactional table, nt - non
      transaction table):

        BEGIN
        INSERT INTO tt ...
        INSERT INTO tt ...
        INSERT INTO nt SELECT FROM tt  <============
        UPDATE tt
        INSERT INTO tt
        COMMIT

      Given that after WL#2687 we have two caches (one for
      t-statements and one for nt-statements that are flushed
      independently), I think it should be documented what
      happens in the statement marked with <====... wrt to
      semisynchronous replication:
   
        - Do we get a slave ACK? (I think so)
        - Which changes is the slave ACKing?
        - Should the <=== update both nt and tt would the ACKs
          for these changes occur at the same time? If not when
          are they ACKed?

SUGGESTIONS
-----------

  S1. Some suggestions on the commit message. See inline.

DETAILS 
-------
 n/a

Regards,
Luís

> #At file:///home/anders/work/bzrroot/mysql-5.1-rep-semisync/ based on
> revid:zhenxing.he@stripped
> 
>  3127 Li-Bing.Song@stripped	2010-01-20
>       BUG#50157 Assertion !active_tranxs_->is_tranx_end_pos(..) in
> ReplSemiSyncMaster::commitTrx
>       
>       The root cause of the crash is that a TranxNode is freed before it is used.
>       A TranxNode is created when each time some log events are written into binlog
> file
>       and is synchronized. TranxNodes' memories are allocted from mem_root of the
> current
>       thread, and will be freed immediately after current statement ending.

allocted -> allocated

>       Sometimes, a statement ends and its TranxNode is freed before it is cleared
>       from TranxNode list. So the Pointer of the TranxNode in TranxNode list becomes
> a 
>       wild pointer.
>       
>       After this patch, One statement always waits until its log events has been
> replicated
>       completely if it has been binlogged and synchronized. For the TranxNode will
> be

maybe: if it has been flushed and synched instead? You pick.

>       cleared from the TranxNode list after the log event has been replicated.
>      @ sql/rpl_handler.cc
>         params are not initialized.
> 
>     added:
>       mysql-test/suite/rpl/r/rpl_semi_sync_mixed_tables.result
>       mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-master.opt
>       mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-slave.opt
>       mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables.test
>     modified:
>       plugin/semisync/semisync_master_plugin.cc
>       sql/rpl_handler.cc
> === added file 'mysql-test/suite/rpl/r/rpl_semi_sync_mixed_tables.result'
> --- a/mysql-test/suite/rpl/r/rpl_semi_sync_mixed_tables.result	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_semi_sync_mixed_tables.result	2010-01-20 08:37:18
> +0000
> @@ -0,0 +1,36 @@
> +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;
> +
> +##############################################################################
> +# BUG#50157
> +# semi-sync replication crashes when 'CREATE TEMPORARY TABLE `MyISAM_t` SELECT
> +# * FROM `Innodb_t`;
> +##############################################################################
> +SET AUTOCOMMIT= 0;
> +CREATE TABLE t2(c1 INT) ENGINE=innodb;
> +call mtr.add_suppression(".*");
> +INSTALL PLUGIN rpl_semi_sync_master SONAME 'semisync_master.so';
> +SET GLOBAL rpl_semi_sync_master_enabled = 1;
> +SET GLOBAL rpl_semi_sync_master_trace_level= 80;
> +STOP SLAVE;
> +INSTALL PLUGIN rpl_semi_sync_slave SONAME 'semisync_slave.so';
> +SET GLOBAL rpl_semi_sync_slave_enabled = 1;
> +START SLAVE;
> +BEGIN;
> +
> +# Even though it is in a transaction, this statement is binlogged into binlog
> +# file immediately.
> +CREATE TEMPORARY TABLE t1 LIKE t2;
> +
> +# These statements will not binlogged until the transaction is committed
> +INSERT INTO t2 VALUES(11);
> +INSERT INTO t2 VALUES(22);
> +COMMIT;
> +
> +UNINSTALL PLUGIN rpl_semi_sync_slave;
> +UNINSTALL PLUGIN rpl_semi_sync_master;
> +DROP TABLE t1, t2;
> 
> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-master.opt'
> --- a/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-master.opt	1970-01-01
> 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-master.opt	2010-01-20
> 08:37:18
> +0000
> @@ -0,0 +1 @@
> +$SEMISYNC_PLUGIN_OPT
> 
> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-slave.opt'
> --- a/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-slave.opt	1970-01-01
> 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables-slave.opt	2010-01-20
> 08:37:18
> +0000
> @@ -0,0 +1 @@
> +$SEMISYNC_PLUGIN_OPT
> 
> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables.test'
> --- a/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables.test	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_mixed_tables.test	2010-01-20 08:37:18
> +0000
> @@ -0,0 +1,53 @@
> +source include/have_semisync_plugin.inc;
> +source include/not_embedded.inc;
> +source include/have_innodb.inc;
> +source include/have_binlog_format_statement.inc;
> +source include/master-slave.inc;
> +
> +--echo
> +--echo
> ##############################################################################
> +--echo # BUG#50157
> +--echo # semi-sync replication crashes when 'CREATE TEMPORARY TABLE `MyISAM_t`
> SELECT
> +--echo # * FROM `Innodb_t`;
> +--echo
> ##############################################################################
> +
> +SET AUTOCOMMIT= 0;
> +CREATE TABLE t2(c1 INT) ENGINE=innodb;
> +sync_slave_with_master;
> +
> +connection master;
> +call mtr.add_suppression(".*");
> +eval INSTALL PLUGIN rpl_semi_sync_master SONAME '$SEMISYNC_MASTER_PLUGIN';
> +SET GLOBAL rpl_semi_sync_master_enabled = 1;
> +SET GLOBAL rpl_semi_sync_master_trace_level= 80;
> +
> +connection slave;
> +STOP SLAVE;
> +source include/wait_for_slave_to_stop.inc;
> +
> +eval INSTALL PLUGIN rpl_semi_sync_slave SONAME '$SEMISYNC_SLAVE_PLUGIN';
> +SET GLOBAL rpl_semi_sync_slave_enabled = 1;
> +
> +START SLAVE;
> +source include/wait_for_slave_to_start.inc;
> +
> +connection master;
> +BEGIN;
> +--echo
> +--echo # Even though it is in a transaction, this statement is binlogged into
> binlog
> +--echo # file immediately.
> +CREATE TEMPORARY TABLE t1 SELECT c1 FROM t2 where 1=1;
> +--echo
> +--echo # These statements will not binlogged until the transaction is committed
> +INSERT INTO t2 VALUES(11);
> +INSERT INTO t2 VALUES(22);
> +COMMIT;
> +sync_slave_with_master;
> +
> +--echo
> +UNINSTALL PLUGIN rpl_semi_sync_slave;
> +connection master;
> +UNINSTALL PLUGIN rpl_semi_sync_master;
> +
> +DROP TABLE t1, t2;
> +source include/master-slave-end.inc;
> 
> === modified file 'plugin/semisync/semisync_master_plugin.cc'
> --- a/plugin/semisync/semisync_master_plugin.cc	2009-10-18 12:29:03 +0000
> +++ b/plugin/semisync/semisync_master_plugin.cc	2010-01-20 08:37:18 +0000
> @@ -46,10 +46,7 @@ int repl_semi_request_commit(Trans_param
>  
>  int repl_semi_report_commit(Trans_param *param)
>  {
> -
> -  bool is_real_trans= param->flags & TRANS_IS_REAL_TRANS;
> -
> -  if (is_real_trans && param->log_pos)
> +  if (param->log_pos)
>    {
>      const char *binlog_name= param->log_file;
>      return repl_semisync.commitTrx(binlog_name, param->log_pos);
> 
> === modified file 'sql/rpl_handler.cc'
> --- a/sql/rpl_handler.cc	2009-12-04 01:46:33 +0000
> +++ b/sql/rpl_handler.cc	2010-01-20 08:37:18 +0000
> @@ -190,8 +190,8 @@ int Trans_delegate::after_commit(THD *th
>  {
>    Trans_param param;
>    bool is_real_trans= (all || thd->transaction.all.ha_list == 0);
> -  if (is_real_trans)
> -    param.flags |= TRANS_IS_REAL_TRANS;
> +
> +  param.flags = is_real_trans ? TRANS_IS_REAL_TRANS : 0;
>  
>    Trans_binlog_info *log_info=
>      my_pthread_getspecific_ptr(Trans_binlog_info*, RPL_TRANS_BINLOG_INFO);
> @@ -218,8 +218,8 @@ int Trans_delegate::after_rollback(THD *
>  {
>    Trans_param param;
>    bool is_real_trans= (all || thd->transaction.all.ha_list == 0);
> -  if (is_real_trans)
> -    param.flags |= TRANS_IS_REAL_TRANS;
> +
> +  param.flags = is_real_trans ? TRANS_IS_REAL_TRANS : 0;
>  
>    Trans_binlog_info *log_info=
>      my_pthread_getspecific_ptr(Trans_binlog_info*, RPL_TRANS_BINLOG_INFO);
> @@ -228,7 +228,7 @@ int Trans_delegate::after_rollback(THD *
>    param.log_pos= log_info ? log_info->log_pos : 0;
>  
>    int ret= 0;
> -  FOREACH_OBSERVER(ret, after_commit, thd, (&param));
> +  FOREACH_OBSERVER(ret, after_rollback, thd, (&param));
>  
>    /*
>      This is the end of a real transaction or autocommit statement, we
> @@ -249,6 +249,7 @@ int Binlog_storage_delegate::after_flush
>  {
>    Binlog_storage_param param;
>    uint32 flags=0;
> +
>    if (synced)
>      flags |= BINLOG_STORAGE_IS_SYNCED;
>  

Thread
bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127) Bug#50157Li-Bing.Song20 Jan
Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Luís Soares21 Jan