Hi Jasonh,
He Zhenxing wrote:
> Alfranio Correia wrote:
>> Hi all,
>>
>> Daogang Qu wrote:
>>> Libing Song wrote:
>>>> Hi Daogang,
>>>>
>>>>
>>>> Nice Work. Please find my review comments below.
>>>>
>>>> STATUS
>>>> ------
>>>> Not approved.
>>>>
>>>> REQUIRED CHANGES
>>>> ----------------
>>>>
>>>> RC1. I think the following patch is better.
>>>>
>>>> === modified file 'sql/sql_class.h'
>>>> --- sql/sql_class.h 2009-12-02 13:49:21 +0000
>>>> +++ sql/sql_class.h 2009-12-29 09:29:10 +0000
>>>> @@ -2213,7 +2213,8 @@
>>>> ("temporary_tables: %s, in_sub_stmt: %s, system_thread:
>>>> %s",
>>>> YESNO(temporary_tables), YESNO(in_sub_stmt),
>>>> show_system_thread(system_thread)));
>>>> - if ((temporary_tables == NULL) && (in_sub_stmt == 0)
> &&
>>>> + if ((temporary_tables == NULL || !current_stmt_binlog_row_based)
>>>>
>>> Good. Maybe the following is better:
>>>
>>> - if ((temporary_tables == NULL) && (in_sub_stmt == 0) &&
>>> + if ((temporary_tables == NULL || (!current_stmt_binlog_row_based
> &&
>>> + variables.binlog_format == BINLOG_FORMAT_ROW)) &&
> (in_sub_stmt
>>> == 0) &&
>>> (system_thread != SYSTEM_THREAD_NDBCLUSTER_BINLOG))
>>
>> Although I said in the IRC that it was possible to restore the correct value in
> the
>> reset_current_stmt_binlog_row_based, I was wrong. It is not possible.
>> Consider for instance the following example:
>>
>> --source include/master-slave.inc
>> --source include/have_binlog_format_mixed.inc
>>
>> CREATE TABLE t (a int);
>>
>> INSERT INTO t VALUES(1), (2);
>> CREATE TEMPORARY TABLE tt (a int, b int) SELECT *, UUID() from t;
>> INSERT INTO t VALUES(3), (4);
>>
>> DELIMITER |;
>> CREATE PROCEDURE p1() BEGIN SELECT 1 ; END |
>> DELIMITER ;|
>>
>> INSERT INTO tt(a) VALUES(1), (2);
>> INSERT INTO t VALUES(5), (6);
>>
>> DROP TABLE t;
>>
>> SHOW BINLOG EVENTS;
>>
>> sync_slave_with_master;
>>
>> exit;
>>
>>
>> WRONG RESULT:
>> -------------
>>
>> Log_name Pos Event_type Server_id End_log_pos Info
>> master-bin.000001 4 Format_desc 1 106 Server ver:
> 5.1.42-debug-log, Binlog ver: 4
>> master-bin.000001 106 Query 1 191 use `test`; CREATE TABLE
> t (a int)
>> master-bin.000001 191 Query 1 282 use `test`; INSERT INTO t
> VALUES(1), (2)
>> master-bin.000001 282 Query 1 350 BEGIN
>> master-bin.000001 350 Table_map 1 390 table_id: 23
> (test.t)
>> master-bin.000001 390 Write_rows 1 429 table_id: 23
> flags: STMT_END_F
>> master-bin.000001 429 Query 1 498 COMMIT
>> master-bin.000001 498 Query 1 633 use `test`; CREATE
> DEFINER=`root`@`localhost` PROCEDURE `p1`()
>> BEGIN SELECT 1 ; END
>> master-bin.000001 633 Query 1 728 use `test`; INSERT INTO
> tt(a) VALUES(1), (2)
>> master-bin.000001 728 Query 1 819 use `test`; INSERT INTO t
> VALUES(5), (6)
>> master-bin.000001 819 Query 1 894 use `test`; DROP TABLE t
>> master-bin.000001 894 Query 1 1004 use `test`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `tt`
>>
>>
>> RIGHT RESULT:
>> -------------
>>
>> master-bin.000001 4 Format_desc 1 106 Server ver:
> 5.1.42-debug-log, Binlog ver: 4
>> master-bin.000001 106 Query 1 191 use `test`; CREATE TABLE
> t (a int)
>> master-bin.000001 191 Query 1 282 use `test`; INSERT INTO t
> VALUES(1), (2)
>> master-bin.000001 282 Query 1 350 BEGIN
>> master-bin.000001 350 Table_map 1 390 table_id: 23
> (test.t)
>> master-bin.000001 390 Write_rows 1 429 table_id: 23
> flags: STMT_END_F
>> master-bin.000001 429 Query 1 498 COMMIT
>> master-bin.000001 498 Query 1 633 use `test`; CREATE
> DEFINER=`root`@`localhost` PROCEDURE `p1`()
>> BEGIN SELECT 1 ; END
>> master-bin.000001 282 Query 1 350 BEGIN
>> master-bin.000001 350 Table_map 1 390 table_id: 23
> (test.t)
>> master-bin.000001 390 Write_rows 1 429 table_id: 23
> flags: STMT_END_F
>> master-bin.000001 429 Query 1 498 COMMIT
>> master-bin.000001 894 Query 1 1004 use `test`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `tt`
>>
>>
>>
>> It is not possible to figure out what is the current value.
>> In this case, after creating the procedure current_stmt_binlog_row_based
>> should be TRUE.
>>
>> So, either
>>
>> 1 - we create a "global" variable to save the current value and restore it
>> when necessary.
>>
>> 2 - we create a "local" variable to save the current value and restore it.
>>
>
> I think it's a good principle to do save/change/restore within the same
> function, so that we are sure that the function will not have unexpected
> side effects. Unexpected side effects of functions are hard to
> understand and usually are the source of bugs.
>
>> 3 - we force the current_stmt_binlog_row_based to be TRUE if there is
>> a temporary table and the format is MIXED.
>>
>>
>> Note that the current patch and several other places in the code are wrong
>> as the value should be saved and restored.
>>
>
> Agree, and please also test the problem within stored
> functions/procuders (in_sub_stmt), and NDB engine
> (SYSTEM_THREAD_NDBCLUSTER_BINLOG). And make sure the problem is fixed
> for these cases too.
I agree.
Cheers.
>
>> Cheers.
>>
>>
>>
>>>
