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, (¶m));
> + FOREACH_OBSERVER(ret, after_rollback, thd, (¶m));
>
> /*
> 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;
>
