Hi Daogang,
Good work, patch approved!
/Sven
Daogang Qu wrote:
> Hi Sven,
> Thanks for these good comments.
> Please review the newest patch.
>
> http://lists.mysql.com/commits/97479
>
> Best Regards,
>
> Daogang
>
> Sven Sandberg wrote:
>> Hi Dao-Gang,
>>
>> Thanks for fixing this. Some comments:
>>
>> - I think your patch works, and it even fixes BUG#45856. The test
>> case is good too, and it actually also tests BUG#45856.
>>
>> - I think the check should not be done in sys_var_thd_enum::update(),
>> that's a member function of the more generic super class. It would be
>> better to check the condition in sys_var_thd_binlog_format::check()
>> (and get rid of the 'offset == &system_variables::binlog_format' test).
>>
>> - I suggest some changes to comments, both in the test case and in
>> the code (see comments inline).
>>
>> /Sven
>>
>>
>> Dao-Gang.Qu@stripped wrote:
>>> #At file:///home/daogangqu/mysql/bzrwork/bug45855/mysql-5.1-bugteam/
>>> based on revid:joro@stripped
>>>
>>> 3316 Dao-Gang.Qu@stripped 2010-01-19
>>> Bug #45855 row events in binlog after switch from
>>> binlog_fmt=mix to stmt with open tmp tbl
>>> If binlog_format=MIXED, there are open temporary tables,
>>> an unsafe statement
>>> is executed, and the user issues 'SET @@session.binlog_format =
>>> STATEMENT',
>>> then subsequent DML statements will be written in row format
>>> despite binlog_format=STATEMENT. Because the binlog format
>>> can't be reset to
>>> statement based by 'reset_current_stmt_binlog_row_based' function.
>>> To fix the problem, generate
>>> ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR also
>>> and forbid switching from MIXED to STATEMENT when there are
>>> open temp tables
>>> and we are logging in row based format.
>>> @
>>> mysql-test/suite/rpl/r/rpl_binlog_format_switch_in_tmp_table.result
>>> Test result for Bug#45855.
>>> @ mysql-test/suite/rpl/t/rpl_binlog_format_switch_in_tmp_table.test
>>> Added the test file to verify if the program will generate
>>> ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR error and forbid
>>> switching from MIXED to STATEMENT when there are open temp
>>> tables and we are logging in row based format.
>>>
>>> added:
>>>
>>> mysql-test/suite/rpl/r/rpl_binlog_format_switch_in_tmp_table.result
>>> mysql-test/suite/rpl/t/rpl_binlog_format_switch_in_tmp_table.test
>>> modified:
>>> sql/set_var.cc
>>> === added file
>>> 'mysql-test/suite/rpl/r/rpl_binlog_format_switch_in_tmp_table.result'
>>> ---
>>> a/mysql-test/suite/rpl/r/rpl_binlog_format_switch_in_tmp_table.result
>>> 1970-01-01 00:00:00 +0000
>>> +++
>>> b/mysql-test/suite/rpl/r/rpl_binlog_format_switch_in_tmp_table.result
>>> 2010-01-19 03:44:37 +0000
>>> @@ -0,0 +1,62 @@
>>> +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;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +MIXED
>>> +CREATE TABLE t1 (a VARCHAR(100));
>>> +CREATE TEMPORARY TABLE t2 (a VARCHAR(100));
>>> +# Test allow switching from MIXED to STATEMENT when there are open temp
>>> +# tables and we are logging in statement based format.
>>> +SET SESSION binlog_format = STATEMENT;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +STATEMENT
>>> +INSERT INTO t1 VALUES ('statement based');
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +STATEMENT
>>> +# Test allow switching from STATEMENT to MIXED when there are open
>>> temp tables.
>>> +SET SESSION binlog_format = MIXED;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +MIXED
>>> +INSERT INTO t2 VALUES (UUID());
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +MIXED
>>> +# Test forbit switching from MIXED to STATEMENT when there are open
>>> temp
>>> +# tables and we are logging in row based format.
>>> +SET SESSION binlog_format = STATEMENT;
>>> +ERROR HY000: Cannot switch out of the row-based binary log format
>>> when the session has open temporary tables
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +MIXED
>>> +SET SESSION binlog_format = ROW;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +ROW
>>> +INSERT INTO t1 VALUES ('row based');
>>> +# Test allow switching from ROW to MIXED when there are open temp
>>> tables.
>>> +SET SESSION binlog_format = MIXED;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +MIXED
>>> +INSERT INTO t1 VALUES ('row based');
>>> +# Test allow switching from MIXED to ROW when there are open temp
>>> tables.
>>> +SET SESSION binlog_format = ROW;
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +ROW
>>> +INSERT INTO t1 VALUES ('row based');
>>> +# Test forbit switching from ROW to STATEMENT when there are open
>>> temp tables.
>>> +SET SESSION binlog_format = STATEMENT;
>>> +ERROR HY000: Cannot switch out of the row-based binary log format
>>> when the session has open temporary tables
>>> +SELECT @@SESSION.binlog_format;
>>> +@@SESSION.binlog_format
>>> +ROW
>>> +DROP TEMPORARY TABLE t2;
>>> +DROP TABLE t1;
>>>
>>> === added file
>>> 'mysql-test/suite/rpl/t/rpl_binlog_format_switch_in_tmp_table.test'
>>> ---
>>> a/mysql-test/suite/rpl/t/rpl_binlog_format_switch_in_tmp_table.test
>>> 1970-01-01 00:00:00 +0000
>>> +++
>>> b/mysql-test/suite/rpl/t/rpl_binlog_format_switch_in_tmp_table.test
>>> 2010-01-19 03:44:37 +0000
>>> @@ -0,0 +1,58 @@
>>> +#
>>> +# Bug #45855
>>
>> Please write out the bug title, like:
>>
>> BUG#45855: row events in binlog after switch from binlog_fmt=mix to
>> stmt with open tmp tbl
>>
>> Also, since you have fixed BUG#45856 too, please write that here too.
>>
>>> +# This test verfies if the program will generate
>>> ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR
>>> +# error and forbid switching from MIXED to STATEMENT when there are
>>> open temp
>>> +# tables and we are logging in row based format.
>>
>> We test more than that:
>> - there is an error when switching from MIXED or ROW to STATEMENT
>> - there is no error in any other case.
>>
>>> +#
>>> +
>>> +source include/master-slave.inc;
>>> +source include/have_binlog_format_mixed.inc;
>>> +
>>> +SELECT @@SESSION.binlog_format;
>>> +CREATE TABLE t1 (a VARCHAR(100));
>>> +CREATE TEMPORARY TABLE t2 (a VARCHAR(100));
>>> +
>>> +--echo # Test allow switching from MIXED to STATEMENT when there are
>>> open temp
>>> +--echo # tables and we are logging in statement based format.
>>> +SET SESSION binlog_format = STATEMENT;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +INSERT INTO t1 VALUES ('statement based');
>>> +SELECT @@SESSION.binlog_format;
>>> +--echo # Test allow switching from STATEMENT to MIXED when there are
>>> open temp tables.
>>> +SET SESSION binlog_format = MIXED;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +INSERT INTO t2 VALUES (UUID());
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +--echo # Test forbit switching from MIXED to STATEMENT when there
>>> are open temp
>>> +--echo # tables and we are logging in row based format.
>>> +--ERROR ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR
>>> +SET SESSION binlog_format = STATEMENT;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +SET SESSION binlog_format = ROW;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +INSERT INTO t1 VALUES ('row based');
>>> +--echo # Test allow switching from ROW to MIXED when there are open
>>> temp tables.
>>> +SET SESSION binlog_format = MIXED;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +INSERT INTO t1 VALUES ('row based');
>>> +--echo # Test allow switching from MIXED to ROW when there are open
>>> temp tables.
>>> +SET SESSION binlog_format = ROW;
>>> +SELECT @@SESSION.binlog_format;
>>> +
>>> +INSERT INTO t1 VALUES ('row based');
>>> +--echo # Test forbit switching from ROW to STATEMENT when there are
>>> open temp tables.
>>> +--ERROR ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR
>>> +SET SESSION binlog_format = STATEMENT;
>>> +SELECT @@SESSION.binlog_format;
>>
>> For completeness, I suggest that you also test to execute
>> 'SET binlog_format = STATEMENT' when binlog_format is already
>> STATEMENT. (And similar for row and mixed.)
>>
>>> +
>>> +DROP TEMPORARY TABLE t2;
>>> +DROP TABLE t1;
>>> +
>>> +SYNC_SLAVE_WITH_MASTER;
>>> +
>>>
>>> === modified file 'sql/set_var.cc'
>>> --- a/sql/set_var.cc 2009-12-17 11:45:13 +0000
>>> +++ b/sql/set_var.cc 2010-01-19 03:44:37 +0000
>>> @@ -1274,23 +1274,6 @@ bool sys_var_thd_binlog_format::is_reado
>>> */
>>> THD *thd= current_thd;
>>> /*
>>> - If RBR and open temporary tables, their CREATE TABLE may not be
>>> in the
>>> - binlog, so we can't toggle to SBR in this connection.
>>> - The test below will also prevent SET GLOBAL, well it was not
>>> easy to test
>>> - if global or not here.
>>> - And this test will also prevent switching from RBR to RBR (a
>>> no-op which
>>> - should not happen too often).
>>> -
>>> - If we don't have row-based replication compiled in, the variable
>>> - is always read-only.
>>> - */
>>> - if ((thd->variables.binlog_format == BINLOG_FORMAT_ROW) &&
>>> - thd->temporary_tables)
>>> - {
>>> - my_error(ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR, MYF(0));
>>> - return 1;
>>> - }
>>> - /*
>>> if in a stored function/trigger, it's too late to change mode
>>> */
>>> if (thd->in_sub_stmt)
>>> @@ -1910,7 +1893,32 @@ bool sys_var_thd_enum::update(THD *thd, if
>>> (var->type == OPT_GLOBAL)
>>> global_system_variables.*offset= var->save_result.ulong_value;
>>> else
>>> + {
>>> + /*
>>> + If RBR and open temporary tables, their CREATE TABLE may not
>>> be in the
>>> + binlog, so we can't toggle to SBR in this connection.
>>> +
>>> + If binlog_format=MIXED, there are open temporary tables, an
>>> unsafe
>>> + statement is executed, and the user issues 'SET
>>> @@session.binlog_format
>>> + = STATEMENT', then subsequent DML statements will be written
>>> in row
>>> + format despite binlog_format=STATEMENT. So forbid switching
>>> from MIXED
>>> + to STATEMENT when there are open temp tables and we are
>>> logging in row
>>> + based format.
>>
>> After your patch, it is not true that subsequent DML statements will
>> be written in row format. So the comment has to be changed slightly. I
>> would suggest something like: "If binlog_format=MIXED, there are open
>> temporary tables, and an unsafe statement is executed, then subsequent
>> statements are logged in row format and hence changes to temporary
>> tables may be lost. So we forbid switching from MIXED to STATEMENT
>> when there are open temp tables and we are logging in row format.
>>
>>> +
>>> + If we don't have row-based replication compiled in, the variable
>>> + is always read-only.
>>
>> I think this comment is out of date: row-based replication is always
>> compiled in, so the comment can be removed.
>>
>>> + */
>>> + if (thd->temporary_tables && offset ==
>>> &system_variables::binlog_format &&
>>> + var->save_result.ulong_value == BINLOG_FORMAT_STMT &&
>>> + ((thd->variables.binlog_format == BINLOG_FORMAT_MIXED &&
>>> + thd->current_stmt_binlog_row_based) ||
>>> + thd->variables.binlog_format == BINLOG_FORMAT_ROW))
>>> + {
>>> + my_error(ER_TEMP_TABLE_PREVENTS_SWITCH_OUT_OF_RBR, MYF(0));
>>> + return 1;
>>> + }
>>> thd->variables.*offset= var->save_result.ulong_value;
>>> + }
>>> return 0;
>>> }
>>>
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>
>>
>
>
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
