List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:January 20 2010 9:03am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3316)
Bug#45855
View as plain text  
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
Thread
bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3316) Bug#45855Dao-Gang.Qu19 Jan
  • Re: bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3316)Bug#45855Sven Sandberg19 Jan
    • Re: bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3316)Bug#45855Daogang Qu20 Jan
      • Re: bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3316)Bug#45855Sven Sandberg20 Jan