List:Commits« Previous MessageNext Message »
From:Luís Soares Date:June 1 2011 11:18am
Subject:Re: bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494
View as plain text  
Hi Sven,

   Thanks for the review.
   Find some answers to your questions below.

On 06/01/2011 11:14 AM, Sven Sandberg wrote:
> Hi Luís,
>
> Thanks for this work. The design is nice and straightforward and I see no problems.
> The implementation looks good and it
> is sufficiently commented. The test case is good too. I have some questions about
> specific details the test case, but
> I'm marking it as reviewed. See comments below.
>
>> 3682 Luis Soares 2011-05-26
>> BUG#12428494
>>
>> If setting slave-skip-errors=all and using RBR, then slave
>> will not skip errors returned while:
>>
>> 1. opening tables that do not exist on the slave;
>> 2. checking that table definitions on the slave match
>> the ones on the master.
>>
>> On SBR, however, the slave will properly skip the errors
>> as instructed by the slave-skip-errors option setting.
>>
>> We fix this by checking if the error returned on
>> simple_open_n_lock_tables and on m_tabledef.compatible_with
>> should be ignored or not before actually reporting/returning
>> the error.
>>
>> added:
>> mysql-test/suite/rpl/r/rpl_skip_error_all.result
>> mysql-test/suite/rpl/t/rpl_skip_error_all-slave.opt
>> mysql-test/suite/rpl/t/rpl_skip_error_all.test
>> modified:
>> sql/log_event.cc
>> === added file 'mysql-test/suite/rpl/r/rpl_skip_error_all.result'
>> --- a/mysql-test/suite/rpl/r/rpl_skip_error_all.result 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_skip_error_all.result 2011-05-26 22:57:20 +0000
>> @@ -0,0 +1,16 @@
>> +include/master-slave.inc
>> +[connection master]
>> +include/rpl_reset.inc
>> +SET SQL_LOG_BIN=0;
>> +CREATE TABLE t1 (a CHAR) ENGINE=InnoDB;
>> +SET SQL_LOG_BIN=1;
>> +call mtr.add_suppression ("Slave SQL: Table definition on master and slave does
> not match: Column 0 type mismatch.*");
>> +CREATE TABLE t1 (a DATE) ENGINE=InnoDB;
>> +INSERT INTO t1 VALUES ('a'), ('b');
>> +DROP TABLE t1;
>> +include/rpl_reset.inc
>> +CREATE TABLE t1 (id INT) ENGINE=MyISAM;
>> +DROP TABLE t1;
>> +INSERT INTO t1 VALUES(1);
>> +DROP TABLE t1;
>> +include/rpl_end.inc
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_skip_error_all-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_skip_error_all-slave.opt 1970-01-01 00:00:00
> +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_skip_error_all-slave.opt 2011-05-26 22:57:20
> +0000
>> @@ -0,0 +1 @@
>> +--slave-skip-errors=all
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_skip_error_all.test'
>> --- a/mysql-test/suite/rpl/t/rpl_skip_error_all.test 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_skip_error_all.test 2011-05-26 22:57:20 +0000
>> @@ -0,0 +1,47 @@
>> +--source include/master-slave.inc
>> +--source include/have_innodb.inc
>
> Why do we need innodb? I think we can just drop the engine clauses and use the
> default.

Hmm... I don't remember! It might have been because I was
testing this in 5.1 and the default there is MyISAM. Thus
I also wanted to test with InnoDB. I guess because of that,
the engine clause got in the cset. I'll remove.

>> +
>> +#
>> +# BUG#12428494: IN ROW-BASED REPLICATION SLAVE-SKIP-ERRORS=ALL DOES NOT SKIP ALL
> ERRORS
>> +#
>> +
>> +# Test 1
>> +#
>> +# Test that different table definitions error
>> +# will be skipped when slave-skip-errors=all
>> +--source include/rpl_reset.inc
>
> master-slave.inc does everything that rpl_reset.inc does, so the above line can be
> removed.

I have placed it there to be sure that if someone adds a test
case before this one, it will not break things, because this
test always starts from a clean state.

>> +
>> +SET SQL_LOG_BIN=0;
>> +CREATE TABLE t1 (a CHAR) ENGINE=InnoDB;
>> +SET SQL_LOG_BIN=1;
>> +
>> +--connection slave
>> +call mtr.add_suppression ("Slave SQL: Table definition on master and slave does
> not match: Column 0 type mismatch.*");
>
> Why suppress this? Don't we expect that nothing is written to the error log?

There is still an error written to the error log:

Slave SQL: Table definition on master and slave does not match: Column 0 type mismatch -
received type 254, test.t1 
has type 10, Error_code: 1535

This is because ER_BINLOG_ROW_WRONG_TABLE_DEF is thrown at
table_def::compatible_with

Now that you mention it, I wonder if we should add a parameter to
compatible_with (..., bool suppress_error= false)

What do you think ? Perhaps it is worth, since monitoring tools
may be looking into the error log and things won't add up...

>> +
>> +CREATE TABLE t1 (a DATE) ENGINE=InnoDB;
>> +
>> +--connection master
>> +INSERT INTO t1 VALUES ('a'), ('b');
>> +--sync_slave_with_master
>> +--connection master
>> +DROP TABLE t1;
>> +--sync_slave_with_master
>> +
>> +# Test 2
>> +#
>> +# Test that when there is no table at the slave and if
>> +# slave-skil-errors=all the slave will skip it
>> +--source include/rpl_reset.inc
>> +
>> +--connection master
>> +
>> +CREATE TABLE t1 (id INT) ENGINE=MyISAM;
>> +--sync_slave_with_master
>> +DROP TABLE t1;
>> +
>> +--connection master
>> +INSERT INTO t1 VALUES(1);
>> +DROP TABLE t1;
>> +--sync_slave_with_master
>
> (FYI, sync_slave_with_master is not needed before rpl_end.inc, but it doesn't hurt
> either.)

Let it be there. It marks the end of the test case. As with
rpl_reset.inc at the beginning, it marks the boundaries of
the test case.

Regards,
Luís
Thread
bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494Luis Soares27 May
Re: bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494Sven Sandberg1 Jun
  • Re: bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494Luís Soares1 Jun
Re: bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494Luís Soares1 Jun