MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:November 28 2008 9:36am
Subject:Re: bzr commit into mysql-5.1 branch (aelkin:2712) Bug#40221
View as plain text  
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;


> +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.

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.

>    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);
>  }
>  
> 


Thread
bzr commit into mysql-5.1 branch (aelkin:2712) Bug#40221Andrei Elkin26 Nov
  • Re: bzr commit into mysql-5.1 branch (aelkin:2712) Bug#40221He Zhenxing28 Nov