Hi Libing,
Great work.
I agree with all the comments made by Luis.
Cheers.
Luís Soares wrote:
> Hi Li-Bing,
>
> Nice Work. Please find my review comments below. I think we
> just need some changes on where to place the test case. Then I
> think it is ok to push.
>
> By the way, nice that you spotted the case for the extra DROP
> TEMPORARY TABLE IF EXISTS when in RBL and no existed.
>
> STATUS
> ------
>
> Not Approved.
>
> REQUIRED CHANGES
> ----------------
>
> RC1. I think this test case should be moved to the binlog
> suite. In fact, I think we can add it to the existing file
> binlog_row_drop_tmp_tbl.test . Any objections?
>
> RC2. Please add some more comments to the test case (for
> instance before the seconds drops to state what the test
> is checking for).
>
> RC3. Rewrite the commit message in a more clear way. I can't
> really parse it ;).
>
> REQUESTS
> --------
> n/a
>
> SUGGESTIONS
> -----------
> n/a
>
> DETAILS
> -------
> See some comments inline.
>
> On Mon, 2009-09-21 at 07:30 +0000, Li-Bing.Song@stripped wrote:
>
>> #At file:///home/anders/work/bzrwork/July/bug45574/mysql-5.1-bugteam/ based on
> revid:joro@stripped
>>
>> 3117 Li-Bing.Song@stripped 2009-09-21
>> BUG #46572 DROP temp-table IF EXISTS does not have a consistent behavior in
> ROW mode
>>
>> In RBR, 'DROP TEMPORARY TABLE IF EXISTS...' statement is binlogged when the
> table
>> is not exists.
>> In RBR, 'DROP TEMPORARY TABLE ...' statement is never binlogged.
>> This patch add an extra condition to fix the logic.
>>
>
>
> This message is a bit obscure. Can you please rewrite it in a more clear
> way?
>
>
>> added:
>> mysql-test/suite/rpl/r/rpl_drop_temp_row.result
>> mysql-test/suite/rpl/t/rpl_drop_temp_row.test
>> modified:
>> sql/sql_table.cc
>> === added file 'mysql-test/suite/rpl/r/rpl_drop_temp_row.result'
>> --- a/mysql-test/suite/rpl/r/rpl_drop_temp_row.result 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_drop_temp_row.result 2009-09-21 07:30:43 +0000
>> @@ -0,0 +1,32 @@
>> +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;
>> +DROP TABLE IF EXISTS tmp;
>> +DROP TABLE IF EXISTS tmp1;
>> +DROP TABLE IF EXISTS tmp2;
>> +DROP TABLE IF EXISTS tmp3;
>> +DROP TABLE IF EXISTS t;
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp3(c1 int);
>> +CREATE TABLE t(c1 int);
>> +DROP TEMPORARY TABLE IF EXISTS tmp;
>> +DROP TEMPORARY TABLE IF EXISTS tmp;
>> +DROP TEMPORARY TABLE IF EXISTS tmp, tmp1;
>> +DROP TABLE IF EXISTS tmp2, t;
>> +DROP TABLE IF EXISTS tmp2, t;
>> +DROP TEMPORARY TABLE tmp3;
>> +show binlog events from <binlog_start>;
>> +Log_name Pos Event_type Server_id End_log_pos Info
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS tmp
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS tmp1
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS tmp2
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS tmp3
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS t
>> +master-bin.000001 # Query # # use `test`; CREATE TABLE t(c1 int)
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS `t` /* generated
> by server */
>> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS tmp2, t
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_drop_temp_row.test'
>> --- a/mysql-test/suite/rpl/t/rpl_drop_temp_row.test 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_drop_temp_row.test 2009-09-21 07:30:43 +0000
>> @@ -0,0 +1,32 @@
>> +#############################################################################
>> +# BUG#46572 DROP temp-table IF EXISTS does not have a consistent behavior in
>> +# ROW mode
>> +#
>> +# In RBR, 'DROP TEMPORARY TABLE ...' statement is always not binlogged.
>> +#############################################################################
>> +source include/master-slave.inc;
>> +source include/have_binlog_format_row.inc;
>> +disable_warnings;
>> +
>> +DROP TABLE IF EXISTS tmp;
>> +DROP TABLE IF EXISTS tmp1;
>> +DROP TABLE IF EXISTS tmp2;
>> +DROP TABLE IF EXISTS tmp3;
>> +DROP TABLE IF EXISTS t;
>> +
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2(c1 int);
>> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp3(c1 int);
>> +CREATE TABLE t(c1 int);
>> +
>> +DROP TEMPORARY TABLE IF EXISTS tmp;
>> +DROP TEMPORARY TABLE IF EXISTS tmp;
>> +DROP TEMPORARY TABLE IF EXISTS tmp, tmp1;
>> +DROP TABLE IF EXISTS tmp2, t;
>> +DROP TABLE IF EXISTS tmp2, t;
>> +DROP TEMPORARY TABLE tmp3;
>> +
>> +source include/show_binlog_events.inc;
>> +
>> +source include/master-slave-end.inc;
>>
>> === modified file 'sql/sql_table.cc'
>> --- a/sql/sql_table.cc 2009-09-18 13:01:18 +0000
>> +++ b/sql/sql_table.cc 2009-09-21 07:30:43 +0000
>> @@ -1949,7 +1949,7 @@ int mysql_rm_table_part2(THD *thd, TABLE
>> being built. The string always end in a comma and the comma
>> will be chopped off before being written to the binary log.
>> */
>> - if (thd->current_stmt_binlog_row_based && !dont_log_query)
>> + if (!drop_temporary && thd->current_stmt_binlog_row_based
> && !dont_log_query)
>> {
>> non_temp_tables_count++;
>> /*
>>
>>
