Andrei Elkin wrote:
> He Zhenxing, hello.
>
>> Hi Andrei
>>
>> Patch approved, but I have some suggestions in the following inline
>> comments!
>>
>> Andrei Elkin wrote:
>>> #At
> file:///home/andrei/MySQL/BZR/FIXES/5.1-bt-bug40221-rbr-stmt-no-stmt-reset/
>>>
>>> 2712 Andrei Elkin 2008-11-26
>>> 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/binlog/r/binlog_innodb.result
>>> mysql-test/suite/binlog/t/binlog_innodb.test
>>> sql/log.cc
>>>
>>> per-file messages:
>>> mysql-test/suite/binlog/r/binlog_innodb.result
>>> bug#40221 changed results.
>>> mysql-test/suite/binlog/t/binlog_innodb.test
>>> a regression test for the bug#40221 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/binlog/r/binlog_innodb.result'
>>> --- a/mysql-test/suite/binlog/r/binlog_innodb.result 2008-08-27 14:17:55
> +0000
>>> +++ b/mysql-test/suite/binlog/r/binlog_innodb.result 2008-11-26 15:17:46
> +0000
>>> @@ -115,14 +115,14 @@ master-bin.000001 # Xid # # COMMIT /* XI
>>> DROP TABLE t1;
>>> show status like "binlog_cache_use";
>>> Variable_name Value
>>> -Binlog_cache_use 15
>>> +Binlog_cache_use 13
>>> show status like "binlog_cache_disk_use";
>>> Variable_name Value
>>> Binlog_cache_disk_use 0
>>> create table t1 (a int) engine=innodb;
>>> show status like "binlog_cache_use";
>>> Variable_name Value
>>> -Binlog_cache_use 16
>>> +Binlog_cache_use 14
>>> show status like "binlog_cache_disk_use";
>>> Variable_name Value
>>> Binlog_cache_disk_use 1
>>> @@ -131,7 +131,7 @@ delete from t1;
>>> commit;
>>> show status like "binlog_cache_use";
>>> Variable_name Value
>>> -Binlog_cache_use 17
>>> +Binlog_cache_use 15
>>> show status like "binlog_cache_disk_use";
>>> Variable_name Value
>>> Binlog_cache_disk_use 1
>>> @@ -169,4 +169,21 @@ show master status /* there must be no U
>>> File Position Binlog_Do_DB Binlog_Ignore_DB
>>> master-bin.000001 106
>>> drop table t1, t2;
>>> +SET @@session.binlog_format = 'row';
>>> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
>>> +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;
>>> End of tests
>>>
>>> === modified file 'mysql-test/suite/binlog/t/binlog_innodb.test'
>>> --- a/mysql-test/suite/binlog/t/binlog_innodb.test 2007-08-14 21:35:19 +0000
>>> +++ b/mysql-test/suite/binlog/t/binlog_innodb.test 2008-11-26 15:17:46 +0000
>>> @@ -169,4 +169,26 @@ show master status /* there must be no U
>>> # cleanup bug#27716
>>> drop table t1, t2;
>>>
>>> +#
>>> +# Bug #40221 Replication failure on RBR + UPDATE the primary key
>>> +#
>>> +
>>> +SET @@session.binlog_format = 'row';
>> Please don't set the binlog_format in the code, use the following
>> instead:
>>
>> source include/have_binlog_format_row.inc;
>>
>
> Ok, considering you and Mats both don't like the run time assignment
> to the session binlog_format, I moved the bug's regression test to a
> new file: binlog_innodb_row.
>
>
>>> +CREATE TABLE t1 (pk int auto_increment primary key) ENGINE=innodb;
>>> +reset master;
>>> +
>>> +let $before_pos = query_get_value(SHOW MASTER STATUS, Position, 1);
>>> +
>>> +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;
>>> +
>>> --echo End of tests
>>>
>>> === modified file 'sql/log.cc'
>>> --- a/sql/log.cc 2008-11-06 14:18:25 +0000
>>> +++ b/sql/log.cc 2008-11-26 15:17:46 +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;
>
>> I would suggest to wrap this in a function named something like
>> THD::binlog_end_stmt or THD::binlog_reset_stmt_begin.
>>
> Well, I am not sure in what is the best style.
> Personally, I would call a new
>
> trx_data->reset_stmt();
>
> However I am not keen on making any change for this part for few
> reasons. First, there are other refactoring ideas. E.g
> an accessor and a setter to trx_data->before_stmt_pos both are
> missed, instead trx_data->before_stmt_pos is defined public and used
> widely across the code. Second, mine proposal
> binlog_trx_data::reset_stmt(), if implemented ever, is not just
> trx_data->before_stmt_pos = MY_OFF_T_UNDEF but should include other
> activities such as flushing or removing the pending event.
trx_data->reset() already does that.
> Third, I don't think there is a need for defining a method that body
> contains merely a line, esp rather self-describing one (I did not
> disagree once i saw you defined that as well although let you know on
> my optinion).
>
> I hope you'll agree to leave all this in the current form (except to
> relocate from binlog_end_trans, see the next paragraph) till a
> better time.
>
>
>
>
>> And I'd also suggest to move this to binlog_rollback (instead of
>> binlog_end_trans), so that the statement begin position is reset in
>> binlog_rollback and binlog_commit.
>>
>
> There is a sense for doing that, agreed.
>
>
>>> DBUG_RETURN(error);
>>> }
>>>
>>> @@ -1555,6 +1555,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; // part of the stmt
> commit
>>> DBUG_RETURN(0);
>>> }
>>>
>>>
>
> You are welcome to make another look to just have committed patch.
>
> cheers,
>
> Andrei
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
