List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:June 17 2009 2:40pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2840)
Bug#42829
View as plain text  
Hi Luis,

I don't have comments on the code.
I think the possible problems were already mentioned by Jasonh.

Comments on the test case:

1 - Collapse the tests in one single place but keep different test cases
for mixed and statement modes.
2 - Incorporate the test case that I wrote while discussing isolation
levels with Sven.
3 - Add multi-update statements.
4 - Add statements that access both databases.
5 - Add comments/output (i.e. echo) stating what is the expected output.
6 - For the sake of completeness would be great to have the row based mode.
(This is up to you. I don't have a strong opinion on that.)

Find just one comment in-line.

Cheers.

Luis Soares wrote:
> #At file:///home/lsoares/Workspace/mysql-server/bugfix/42829/5.1-bt/ based on
> revid:matthias.leich@stripped
>
>  2840 Luis Soares	2009-04-23
>       BUG#42829: binlogging enabled for all schemas regardless of
>       binlog-db-db / binlog-ignore-db
>       
>       InnoDB will return an error if statement based replication is used
>       along with transaction isolation level READ-COMMITTED (or stricter),
>       even if the statement in question is filtered out according to the
>       binlog-do-db rules set. In this case, an error should not be printed.
>       
>       This patch addresses this issue by extending the existing check in
>       external_lock to take into account the filter rules before deciding to
>       print an error. Furthermore, it also changes decide_logging_format to
>       take into consideration whether the statement is filtered out from 
>       binlog before decision is made.
>      @ mysql-test/suite/binlog/r/binlog_mix_do_db.result
>         Result file.
>      @ mysql-test/suite/binlog/r/binlog_stm_do_db.result
>         Result file.
>      @ mysql-test/suite/binlog/t/binlog_mix_do_db.test
>         Added test case for mixed format to check that statements are
>         turned into unsafe and logged in rows and that the additional
>         clause added in decide logging format produces no side-effects.
>      @ mysql-test/suite/binlog/t/binlog_stm_do_db.test
>         Test case for statement based logging. This format triggered
>         the bug reported.
>      @ sql/sql_base.cc
>         Changed the check on decide_logging_format to take into account
>         whether statement is filtered or not (and if it is in mixed mode)
>         before making the decision on log format (error checking).
>      @ sql/sql_class.cc
>         Added the thd_binlog_filter_ok to INNODB_COMPATIBILITY_HOOKS set.
>      @ storage/innobase/handler/ha_innodb.cc
>         Extended check in external_lock to take into consideration the
>         filtering when deciding to throw an error.
>      @ storage/innobase/handler/ha_innodb.h
>         Added declaration of new hook.
>
>     added:
>       mysql-test/suite/binlog/r/binlog_mix_do_db.result
>       mysql-test/suite/binlog/r/binlog_stm_do_db.result
>       mysql-test/suite/binlog/t/binlog_mix_do_db-master.opt
>       mysql-test/suite/binlog/t/binlog_mix_do_db.test
>       mysql-test/suite/binlog/t/binlog_stm_do_db-master.opt
>       mysql-test/suite/binlog/t/binlog_stm_do_db.test
>     modified:
>       sql/sql_base.cc
>       sql/sql_class.cc
>       storage/innobase/handler/ha_innodb.cc
>       storage/innobase/handler/ha_innodb.h
> === added file 'mysql-test/suite/binlog/r/binlog_mix_do_db.result'
> --- a/mysql-test/suite/binlog/r/binlog_mix_do_db.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_mix_do_db.result	2009-04-23 11:31:32 +0000
> @@ -0,0 +1,42 @@
> +SET @old_isolation_level= @@tx_isolation;
> +SET tx_isolation= 'READ-COMMITTED';
> +CREATE DATABASE db_not_filtered;
> +use db_not_filtered;
> +CREATE TABLE t1 (x int, y int) engine=InnoDB;
> +CREATE TABLE t2 (x int, y int) engine=InnoDB;
> +CREATE DATABASE db_filtered;
> +use db_filtered;
> +CREATE TABLE t1 (x int, y int) engine=InnoDB;
> +CREATE TABLE t2 (x int, y int) engine=InnoDB;
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
> +INSERT INTO db_not_filtered.t1 VALUES (1, 2);
> +INSERT INTO db_not_filtered.t1 VALUES (UUID(), 2);
> +use db_not_filtered;
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
> +DROP DATABASE db_not_filtered;
> +DROP DATABASE db_filtered;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	CREATE DATABASE db_not_filtered
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; CREATE TABLE t1 (x int, y int)
> engine=InnoDB
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; CREATE TABLE t2 (x int, y int)
> engine=InnoDB
> +master-bin.000001	#	Query	#	#	use `db_filtered`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (db_not_filtered.t1)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +master-bin.000001	#	Query	#	#	use `db_filtered`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (db_not_filtered.t1)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (db_not_filtered.t2)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (db_not_filtered.t1)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +master-bin.000001	#	Query	#	#	DROP DATABASE db_not_filtered
> +SET @@tx_isolation= @old_isolation_level;
>
> === added file 'mysql-test/suite/binlog/r/binlog_stm_do_db.result'
> --- a/mysql-test/suite/binlog/r/binlog_stm_do_db.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_stm_do_db.result	2009-04-23 11:31:32 +0000
> @@ -0,0 +1,26 @@
> +SET @old_isolation_level= @@tx_isolation;
> +SET tx_isolation= 'READ-COMMITTED';
> +CREATE DATABASE db_not_filtered;
> +use db_not_filtered;
> +CREATE TABLE t1 (x int, y int) engine=InnoDB;
> +CREATE TABLE t2 (x int, y int) engine=InnoDB;
> +CREATE DATABASE db_filtered;
> +use db_filtered;
> +CREATE TABLE t1 (x int, y int) engine=InnoDB;
> +CREATE TABLE t2 (x int, y int) engine=InnoDB;
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
> +use db_not_filtered;
> +INSERT INTO t2 VALUES (1,2);
> +ERROR HY000: Binary logging not possible. Message: Transaction level
> 'READ-COMMITTED' in InnoDB is not safe for binlog mode 'STATEMENT'
> +INSERT INTO t1 SELECT * FROM t2;
> +ERROR HY000: Binary logging not possible. Message: Transaction level
> 'READ-COMMITTED' in InnoDB is not safe for binlog mode 'STATEMENT'
> +DROP DATABASE db_not_filtered;
> +DROP DATABASE db_filtered;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	CREATE DATABASE db_not_filtered
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; CREATE TABLE t1 (x int, y int)
> engine=InnoDB
> +master-bin.000001	#	Query	#	#	use `db_not_filtered`; CREATE TABLE t2 (x int, y int)
> engine=InnoDB
> +master-bin.000001	#	Query	#	#	DROP DATABASE db_not_filtered
> +SET @@tx_isolation= @old_isolation_level;
>
> === added file 'mysql-test/suite/binlog/t/binlog_mix_do_db-master.opt'
> --- a/mysql-test/suite/binlog/t/binlog_mix_do_db-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/binlog/t/binlog_mix_do_db-master.opt	2009-04-23 11:31:32
> +0000
> @@ -0,0 +1 @@
> +--binlog-do-db=db_not_filtered
>
> === added file 'mysql-test/suite/binlog/t/binlog_mix_do_db.test'
> --- a/mysql-test/suite/binlog/t/binlog_mix_do_db.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/t/binlog_mix_do_db.test	2009-04-23 11:31:32 +0000
> @@ -0,0 +1,53 @@
> +# BUG#42829: binlogging enabled for all schemas regardless of binlog-db-db /
> +# binlog-ignore-db
> +#
> +# DESCRIPTION
> +# ===========
> +#
> +#   The test is implemented as follows:
> +#     i) set tx_isolation to read-committed.
> +#    ii) create two databases (one filtered other not - using binlog-do-db)
> +#   iii) Create statements that are to be filtered on filtered db
> +#       - At this point before the patch an error would be thrown.
> +#    iv) Create statements while using db_filtered that change db_not_filtered
> +#    tables. (These should be marked as unsafe and make it into the binlog)
> +#    v) Insert into tables in the not filtered database and check that 
> +#        no error exists because these are logged as row events.
> +
> +source include/have_log_bin.inc;
> +source include/have_innodb.inc;
> +source include/have_binlog_format_mixed.inc;
> +
> +SET @old_isolation_level= @@tx_isolation;
> +SET tx_isolation= 'READ-COMMITTED';
> +
> +let $engine= InnoDB;
> +
> +CREATE DATABASE db_not_filtered;
> +use db_not_filtered;
> +eval CREATE TABLE t1 (x int, y int) engine=$engine;
> +eval CREATE TABLE t2 (x int, y int) engine=$engine;
> +
> +CREATE DATABASE db_filtered;
> +use db_filtered;
> +eval CREATE TABLE t1 (x int, y int) engine=$engine;
> +eval CREATE TABLE t2 (x int, y int) engine=$engine;
> +
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
> +
>   
Please, do not disable the warning message.
> +INSERT INTO db_not_filtered.t1 VALUES (1, 2);
> +disable_warnings;
> +INSERT INTO db_not_filtered.t1 VALUES (UUID(), 2);
> +enable_warnings;
> +
> +use db_not_filtered;
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
>   
Just for the sake of completeness do also:

+INSERT INTO db_filtered.t1 VALUES (1, 2);
+INSERT INTO db_filtered.t1 VALUES (UUID(), 2);


> +
> +DROP DATABASE db_not_filtered;
> +DROP DATABASE db_filtered;
> +
> +source include/show_binlog_events.inc;
> +
> +SET @@tx_isolation= @old_isolation_level;
>
> === added file 'mysql-test/suite/binlog/t/binlog_stm_do_db-master.opt'
> --- a/mysql-test/suite/binlog/t/binlog_stm_do_db-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/binlog/t/binlog_stm_do_db-master.opt	2009-04-23 11:31:32
> +0000
> @@ -0,0 +1 @@
> +--binlog-do-db=db_not_filtered
>
> === added file 'mysql-test/suite/binlog/t/binlog_stm_do_db.test'
> --- a/mysql-test/suite/binlog/t/binlog_stm_do_db.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/t/binlog_stm_do_db.test	2009-04-23 11:31:32 +0000
> @@ -0,0 +1,52 @@
> +#
> +# BUG#42829: binlogging enabled for all schemas regardless of binlog-db-db /
> +# binlog-ignore-db
> +#
> +# DESCRIPTION
> +# ===========
> +#
> +#   The test is implemented as follows:
> +#     i) set tx_isolation to read-committed.
> +#    ii) create two databases (one filtered other not - using binlog-do-db)
> +#   iii) Create statements that are to be filtered on filtered db
> +#       - At this point before the patch an error would be thrown.
> +#    iv) do the same thing for not the filtered database and check that
> +#        events throw an error:
> +#
> +#      - Error: 1598 SQLSTATE: HY000  (ER_BINLOG_LOGGING_IMPOSSIBLE) 
> +#
> +
> +source include/have_log_bin.inc;
> +source include/have_innodb.inc;
> +source include/have_binlog_format_statement.inc;
> +
> +SET @old_isolation_level= @@tx_isolation;
> +SET tx_isolation= 'READ-COMMITTED';
> +
> +let $engine= InnoDB;
> +
> +CREATE DATABASE db_not_filtered;
> +use db_not_filtered;
> +eval CREATE TABLE t1 (x int, y int) engine=$engine;
> +eval CREATE TABLE t2 (x int, y int) engine=$engine;
> +
> +CREATE DATABASE db_filtered;
> +use db_filtered;
> +eval CREATE TABLE t1 (x int, y int) engine=$engine;
> +eval CREATE TABLE t2 (x int, y int) engine=$engine;
> +
> +INSERT INTO t2 VALUES (1,2);
> +INSERT INTO t1 SELECT * FROM t2;
> +
> +use db_not_filtered;
> +--error 1598
> +INSERT INTO t2 VALUES (1,2);
> +--error 1598
> +INSERT INTO t1 SELECT * FROM t2;
> +
> +DROP DATABASE db_not_filtered;
> +DROP DATABASE db_filtered;
> +
> +source include/show_binlog_events.inc;
> +
> +SET @@tx_isolation= @old_isolation_level;
>
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2009-03-27 12:55:14 +0000
> +++ b/sql/sql_base.cc	2009-04-23 11:31:32 +0000
> @@ -5089,7 +5089,19 @@ static void mark_real_tables_as_free_for
>  
>  int decide_logging_format(THD *thd, TABLE_LIST *tables)
>  {
> -  if (mysql_bin_log.is_open() && (thd->options & OPTION_BIN_LOG))
> +  /*
> +    Only deciding if:
> +
> +     1. binlog is open and option bin log is set
> +     
> +     2. AND statement is not filtered out from binlog OR binlog format
> +     is mixed (meaning that filter rules may present different results
> +     based on the logging format choosen later - see BUG#43457 for
> +     details).
> +   */
> +  if (mysql_bin_log.is_open() && (thd->options & OPTION_BIN_LOG)
> && 
> +      (thd->variables.binlog_format == BINLOG_FORMAT_MIXED || 
> +       binlog_filter->db_ok(thd->db)))
>    {
>      /*
>        Compute the starting vectors for the computations by creating a
>
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc	2009-03-24 13:58:52 +0000
> +++ b/sql/sql_class.cc	2009-04-23 11:31:32 +0000
> @@ -2894,6 +2894,11 @@ extern "C" void thd_mark_transaction_to_
>  {
>    mark_transaction_to_rollback(thd, all);
>  }
> +
> +extern "C" bool thd_binlog_filter_ok(const MYSQL_THD thd)
> +{
> +  return binlog_filter->db_ok(thd->db);
> +}
>  #endif // INNODB_COMPATIBILITY_HOOKS */
>  
>  /****************************************************************************
>
> === modified file 'storage/innobase/handler/ha_innodb.cc'
> --- a/storage/innobase/handler/ha_innodb.cc	2009-03-24 13:58:52 +0000
> +++ b/storage/innobase/handler/ha_innodb.cc	2009-04-23 11:31:32 +0000
> @@ -6810,8 +6810,10 @@ ha_innobase::external_lock(
>  	{
>  		ulong const binlog_format= thd_binlog_format(thd);
>  		ulong const tx_isolation = thd_tx_isolation(current_thd);
> +                bool const is_binlog_filter_ok= thd_binlog_filter_ok(thd);
>  		if (tx_isolation <= ISO_READ_COMMITTED &&
> -		    binlog_format == BINLOG_FORMAT_STMT)
> +		    binlog_format == BINLOG_FORMAT_STMT &&
> +                    is_binlog_filter_ok)
>  		{
>  			char buf[256];
>  			my_snprintf(buf, sizeof(buf),
>
> === modified file 'storage/innobase/handler/ha_innodb.h'
> --- a/storage/innobase/handler/ha_innodb.h	2008-12-14 20:59:50 +0000
> +++ b/storage/innobase/handler/ha_innodb.h	2009-04-23 11:31:32 +0000
> @@ -252,4 +252,11 @@ int thd_binlog_format(const MYSQL_THD th
>    @param  all   TRUE <=> rollback main transaction.
>  */
>  void thd_mark_transaction_to_rollback(MYSQL_THD thd, bool all);
> +
> +/**
> +  Check if binary logging is filtered for thread's current db.
> +  @param  thd   Thread handle
> +  @retval 1 the query is not filtered, 0 otherwise.
> +*/
> +bool thd_binlog_filter_ok(const MYSQL_THD thd);
>  }
>
>   
> ------------------------------------------------------------------------
>
>
>   

Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:2840) Bug#42829Luis Soares23 Apr
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2840)Bug#42829He Zhenxing27 Apr
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2840)Bug#42829Alfranio Correia17 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2840)Bug#42829Alfranio Correia17 Jun
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2840)Bug#42829Luís Soares17 Jun