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 List-Archive: http://lists.mysql.com/commits/118579 Message-Id: <20100920100453.GA7499@mockturtle> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hello Li-Bing! Here are my comments about new version of your patch: * 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