Hi Andrei,
Patch approved, but I think you should get rid of the goto I mentioned in the
previous review before pushing,
Just my few cents,
Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/5.1-bt-bug40221-rbr-stmt-no-stmt-reset/
>
> 2727 Andrei Elkin 2008-12-02
> 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().
> Asserting emptiness the transaction cache after reset() and the uninitilized
> value for the statement's savepoint binlog position.
> === 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-12-02 17:32:07 +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-12-02 17:32:07 +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-12-02 17:32:07 +0000
> @@ -207,6 +207,7 @@ public:
> truncate(0);
> before_stmt_pos= MY_OFF_T_UNDEF;
> trans_log.end_of_file= max_binlog_cache_size;
> + DBUG_ASSERT(empty());
> }
>
> Rows_log_event *pending() const
> @@ -1377,8 +1378,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 +1386,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
> @@ -1435,6 +1435,7 @@ binlog_end_trans(THD *thd, binlog_trx_da
> mysql_bin_log.update_table_map_version();
> }
>
> + DBUG_ASSERT(thd->binlog_get_pending_rows_event() == NULL);
> DBUG_RETURN(error);
> }
>
> @@ -1466,6 +1467,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 +1554,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);
You still have the goto out of the conditional here. Why are you keeping it?
> }
>
> /**
> @@ -1615,6 +1621,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
