List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 20 2010 10:04am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3489)
Bug#56226
View as plain text  
Hello Li-Bing!

Here are my comments about new version of your patch:

* Li-Bing.Song@stripped <Li-Bing.Song@stripped> [10/09/20 11:57]:
> #At file:///home/anders/work/bzrwork1/wt2/mysql-5.1-bugteam/ based on
> revid:mattias.jonsson@stripped
> 
>  3489 Li-Bing.Song@stripped	2010-09-20
>       Bug#56226  Table map set to 0 after altering MyISAM table
>       

IMO it makes sense to explain in ChangeSet comment what the
problem was in terms which can be understood by our users.
This allows outside reader better understand what the problem
was and makes life easier for our Documentation team, as such
explanation can be reused for ChangeLog.

I think something like:

After ALTER TABLE which changed only table's metadata, row-based
binlog sometimes got corrupted since the tablemap was unexpectedly
set to 0 for subsequent updates to the same table.

will do the trick in this case (I have derived this explanation
from the one in bug report).

I also think that it is worth to mention in the ChangeSet comment
that this bug didn't affect 5.5+ versions of server.

>       A MyISAM table's table_map_id will always be reset to 0 after altered
>       only its metadata. 0 is a valid value of table_map_id. But the problem will
>       cause that more than one tables have the same table_map_id 0. If more than
>       one tables which's table_map_id are 0 are updated in one statement, the update
>       of these different tables will be write into only one rows event. It will
>       cause slave server to crash. 

I think we can make the above paragraph a bit more clear.
For example by rewriting it in the following way:

ALTER TABLE which changed only table's metadata always reset
table_map_id for the table share to 0. Despite the fact that
0 is a valid value for table_map_id, this step caused problems
as it could have created situation in which we had more than
one table share with table_map_id equal 0. If more than one
table with table_map_id are 0 were updated in the same statement,
updates to these different tables were written into the same
rows event. This caused slave server to crash.

>       
>       After this patch, table_map_id always be refreshed as a correct value after
>       altered only its metadata.

I suggest to make the above comment a bit more elaborate:

This patch solves this problem by ensuring that ALTER TABLE
statements which change metadata only never reset table_map_id
to 0. To do this it changes reopen_table() to correctly use
refreshed table_map_id value instead of using the old one/
resetting it.

>      @ mysql-test/suite/rpl/r/rpl_alter.result
>         Add test for BUG#56226
>      @ mysql-test/suite/rpl/t/rpl_alter.test
>         Add test for BUG#56226
> 
>     modified:
>       mysql-test/suite/rpl/r/rpl_alter.result
>       mysql-test/suite/rpl/t/rpl_alter.test
>       sql/sql_base.cc
> === modified file 'mysql-test/suite/rpl/r/rpl_alter.result'
> --- a/mysql-test/suite/rpl/r/rpl_alter.result	2007-06-27 12:28:02 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_alter.result	2010-09-20 07:53:38 +0000
> @@ -19,3 +19,23 @@ select * from mysqltest.t3;
>  n
>  45
>  drop database mysqltest;
> +
> +# BUG#56226 Table map set to 0 after altering MyISAM table
> +
> +SET SESSION binlog_format='ROW';
> +USE test;
> +CREATE TABLE t1 (a INT, b INT) ENGINE MyISAM;
> +CREATE TABLE t2 (a VARCHAR(255), b VARCHAR(255)) ENGINE MyISAM;
> +CREATE TRIGGER trg_t1ai 
> +AFTER INSERT ON t1 FOR EACH ROW 
> +BEGIN
> +INSERT INTO t2 (a) VALUES (NEW.a);
> +END;//
> +ALTER TABLE t1 CHANGE b c INT;
> +ALTER TABLE t2 CHANGE b c VARCHAR(255);
> +
> +INSERT INTO t1 (a) VALUES(2);
> +Comparing tables master:test.t1 and slave:test.t1
> +Comparing tables master:test.t2 and slave:test.t2
> +
> +DROP TABLE t1, t2;
> 
> === modified file 'mysql-test/suite/rpl/t/rpl_alter.test'
> --- a/mysql-test/suite/rpl/t/rpl_alter.test	2007-06-27 12:28:02 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_alter.test	2010-09-20 07:53:38 +0000
> @@ -10,15 +10,62 @@ insert into mysqltest.t1 values (1,2);
>  create table mysqltest.t2 (n int);
>  insert into mysqltest.t2 values (45);
>  rename table mysqltest.t2 to mysqltest.t3, mysqltest.t1 to mysqltest.t2;
> -save_master_pos;
> -connection slave;
> -sync_with_master;
> +
> +sync_slave_with_master;
>  select * from mysqltest.t2;
>  select * from mysqltest.t3;
> +
>  connection master;
>  drop database mysqltest;
> -save_master_pos;
> -connection slave;
> -sync_with_master;
> +sync_slave_with_master;
> +
> +--echo
> +--echo # BUG#56226 Table map set to 0 after altering MyISAM table
> +--echo
> +connection master;
> +SET SESSION binlog_format='ROW';

Since you are changing binlog_format session for 'master'
connection here shouldn't you restore it to its old value
somewhere?

> +USE test;
> +
> +CREATE TABLE t1 (a INT, b INT) ENGINE MyISAM;
> +CREATE TABLE t2 (a VARCHAR(255), b VARCHAR(255)) ENGINE MyISAM;
> +
> +--delimiter //
> +CREATE TRIGGER trg_t1ai 
> +  AFTER INSERT ON t1 FOR EACH ROW 
> +BEGIN
> +  INSERT INTO t2 (a) VALUES (NEW.a);
> +END;//
> +--delimiter ;
> +
> +ALTER TABLE t1 CHANGE b c INT;
> +ALTER TABLE t2 CHANGE b c VARCHAR(255);
> +let $binlog_file= query_get_value(SHOW MASTER STATUS, File, 1);
> +let $binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1);
> +
> +--echo
> +INSERT INTO t1 (a) VALUES(2);
> +let _info= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM $binlog_start,
> Info, 2);
> +# Table map id should not be 0.

How about using something like:

--echo # Check that table map id is not 0 in binary log events.

here, to make .result file a bit more readable?

> +if (`SELECT 'table_id: 0 (test.t1)' = '$_info'`)
> +{
> +  --echo test.t1's table map id is 0;
> +  --die test.t1's table map id is 0;
> +}
> +let _info= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM $binlog_start,
> Info, 3);
> +if (`SELECT 'table_id: 0 (test.t2)' = '$_info'`)
> +{
> +  --echo test.t2's table map id is 0;
> +  --die test.t2's table map id is 0;
> +}
> +
> +let diff_table= test.t1;
> +source include/rpl_diff_tables.inc;
> +let diff_table= test.t2;
> +source include/rpl_diff_tables.inc;
> +
> +--echo
> +connection master;
> +DROP TABLE t1, t2;
>  
> +source include/master-slave-end.inc;
>  # End of 4.1 tests

Should not this test go somewhere after this comment?

> 

Otherwise I am OK with your patch and think that it can
be pushed after considering the above comments.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3489) Bug#56226Li-Bing.Song20 Sep
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3489)Bug#56226Dmitry Lenev20 Sep