Hi Andrei,
Comments inline.
Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/5.1-bt-bug40221-rbr-stmt-no-stmt-reset/
>
> 2712 Andrei Elkin 2008-11-28
> 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.
> added:
> mysql-test/suite/binlog/r/binlog_innodb_row.result
> mysql-test/suite/binlog/t/binlog_innodb_row.test
> modified:
> sql/log.cc
>
> per-file messages:
> mysql-test/suite/binlog/r/binlog_innodb_row.result
> a new test file to contain tests dealing with binlogging innodb with
> the row-based format.
> mysql-test/suite/binlog/t/binlog_innodb_row.test
> a new test file to contain tests dealing with binlogging innodb with
> the row-based format.
> 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 in binlog_commit() and binlog_rollback().
> === added file 'mysql-test/suite/binlog/r/binlog_innodb_row.result'
> --- a/mysql-test/suite/binlog/r/binlog_innodb_row.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_innodb_row.result 2008-11-28 16:56:11 +0000
> @@ -0,0 +1,22 @@
> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
> +show create table t1;
> +Table Create Table
> +t1 CREATE TABLE `t1` (
> + `pk` int(11) NOT NULL AUTO_INCREMENT,
> + PRIMARY KEY (`pk`)
> +) ENGINE=InnoDB DEFAULT CHARSET=latin1
> +reset master;
> +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;
> +*** Results of the test: 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 */
> +drop table t1;
>
> === added file 'mysql-test/suite/binlog/t/binlog_innodb_row.test'
> --- a/mysql-test/suite/binlog/t/binlog_innodb_row.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/t/binlog_innodb_row.test 2008-11-28 16:56:11 +0000
> @@ -0,0 +1,26 @@
> +#
> +# Tests of innodb/binlog with the row binlog format
> +#
> +source include/have_innodb.inc;
> +source include/have_log_bin.inc;
> +source include/have_binlog_format_row.inc;
> +
> +#
> +# Bug #40221 Replication failure on RBR + UPDATE the primary key
> +#
> +
> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
> +show create table t1;
> +reset master;
> +
> +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 *** Results of the test: the binlog must have only Write_rows events not any
> Update_rows ***
> +source include/show_binlog_events.inc;
> +
> +drop table t1;
>
> === modified file 'sql/log.cc'
> --- a/sql/log.cc 2008-11-06 14:18:25 +0000
> +++ b/sql/log.cc 2008-11-28 16:56:11 +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
If you remove the code above, there is always a risk that the pending event is
left (perhaps due to a bug elsewhere). Please add an assertion checking that
there is no pending event before returning from this function.
Please add an assertion checking that the transaction cache is empty (when it
should be empty) as well: we've had problems with this piece of code too often.
> @@ -1466,6 +1465,7 @@ static int binlog_prepare(handlerton *ht
> */
> static int binlog_commit(handlerton *hton, THD *thd, bool all)
> {
> + int error= 0;
> DBUG_ENTER("binlog_commit");
> binlog_trx_data *const trx_data=
> (binlog_trx_data*) thd_get_ha_data(thd, binlog_hton);
> @@ -1552,10 +1552,14 @@ static int binlog_commit(handlerton *hto
> {
> Query_log_event qev(thd, STRING_WITH_LEN("COMMIT"), TRUE, FALSE);
> qev.error_code= 0; // see comment in MYSQL_LOG::write(THD, IO_CACHE)
> - int error= binlog_end_trans(thd, trx_data, &qev, all);
> - DBUG_RETURN(error);
> + error= binlog_end_trans(thd, trx_data, &qev, all);
> + goto end;
> }
> - DBUG_RETURN(0);
> +
> +end:
> + if (!all)
> + trx_data->before_stmt_pos = MY_OFF_T_UNDEF; // part of the stmt commit
> + DBUG_RETURN(error);
What on earth does the goto do here? You are just using goto to go to the next
line!? The ending block is an if statement, so there is no loop to leave either;
there is no point in using a goto here.
> }
>
> /**
> @@ -1615,6 +1619,8 @@ static int binlog_rollback(handlerton *h
> */
> error= binlog_end_trans(thd, trx_data, 0, all);
> }
> + if (!all)
> + trx_data->before_stmt_pos = MY_OFF_T_UNDEF; // part of the stmt rollback
> DBUG_RETURN(error);
> }
>
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
