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.
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;
> +
> +#
> +# 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.
> +
> +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?
> === 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?
> === 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.
> 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
>
>
>
> ------------------------------------------------------------------------
>
>
--
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
