List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:June 1 2011 10:14am
Subject:Re: bzr commit into mysql-5.1 branch (luis.soares:3682) Bug#12428494
View as plain text  
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
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