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++;
> /*
>
--
Luís
