Hi Mats,
Thanks for the review.
See some comments in-line.
Cheers.
Mats Kindahl wrote:
> Hi Alfranio,
>
> Patch looks fine, I have some minor comments below. Patch is approved (I'll set
> the status) provided you add the locks we discussed to guarantee that the DROP
> statements are written to the binary log when the threads are closed.
>
I will do that.
> With regard to the comments below, it is up to you to include the suggested changes.
>
> Just my few cents,
> Mats Kindahl
>
> Alfranio Correia wrote:
>
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-46861/b46861-5.0.72sp/
> based on revid:joerg@stripped
>>
>> 2720 Alfranio Correia 2009-08-25
>> BUG#46861 Auto-closing of temporary tables broken by replicate-rewrite-db
>>
>> When a connection is dropped any remaining temporary table is also
> automatically
>> dropped and the SQL statement of this operation is written to the binary
> log in
>> order to drop such tables on the slave and keep the slave in sync.
> Specifically,
>> the current code base creates the following type of statement:
>> DROP /*!40005 TEMPORARY */ TABLE IF EXISTS `db`.`table`;
>>
>> Unfortunately, appending the database to the table name in this manner
> circumvents
>> the replicate-rewrite-db option (and any options that check the current
> database).
>> To solve the issue, we started writing the statement to the binary as
> follows:
>> use `db`; DROP /*!40005 TEMPORARY */ TABLE IF EXISTS `table`;
>>
>> modified:
>> mysql-test/r/rpl_rewrite_db.result
>> mysql-test/t/rpl_rewrite_db-slave.opt
>> mysql-test/t/rpl_rewrite_db.test
>> sql/sql_base.cc
>> === modified file 'mysql-test/r/rpl_rewrite_db.result'
>> --- a/mysql-test/r/rpl_rewrite_db.result 2006-07-20 08:41:12 +0000
>> +++ b/mysql-test/r/rpl_rewrite_db.result 2009-08-25 15:57:32 +0000
>> @@ -91,4 +91,95 @@ a b
>> 3 row 3
>> 0
>> drop database rewrite;
>> -drop table t1;
>> +
>> +#
>> +# Bug #46861 Auto-closing of temporary tables broken by replicate-rewrite-db
>> +#
>> +
>> +#
>> +# Preparing the environment
>> +#
>> +SET sql_log_bin= 0;
>> +CREATE DATABASE database_master_temp_01;
>> +CREATE DATABASE database_master_temp_02;
>> +CREATE DATABASE database_master_temp_03;
>> +SET sql_log_bin= 1;
>> +SET sql_log_bin= 0;
>> +CREATE DATABASE database_slave_temp_01;
>> +CREATE DATABASE database_slave_temp_02;
>> +CREATE DATABASE database_slave_temp_03;
>> +SET sql_log_bin= 1;
>>
>
> Is there a special reason to not use replication to create the databases on both
> the master and the slave? That is:
>
> connection master;
> CREATE DATABASE database_master_temp_01;
> CREATE DATABASE database_master_temp_02;
> CREATE DATABASE database_master_temp_03;
> sync_slave_with_master;
>
If you don't mind I will keep this.
>
>> +
>> +#
>> +# Creating temporary tables on different databases with different connections
>> +#
>> +# con_temp_01 --> creates
>> +# t_01_01_temp on database_master_temp_01
>> +#
>> +# con_temp_02 --> creates
>> +# t_01_01_temp on database_master_temp_01
>> +# t_02_01_temp, t_02_02_temp on database_master_temp_02
>> +#
>> +# con_temp_02 --> creates
>> +# t_01_01_temp on database_master_temp_01
>> +# t_02_01_temp, t_02_02_temp on database_master_temp_02
>> +# t_03_01_temp, t_03_02_temp, t_03_03_temp on
> database_master_temp_03
>> +#
>> +
>> +con_temp_01
>>
>
> Please use some extra marker: it is easy to confuse as a command of some sort.
> Four stars (****) is commonly used, but anything that makes the line stand out
> as not a command will do.
>
Done.
>
>> +
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +
>> +con_temp_02
>> +
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +USE database_master_temp_02;
>> +CREATE TEMPORARY TABLE t_02_01_temp(a int);
>> +INSERT INTO t_02_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_02_02_temp(a int);
>> +INSERT INTO t_02_02_temp VALUES(1);
>> +
>> +con_temp_03
>> +
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +USE database_master_temp_02;
>> +CREATE TEMPORARY TABLE t_02_01_temp(a int);
>> +INSERT INTO t_02_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_02_02_temp(a int);
>> +INSERT INTO t_02_02_temp VALUES(1);
>> +USE database_master_temp_03;
>> +CREATE TEMPORARY TABLE t_03_01_temp(a int);
>> +INSERT INTO t_03_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_03_02_temp(a int);
>> +INSERT INTO t_03_02_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_03_03_temp(a int);
>> +INSERT INTO t_03_03_temp VALUES(1);
>> +
>> +# Checking the binary log
>> +
>> +show binlog events from <binlog_start>;
>> +Log_name Pos Event_type Server_id End_log_pos Info
>> +master-bin.000001 # Query # # use `database_master_temp_02`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `t_02_02_temp`,`t_02_01_temp`
>> +master-bin.000001 # Query # # use `database_master_temp_03`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `t_03_03_temp`,`t_03_02_temp`,`t_03_01_temp`
>> +master-bin.000001 # Query # # use `database_master_temp_02`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `t_02_02_temp`,`t_02_01_temp`
>> +master-bin.000001 # Query # # use `database_master_temp_01`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `t_01_01_temp`
>> +master-bin.000001 # Query # # use `database_master_temp_01`; DROP /*!40005
> TEMPORARY */ TABLE IF EXISTS `t_01_01_temp`
>> +#
>> +# Cleaning up the test case
>> +#
>> +SET sql_log_bin= 0;
>> +DROP DATABASE database_master_temp_01;
>> +DROP DATABASE database_master_temp_02;
>> +DROP DATABASE database_master_temp_03;
>> +SET sql_log_bin= 1;
>> +SET sql_log_bin= 0;
>> +DROP DATABASE database_slave_temp_01;
>> +DROP DATABASE database_slave_temp_02;
>> +DROP DATABASE database_slave_temp_03;
>> +SET sql_log_bin= 1;
>>
>
> Same here, you could let replication handle the clean-up on the slave. Doesn't
> it work with the rewrite in effect?
>
If you don't mind I will keep this.
>
>> === modified file 'mysql-test/t/rpl_rewrite_db-slave.opt'
>> --- a/mysql-test/t/rpl_rewrite_db-slave.opt 2004-11-15 16:03:54 +0000
>> +++ b/mysql-test/t/rpl_rewrite_db-slave.opt 2009-08-25 15:57:32 +0000
>> @@ -1 +1 @@
>> -"--replicate-rewrite-db=test->rewrite"
> "--replicate-rewrite-db=mysqltest1->test"
>> +"--replicate-rewrite-db=test->rewrite"
> "--replicate-rewrite-db=mysqltest1->test" "--replicate-rewrite-db=test->rewrite"
> "--replicate-rewrite-db=database_master_temp_01->database_slave_temp_01"
> "--replicate-rewrite-db=database_master_temp_02->database_slave_temp_02"
> "--replicate-rewrite-db=database_master_temp_03->database_slave_temp_03"
>>
>
> test->rewrite is there twice: is this intentional?
>
This was a copy and paster error. Fixed.
>
>> === modified file 'mysql-test/t/rpl_rewrite_db.test'
>> --- a/mysql-test/t/rpl_rewrite_db.test 2006-01-24 07:30:54 +0000
>> +++ b/mysql-test/t/rpl_rewrite_db.test 2009-08-25 15:57:32 +0000
>> @@ -75,7 +75,129 @@ select * from rewrite.t1;
>>
>> drop database rewrite;
>>
>> +# End of 4.1 tests
>> +
>> +--echo
>> +--echo #
>> +--echo # Bug #46861 Auto-closing of temporary tables broken by
> replicate-rewrite-db
>> +--echo #
>> +--echo
>> +
>> +--echo #
>> +--echo # Preparing the environment
>> +--echo #
>> connection master;
>> -drop table t1;
>>
>> -# End of 4.1 tests
>> +connect (con_temp_03,127.0.0.1,root,,test,$MASTER_MYPORT,);
>> +connect (con_temp_02,127.0.0.1,root,,test,$MASTER_MYPORT,);
>> +connect (con_temp_01,127.0.0.1,root,,test,$MASTER_MYPORT,);
>> +
>> +connection master;
>> +SET sql_log_bin= 0;
>> +CREATE DATABASE database_master_temp_01;
>> +CREATE DATABASE database_master_temp_02;
>> +CREATE DATABASE database_master_temp_03;
>> +SET sql_log_bin= 1;
>> +
>> +connection slave;
>> +SET sql_log_bin= 0;
>> +CREATE DATABASE database_slave_temp_01;
>> +CREATE DATABASE database_slave_temp_02;
>> +CREATE DATABASE database_slave_temp_03;
>> +SET sql_log_bin= 1;
>> +
>> +--echo
>> +--echo #
>> +--echo # Creating temporary tables on different databases with different
> connections
>> +--echo #
>> +--echo # con_temp_01 --> creates
>> +--echo # t_01_01_temp on database_master_temp_01
>> +--echo #
>> +--echo # con_temp_02 --> creates
>> +--echo # t_01_01_temp on database_master_temp_01
>> +--echo # t_02_01_temp, t_02_02_temp on database_master_temp_02
>> +--echo #
>> +--echo # con_temp_02 --> creates
>> +--echo # t_01_01_temp on database_master_temp_01
>> +--echo # t_02_01_temp, t_02_02_temp on database_master_temp_02
>> +--echo # t_03_01_temp, t_03_02_temp, t_03_03_temp on
> database_master_temp_03
>> +--echo #
>> +
>> +--echo
>> +--echo con_temp_01
>> +--echo
>> +connection con_temp_01;
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +
>> +--echo
>> +--echo con_temp_02
>> +--echo
>> +connection con_temp_02;
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +USE database_master_temp_02;
>> +CREATE TEMPORARY TABLE t_02_01_temp(a int);
>> +INSERT INTO t_02_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_02_02_temp(a int);
>> +INSERT INTO t_02_02_temp VALUES(1);
>> +
>> +--echo
>> +--echo con_temp_03
>> +--echo
>> +connection con_temp_03;
>> +USE database_master_temp_01;
>> +CREATE TEMPORARY TABLE t_01_01_temp(a int);
>> +INSERT INTO t_01_01_temp VALUES(1);
>> +USE database_master_temp_02;
>> +CREATE TEMPORARY TABLE t_02_01_temp(a int);
>> +INSERT INTO t_02_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_02_02_temp(a int);
>> +INSERT INTO t_02_02_temp VALUES(1);
>> +USE database_master_temp_03;
>> +CREATE TEMPORARY TABLE t_03_01_temp(a int);
>> +INSERT INTO t_03_01_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_03_02_temp(a int);
>> +INSERT INTO t_03_02_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_03_03_temp(a int);
>> +INSERT INTO t_03_03_temp VALUES(1);
>> +
>> +# Dropping the connections
>> +connection master;
>> +let $binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1);
>>
>
> Very nice! This keeps the uninteresting events from the test output.
>
>
>> +disconnect con_temp_01;
>> +disconnect con_temp_02;
>> +disconnect con_temp_03;
>> +
>> +--echo
>> +--echo # Checking the binary log
>> +--echo
>> +connection master;
>> +sync_slave_with_master;
>> +connection master;
>> +
>> +--source include/show_binlog_events.inc
>> +
>> +--echo #
>> +--echo # Cleaning up the test case
>> +--echo #
>> +connection master;
>> +SET sql_log_bin= 0;
>> +DROP DATABASE database_master_temp_01;
>> +DROP DATABASE database_master_temp_02;
>> +DROP DATABASE database_master_temp_03;
>> +SET sql_log_bin= 1;
>> +
>> +connection slave;
>> +SET sql_log_bin= 0;
>> +DROP DATABASE database_slave_temp_01;
>> +DROP DATABASE database_slave_temp_02;
>> +DROP DATABASE database_slave_temp_03;
>> +SET sql_log_bin= 1;
>> +
>> +connection master;
>> +sync_slave_with_master;
>> +
>> +# end of 5.0 tests
>>
>> === modified file 'sql/sql_base.cc'
>> --- a/sql/sql_base.cc 2008-10-07 21:34:00 +0000
>> +++ b/sql/sql_base.cc 2009-08-25 15:57:32 +0000
>> @@ -769,19 +769,20 @@ void close_temporary_tables(THD *thd)
>> {
>> /* Set pseudo_thread_id to be that of the processed table */
>> thd->variables.pseudo_thread_id= tmpkeyval(thd, table);
>> + String db;
>> + db.append(table->s->db);
>> /* Loop forward through all tables within the sublist of
>> common pseudo_thread_id to create single DROP query */
>> for (s_query.length(stub_len);
>> table && is_user_table(table) &&
>> - tmpkeyval(thd, table) == thd->variables.pseudo_thread_id;
>> + tmpkeyval(thd, table) == thd->variables.pseudo_thread_id
> &&
>> + !strcmp(table->s->db, db.ptr());
>>
>
> I prefer to write it as strcmp(table->s->db, db.ptr()) == 0 and reserve "!"
> for
> use with truly boolean values. It makes it clearer what it means, and easier to
> read.
>
Done.
>
>> table= next)
>> {
>> /*
>> We are going to add 4 ` around the db/table names and possible more
>> due to special characters in the names
>> */
>> - append_identifier(thd, &s_query, table->s->db,
> strlen(table->s->db));
>> - s_query.q_append('.');
>> append_identifier(thd, &s_query, table->s->table_name,
>> strlen(table->s->table_name));
>> s_query.q_append(',');
>> @@ -794,6 +795,7 @@ void close_temporary_tables(THD *thd)
>> Query_log_event qinfo(thd, s_query.ptr(),
>> s_query.length() - 1 /* to remove trailing ',' */,
>> 0, FALSE);
>> + qinfo.db= db.ptr();
>> thd->variables.character_set_client= cs_save;
>> /*
>> Imagine the thread had created a temp table, then was doing a SELECT,
> and
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>>
>
>
