Hi Jasonh,
Thank you for the review.
I will address all your requests.
Cheers.
He Zhenxing wrote:
> Hi Alfranio,
>
> The patch looks OK, approved after consideration of the follow comments.
>
> Alfranio Correia wrote:
>
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-47287/mysql-5.1-bugteam/
> based on revid:satya.bn@stripped
>>
>> 3111 Alfranio Correia 2009-09-17
>> BUG#47287 RBR: replication diff on basic case with txn- and non-txn tables
> in a statement
>>
>> Let
>> - T be a transactional table and N non-transactional table.
>> - B be begin, C commit and R rollback.
>> - M be a mixed statement, i.e. a statement that updates both T and N.
>> - M* be a mixed statement that fails while updating either T or N.
>>
>> This patch restore the behavior presented in 5.1.37 for rows either
> produced in
>> the RBR or MIXED modes, when a M* statement that happened early in a
> transaction
>> had their changes written to the binary log outside the boundaries of the
>> transaction and wrapped in a BEGIN/ROLLBACK. This was done to keep the
> slave
>> consistent with with the master as the rollback would keep the changes on N
> and
>> undo them on T. In particular, we do what folllows:
>>
>>
>
> type: follows
>
>
>> . B M* T C would log - B M* R B T C.
>>
>> Note that, we are not preserving history from the master as we are
> introducing a
>> rollback that never happened. However, this seems to be more acceptable
> than
>> making the slave diverge. We do not fix the following case:
>>
>> . B T M* C would log B T M* C.
>>
>> The slave will diverge as the changes on T tables that originated from the
> M
>> statement are rolled back on the master but not on the slave.
> Unfortunately, we
>> cannot simply rollback the transaction as this would undo any uncommitted
>> changes on T tables.
>>
>> SBR is not considered in this patch because a failing statement is written
> to
>> the binary along with the error code and a slave executes and then rolls
> back
>> the statement when it has an associated error code, thus undoing the
> effects
>> on T. In RBR and MBR, a full-fledge fix will be pushed after the WL 2687.
>>
>> added:
>> mysql-test/extra/rpl_tests/rpl_failure_mixing_engines.test
>> mysql-test/suite/rpl/r/rpl_mixed_failure_mixing_engines.result
>> mysql-test/suite/rpl/r/rpl_row_failure_mixing_engines.result
>> mysql-test/suite/rpl/t/rpl_mixed_failure_mixing_engines.test
>> mysql-test/suite/rpl/t/rpl_row_failure_mixing_engines.test
>> modified:
>> sql/log.cc
>> === added file 'mysql-test/extra/rpl_tests/rpl_failure_mixing_engines.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_failure_mixing_engines.test 1970-01-01
> 00:00:00 +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_failure_mixing_engines.test 2009-09-17
> 15:22:06 +0000
>> @@ -0,0 +1,123 @@
>>
> +################################################################################
>> +# Let
>> +# - T be a transactional table and N non-transactional table.
>> +# - B be begin, C commit and R rollback.
>> +# - M be a mixed statement, i.e. a statement that updates both T and N.
>> +# - M* be a mixed statement that fails while updating either T or N.
>> +#
>> +# In this test case, when changes are logged as rows either in the RBR or MIXED
>> +# modes, we check if a M* statement that happens early in a transaction is
>> +# written to the binary log outside the boundaries of the transaction and
>> +# wrapped in a BEGIN/ROLLBACK. This is done to keep the slave consistent with
>> +# with the master as the rollback will keep the changes on N and undo them on
>> +# T. In particular, we expect the following behavior:
>> +#
>> +# 1. B M* T C would log - B M* R B T C.
>> +# 2. B T M* C would log B T M* C.
>> +#
>> +# SBR is not considered in this test because a failing statement is written to
>> +# the binary along with the error code and a slave executes and then rolls back
>> +# the statement when it has an associated error code, thus undoing the effects
>> +# on T.
>> +#
>> +# Note that, in the first case, we are not preserving history from the master
> as
>> +# we are introducing a rollback that never happened. However, this seems to be
>> +# more acceptable than making the slave diverge. In the second case, the slave
>> +# will diverge as the changes on T tables that originated from the M statement
>> +# are rolled back on the master but not on the slave. Unfortunately, we cannot
>> +# simply rollback the transaction as this would undo any uncommitted changes
>> +# on T tables.
>> +#
>> +# Such issues do not happen in SBR. In RBR and MBR, a fix will be pushed after
>> +# the WL 2687.
>>
> +################################################################################
>> +
>> +
>> +--echo
> ###################################################################################
>> +--echo # CONFIGURATION
>> +--echo
> ###################################################################################
>> +connection master;
>> +
>> +SET SQL_LOG_BIN=0;
>> +CREATE TABLE nt_1 (a text, b int PRIMARY KEY, c text) ENGINE = MyISAM;
>> +CREATE TABLE nt_2 (a text, b int PRIMARY KEY, c text) ENGINE = MyISAM;
>> +CREATE TABLE tt_1 (a text, b int PRIMARY KEY, c text) ENGINE = Innodb;
>> +CREATE TABLE tt_2 (a text, b int PRIMARY KEY, c text) ENGINE = Innodb;
>> +SET SQL_LOG_BIN=1;
>> +
>> +connection slave;
>> +
>> +SET SQL_LOG_BIN=0;
>> +CREATE TABLE nt_1 (a text, b int PRIMARY KEY, c text) ENGINE = MyISAM;
>> +CREATE TABLE nt_2 (a text, b int PRIMARY KEY, c text) ENGINE = MyISAM;
>> +CREATE TABLE tt_1 (a text, b int PRIMARY KEY, c text) ENGINE = Innodb;
>> +CREATE TABLE tt_2 (a text, b int PRIMARY KEY, c text) ENGINE = Innodb;
>>
>
> I think the 'c' column is not necessary and can be removed for this
> test.
>
Ok. I will change it.
>
>> +SET SQL_LOG_BIN=1;
>> +
>> +connection master;
>> +
>> +DELIMITER |;
>> +
>> +CREATE TRIGGER tr_i_tt_1_to_nt_1 BEFORE INSERT ON tt_1 FOR EACH ROW
>> +BEGIN
>> + INSERT INTO nt_1 VALUES (NEW.a, NEW.b, NEW.c);
>> +END|
>> +
>> +CREATE TRIGGER tr_i_nt_2_to_tt_2 BEFORE INSERT ON nt_2 FOR EACH ROW
>> +BEGIN
>> + INSERT INTO tt_2 VALUES (NEW.a, NEW.b, NEW.c);
>> +END|
>> +
>> +DELIMITER ;|
>> +
>> +--echo
> ###################################################################################
>> +--echo # CHECK HISTORY IN BINLOG
>> +--echo
> ###################################################################################
>> +let $binlog_start= query_get_value("SHOW MASTER STATUS", Position, 1);
>> +
>> +
>> +--echo *** "B M* T C" with error in M* generates in the binlog the "M B T C"
> entries
>>
>
> I think the binlog should be: B M R B T C
>
>
>> +INSERT INTO nt_1 VALUES ("new text 1", 1, '');
>> +BEGIN;
>> +--error ER_DUP_ENTRY
>> +INSERT INTO tt_1 VALUES ("new text 2", 2, USER()), ("new text 1", 1, USER());
>> +INSERT INTO tt_1 VALUES ("new text 3", 3, ''), ("new text 4", 4, '');
>> +COMMIT;
>> +
>> +INSERT INTO tt_2 VALUES ("new text 5", 5, '');
>> +BEGIN;
>> +--error ER_DUP_ENTRY
>> +INSERT INTO nt_2 VALUES ("new text 6", 6, USER()), ("new text 5", 5, USER());
>> +INSERT INTO tt_2 VALUES ("new text 7", 7, '');
>> +COMMIT;
>> +
>> +--echo *** "B M M T C" with error in M* generates in the binlog the "B M M* T C"
> entries
>>
>
> typo: B M M* T C
>
> [ snip ]
>
>
>> === modified file 'sql/log.cc'
>> --- a/sql/log.cc 2009-09-11 17:06:27 +0000
>> +++ b/sql/log.cc 2009-09-17 15:22:06 +0000
>> @@ -182,7 +182,10 @@ public:
>> {
>> DBUG_PRINT("info", ("truncating to position %lu", (ulong) pos));
>> DBUG_PRINT("info", ("before_stmt_pos=%lu", (ulong) pos));
>> - delete pending();
>> + if (pending())
>> + {
>> + delete pending();
>> + }
>> set_pending(0);
>> reinit_io_cache(&trans_log, WRITE_CACHE, pos, 0, 0);
>> trans_log.end_of_file= max_binlog_cache_size;
>> @@ -205,8 +208,7 @@ public:
>> completely.
>> */
>> void reset() {
>> - if (!empty())
>> - truncate(0);
>> + truncate(0);
>> before_stmt_pos= MY_OFF_T_UNDEF;
>> incident= FALSE;
>> trans_log.end_of_file= max_binlog_cache_size;
>> @@ -1539,9 +1541,10 @@ static int binlog_commit(handlerton *hto
>> {
>> Query_log_event qev(thd, STRING_WITH_LEN("COMMIT"), TRUE, TRUE, 0);
>> error= binlog_end_trans(thd, trx_data, &qev, all);
>> - goto end;
>> }
>>
>> + trx_data->at_least_one_stmt = my_b_tell(&trx_data->trans_log) >
> 0;
>> +
>>
>
> I'd suggest to make 'at_least_one_stmt' to be a inline function like
> this:
>
> bool at_least_one_stmt()
> {
> return my_b_tell(&trx_data->trans_log) > 0;
> }
>
> So that we only compute the value when needed, and do not need to update
> it's value after every call to binlog_commit or truncate.
>
Great idea. I will change it.
>
>> end:
>> if (!all)
>> trx_data->before_stmt_pos = MY_OFF_T_UNDEF; // part of the stmt commit
>> @@ -1617,6 +1620,9 @@ static int binlog_rollback(handlerton *h
>> if ((all && thd->transaction.all.modified_non_trans_table) ||
>> (!all && thd->transaction.stmt.modified_non_trans_table
> &&
>> !(thd->options & (OPTION_BEGIN | OPTION_NOT_AUTOCOMMIT))) ||
>> + (!all && thd->transaction.stmt.modified_non_trans_table
> &&
>> + !trx_data->at_least_one_stmt &&
>> + thd->current_stmt_binlog_row_based) ||
>>
>
> I think the above can be simplified as:
>
> if ((all && thd->transaction.all.modified_non_trans_table) ||
> (!all && thd->transaction.stmt.modified_non_trans_table &&
> !(thd->options & (OPTION_BEGIN | OPTION_NOT_AUTOCOMMIT)) ||
> (!trx_data->at_least_one_stmt &&
> thd->current_stmt_binlog_row_based)) ||
> )
>
> Please also update the comments of the code before this.
>
I will check that.
>
>> ((thd->options & OPTION_KEEP_LOG)))
>> {
>> Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, TRUE, 0);
>>
>>
>
>
