Hi Dao-Gang,
Thanks for fixing this. You did a good job, but I think some more work
is needed, especially with the test case. Some comments:
(C1) In the test case, I think we should test to change binlog_format in
the following scenarios:
(1) with AUTOCOMMIT=1, outside a transaction
(2) right after BEGIN
(3) after BEGIN + non-transactional update
(4) after BEGIN + transactional update
(5) with AUTOCOMMIT=0, right after COMMIT
(6) with AUTOCOMMIT=0, after COMMIT + non-transactional update
(7) with AUTOCOMMIT=0, after COMMIT + transactional update
For each case above, I think we should test both @@global.binlog_format
and @@session.binlog_format. I think there should be an error for
@@session.binlog_format in case (3), (4), (6), (7), but not (1), (2),
(5). I think there should never be any error for @global.binlog_format.
(C2) The test does not have to be a replication test, we don't check
anything on the slave. I suggest to move the test to the suite/binlog.
(C3) Suggest simplification of the test case - see comment inline.
(C4) Please use named constant instead of numeric error code in test
case - see comment inline.
(C5) I suggest to only make the session variable read-only in
transactions. The global variable can still be changed. See details inline.
(C6) I suggest a small rewording of the error message.
/Sven
Dao-Gang.Qu@stripped wrote:
> #At file:///home/daogangqu/mysql/bzrwork/bug47863/mysql-5.1-rep%2B3/ based on
> revid:aelkin@stripped
>
> 3125 Dao-Gang.Qu@stripped 2009-12-14
> Bug #47863 binlog_format should be writable only at transaction boundaries
>
> When @@session.binlog_format is modified inside a transaction,
> it can cause slave to go out of sync.
>
> To fix the problem, make the session variable 'binlog_format'
> read-only inside a transaction.
> @ mysql-test/suite/rpl/r/rpl_binlog_format.result
> Test result for bug#47863.
> @ mysql-test/suite/rpl/t/rpl_binlog_format.test
> Added the test file to verify if the session variable 'binlog_format'
> is read-only inside a transaction and in sub-statements.
> @ sql/set_var.cc
> Added code to make the session variable 'binlog_format'
> read-only inside a transaction.
>
> added:
> mysql-test/suite/rpl/r/rpl_binlog_format.result
> mysql-test/suite/rpl/t/rpl_binlog_format.test
> modified:
> sql/set_var.cc
> sql/share/errmsg.txt
> === added file 'mysql-test/suite/rpl/r/rpl_binlog_format.result'
> --- a/mysql-test/suite/rpl/r/rpl_binlog_format.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_binlog_format.result 2009-12-14 06:20:05 +0000
> @@ -0,0 +1,78 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +call mtr.add_suppression("Unsafe statement binlogged in statement format "
> + "since BINLOG_FORMAT .* STATEMENT. Reason for "
> + "unsafeness: Non-transactional reads or writes "
> + "are unsafe if they occur after transactional "
> + "reads or writes inside a transaction.");
> +create table t1 (a int) engine= myisam;
> +create table t2 (a int) engine= innodb;
> +set @@session.binlog_format= statement;
> +# The value of 'binlog_format' is changed to STATEMENT.
> +insert into t1 values (1);
> +begin;
> +insert into t2 select * from t1;
> +Warnings:
> +Note 1592 Unsafe statement binlogged in statement format since BINLOG_FORMAT =
> STATEMENT. Reason for unsafeness: Non-transactional reads or writes are unsafe if they
> occur after transactional reads or writes inside a transaction.
> +# Check the session variable 'binlog_format' is read-only
> +# inside a transaction.
> +set @@global.binlog_format= row;
> +ERROR HY000: Cannot change binary logging format while a transaction is in progress
> +insert into t1 values (2);
> +Warnings:
> +Note 1592 Unsafe statement binlogged in statement format since BINLOG_FORMAT =
> STATEMENT. Reason for unsafeness: Non-transactional reads or writes are unsafe if they
> occur after transactional reads or writes inside a transaction.
> +commit;
> +begin;
> +insert into t2 values (3);
> +# Check the session variable 'binlog_format' is read-only
> +# inside a transaction.
> +set @@global.binlog_format= row;
> +ERROR HY000: Cannot change binary logging format while a transaction is in progress
> +insert into t2 values (4);
> +commit;
> +# The value of 'binlog_format' is not changed to ROW.
> +select * from t2;
> +a
> +1
> +3
> +4
> +select * from t1;
> +a
> +1
> +2
> +select * from t2;
> +a
> +1
> +3
> +4
> +select * from t1;
> +a
> +1
> +2
> +create table t3(a int, b int) engine= innodb;
> +create table t4(a int) engine= innodb;
> +create table t5(a int) engine= innodb;
> +create trigger tr2 after insert on t3 for each row begin
> +insert into t4(a) values(1);
> +set @@session.binlog_format= row;
> +insert into t4(a) values(2);
> +insert into t5(a) values(3);
> +end |
> +# Check the session variable 'binlog_format' is read-only
> +# in sub-statements.
> +insert into t3(a,b) values(1,1);
> +ERROR HY000: Cannot change the binary logging format inside a stored function or
> trigger
> +# The value of 'binlog_format' is not changed to ROW.
> +select * from t4;
> +a
> +select * from t4;
> +a
> +drop table t1;
> +drop table t2;
> +drop table t3;
> +drop table t4;
> +drop table t5;
>
> === added file 'mysql-test/suite/rpl/t/rpl_binlog_format.test'
> --- a/mysql-test/suite/rpl/t/rpl_binlog_format.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_binlog_format.test 2009-12-14 06:20:05 +0000
> @@ -0,0 +1,94 @@
> +#
> +# BUG#47863
> +# This test verifies if the session variable 'binlog_format'
> +# is read-only inside a transaction and in sub-statements.
> +#
> +
> +source include/master-slave.inc;
> +source include/have_innodb.inc;
> +source include/have_binlog_format_row.inc;
> +
> +call mtr.add_suppression("Unsafe statement binlogged in statement format "
> + "since BINLOG_FORMAT .* STATEMENT. Reason for "
> + "unsafeness: Non-transactional reads or writes "
> + "are unsafe if they occur after transactional "
> + "reads or writes inside a transaction.");
> +create table t1 (a int) engine= myisam;
> +create table t2 (a int) engine= innodb;
> +
> +set @@session.binlog_format= statement;
> +if (`SELECT @@session.binlog_format = 'STATEMENT'`)
> +{
> + --echo # The value of 'binlog_format' is changed to STATEMENT.
> +}
(C3) I think the I think the 'if' construct is too complicated - it's
more clear to just execute 'SELECT @@session.binlog_format'.
> +
> +insert into t1 values (1);
> +
> +begin;
> + insert into t2 select * from t1;
> +--echo # Check the session variable 'binlog_format' is read-only
> +--echo # inside a transaction.
> +--error 1658
(C4) Please use named constants instead of numeric error codes:
--error ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_BINLOG_FORMAT
(same suggestion at two places below)
> + set @@global.binlog_format= row;
> + insert into t1 values (2);
> +commit;
> +
> +begin;
> + insert into t2 values (3);
> +--echo # Check the session variable 'binlog_format' is read-only
> +--echo # inside a transaction.
> +--error 1658
(C4) Please use named constant (see above).
> + set @@global.binlog_format= row;
> + insert into t2 values (4);
> +commit;
> +
> +if (`SELECT @@session.binlog_format = 'STATEMENT'`)
> +{
> + --echo # The value of 'binlog_format' is not changed to ROW.
> +}
(C3) Please execute 'SELECT @@session.binlog_format' instead (see above).
> +
> +select * from t2;
> +select * from t1;
> +
> +sync_slave_with_master;
> +connection slave;
> +select * from t2;
> +select * from t1;
> +
> +connection master;
> +create table t3(a int, b int) engine= innodb;
> +create table t4(a int) engine= innodb;
> +create table t5(a int) engine= innodb;
> +delimiter |;
> +eval create trigger tr2 after insert on t3 for each row begin
> + insert into t4(a) values(1);
> + set @@session.binlog_format= row;
> + insert into t4(a) values(2);
> + insert into t5(a) values(3);
> +end |
> +delimiter ;|
> +
> +--echo # Check the session variable 'binlog_format' is read-only
> +--echo # in sub-statements.
> +--error 1560
(C4) Please use named constant (see above).
> +insert into t3(a,b) values(1,1);
> +
> +if (`SELECT @@session.binlog_format = 'STATEMENT'`)
> +{
> + --echo # The value of 'binlog_format' is not changed to ROW.
> +}
(C3) Please execute 'SELECT @@session.binlog_format' instead (see above).
> +
> +select * from t4;
> +
> +sync_slave_with_master;
> +connection slave;
> +select * from t4;
> +
> +connection master;
> +drop table t1;
> +drop table t2;
> +drop table t3;
> +drop table t4;
> +drop table t5;
> +sync_slave_with_master;
> +
>
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc 2009-11-30 18:20:26 +0000
> +++ b/sql/set_var.cc 2009-12-14 06:20:05 +0000
> @@ -1288,6 +1288,14 @@ bool sys_var_thd_binlog_format::is_reado
> my_error(ER_STORED_FUNCTION_PREVENTS_SWITCH_BINLOG_FORMAT, MYF(0));
> return 1;
> }
> + /*
> + Make the session variable 'binlog_format' read-only inside a transaction.
> + */
> + if (thd->active_transaction())
> + {
> + my_error(ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_BINLOG_FORMAT, MYF(0));
> + return 1;
> + }
> return sys_var_thd_enum::is_readonly();
> }
(C5) This will make both the global and the session variable read-only.
I think it would be enough to make the session variable read-only. I
think we can do that by using the function
sys_var_thd_binlog_format::check(THD *thd, set_var *var)
in set_var.cc instead of is_readonly(). Then you can generate an error
only if var->type==OPT_SESSION.
>
>
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2009-11-30 18:20:26 +0000
> +++ b/sql/share/errmsg.txt 2009-12-14 06:20:05 +0000
> @@ -6249,3 +6249,5 @@ ER_DEBUG_SYNC_TIMEOUT
> ER_DEBUG_SYNC_HIT_LIMIT
> eng "debug sync point hit limit reached"
> ger "Debug Sync Point Hit Limit erreicht"
> +ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_BINLOG_FORMAT
> + eng "Cannot change binary logging format while a transaction is in progress"
(C6) I suggest a small change in wording: "Cannot modify
@@session.binlog_format inside a transaction".
("@@session.binlog_format" is more specific than "binary logging
format". "inside a transaction" states more clearly that the transaction
and the change of binlog format are in the same thread - "while a
transaction is in progress" could mean that another client executes a
transaction).
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
