Hi Andrei!
Comments inline below.
Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/bug40221-RBR_UPDATE_pk/
>
> 2701 Andrei Elkin 2008-11-21
> Bug #40221 Replication failure on RBR + UPDATE the primary key
>
> A transaction could result in having an extra event after a query that errored
> e.g because of
> a dup key. Such a query is rolled back in innodb, as specified, but has not
> been in binlog.
> It appeares that the binlog engine did not always register for a query
> (statement) because
> the previous query had not reset at its statement commit time. Because of that
> fact there was no
> roll-back to the trx_data->before_stmt_pos position and a the pending event
> of the errorred query
> could become flushed to the binlog file.
>
> Fixed with deploying the reset of trx_data->before_stmt_pos at the end of
> the query processing.
> modified:
> mysql-test/suite/rpl/r/rpl_innodb.result
> mysql-test/suite/rpl/t/rpl_innodb.test
> sql/log.cc
>
> per-file messages:
> mysql-test/suite/rpl/r/rpl_innodb.result
> results gained bug#40221 regression test part.
> mysql-test/suite/rpl/t/rpl_innodb.test
> bug#40221 regression test is added.
> sql/log.cc
> Flushing the pending row event at binlog_end_trans() is moved down to the commit
> branch.
> Resetting of trx_data->before_stmt_pos to the uninitialized value at the end
> of the statement is
> implemented so that both binlog_commit() and binlog_rollback() execututes it.
> === modified file 'mysql-test/suite/rpl/r/rpl_innodb.result'
> --- a/mysql-test/suite/rpl/r/rpl_innodb.result 2008-10-29 13:25:03 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_innodb.result 2008-11-21 12:41:52 +0000
> @@ -81,4 +81,27 @@ FLUSH LOGS;
> -------- switch to master --------
> FLUSH LOGS;
> DROP DATABASE mysqltest1;
> +DROP TEMPORARY TABLE mysqltest1.tmp2;
> +SET @@session.binlog_format = 'row';
Please don't set the format explicitly. I have done an extensive refactoring in
BUG#39934 to separate out the intermixed tests and let the combinations file
execute the tests with different values of binlog_format.
The original intent was to not execute the suites under different modes, but
since MTR has changed, this has changed the rules of the game.
> +drop table if exists t1;
> +Warnings:
> +Note 1051 Unknown table 't1'
Since you are using "if exists", you expect that the table might not exist,
hence you should disable the warnings over this statement to not get a test failure.
> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
> +reset master;
> +show binlog events from <binlog_start>;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +begin;
> +insert into t1 values (1),(2);
> +*** the following UPDATE query wont generate any updates for the binlog ***
> +update t1 set pk = 3 where pk < 3;
> +ERROR 23000: Duplicate entry '3' for key 'PRIMARY'
> +commit;
> +*** the binlog must have only Write_rows events not any Update_rows ***
> +show binlog events from <binlog_start>;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +master-bin.000001 # Query # # use `test`; BEGIN
> +master-bin.000001 # Table_map # # table_id: # (test.t1)
> +master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F
> +master-bin.000001 # Xid # # COMMIT /* XID */
Please do not show parts of the binary log in replication tests. This causes a
significant amount of extra work when adding new features and fixing bugs.
In this particular case, it is sufficient to check that nothing was written to
the binary log when the update was executed, which can be done with something
like this:
let $before_pos = query_get_value(SHOW MASTER STATUS, Position, 1);
update ...;
let $after_pos = query_get_value(SHOW MASTER STATUS, Position, 1);
eval SELECT $after_pos - $before_pos AS Delta;
> +drop table t1;
> End of 5.1 tests
>
> === modified file 'mysql-test/suite/rpl/t/rpl_innodb.test'
> --- a/mysql-test/suite/rpl/t/rpl_innodb.test 2008-10-29 13:25:03 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_innodb.test 2008-11-21 12:41:52 +0000
> @@ -120,6 +120,31 @@ connection master;
> FLUSH LOGS;
>
> DROP DATABASE mysqltest1;
> +DROP TEMPORARY TABLE mysqltest1.tmp2;
> -- source include/master-slave-end.inc
>
> +#
> +# Bug #40221 Replication failure on RBR + UPDATE the primary key
> +#
> +
> +connection master;
> +
> +SET @@session.binlog_format = 'row';
> +drop table if exists t1;
> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
> +reset master;
Please do not show the contents of the binary log in replication tests. In this
case it is not even necessary. In addition, this test suffer from a race
condition if the dump thread has not been able to deliver the create table to
the slave before the reset is executed.
> +source include/show_binlog_events.inc;
> +
> +begin;
> +insert into t1 values (1),(2);
> +--echo *** the following UPDATE query wont generate any updates for the binlog ***
> +--error ER_DUP_ENTRY
> +update t1 set pk = 3 where pk < 3;
> +commit;
> +--echo *** the binlog must have only Write_rows events not any Update_rows ***
> +source include/show_binlog_events.inc;
See above.
> +
> +drop table t1;
> +
> +
> --echo End of 5.1 tests
>
> === modified file 'sql/log.cc'
> --- a/sql/log.cc 2008-11-12 17:51:47 +0000
> +++ b/sql/log.cc 2008-11-21 12:41:52 +0000
> @@ -1377,8 +1377,6 @@ binlog_end_trans(THD *thd, binlog_trx_da
> FLAGSTR(thd->options, OPTION_NOT_AUTOCOMMIT),
> FLAGSTR(thd->options, OPTION_BEGIN)));
>
> - thd->binlog_flush_pending_rows_event(TRUE);
> -
> /*
> NULL denotes ROLLBACK with nothing to replicate: i.e., rollback of
> only transactional tables. If the transaction contain changes to
> @@ -1387,6 +1385,7 @@ binlog_end_trans(THD *thd, binlog_trx_da
> */
> if (end_ev != NULL)
> {
> + thd->binlog_flush_pending_rows_event(TRUE);
> /*
> Doing a commit or a rollback including non-transactional tables,
> i.e., ending a transaction where we might write the transaction
> @@ -1434,7 +1433,8 @@ binlog_end_trans(THD *thd, binlog_trx_da
> */
> mysql_bin_log.update_table_map_version();
> }
> -
> + if (!all)
> + trx_data->before_stmt_pos = MY_OFF_T_UNDEF;
Please use trx_data->reset() for this. Otherwise, you might end up with data
left in the transaction cache.
> DBUG_RETURN(error);
> }
>
> @@ -1553,6 +1553,8 @@ static int binlog_commit(handlerton *hto
> int error= binlog_end_trans(thd, trx_data, &qev, all);
> DBUG_RETURN(error);
> }
> + if (!all)
> + trx_data->before_stmt_pos = MY_OFF_T_UNDEF;
See above.
> DBUG_RETURN(0);
> }
>
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
