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.
> +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.
> 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.
> ((thd->options & OPTION_KEEP_LOG)))
> {
> Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, TRUE, 0);
>
