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.
> +
> +#
> +# 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.
> +
> +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?
> +
> +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.)
> +
> +--source include/rpl_end.inc
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2011-03-24 10:52:40 +0000
> +++ b/sql/log_event.cc 2011-05-26 22:57:20 +0000
> @@ -7362,6 +7362,7 @@ int Rows_log_event::do_apply_event(Relay
> {
> DBUG_ENTER("Rows_log_event::do_apply_event(Relay_log_info*)");
> int error= 0;
> + TABLE *table= NULL;
> /*
> If m_table_id == ~0UL, then we have a dummy event that does not
> contain any data. In that case, we just remove all tables in the
> @@ -7446,13 +7447,18 @@ int Rows_log_event::do_apply_event(Relay
> if (simple_open_n_lock_tables(thd, rli->tables_to_lock))
> {
> uint actual_error= thd->main_da.sql_errno();
> - if (thd->is_slave_error || thd->is_fatal_error)
> + if (ignored_error_code(actual_error))
> + {
> + clear_all_errors(thd, const_cast<Relay_log_info*>(rli));
> + error= 0;
> + actual_error= 0;
> + goto end;
> + }
> + else if (thd->is_slave_error || thd->is_fatal_error)
> {
> /*
> Error reporting borrowed from Query_log_event with many excessive
> simplifications.
> - We should not honour --slave-skip-errors at this point as we are
> - having severe errors which should not be skiped.
> */
> rli->report(ERROR_LEVEL, actual_error,
> "Error '%s' on opening tables",
> @@ -7478,14 +7484,21 @@ int Rows_log_event::do_apply_event(Relay
> {
> if (ptr->m_tabledef.compatible_with(rli, ptr->table))
> {
> - /*
> - We should not honour --slave-skip-errors at this point as we are
> - having severe errors which should not be skiped.
> - */
> mysql_unlock_tables(thd, thd->lock);
> thd->lock= 0;
> thd->is_slave_error= 1;
> const_cast<Relay_log_info*>(rli)->clear_tables_to_lock();
> +
> + // skip the error if the slave is instructed to do it.
> + uint actual_error=
> const_cast<Relay_log_info*>(rli)->last_error().number;
> + if (ignored_error_code(actual_error))
> + {
> + clear_all_errors(thd, const_cast<Relay_log_info*>(rli));
> + error= 0;
> + actual_error= 0;
> + goto end;
> + }
> +
> DBUG_RETURN(ERR_BAD_TABLE_DEF);
> }
> }
> @@ -7514,9 +7527,7 @@ int Rows_log_event::do_apply_event(Relay
> #endif
> }
>
> - TABLE*
> - table=
> - m_table=
> const_cast<Relay_log_info*>(rli)->m_table_map.get_table(m_table_id);
> + table= m_table=
> const_cast<Relay_log_info*>(rli)->m_table_map.get_table(m_table_id);
>
> DBUG_PRINT("debug", ("m_table: 0x%lx, m_table_id: %lu", (ulong) m_table,
> m_table_id));
>
> @@ -7666,6 +7677,8 @@ int Rows_log_event::do_apply_event(Relay
> }
> } // if (table)
>
> +end:
> +
> /*
> We need to delay this clear until here bacause unpack_current_row() uses
> master-side table definitions stored in rli.
/Sven
