Hi all,
I added comments inline.
He Zhenxing 写道:
> Leonard Zhou wrote:
>> #At file:///home/zhl/mysql/rep/5.1/bug40013/
>>
>> 2793 Leonard Zhou 2009-02-16
>> Bug#40013 Mixed replication can work for the operations of row based temp
> table.
>>
>
> the title of the bug is incorrect.
ok
>
>> After we do 'insert into t1 select UUID()', the current format is changed
> to 'row' but the default format is still 'MIXED'.
>> So we have problems when we do some operations after that clause. eg: can't
> create tmp table in slave...
>>
>
> the switch of binlog format to ROW in MIXED mode can be invoked by many
> ways, using UUID() is just one of it
I just describe the cause of this bug and don't want to list all the
causation.
>
> and the problem is not can't create tmp table in slave, the real problem
> is after switching to ROW format, the master will not binlog all the
> CREATE/DROP statements on temporary tables.
That's the same meaning with me. I mean replication can't create tmp
table in slave. I will add more description here.
>
> BTW: Please also check if ALTER command is affected or not.
>
ok
>>
>> Our solution is to allow create/drop table if the default/current format is
> mixed/row and clear row based format flag.
>
> the solution is to binlog CREATE/DROP temporary tables if the default
> format is mixed.
>
>> added:
>> mysql-test/suite/rpl/r/rpl_format_mix_row.result
>> mysql-test/suite/rpl/t/rpl_format_mix_row.test
>> modified:
>> sql/sql_class.cc
>> sql/sql_table.cc
>>
>> per-file messages:
>> mysql-test/suite/rpl/r/rpl_format_mix_row.result
>> Test result file
>> mysql-test/suite/rpl/t/rpl_format_mix_row.test
>> Test file
>> sql/sql_class.cc
>> Clear row based format if the default format is mixed.
>> sql/sql_table.cc
>> Create/Drop tables if the default format is mixed and current format is row.
>> === added file 'mysql-test/suite/rpl/r/rpl_format_mix_row.result'
>> --- a/mysql-test/suite/rpl/r/rpl_format_mix_row.result 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_format_mix_row.result 2009-02-16 09:42:08 +0000
>> @@ -0,0 +1,51 @@
>> +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;
>> +==== The Tirst Test Initialize ====
>> +[on master]
>> +CREATE TABLE t1 (a INT);
>> +CREATE TEMPORARY TABLE t1_tmp1(a INT);
>> +INSERT INTO t1 VALUES (UUID());
>> +[on slave]
>> +==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +Variable_name Value
>> +Slave_open_temp_tables 1
>> +[on master]
>> +[on slave]
>> +==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +Variable_name Value
>> +Slave_open_temp_tables 0
>> +==== Clean up ====
>> +[on master]
>> +DROP TABLE t1;
>> +[on slave]
>> +==== The Second Test Initialize ====
>> +[on master]
>> +CREATE TABLE t1 (a INT);
>> +CREATE TEMPORARY TABLE t1_tmp1(a INT);
>> +INSERT INTO t1 VALUES (UUID());
>> +CREATE TEMPORARY TABLE t1_tmp2(a INT);
>> +CREATE TEMPORARY TABLE t1_tmp3(a INT);
>> +DROP TEMPORARY TABLE t1_tmp1;
>> +[on slave]
>> +==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +Variable_name Value
>> +Slave_open_temp_tables 2
>> +[on master]
>> +CREATE TEMPORARY TABLE t1_tmp4(a INT);
>> +INSERT INTO t1 VALUES (UUID());
>> +[on slave]
>> +==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +Variable_name Value
>> +Slave_open_temp_tables 3
>> +==== Clean up ====
>> +[on master]
>> +DROP TABLE t1;
>> +[on slave]
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_format_mix_row.test'
>> --- a/mysql-test/suite/rpl/t/rpl_format_mix_row.test 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_format_mix_row.test 2009-02-16 09:42:08 +0000
>
> there is already a rpl_drop_temp.test, you can use that, or name your
> test like rpl_drop_temp_mix_row.test
Our test is not only for '*DROP* temp table', it could be *ADD* temp
table or the condition just as bug description.
>
>> @@ -0,0 +1,93 @@
>> +# ==== Purpose ====
>> +#
>> +# Test that mix replication can work for row based temp table.
>> +#
>
> Please use something like: Test that temporary tables are correctly
> replicated when switching to ROW format in MIXED mode
>
> Please add comments to explain how the test works, what it tests.
I will add comments before the test statements.
>
> Please also add reference to the bug.
>
ok
>> +
>> +source include/master-slave.inc;
>> +source include/have_binlog_format_mixed.inc;
>> +
>> +--echo ==== The Tirst Test Initialize ====
>> +
>
> Tirst -> First
>
>> +--echo [on master]
>> +--connection master
>> +
>> +CREATE TABLE t1 (a INT);
>
> use CHAR(48) instead of INT to get ride of the warning of the INSERT
> statement below.
>
>> +CREATE TEMPORARY TABLE t1_tmp1(a INT);
>> +--disable_warnings
>> +INSERT INTO t1 VALUES (UUID());
>> +--enable_warnings
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>> +
>> +--echo ==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +
>> +--echo [on master]
>> +--connection master
>> +
>> +disconnect master;
>> +--connection master1
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>> +
>> +--echo ==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +
>> +--echo ==== Clean up ====
>> +
>
> First Test Clean up
>
>> +--echo [on master]
>> +--connection master1
>> +DROP TABLE t1;
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>> +
>> +#
>> +# The second test for create and drop temp table in row based format
>> +#
>> +
>> +--echo ==== The Second Test Initialize ====
>> +
>> +--echo [on master]
>> +--connection master1
>> +
>
>> +CREATE TABLE t1 (a INT);
>
> use CHAR(48) instead of INT
>
>> +CREATE TEMPORARY TABLE t1_tmp1(a INT);
>> +--disable_warnings
>> +INSERT INTO t1 VALUES (UUID());
>> +--enable_warnings
>> +CREATE TEMPORARY TABLE t1_tmp2(a INT);
>> +CREATE TEMPORARY TABLE t1_tmp3(a INT);
>> +DROP TEMPORARY TABLE t1_tmp1;
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>> +
>> +--echo ==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +
>> +--echo [on master]
>> +
>> +--connection master1
>> +
>> +CREATE TEMPORARY TABLE t1_tmp4(a INT);
>> +--disable_warnings
>> +INSERT INTO t1 VALUES (UUID());
>> +--enable_warnings
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>> +
>> +--echo ==== Verify results on slave ====
>> +SHOW STATUS LIKE "Slave_open_temp_tables";
>> +
>> +--echo ==== Clean up ====
>> +
>> +--echo [on master]
>> +--connection master1
>> +DROP TABLE t1;
>> +
>> +--echo [on slave]
>> +sync_slave_with_master;
>>
>> === modified file 'sql/sql_class.cc'
>> --- a/sql/sql_class.cc 2009-02-06 09:53:20 +0000
>> +++ b/sql/sql_class.cc 2009-02-16 09:42:08 +0000
>> @@ -869,6 +869,9 @@ void THD::cleanup(void)
>> mysql_ha_cleanup(this);
>> delete_dynamic(&user_var_events);
>> hash_free(&user_vars);
>> + if (variables.binlog_format == BINLOG_FORMAT_MIXED &&
>> + current_stmt_binlog_row_based)
>> + clear_current_stmt_binlog_row_based();
>
> This is not needed and incorrect
We need this. This part will fix the condition as bug describe.
And yes maybe it is not correct in some condition, so we should do
something like 'reset_current_stmt_binlog_row_base()' but need to change
some lines.
>
>> close_temporary_tables(this);
>> my_free((char*) variables.time_format, MYF(MY_ALLOW_ZERO_PTR));
>> my_free((char*) variables.date_format, MYF(MY_ALLOW_ZERO_PTR));
>>
>> === modified file 'sql/sql_table.cc'
>> --- a/sql/sql_table.cc 2009-02-05 06:16:00 +0000
>> +++ b/sql/sql_table.cc 2009-02-16 09:42:08 +0000
>> @@ -1790,6 +1790,19 @@ int mysql_rm_table_part2(THD *thd, TABLE
>> */
>> write_bin_log(thd, !error, thd->query, thd->query_length);
>> }
>> + else if (thd->current_stmt_binlog_row_based &&
>> + thd->variables.binlog_format == BINLOG_FORMAT_MIXED
> &&
>> + tmp_table_deleted)
>> + {
>> + /*
>> + In this case, we are using row-based replication but
>> + the default format is mixed. eg, we change the format via:
>> + INSERT INTO t1 VALUES (UUID());
>> + In this case, we need write the original query into
>> + the binary log, so the slave can drop temp tables correctly.
>> + */
>> + write_bin_log(thd, !error, thd->query, thd->query_length);
>> + }
>
> Please combine the conditions to make it shorter and clearer.
combine with the first condition?
I think we should seperate them for clear.
>
>> else if (thd->current_stmt_binlog_row_based &&
>> non_temp_tables_count > 0 &&
>> tmp_table_deleted)
>> @@ -3589,9 +3602,12 @@ bool mysql_create_table_no_lock(THD *thd
>> - Row-based logging is used and it we are creating a temporary table, or
>> - The binary log is not open.
>> Otherwise, the statement shall be binlogged.
>> + We will write to binlog if the current format is row but default format is
> mixed.
>> */
>> if (!internal_tmp_table &&
>> (!thd->current_stmt_binlog_row_based ||
>> + (thd->current_stmt_binlog_row_based &&
>> + thd->variables.binlog_format == BINLOG_FORMAT_MIXED) ||
>
> Here too, make it less redundant
>
>> (thd->current_stmt_binlog_row_based &&
>> !(create_info->options & HA_LEX_CREATE_TMP_TABLE))))
>> write_bin_log(thd, TRUE, thd->query, thd->query_length);
>>
>>
>
--
Leonard (Li) Zhou, Software Engineer, MySQL Replication
Sun Microsystems, www.sun.com
Office: 86-10-65054020 Ext: 299
