MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:November 22 2007 11:49am
Subject:Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552
View as plain text  
Hi Andrei!

Here is the review. Detailed comments below. Basically, I want you to 
handle the following:

    * The definitions of the macros for bit-manipulations need to be fixed
    * Do not use function calls or constructors in static initializers
    * Some language fixes in comments and strings (e.g., typos)
    * Remove some hard-coding for the NDB engine
    * Add a missing condition (?) to an if-statement
    * I would like to have a slightly different error message for the
      "idempotency error"
    * I would like to have a different text for describing the
      slave_exec_mode
    * I would like to have a function instead of a macro.
    * I would like to get rid entirely of the length array you use for
      the names

Just my few cents,
Mats Kindahl

Andrei Elkin wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of elkin. When elkin does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-11-20 15:15:49+02:00, aelkin@stripped +34 -0
>   Bug#31552 Replication breaks when deleting rows from out-of-sync table
>       without PK
>   Bug#31609 Not all RBR slave errors reported as errors
>   bug#32468 delete rows event on a table with foreign key constraint fails
>   
>   The first two bugs comprise idemptency issues.
>   First, there was no error code reported under conditions of the bug
>   description although the slave sql thread halted.
>   Second, executions were different with and without presence of prim key in
>   the table.
>   Third, there was no way to instruct the slave whether to ignore an error
>   and skip to the following event or to halt.
>   Fourth, there are handler errors which might happen due to idempotant
>   applying of binlog but those were not listed among the "idempotent" error
>   list.
>   
>   All the named issues are addressed.
>   Wrt to the 3rd, there is the new global system variable, changeble at run
>   time, which controls the slave sql thread behaviour.
>   The new variable allows further extensions to mimic the sql_mode
>   session/global variable.
>   To address the 4th, the new bug#32468 had to be fixed as it was staying
>   in the way.
>
>   include/my_bitmap.h@stripped, 2007-11-20 15:15:35+02:00, aelkin@stripped +18
> -0
>     basic operations with bits of an integer type are added.
>
>   mysql-test/extra/rpl_tests/rpl_foreign_key.test@stripped, 2007-11-20 15:15:36+02:00,
> aelkin@stripped +31 -0
>     regression test for bug#32468
>
>   mysql-test/extra/rpl_tests/rpl_row_basic.test@stripped, 2007-11-20 15:15:36+02:00,
> aelkin@stripped +10 -0
>     changes due to bug#31552/31609 idemptency is not default any longer
>
>   mysql-test/extra/rpl_tests/rpl_row_tabledefs.test@stripped, 2007-11-20 15:15:36+02:00,
> aelkin@stripped +6 -0
>     changes due to bug#31552/31609 idemptency is not default any longer
>
>   mysql-test/suite/rpl/r/rpl_foreign_key_innodb.result@stripped, 2007-11-20
> 15:15:36+02:00, aelkin@stripped +13 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_idempotency.result@stripped, 2007-11-20 15:15:36+02:00,
> aelkin@stripped +160 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_ignore_table.result@stripped, 2007-11-20 15:15:36+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_USER.result@stripped, 2007-11-20 15:15:37+02:00,
> aelkin@stripped +3 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result@stripped, 2007-11-20 15:15:37+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result@stripped, 2007-11-20 15:15:37+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result@stripped, 2007-11-20 15:15:37+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_mystery22.result@stripped, 2007-11-20 15:15:37+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result@stripped, 2007-11-20
> 15:15:37+02:00, aelkin@stripped +4 -2
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result@stripped, 2007-11-20
> 15:15:38+02:00, aelkin@stripped +4 -2
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_temporary_errors.result@stripped, 2007-11-20 15:15:38+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/t/rpl_idempotency-master.opt@stripped, 2007-11-20 15:15:42+02:00,
> aelkin@stripped +2 -0
>     innodb is necessary
>
>   mysql-test/suite/rpl/t/rpl_idempotency-master.opt@stripped, 2007-11-20 15:15:42+02:00,
> aelkin@stripped +0 -0
>
>   mysql-test/suite/rpl/t/rpl_idempotency-slave.opt@stripped, 2007-11-20 15:15:42+02:00,
> aelkin@stripped +2 -0
>     innodb is necessary, as well as the tests start with non-default
>     IDEMPOTENT slave execution mode.
>     
>
>   mysql-test/suite/rpl/t/rpl_idempotency-slave.opt@stripped, 2007-11-20 15:15:42+02:00,
> aelkin@stripped +0 -0
>
>   mysql-test/suite/rpl/t/rpl_idempotency.test@stripped, 2007-11-20 15:15:38+02:00,
> aelkin@stripped +329 -0
>     extenstions to the test providing testing of complements to the
>     idempotent error set and checking how slave halts when it faces an error
>     from the list when the mode is STRICT.
>
>   mysql-test/suite/rpl/t/rpl_ignore_table.test@stripped, 2007-11-20 15:15:38+02:00,
> aelkin@stripped +8 -0
>     changes due to bug#31552/31609 idemptency is not default any longer.
>     The fix ensures no row-based events should happen due to DROP database.
>
>   mysql-test/suite/rpl/t/rpl_row_USER.test@stripped, 2007-11-20 15:15:38+02:00,
> aelkin@stripped +4 -0
>     changes due to bug#31552/31609 idemptency is not default any longer.
>     The fix ensures no row-based events should happen due to DROP database.
>
>   mysql-test/suite/rpl/t/rpl_row_basic_11bugs.test@stripped, 2007-11-20 15:15:39+02:00,
> aelkin@stripped +3 -0
>     changes due to bug#31552/31609 idemptency is not default any longer.
>
>   mysql-test/suite/rpl/t/rpl_row_mystery22.test@stripped, 2007-11-20 15:15:39+02:00,
> aelkin@stripped +5 -0
>     changes due to bug#31552/31609 idemptency is not default any longer
>
>   mysql-test/suite/rpl/t/rpl_temporary_errors.test@stripped, 2007-11-20 15:15:39+02:00,
> aelkin@stripped +3 -0
>     changes due to bug#31552/31609 idemptency is not default any longer
>
>   mysql-test/suite/rpl_ndb/r/rpl_ndb_dd_advance.result@stripped, 2007-11-20
> 15:15:39+02:00, aelkin@stripped +1 -0
>     results changed
>
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test@stripped, 2007-11-20 15:15:39+02:00,
> aelkin@stripped +3 -0
>     changes due to bug#31552/31609 idemptency is not default any longer
>
>   sql/log_event.cc@stripped, 2007-11-20 15:15:39+02:00, aelkin@stripped +131
> -92
>     the fix for bug#32468 delete rows event on a table with foreign key constraint
> fails
>     ensures the flags are set at proper time so that their values will be caught
>     by innodb.
>     reseting the flags is done along the common error and errorless execution
>     path.
>     The list of idempotent error is extended with foreign keys related items.
>     NDB engine write events are designed with the replace sematics in mind.
>     Therefore the corrsponding ndb handler's flag are (re)set regardless of
>     the slave's execution mode.
>     Rows_log_event::write_row() starts using the bool replace argument as its
>     caller sets it depending on the event's execution mode.
>
>   sql/log_event.h@stripped, 2007-11-20 15:15:40+02:00, aelkin@stripped +6 -0
>     adding a new member to hold the slave's mode during execution of the event.
>
>   sql/mysql_priv.h@stripped, 2007-11-20 15:15:40+02:00, aelkin@stripped +1 -0
>     changes to link the command line option with the new global sys var.
>
>   sql/mysqld.cc@stripped, 2007-11-20 15:15:40+02:00, aelkin@stripped +17 -2
>     introduction of the new command line option.
>     providing its initialization to a default.
>     changes to link the command line option with the new global sys var.
>
>   sql/rpl_rli.cc@stripped, 2007-11-20 15:15:41+02:00, aelkin@stripped +5 -0
>     rli post-event-execution cleanup restores the default bits.
>
>   sql/set_var.cc@stripped, 2007-11-20 15:15:41+02:00, aelkin@stripped +78 -0
>     The new "standard" sys_var_set class' and the new global system var related
>     declarations and definitions.
>     fix_slave_exec_mode() is used as with the update method of a new class so
>     as at time of the command line arguments parsing.
>
>   sql/set_var.h@stripped, 2007-11-20 15:15:41+02:00, aelkin@stripped +38 -1
>     new declarations. The class for the new global sys var is based on
>     yet another new "standard" one.
>
>   sql/share/errmsg.txt@stripped, 2007-11-20 15:15:42+02:00, aelkin@stripped +2
> -0
>     slave_exec_mode setting error;
>     slave inconsistency error which may be not an error when the intention
>     is "idempotent". I.e consisting of row-based events binlog is being
>     applied for the 2nd (more) time.
>
>   sql/sql_class.h@stripped, 2007-11-20 15:15:41+02:00, aelkin@stripped +2 -0
>     The names for the bits of the new sever slave_exec_mode_options.
>
> diff -Nrup a/include/my_bitmap.h b/include/my_bitmap.h
> --- a/include/my_bitmap.h	2007-07-25 16:29:28 +03:00
> +++ b/include/my_bitmap.h	2007-11-20 15:15:35 +02:00
> @@ -159,6 +159,24 @@ static inline my_bool bitmap_cmp(const M
>  #define bitmap_set_all(MAP) \
>    (memset((MAP)->bitmap, 0xFF, 4*no_words_in_map((MAP))))
>  
> +/**
> +   like _bitmap's versions but `i' is an integer. If the bit of interest
> +   is outside of the integer's size sensible results are returned.
> +*/
> +#define is_bit_set(i, b) (uint) ((sizeof((i))*8 > b)?       \
> +                                 ((uchar*) (&(i)))[(b) / 8] \
> +                                 & (1 << ((b) & 7))         \
> +                                 : 0)
> +#define do_set_bit(i, b) (int) ((sizeof((i))*8 > b)?        \
> +                                (((uchar*)(&(i)))[(b) / 8]  \
> +                                 |= (1 << ((b) & 7)))       \
> +                                : -1)
> +#define do_clear_bit(i, b) (int) ((sizeof((i))*8 > b)?          \
> +                                  (((uchar*)(&(i)))[(b) / 8]    \
> +                                   &= ~ (1 << ((b) & 7)))       \
> +                                  : -1)
> +
>   

Although I think that macros or functions for this purpose is a good 
thing, these macros makes a large number of assumptions that is 
incorrect and cause confusing error or incorrect behavior.

   1. It assumes that all bytes are 8 bits large, which is not the case.
   2. It assumes that whatever is passed as 'i' can be dereferenced by
      taking the address of it, which is easy to make a mistake about
      and get a very confusing error back.
   3. It dereferences the entity under 'i', which means that many
      optimizations that can be done is not done (taking the address of
      anything is a sure optimization-killer).
   4. It is endian-dependent. For example is_bit_set(0x1, 0) will be
      'false' on a big-endian machine, but not on a little-endian machine.
   5. It does not work if something is passed as 'i' that has a size
      larger than 'int' (e.g., unsigned long long).
   6. I would prefer to have a naming of the macros with a common
      prefix, e.g., "bit_" since this is how all other naming is done
      (e.g., "my_", "mysql_", "COM_", "SQLCOM_")
   7. IMHO, do_set_bit() and do_clear_bit() are statements and should
      not have a value, but this is not important.
   8. Please clarify what result that you consider "sensible".
   9. If you are adding them to my_bitmap, I would like to see better
      documentation.

Here are optimization-friendly versions (untested, so please make sure 
they behave as expected).

#define bit_is_set(I,B) (sizeof(I) * CHAR_BIT > (B) ? (I) & (ULL(1) << 
(B)) : 0)
#define bit_do_set(I,B) (sizeof(I) * CHAR_BIT > (B) && ((I) |= (ULL(1) 
<< (B)))#define #define bit_do_clr(I,B) (sizeof(I) * CHAR_BIT > (B) && 
((I) &= ~(ULL(1) << (B)))

In general, these are better off being defined as inline functions 
instead of macros, since that handles a large part of the type-safety 
without having to resort to contorted code.

> +
>  #ifdef	__cplusplus
>  }
>  #endif
> diff -Nrup a/mysql-test/extra/rpl_tests/rpl_foreign_key.test
> b/mysql-test/extra/rpl_tests/rpl_foreign_key.test
> --- a/mysql-test/extra/rpl_tests/rpl_foreign_key.test	2007-06-06 20:48:52 +03:00
> +++ b/mysql-test/extra/rpl_tests/rpl_foreign_key.test	2007-11-20 15:15:36 +02:00
> @@ -32,3 +32,34 @@ SET FOREIGN_KEY_CHECKS=0;
>  DROP TABLE IF EXISTS t1,t2,t3;
>  SET FOREIGN_KEY_CHECKS=1;
>  sync_slave_with_master;
> +
> +#
> +# Bug #32468 delete rows event on a table with foreign key constraint fails
> +#
> +
> +connection master;
> +
> +eval create table t1 (b int primary key) engine = $engine_type;
> +eval create table t2 (a int primary key, b int, foreign key (b) references t1(b))
> +       engine = $engine_type;
> +
> +insert into t1 set b=1;
> +insert into t2 set a=1, b=1;
> +
> +set foreign_key_checks=0;
> +set @@session.binlog_format=row;
> +delete from t1;
> +
> +--echo must sync w/o a problem (could not with the buggy code)
> +sync_slave_with_master;
> +select count(*) from t1 /* must be zero */;
> +
> +
> +# cleanup for bug#32468
> +
> +connection master;
> +drop table t2,t1;
> +
> +sync_slave_with_master;
> +
> +
> diff -Nrup a/mysql-test/extra/rpl_tests/rpl_row_basic.test
> b/mysql-test/extra/rpl_tests/rpl_row_basic.test
> --- a/mysql-test/extra/rpl_tests/rpl_row_basic.test	2007-06-19 00:51:07 +03:00
> +++ b/mysql-test/extra/rpl_tests/rpl_row_basic.test	2007-11-20 15:15:36 +02:00
> @@ -174,11 +174,16 @@ sync_slave_with_master;
>  INSERT INTO t7 VALUES (1,3), (2,6), (3,9);
>  SELECT * FROM t7 ORDER BY C1;
>  
> +# bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>   

Consider adding such a setting to master-slave.inc as well, since most 
tests assume that RBR is idempotent. Not sure if that is necessary 
though. You have added settings to each test below, and I would accept 
that approach as well.

> +
>  connection master;
>  --echo --- on master: new values inserted ---
>  INSERT INTO t7 VALUES (1,2), (2,4), (3,6);
>  SELECT * FROM t7 ORDER BY C1;
>  sync_slave_with_master;
> +
> +set @@global.slave_exec_mode= 'STRICT';
>  --echo --- on slave: old values should be overwritten by replicated values ---
>  SELECT * FROM t7 ORDER BY C1;
>  
> @@ -206,12 +211,17 @@ SELECT * FROM t8 ORDER BY a;
>  INSERT INTO t8 VALUES (1,2,3), (2,4,6), (3,6,9);
>  SELECT * FROM t8 ORDER BY a;
>  
> +# bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
> +
>  connection master;
>  --echo --- on master ---
>  # We insert a row that will cause conflict on the primary key but not
>  # on the other keys.
>  INSERT INTO t8 VALUES (2,4,8);
>  sync_slave_with_master;
> +set @@global.slave_exec_mode= default;
> +
>  --echo --- on slave ---
>  SELECT * FROM t8 ORDER BY a;
>  
> diff -Nrup a/mysql-test/extra/rpl_tests/rpl_row_tabledefs.test
> b/mysql-test/extra/rpl_tests/rpl_row_tabledefs.test
> --- a/mysql-test/extra/rpl_tests/rpl_row_tabledefs.test	2007-08-21 15:32:48 +03:00
> +++ b/mysql-test/extra/rpl_tests/rpl_row_tabledefs.test	2007-11-20 15:15:36 +02:00
> @@ -69,6 +69,10 @@ ALTER TABLE t8 ADD e1 INT NOT NULL DEFAU
>          
>  # Insert some values for tables on slave side. These should not be
>  # modified when the row from the master is applied.
> +# before bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
> +
> +# so the inserts are going to be overriden
>  INSERT INTO t1_int  VALUES (2, 4, 4711);
>  INSERT INTO t1_char VALUES (2, 4, 'Foo is a bar');
>  INSERT INTO t1_bit  VALUES (2, 4, b'101', b'11100', b'01');
> @@ -86,6 +90,8 @@ SELECT * FROM t1_bit ORDER BY a;
>  SELECT * FROM t1_char ORDER BY a;
>  --echo **** On Slave ****
>  sync_slave_with_master;
> +set @@global.slave_exec_mode= default;
> +
>  SELECT a,b,x FROM t1_int ORDER BY a;
>  SELECT a,b,HEX(x),HEX(y),HEX(z) FROM t1_bit ORDER BY a;
>  SELECT a,b,x FROM t1_char ORDER BY a;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_foreign_key_innodb.result
> b/mysql-test/suite/rpl/r/rpl_foreign_key_innodb.result
> --- a/mysql-test/suite/rpl/r/rpl_foreign_key_innodb.result	2007-06-27 15:27:28
> +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_foreign_key_innodb.result	2007-11-20 15:15:36
> +02:00
> @@ -40,3 +40,16 @@ Got one of the listed errors
>  SET FOREIGN_KEY_CHECKS=0;
>  DROP TABLE IF EXISTS t1,t2,t3;
>  SET FOREIGN_KEY_CHECKS=1;
> +create table t1 (b int primary key) engine = INNODB;
> +create table t2 (a int primary key, b int, foreign key (b) references t1(b))
> +engine = INNODB;
> +insert into t1 set b=1;
> +insert into t2 set a=1, b=1;
> +set foreign_key_checks=0;
> +set @@session.binlog_format=row;
> +delete from t1;
> +must sync w/o a problem (could not with the buggy code)
> +select count(*) from t1 /* must be zero */;
> +count(*)
> +0
> +drop table t2,t1;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_idempotency.result
> b/mysql-test/suite/rpl/r/rpl_idempotency.result
> --- a/mysql-test/suite/rpl/r/rpl_idempotency.result	2007-10-30 22:17:14 +02:00
> +++ b/mysql-test/suite/rpl/r/rpl_idempotency.result	2007-11-20 15:15:36 +02:00
> @@ -69,3 +69,163 @@ a
>  Last_SQL_Error
>  0
>  DROP TABLE t1, t2;
> +select @@global.slave_exec_mode /* must be IDEMPOTENT */;
> +@@global.slave_exec_mode
> +IDEMPOTENT
> +create table ti1 (b int primary key) engine = innodb;
> +create table ti2 (a int primary key, b int, foreign key (b) references ti1(b))
> +engine = innodb;
> +set foreign_key_checks=1 /* ensure the check */;
> +insert into ti1 values (1),(2),(3);
> +insert into ti2 set a=2, b=2;
> +select * from ti1 order by b /* must be (1),(2),(3) */;
> +b
> +1
> +2
> +3
> +insert into ti2 set a=1, b=1;
> +select * from ti2 order by b /* must be (1,1) (2,2) */;
> +a	b
> +1	1
> +2	2
> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= row;
> +delete from ti1 where b=1;
> +select * from ti1 order by b /* must be (2),(3) */;
> +b
> +2
> +3
> +select * from ti1 order by b /* must stays as were on master (1),(2),(3) */;
> +b
> +1
> +2
> +3
> +consipire the following insert failure
> +delete from ti1 where b=3;
> +insert into ti2 set a=3, b=3;
> +select * from ti2 order by b /* must be (1,1),(2,2) - not inserted */;
> +a	b
> +1	1
> +2	2
> +set global slave_exec_mode='STRICT';
> +set global slave_exec_mode='IDEMPOTENT';
> +set global slave_exec_mode='IDEMPOTENT,STRICT';
> +Warnings:
> +Warning	1607	Ambiguous slave modes combination. STRICT will be used
> +show warnings /* must be the warning */;
> +Level	Code	Message
> +Warning	1607	Ambiguous slave modes combination. STRICT will be used
> +select @@global.slave_exec_mode /* must be STRICT */;
> +@@global.slave_exec_mode
> +STRICT
> +foreign keys errors as above now forces to stop
> +set foreign_key_checks=0;
> +drop table ti2, ti1;
> +create table ti1 (b int primary key) engine = innodb;
> +create table ti2 (a int primary key, b int, foreign key (b) references ti1(b))
> +engine = innodb;
> +set foreign_key_checks=1 /* ensure the check */;
> +insert into ti1 values (1),(2),(3);
> +insert into ti2 set a=2, b=2;
> +select * from ti1 order by b /* must be (1),(2),(3) */;
> +b
> +1
> +2
> +3
> +conspire future problem
> +insert into ti2 set a=1, b=1;
> +select * from ti2 order by b /* must be (1,1) (2,2) */;
> +a	b
> +1	1
> +2	2
> +delete from ti1 where b=1 /* offending delete event */;
> +select * from ti1 order by b /* must be (2),(3) */;
> +b
> +2
> +3
> +slave must stop
> +Last_SQL_Error
> +0
> +select * from ti1 order by b /* must be (1),(2),(3) - not deleted */;
> +b
> +1
> +2
> +3
> +set foreign_key_checks= 0;
> +delete from ti2 where b=1;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +conspire the following insert failure
> +conspire future problem
> +delete from ti1 where b=3;
> +insert into ti2 set a=3, b=3 /* offending write event */;
> +slave must stop
> +Last_SQL_Error
> +1452
> +select * from ti2 order by b /* must be (2,2) */;
> +a	b
> +2	2
> +set foreign_key_checks= 0;
> +insert into ti1 set b=3;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +select * from ti2 order by b /* must be (2,2),(3,3) */;
> +a	b
> +2	2
> +3	3
> +other errors
> +conspiring query
> +insert into ti1 set b=1;
> +insert into ti1 set b=1 /* offending write event */;
> +slave must stop
> +Last_SQL_Error
> +1062
> +set foreign_key_checks= 0;
> +delete from ti1 where b=1;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +CREATE TABLE t1 (a INT PRIMARY KEY);
> +CREATE TABLE t2 (a INT);
> +INSERT INTO t1 VALUES (-1),(-2),(-3);
> +INSERT INTO t2 VALUES (-1),(-2),(-3);
> +DELETE FROM t1 WHERE a = -2;
> +DELETE FROM t2 WHERE a = -2;
> +DELETE FROM t1 WHERE a = -2;
> +slave must stop
> +Last_SQL_Error
> +1032
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +DELETE FROM t2 WHERE a = -2;
> +slave must stop
> +Last_SQL_Error
> +0
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +UPDATE t1 SET a = 1 WHERE a = -1;
> +UPDATE t2 SET a = 1 WHERE a = -1;
> +UPDATE t1 SET a = 1 WHERE a = -1;
> +slave must stop
> +Last_SQL_Error
> +1032
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +UPDATE t2 SET a = 1 WHERE a = -1;
> +slave must stop
> +Last_SQL_Error
> +0
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +set global slave_exec_mode='STRICT';
> +set @@session.binlog_format= @save_binlog_format;
> +drop table t1,t2,ti2,ti1;
> +end of tests
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_ignore_table.result
> b/mysql-test/suite/rpl/r/rpl_ignore_table.result
> --- a/mysql-test/suite/rpl/r/rpl_ignore_table.result	2007-06-27 15:27:32 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_ignore_table.result	2007-11-20 15:15:36 +02:00
> @@ -117,10 +117,14 @@ Grants for mysqltest4@localhost
>  GRANT USAGE ON *.* TO 'mysqltest4'@'localhost' IDENTIFIED BY PASSWORD
> '*196BDEDE2AE4F84CA44C47D54D78478C7E2BD7B7'
>  drop table t1, t4, mysqltest2.t2;
>  drop database mysqltest2;
> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= mixed;
>  delete from mysql.user where user like "mysqltest%";
> +set @@session.binlog_format= @save_binlog_format;
>  delete from mysql.db where user like "mysqltest%";
>  delete from mysql.columns_priv where user like "mysqltest%";
>  delete from mysql.tables_priv where user like "mysqltest%";
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  delete from mysql.tables_priv where user like "mysqltest%";
>  DROP TABLE IF EXISTS t5;
>  CREATE TABLE t5 (
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_USER.result
> b/mysql-test/suite/rpl/r/rpl_row_USER.result
> --- a/mysql-test/suite/rpl/r/rpl_row_USER.result	2007-06-27 15:27:30 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_USER.result	2007-11-20 15:15:37 +02:00
> @@ -31,7 +31,10 @@ a	users
>  2	@localhost%
>  3	tester@localhost
>  4	@localhost%
> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= mixed;
>  DROP DATABASE mysqltest1;
> +set @@session.binlog_format= @save_binlog_format;
>  REVOKE ALL ON mysqltest1.* FROM 'tester'@'%';
>  REVOKE ALL ON mysqltest1.* FROM ''@'localhost%';
>  DROP USER tester@'%';
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result
> b/mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result
> --- a/mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result	2007-10-24 17:02:31 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result	2007-11-20 15:15:37 +02:00
> @@ -257,6 +257,7 @@ SELECT * FROM t1 ORDER BY a;
>  a	b
>  2	master,slave
>  5	slave
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  **** On Master ****
>  UPDATE t1 SET a = 5, b = 'master' WHERE a = 1;
>  SELECT * FROM t1 ORDER BY a;
> @@ -264,6 +265,7 @@ a	b
>  2	master,slave
>  5	master
>  **** On Slave ****
> +set @@global.slave_exec_mode= default;
>  Last_SQL_Error
>  
>  SELECT * FROM t1 ORDER BY a;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result
> b/mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result
> --- a/mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result	2007-06-27 15:27:28 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result	2007-11-20 15:15:37 +02:00
> @@ -370,6 +370,7 @@ C1	C2
>  1	3
>  2	6
>  3	9
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  --- on master: new values inserted ---
>  INSERT INTO t7 VALUES (1,2), (2,4), (3,6);
>  SELECT * FROM t7 ORDER BY C1;
> @@ -377,6 +378,7 @@ C1	C2
>  1	2
>  2	4
>  3	6
> +set @@global.slave_exec_mode= 'STRICT';
>  --- on slave: old values should be overwritten by replicated values ---
>  SELECT * FROM t7 ORDER BY C1;
>  C1	C2
> @@ -406,8 +408,10 @@ a	b	c
>  2	4	6
>  3	6	9
>  99	99	99
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  --- on master ---
>  INSERT INTO t8 VALUES (2,4,8);
> +set @@global.slave_exec_mode= default;
>  --- on slave ---
>  SELECT * FROM t8 ORDER BY a;
>  a	b	c
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result
> b/mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result
> --- a/mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result	2007-06-27 15:27:32 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result	2007-11-20 15:15:37 +02:00
> @@ -370,6 +370,7 @@ C1	C2
>  1	3
>  2	6
>  3	9
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  --- on master: new values inserted ---
>  INSERT INTO t7 VALUES (1,2), (2,4), (3,6);
>  SELECT * FROM t7 ORDER BY C1;
> @@ -377,6 +378,7 @@ C1	C2
>  1	2
>  2	4
>  3	6
> +set @@global.slave_exec_mode= 'STRICT';
>  --- on slave: old values should be overwritten by replicated values ---
>  SELECT * FROM t7 ORDER BY C1;
>  C1	C2
> @@ -406,8 +408,10 @@ a	b	c
>  2	4	6
>  3	6	9
>  99	99	99
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  --- on master ---
>  INSERT INTO t8 VALUES (2,4,8);
> +set @@global.slave_exec_mode= default;
>  --- on slave ---
>  SELECT * FROM t8 ORDER BY a;
>  a	b	c
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_mystery22.result
> b/mysql-test/suite/rpl/r/rpl_row_mystery22.result
> --- a/mysql-test/suite/rpl/r/rpl_row_mystery22.result	2007-06-27 15:27:32 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_mystery22.result	2007-11-20 15:15:37 +02:00
> @@ -5,6 +5,7 @@ reset slave;
>  drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>  start slave;
>  create table t1(n int auto_increment primary key, s char(10));
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  insert into t1 values (2,'old');
>  insert into t1 values(NULL,'new');
>  insert into t1 values(NULL,'new');
> @@ -28,3 +29,4 @@ n	s
>  1	new
>  3	new
>  drop table t1;
> +set @@global.slave_exec_mode= default;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result
> b/mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result
> --- a/mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result	2007-09-25 18:44:47
> +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result	2007-11-20 15:15:37
> +02:00
> @@ -37,6 +37,7 @@ ALTER TABLE t8 ADD e1 INT NOT NULL DEFAU
>  ADD e3 INT NOT NULL DEFAULT 0, ADD e4 INT NOT NULL DEFAULT 0,
>  ADD e5 INT NOT NULL DEFAULT 0, ADD e6 INT NOT NULL DEFAULT 0,
>  ADD e7 INT NOT NULL DEFAULT 0, ADD e8 INT NOT NULL DEFAULT 0;
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  INSERT INTO t1_int  VALUES (2, 4, 4711);
>  INSERT INTO t1_char VALUES (2, 4, 'Foo is a bar');
>  INSERT INTO t1_bit  VALUES (2, 4, b'101', b'11100', b'01');
> @@ -60,6 +61,7 @@ a	b
>  1	2
>  2	5
>  **** On Slave ****
> +set @@global.slave_exec_mode= default;
>  SELECT a,b,x FROM t1_int ORDER BY a;
>  a	b	x
>  1	2	42
> @@ -123,7 +125,7 @@ Replicate_Ignore_Table	
>  Replicate_Wild_Do_Table	
>  Replicate_Wild_Ignore_Table	
>  Last_Errno	1364
> -Last_Error	Error in Write_rows event: error during transaction execution on table
> test.t1_nodef. 
> +Last_Error	Error in Write_rows event: error during transaction execution on table
> test.t1_nodef. ; handler error (null); the event's master log and end_log_pos
> master-bin.000001 2674
>  Skip_Counter	0
>  Exec_Master_Log_Pos	#
>  Relay_Log_Space	#
> @@ -141,7 +143,7 @@ Master_SSL_Verify_Server_Cert	No
>  Last_IO_Errno	0
>  Last_IO_Error	
>  Last_SQL_Errno	1364
> -Last_SQL_Error	Error in Write_rows event: error during transaction execution on
> table test.t1_nodef. 
> +Last_SQL_Error	Error in Write_rows event: error during transaction execution on
> table test.t1_nodef. ; handler error (null); the event's master log and end_log_pos
> master-bin.000001 2674
>  SET GLOBAL SQL_SLAVE_SKIP_COUNTER=2;
>  START SLAVE;
>  INSERT INTO t9 VALUES (2);
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result
> b/mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result
> --- a/mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result	2007-09-25 18:44:47
> +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result	2007-11-20 15:15:38
> +02:00
> @@ -37,6 +37,7 @@ ALTER TABLE t8 ADD e1 INT NOT NULL DEFAU
>  ADD e3 INT NOT NULL DEFAULT 0, ADD e4 INT NOT NULL DEFAULT 0,
>  ADD e5 INT NOT NULL DEFAULT 0, ADD e6 INT NOT NULL DEFAULT 0,
>  ADD e7 INT NOT NULL DEFAULT 0, ADD e8 INT NOT NULL DEFAULT 0;
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  INSERT INTO t1_int  VALUES (2, 4, 4711);
>  INSERT INTO t1_char VALUES (2, 4, 'Foo is a bar');
>  INSERT INTO t1_bit  VALUES (2, 4, b'101', b'11100', b'01');
> @@ -60,6 +61,7 @@ a	b
>  1	2
>  2	5
>  **** On Slave ****
> +set @@global.slave_exec_mode= default;
>  SELECT a,b,x FROM t1_int ORDER BY a;
>  a	b	x
>  1	2	42
> @@ -123,7 +125,7 @@ Replicate_Ignore_Table	
>  Replicate_Wild_Do_Table	
>  Replicate_Wild_Ignore_Table	
>  Last_Errno	1364
> -Last_Error	Error in Write_rows event: error during transaction execution on table
> test.t1_nodef. 
> +Last_Error	Error in Write_rows event: error during transaction execution on table
> test.t1_nodef. ; handler error (null); the event's master log and end_log_pos
> master-bin.000001 2944
>  Skip_Counter	0
>  Exec_Master_Log_Pos	#
>  Relay_Log_Space	#
> @@ -141,7 +143,7 @@ Master_SSL_Verify_Server_Cert	No
>  Last_IO_Errno	0
>  Last_IO_Error	
>  Last_SQL_Errno	1364
> -Last_SQL_Error	Error in Write_rows event: error during transaction execution on
> table test.t1_nodef. 
> +Last_SQL_Error	Error in Write_rows event: error during transaction execution on
> table test.t1_nodef. ; handler error (null); the event's master log and end_log_pos
> master-bin.000001 2944
>  SET GLOBAL SQL_SLAVE_SKIP_COUNTER=2;
>  START SLAVE;
>  INSERT INTO t9 VALUES (2);
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_temporary_errors.result
> b/mysql-test/suite/rpl/r/rpl_temporary_errors.result
> --- a/mysql-test/suite/rpl/r/rpl_temporary_errors.result	2007-10-24 17:02:32 +03:00
> +++ b/mysql-test/suite/rpl/r/rpl_temporary_errors.result	2007-11-20 15:15:38 +02:00
> @@ -12,6 +12,7 @@ INSERT INTO t1 VALUES (1,1), (2,2), (3,3
>  SHOW STATUS LIKE 'Slave_retried_transactions';
>  Variable_name	Value
>  Slave_retried_transactions	0
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  UPDATE t1 SET a = 5, b = 47 WHERE a = 1;
>  SELECT * FROM t1;
>  a	b
> @@ -28,6 +29,7 @@ a	b
>  3	3
>  4	4
>  **** On Slave ****
> +set @@global.slave_exec_mode= default;
>  SHOW STATUS LIKE 'Slave_retried_transactions';
>  Variable_name	Value
>  Slave_retried_transactions	0
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_idempotency-master.opt
> b/mysql-test/suite/rpl/t/rpl_idempotency-master.opt
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_idempotency-master.opt	2007-11-20 15:15:42 +02:00
> @@ -0,0 +1,2 @@
> +--innodb
> +
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_idempotency-slave.opt
> b/mysql-test/suite/rpl/t/rpl_idempotency-slave.opt
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_idempotency-slave.opt	2007-11-20 15:15:42 +02:00
> @@ -0,0 +1,2 @@
> +--slave-exec-mode=IDEMPOTENT --innodb
> +
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_idempotency.test
> b/mysql-test/suite/rpl/t/rpl_idempotency.test
> --- a/mysql-test/suite/rpl/t/rpl_idempotency.test	2007-10-30 22:17:14 +02:00
> +++ b/mysql-test/suite/rpl/t/rpl_idempotency.test	2007-11-20 15:15:38 +02:00
> @@ -77,3 +77,332 @@ enable_query_log;
>  connection master;
>  DROP TABLE t1, t2;
>  sync_slave_with_master;
> +
> +# bug#31609 Not all RBR slave errors reported as errors
> +# bug#31552 Replication breaks when deleting rows from out-of-sync table
> +#           without PK
> +
> +#
> +# Idemptonet applying is not default any longer.
>   

Typo "Idemptonet".

> +# Unless --slave-exec-mode option or the namesake global system variable
> +# slave_exec_mode specifies explicitly IDEMPTONET the actual value
> +# of slave_exec_mode is STRICT.

Typo .

Suggest using "The default for slave-exec-mode option and server 
variable slave_exec_mode  is 'STRICT'" instead.

> That forces the slave sql thread to stop
> +# when one has faced any error including one from the "idempotent" set.
>   

"When 'STRICT' mode is set, the slave SQL thread will stop whenever the 
row to change is not found. In 'IDEMPOTENT' mode, the SQL thread will 
continue running and apply the row if the change is deemed idempotent."

> +#
> +
> +# the previous part of the tests was with IDEMPOTENT slave's mode.
> +
> +
> +#
> +# Other than above idempotent errors dealing with foreign keys constraint
> +#
> +
> +select @@global.slave_exec_mode /* must be IDEMPOTENT */;
> +
> +connection master;
> +
> +create table ti1 (b int primary key) engine = innodb;
> +create table ti2 (a int primary key, b int, foreign key (b) references ti1(b))
> +     engine = innodb;
> +set foreign_key_checks=1 /* ensure the check */;
> +
> +insert into ti1 values (1),(2),(3);
> +insert into ti2 set a=2, b=2;
> +
> +sync_slave_with_master;
> +
> +#connection slave;
>   

Redundant row. Not necessary to change.

> +select * from ti1 order by b /* must be (1),(2),(3) */;
> +insert into ti2 set a=1, b=1;
> +select * from ti2 order by b /* must be (1,1) (2,2) */;
> +
> +connection master;
> +
> +# from now on checking rbr specific idempotent errors
>   

RBR is an acronym, i.e., use capital letters. Not necessary to change.

> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= row;
> +delete from ti1 where b=1;
> +
> +select * from ti1 order by b /* must be (2),(3) */;
> +
> +# slave must catch up (expect some warnings in error.log)
> +sync_slave_with_master;
> +
> +#connection slave;
> +select * from ti1 order by b /* must stays as were on master (1),(2),(3) */;
>   

"must be as on master ..."

> +
> +--echo consipire the following insert failure
>   

Please qualify comments written to the output of the test case with some 
marker so that they are easily distinguishable from the surrounding 
statements and results. For example "****" is used elsewhere. Also, what 
do you mean with "conspire" (apart from that it's a typo). "Conspire" 
basically means to plan something evil or illegal... I suggest to use 
another word for whatever you want to describe.

> +delete from ti1 where b=3;
> +
> +connection master;
> +insert into ti2 set a=3, b=3;
> +
> +# slave must catch up (expect some warnings in error.log)
> +sync_slave_with_master;
> +
> +#connection slave;
> +select * from ti2 order by b /* must be (1,1),(2,2) - not inserted */;
> +
> +
> +#
> +# Checking the new global sys variable
> +#
> +
> +connection slave;
> +
> +set global slave_exec_mode='STRICT';
> +set global slave_exec_mode='IDEMPOTENT';
> +
> +# checking mutual exclusion for the options
> +set global slave_exec_mode='IDEMPOTENT,STRICT';
> +show warnings /* must be the warning */;
> +
> +select @@global.slave_exec_mode /* must be STRICT */;
> +
> +#
> +# Checking stops
> +#
> +
> +#
> +--echo foreign keys errors as above now forces to stop
>   

See above.

> +#
> +
> +connection master;
> +
> +set foreign_key_checks=0;
> +drop table ti2, ti1;
> +
> +create table ti1 (b int primary key) engine = innodb;
> +create table ti2 (a int primary key, b int, foreign key (b) references ti1(b))
> +     engine = innodb;
> +set foreign_key_checks=1 /* ensure the check */;
> +
> +insert into ti1 values (1),(2),(3);
> +insert into ti2 set a=2, b=2;
> +
> +sync_slave_with_master;
> +
> +#connection slave;
> +select * from ti1 order by b /* must be (1),(2),(3) */;
> +--echo conspire future problem
>   

See above.

> +insert into ti2 set a=1, b=1;
> +select * from ti2 order by b /* must be (1,1) (2,2) */;
> +
> +connection master;
> +
> +delete from ti1 where b=1 /* offending delete event */;
> +select * from ti1 order by b /* must be (2),(3) */;
> +
> +# foreign key: row is referenced
> +
> +--echo slave must stop
>   

See above.

> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +select * from ti1 order by b /* must be (1),(2),(3) - not deleted */;
> +set foreign_key_checks= 0;
> +delete from ti2 where b=1;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +connection master;
> +
> +sync_slave_with_master;
> +
> +#connection slave;
> +--echo conspire the following insert failure
>   

See above.

> +# foreign key: no referenced row
> +
> +--echo conspire future problem
>   

See above.

> +delete from ti1 where b=3;
> +
> +connection master;
> +insert into ti2 set a=3, b=3 /* offending write event */;
> +--echo slave must stop
>   

See above. IMHO, you don't need this in the test output.

> +
> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +select * from ti2 order by b /* must be (2,2) */;
> +set foreign_key_checks= 0;
> +insert into ti1 set b=3;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +connection master;
> +
> +sync_slave_with_master;
> +
> +select * from ti2 order by b /* must be (2,2),(3,3) */;
> +
> +# 
> +--echo other errors
>   

See above.

> +# 
> +
> +# dup key insert
> +
> +#connection slave;
> +--echo conspiring query
>   

See above.

> +insert into ti1 set b=1;
> +
> +connection master;
> +insert into ti1 set b=1 /* offending write event */;
> +
> +--echo slave must stop
>   

See above.

> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +set foreign_key_checks= 0;
> +delete from ti1 where b=1;
> +set foreign_key_checks= 1;
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +# key not found
> +
> +connection master;
> +
> +CREATE TABLE t1 (a INT PRIMARY KEY);
> +CREATE TABLE t2 (a INT);
> +INSERT INTO t1 VALUES (-1),(-2),(-3);
> +INSERT INTO t2 VALUES (-1),(-2),(-3);
> +sync_slave_with_master;
> +
> +#connection slave;
> +DELETE FROM t1 WHERE a = -2;
> +DELETE FROM t2 WHERE a = -2;
> +connection master;
> +DELETE FROM t1 WHERE a = -2;
> +
> +--echo slave must stop
> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +connection master;
> +DELETE FROM t2 WHERE a = -2;
> +--echo slave must stop
>   

See above.

> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +UPDATE t1 SET a = 1 WHERE a = -1;
> +UPDATE t2 SET a = 1 WHERE a = -1;
> +
> +connection master;
> +UPDATE t1 SET a = 1 WHERE a = -1;
> +
> +--echo slave must stop
>   

See above.

> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +
> +connection master;
> +UPDATE t2 SET a = 1 WHERE a = -1;
> +
> +--echo slave must stop
>   

See above.

> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +connection slave;
> +
> +let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1);
> +disable_query_log;
> +eval SELECT "$last_error" AS Last_SQL_Error;
> +enable_query_log;
> +
> +set global slave_exec_mode='IDEMPOTENT';
> +start slave sql_thread;
> +connection master;
> +sync_slave_with_master;
> +#connection slave;
> +set global slave_exec_mode='STRICT';
> +
> +
> +# cleanup for bug#31609 tests
> +
> +connection master;
> +set @@session.binlog_format= @save_binlog_format;
> +drop table t1,t2,ti2,ti1;
> +
> +sync_slave_with_master;
> +
> +
> +--echo end of tests
> +
> +
> +
> +
> +
> +
> +
> +
> +
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_ignore_table.test
> b/mysql-test/suite/rpl/t/rpl_ignore_table.test
> --- a/mysql-test/suite/rpl/t/rpl_ignore_table.test	2007-06-27 15:27:30 +03:00
> +++ b/mysql-test/suite/rpl/t/rpl_ignore_table.test	2007-11-20 15:15:38 +02:00
> @@ -122,8 +122,12 @@ show grants for mysqltest4@localhost;
>  connection master;
>  drop table t1, t4, mysqltest2.t2;
>  drop database mysqltest2;
> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= mixed;
>  delete from mysql.user where user like "mysqltest%";
> +set @@session.binlog_format= @save_binlog_format;
>  delete from mysql.db where user like "mysqltest%";
> +
>  #
>  # BUG 27606 causes failure to replicate this statement
>  # move it to slave instead
> @@ -133,6 +137,10 @@ sync_slave_with_master;
>  
>  #BUG27606
>  delete from mysql.tables_priv where user like "mysqltest%";
> +
> +# bug#31552/31609 idempotency made idempotent non-default so it has to be set
> +# because of the inconsistency just have done on slave
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  
>  connection master;
>  
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_row_USER.test
> b/mysql-test/suite/rpl/t/rpl_row_USER.test
> --- a/mysql-test/suite/rpl/t/rpl_row_USER.test	2007-06-27 15:27:26 +03:00
> +++ b/mysql-test/suite/rpl/t/rpl_row_USER.test	2007-11-20 15:15:38 +02:00
> @@ -49,7 +49,11 @@ connection master;
>  # Lets cleanup
>  #show binlog events;
>  
> +set @save_binlog_format= @@session.binlog_format;
> +set @@session.binlog_format= mixed;
>  DROP DATABASE mysqltest1;
> +set @@session.binlog_format= @save_binlog_format;
> +
>  REVOKE ALL ON mysqltest1.* FROM 'tester'@'%';
>  REVOKE ALL ON mysqltest1.* FROM ''@'localhost%';
>  DROP USER tester@'%';
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_row_basic_11bugs.test
> b/mysql-test/suite/rpl/t/rpl_row_basic_11bugs.test
> --- a/mysql-test/suite/rpl/t/rpl_row_basic_11bugs.test	2007-10-24 17:02:32 +03:00
> +++ b/mysql-test/suite/rpl/t/rpl_row_basic_11bugs.test	2007-11-20 15:15:39 +02:00
> @@ -242,12 +242,15 @@ INSERT INTO t1 VALUES (1,'master,slave')
>  sync_slave_with_master;
>  UPDATE t1 SET a = 5, b = 'slave' WHERE a = 1;
>  SELECT * FROM t1 ORDER BY a;
> +# bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  --echo **** On Master ****
>  connection master;
>  UPDATE t1 SET a = 5, b = 'master' WHERE a = 1;
>  SELECT * FROM t1 ORDER BY a;
>  --echo **** On Slave ****
>  sync_slave_with_master;
> +set @@global.slave_exec_mode= default;
>  let $last_error = query_get_value("SHOW SLAVE STATUS", Last_SQL_Error, 1);
>  disable_query_log;
>  eval SELECT "$last_error" AS Last_SQL_Error;
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_row_mystery22.test
> b/mysql-test/suite/rpl/t/rpl_row_mystery22.test
> --- a/mysql-test/suite/rpl/t/rpl_row_mystery22.test	2007-06-27 15:27:25 +03:00
> +++ b/mysql-test/suite/rpl/t/rpl_row_mystery22.test	2007-11-20 15:15:39 +02:00
> @@ -9,6 +9,10 @@
>  # first, cause a duplicate key problem on the slave
>  create table t1(n int auto_increment primary key, s char(10));
>  sync_slave_with_master;
> +
> +# bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
> +
>  insert into t1 values (2,'old');
>  connection master;
>  insert into t1 values(NULL,'new');
> @@ -43,3 +47,4 @@ select * from t1 order by n;
>  connection master;
>  drop table t1;
>  sync_slave_with_master;
> +set @@global.slave_exec_mode= default;
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_temporary_errors.test
> b/mysql-test/suite/rpl/t/rpl_temporary_errors.test
> --- a/mysql-test/suite/rpl/t/rpl_temporary_errors.test	2007-10-24 17:02:32 +03:00
> +++ b/mysql-test/suite/rpl/t/rpl_temporary_errors.test	2007-11-20 15:15:39 +02:00
> @@ -8,6 +8,8 @@ INSERT INTO t1 VALUES (1,1), (2,2), (3,3
>  --echo **** On Slave ****
>  sync_slave_with_master;
>  SHOW STATUS LIKE 'Slave_retried_transactions';
> +# bug#31552/31609 idemptency is not default any longer
> +set @@global.slave_exec_mode= 'IDEMPOTENT';
>  UPDATE t1 SET a = 5, b = 47 WHERE a = 1;
>  SELECT * FROM t1;
>  --echo **** On Master ****
> @@ -17,6 +19,7 @@ SELECT * FROM t1;
>  #SHOW BINLOG EVENTS;
>  --echo **** On Slave ****
>  sync_slave_with_master;
> +set @@global.slave_exec_mode= default;
>  SHOW STATUS LIKE 'Slave_retried_transactions';
>  SELECT * FROM t1;
>  source include/show_slave_status.inc;
> diff -Nrup a/mysql-test/suite/rpl_ndb/r/rpl_ndb_dd_advance.result
> b/mysql-test/suite/rpl_ndb/r/rpl_ndb_dd_advance.result
> --- a/mysql-test/suite/rpl_ndb/r/rpl_ndb_dd_advance.result	2007-07-04 23:06:25
> +03:00
> +++ b/mysql-test/suite/rpl_ndb/r/rpl_ndb_dd_advance.result	2007-11-20 15:15:39
> +02:00
> @@ -301,6 +301,7 @@ SELECT COUNT(*) FROM history;
>  COUNT(*)
>  400
>  *** DUMP MASTER & SLAVE FOR COMPARE ********
> +set @@global.slave_exec_mode='IDEMPOTENT' /* to ignore missed records in mysql.proc
> */;
>  *************** TEST 2 CLEANUP SECTION ********************
>  DROP PROCEDURE IF EXISTS tpcb.load;
>  DROP PROCEDURE IF EXISTS tpcb.trans;
> diff -Nrup a/mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test
> b/mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test
> --- a/mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test	2007-07-25 16:29:29 +03:00
> +++ b/mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test	2007-11-20 15:15:39 +02:00
> @@ -373,6 +373,7 @@ SELECT COUNT(*) FROM history;
>  --echo ****** SLAVE ********
>  --sync_slave_with_master
>  connection slave;
> +
>  USE tpcb;
>  SELECT COUNT(*) FROM history;
>  
> @@ -381,6 +382,8 @@ SELECT COUNT(*) FROM history;
>  --exec $MYSQL_DUMP --compact --order-by-primary --skip-extended-insert tpcb account
> teller branch history > $MYSQLTEST_VARDIR/tmp/RPL_DD_ADV_M.sql
>  
>  --exec $MYSQL_DUMP_SLAVE --compact --order-by-primary --skip-extended-insert tpcb
> account teller branch history > $MYSQLTEST_VARDIR/tmp/RPL_DD_ADV_S.sql
> +
> +set @@global.slave_exec_mode='IDEMPOTENT' /* to ignore missed records in mysql.proc
> */;
>  
>  --echo *************** TEST 2 CLEANUP SECTION ********************
>  connection master;
> diff -Nrup a/sql/log_event.cc b/sql/log_event.cc
> --- a/sql/log_event.cc	2007-10-30 22:23:39 +02:00
> +++ b/sql/log_event.cc	2007-11-20 15:15:39 +02:00
> @@ -36,6 +36,19 @@
>  
>  #define FLAGSTR(V,F) ((V)&(F)?#F" ":"")
>  
> +/**
> +   macro to call from different branches of Rows_log_event::do_apply_event
> +*/
> +#define SLAVE_ROWS_REPORT(level)  rli->report((level), thd->net.last_errno, \
> +                                          "Error in %s event: error during
> transaction execution " \
> +                                          "on table %s.%s. %s; handler error %s; "
> \
> +                                          "the event's master log and end_log_pos %s
> %lu", \
> +                                          get_type_str(), table->s->db.str, \
> +                                          table->s->table_name.str,     \
> +                                          thd->net.last_error ?
> thd->net.last_error : "", \
> +                                          HA_ERR(error),                \
> +                                          RPL_LOG_NAME, (ulong) log_pos)
>   

I suggest using a function instead of a macro for this case. It is 
easier to debug, and more safe. I also suggest a shorter error message 
with info that is easier to scrape by, e.g., a script. Something along 
these lines:

"Error in %s event: error during transaction execution " \
"on table %s.%s. %s; handler error %s; " \
"master_log_file: %s; master_end_log_pos: %lu"

You should not blindly feed the string pointer directly to the printf() statement, since
some printf() implementations will throw a segmentation fault when fed a NULL pointer. A
suggestion is to print either "handler error '%s'" if HA_ERR(error) is NULL, and "unknown
handler error" otherwise. Other alternatives are also possible, like ensuring that
HA_ERR() does not return NULL. (I noted a "(null)" in such an output above.)


> +
>  #ifndef MYSQL_CLIENT
>  static const char *HA_ERR(int i)
>  {
> @@ -90,6 +103,7 @@ static const char *HA_ERR(int i)
>    case HA_ERR_LOGGING_IMPOSSIBLE: return "HA_ERR_LOGGING_IMPOSSIBLE";
>    case HA_ERR_CORRUPT_EVENT: return "HA_ERR_CORRUPT_EVENT";
>    }
> +  return 0;
>  }
>  #endif
>  
> @@ -6060,7 +6074,6 @@ int Rows_log_event::do_apply_event(Relay
>  {
>    DBUG_ENTER("Rows_log_event::do_apply_event(Relay_log_info*)");
>    int error= 0;
> -
>    /*
>      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
> @@ -6106,6 +6119,24 @@ int Rows_log_event::do_apply_event(Relay
>      */
>      lex_start(thd);
>  
> +    /*
> +      There are a few flags that are replicated with each row event.
> +      Make sure to set/clear them before executing the main body of
> +      the event.
> +    */
> +    if (get_flags(NO_FOREIGN_KEY_CHECKS_F))
> +        thd->options|= OPTION_NO_FOREIGN_KEY_CHECKS;
> +    else
> +        thd->options&= ~OPTION_NO_FOREIGN_KEY_CHECKS;
> +
> +    if (get_flags(RELAXED_UNIQUE_CHECKS_F))
> +        thd->options|= OPTION_RELAXED_UNIQUE_CHECKS;
> +    else
> +        thd->options&= ~OPTION_RELAXED_UNIQUE_CHECKS;
> +    /* A small test to verify that objects have consistent types */
> +    DBUG_ASSERT(sizeof(thd->options) == sizeof(OPTION_RELAXED_UNIQUE_CHECKS));
> +
> +
>      while ((error= lock_tables(thd, rli->tables_to_lock,
>                                 rli->tables_to_lock_count, &need_reopen)))
>      {
> @@ -6240,22 +6271,6 @@ int Rows_log_event::do_apply_event(Relay
>        So we call set_time(), like in SBR. Presently it changes nothing.
>      */
>      thd->set_time((time_t)when);
> -    /*
> -      There are a few flags that are replicated with each row event.
> -      Make sure to set/clear them before executing the main body of
> -      the event.
> -    */
> -    if (get_flags(NO_FOREIGN_KEY_CHECKS_F))
> -        thd->options|= OPTION_NO_FOREIGN_KEY_CHECKS;
> -    else
> -        thd->options&= ~OPTION_NO_FOREIGN_KEY_CHECKS;
> -
> -    if (get_flags(RELAXED_UNIQUE_CHECKS_F))
> -        thd->options|= OPTION_RELAXED_UNIQUE_CHECKS;
> -    else
> -        thd->options&= ~OPTION_RELAXED_UNIQUE_CHECKS;
> -    /* A small test to verify that objects have consistent types */
> -    DBUG_ASSERT(sizeof(thd->options) == sizeof(OPTION_RELAXED_UNIQUE_CHECKS));
>  
>      /*
>        Now we are in a statement and will stay in a statement until we
> @@ -6288,8 +6303,9 @@ int Rows_log_event::do_apply_event(Relay
>      if (!get_flags(COMPLETE_ROWS_F))
>        bitmap_intersect(table->write_set,&m_cols);
>  
> +    this->slave_exec_mode= slave_exec_mode_options; // fix the mode
> +
>      // Do event specific preparations 
> -    
>      error= do_before_row_operations(rli);
>  
>      // row processing loop
> @@ -6308,22 +6324,39 @@ int Rows_log_event::do_apply_event(Relay
>        {
>        case 0:
>  	break;
> +      /*
> +        The following list of "idempotent" errors
> +        means that an error from the list might happen
> +        because of idempotent (more than once) 
> +        applying of a binlog file.
> +        Notice, that binlog has a  ddl operation its
>   

DDL is an acronym.

> +        second applying may cause
>   

s/applying/application/

>  
> -      /* Some recoverable errors */
> +        case HA_ERR_TABLE_DEF_CHANGED:
> +        case HA_ERR_CANNOT_ADD_FOREIGN:
> +        
> +        which are not included into to the list.
> +      */
>        case HA_ERR_RECORD_CHANGED:
>        case HA_ERR_RECORD_DELETED:
>        case HA_ERR_KEY_NOT_FOUND:
>        case HA_ERR_END_OF_FILE:
> -        /* Idempotency support: OK if tuple does not exist */
> +      case HA_ERR_FOUND_DUPP_KEY:
> +      case HA_ERR_FOUND_DUPP_UNIQUE:
> +      case HA_ERR_FOREIGN_DUPLICATE_KEY:
> +      case HA_ERR_NO_REFERENCED_ROW:
> +      case HA_ERR_ROW_IS_REFERENCED:
> +
>          DBUG_PRINT("info", ("error: %s", HA_ERR(error)));
> -        error= 0;
> +        if (is_bit_set(slave_exec_mode, SLAVE_MODE_IDEMPOTENT) != 0)
> +        {
> +          if (global_system_variables.log_warnings)
> +            SLAVE_ROWS_REPORT(WARNING_LEVEL);
> +          error= 0;
> +        }
>          break;
> -
> +        
>        default:
> -	rli->report(ERROR_LEVEL, thd->net.last_errno,
> -                    "Error in %s event: row application failed. %s",
> -                    get_type_str(),
> -                    thd->net.last_error ? thd->net.last_error : "");
>  	thd->query_error= 1;
>  	break;
>        }
> @@ -6367,16 +6400,13 @@ int Rows_log_event::do_apply_event(Relay
>    */
>    if (rli->tables_to_lock && get_flags(STMT_END_F))
>      const_cast<Relay_log_info*>(rli)->clear_tables_to_lock();
> -
> +  
>    if (error)
>    {                     /* error has occured during the transaction */
> -    rli->report(ERROR_LEVEL, thd->net.last_errno,
> -                "Error in %s event: error during transaction execution "
> -                "on table %s.%s. %s",
> -                get_type_str(), table->s->db.str,
> -                table->s->table_name.str,
> -                thd->net.last_error ? thd->net.last_error : "");
> -
> +    SLAVE_ROWS_REPORT(ERROR_LEVEL);
> +  }
> +  if (error)
> +  {
>      /*
>        If one day we honour --skip-slave-errors in row-based replication, and
>        the error should be skipped, then we would clear mappings, rollback,
> @@ -6488,6 +6518,7 @@ Rows_log_event::do_update_pos(Relay_log_
>      */
>  
>      thd->reset_current_stmt_binlog_row_based();
> +
>      rli->cleanup_context(thd, 0);
>      if (error == 0)
>      {
> @@ -7170,43 +7201,48 @@ Write_rows_log_event::do_before_row_oper
>  {
>    int error= 0;
>  
> -  /*
> -    We are using REPLACE semantics and not INSERT IGNORE semantics
> -    when writing rows, that is: new rows replace old rows.  We need to
> -    inform the storage engine that it should use this behaviour.
> -  */
> -
> -  /* Tell the storage engine that we are using REPLACE semantics. */
> -  thd->lex->duplicates= DUP_REPLACE;
> +  if (is_bit_set(slave_exec_mode, SLAVE_MODE_IDEMPOTENT) != 0 ||
> +      m_table->s->db_type()->db_type == DB_TYPE_NDBCLUSTER)
>   

Here you're hard-coding for the NBD engine, which is not flexible at 
all. I would suggest adding a property to the handler (e.g., a table 
flag or a virtual function) and check that instead. That would mean that 
other engines that need the same feature can just set that  flag or 
override that function.

> +  {
> +    /*
> +      We are using REPLACE semantics and not INSERT IGNORE semantics
> +      when writing rows, that is: new rows replace old rows.  We need to
> +      inform the storage engine that it should use this behaviour.
> +    */
> +    
> +    /* Tell the storage engine that we are using REPLACE semantics. */
> +    thd->lex->duplicates= DUP_REPLACE;
> +    
> +    /*
> +      Pretend we're executing a REPLACE command: this is needed for
> +      InnoDB and NDB Cluster since they are not (properly) checking the
> +      lex->duplicates flag.
> +    */
> +    thd->lex->sql_command= SQLCOM_REPLACE;
> +    /* 
> +       Do not raise the error flag in case of hitting to an unique attribute
> +    */
> +    m_table->file->extra(HA_EXTRA_IGNORE_DUP_KEY);
> +    /* 
> +       NDB specific: update from ndb master wrapped as Write_rows
> +    */
> +    /*
> +      so that the event should be applied to replace slave's row
> +    */
>   

Merge these two comments.

> +    m_table->file->extra(HA_EXTRA_WRITE_CAN_REPLACE);
> +    /* 
> +       NDB specific: if update from ndb master wrapped as Write_rows
> +       does not find the row it's assumed idempotent binlog applying
> +       is taking place; don't raise the error.
> +    */
> +    m_table->file->extra(HA_EXTRA_IGNORE_NO_KEY);
> +    /*
> +      TODO: the cluster team (Tomas?) says that it's better if the engine knows
> +      how many rows are going to be inserted, then it can allocate needed memory
> +      from the start.
> +    */
> +  }
>  
> -  /*
> -    Pretend we're executing a REPLACE command: this is needed for
> -    InnoDB and NDB Cluster since they are not (properly) checking the
> -    lex->duplicates flag.
> -  */
> -  thd->lex->sql_command= SQLCOM_REPLACE;
> -  /* 
> -     Do not raise the error flag in case of hitting to an unique attribute
> -  */
> -  m_table->file->extra(HA_EXTRA_IGNORE_DUP_KEY);
> -  /* 
> -     NDB specific: update from ndb master wrapped as Write_rows
> -  */
> -  /*
> -    so that the event should be applied to replace slave's row
> -  */
> -  m_table->file->extra(HA_EXTRA_WRITE_CAN_REPLACE);
> -  /* 
> -     NDB specific: if update from ndb master wrapped as Write_rows
> -     does not find the row it's assumed idempotent binlog applying
> -     is taking place; don't raise the error.
> -  */
> -  m_table->file->extra(HA_EXTRA_IGNORE_NO_KEY);
> -  /*
> -    TODO: the cluster team (Tomas?) says that it's better if the engine knows
> -    how many rows are going to be inserted, then it can allocate needed memory
> -    from the start.
> -  */
>    m_table->file->ha_start_bulk_insert(0);
>    /*
>      We need TIMESTAMP_NO_AUTO_SET otherwise ha_write_row() will not use fill
> @@ -7228,18 +7264,22 @@ Write_rows_log_event::do_before_row_oper
>  }
>  
>  int 
> -Write_rows_log_event::do_after_row_operations(const Slave_reporting_capability
> *const, 
> +Write_rows_log_event::do_after_row_operations(const Slave_reporting_capability
> *const,
>                                                int error)
>  {
>    int local_error= 0;
> -  m_table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
> -  m_table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE);
> -  /*
> -    reseting the extra with 
> -    table->file->extra(HA_EXTRA_NO_IGNORE_NO_KEY); 
> -    fires bug#27077
> -    todo: explain or fix
> -  */
> +  if (is_bit_set(slave_exec_mode, SLAVE_MODE_IDEMPOTENT) != 0)
> +  {
>   
> +    m_table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
> +    m_table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE);
>   

The corresponding do_before_row_operations() above is guarded by the 
slave_exec_mode and the database engine (i.e., NDB), but here they are 
not guarded by the check if this is for NDB. Is this correct? Shouldn't 
the "setting" of the extra flags have a corresponding "resetting" of the 
flag?

> +    /*
> +      reseting the extra with 
>   

s/reseting/resetting/

> +      table->file->extra(HA_EXTRA_NO_IGNORE_NO_KEY); 
> +      fires bug#27077
> +      explanation: file->reset() performs this duty
> +      ultimately. Still todo: fix
> +    */
> +  }
>    if ((local_error= m_table->file->ha_end_bulk_insert()))
>    {
>      m_table->file->print_error(local_error, MYF(0));
> @@ -7358,23 +7398,22 @@ Rows_log_event::write_row(const Relay_lo
>  
>    while ((error= table->file->ha_write_row(table->record[0])))
>    {
> -    if (error == HA_ERR_LOCK_DEADLOCK || error == HA_ERR_LOCK_WAIT_TIMEOUT)
> -    {
> -      table->file->print_error(error, MYF(0)); /* to check at
> exec_relay_log_event */
> -      DBUG_RETURN(error);
> -    }
> -    if ((keynum= table->file->get_dup_key(error)) < 0)
> +    if (error == HA_ERR_LOCK_DEADLOCK ||
> +        error == HA_ERR_LOCK_WAIT_TIMEOUT ||
> +        (keynum= table->file->get_dup_key(error)) < 0 ||
> +        !overwrite)
>      {
> -      DBUG_PRINT("info",("Can't locate duplicate key (get_dup_key returns
> %d)",keynum));
> -      table->file->print_error(error, MYF(0));
> +      DBUG_PRINT("info",("get_dup_key returns %d)", keynum));
>        /*
> -        We failed to retrieve the duplicate key
> +        Deadlock, waiting for lock or just an error from the handler
> +        such as HA_ERR_FOUND_DUPP_KEY when overwrite is false.
> +        Retrieval of the duplicate key number may fail
>          - either because the error was not "duplicate key" error
>          - or because the information which key is not available
>        */
> +      table->file->print_error(error, MYF(0));
>        DBUG_RETURN(error);
>      }
> -
>      /*
>         We need to retrieve the old row into record[1] to be able to
>         either update or delete the offending record.  We either:
> @@ -7512,11 +7551,11 @@ int 
>  Write_rows_log_event::do_exec_row(const Relay_log_info *const rli)
>  {
>    DBUG_ASSERT(m_table != NULL);
> -  int error= write_row(rli, TRUE /* overwrite */);
> -  
> +  int error= write_row(rli, is_bit_set(slave_exec_mode, SLAVE_MODE_IDEMPOTENT) !=
> 0);
> +    
>   

Suggest adding a comment as it was in the original code, or split out 
the truth value into a variable with an appropriate name. Not necessary 
to change, just a help for maintenance.

>    if (error && !thd->net.last_errno)
>      thd->net.last_errno= error;
> -      
> +  
>    return error; 
>  }
>  
> diff -Nrup a/sql/log_event.h b/sql/log_event.h
> --- a/sql/log_event.h	2007-10-30 10:51:08 +02:00
> +++ b/sql/log_event.h	2007-11-20 15:15:40 +02:00
> @@ -809,6 +809,12 @@ public:
>  
>    bool cache_stmt;
>  
> +  /**
> +    A storage to cache the global system variable's value.
> +    Handling of a separate event will be governed its member.
> +  */
> +  ulong slave_exec_mode;
> +
>  #ifndef MYSQL_CLIENT
>    THD* thd;
>  
> diff -Nrup a/sql/mysql_priv.h b/sql/mysql_priv.h
> --- a/sql/mysql_priv.h	2007-10-23 18:10:26 +03:00
> +++ b/sql/mysql_priv.h	2007-11-20 15:15:40 +02:00
> @@ -1813,6 +1813,7 @@ extern uint volatile thread_count, threa
>  extern my_bool opt_sql_bin_update, opt_safe_user_create, opt_no_mix_types;
>  extern my_bool opt_safe_show_db, opt_local_infile, opt_myisam_use_mmap;
>  extern my_bool opt_slave_compressed_protocol, use_temp_pool;
> +extern ulong slave_exec_mode_options;
>  extern my_bool opt_readonly, lower_case_file_system;
>  extern my_bool opt_enable_named_pipe, opt_sync_frm, opt_allow_suspicious_udfs;
>  extern my_bool opt_secure_auth;
> diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc
> --- a/sql/mysqld.cc	2007-10-30 10:51:08 +02:00
> +++ b/sql/mysqld.cc	2007-11-20 15:15:40 +02:00
> @@ -445,6 +445,8 @@ ulong thread_stack, what_to_log;
>  ulong query_buff_size, slow_launch_time, slave_open_temp_tables;
>  ulong open_files_limit, max_binlog_size, max_relay_log_size;
>  ulong slave_net_timeout, slave_trans_retries;
> +ulong slave_exec_mode_options;
> +const char *slave_exec_mode_str= "STRICT";
>  ulong thread_cache_size=0, thread_pool_size= 0;
>  ulong binlog_cache_size=0, max_binlog_cache_size=0;
>  ulong query_cache_size=0;
> @@ -5101,7 +5103,8 @@ enum options_mysqld
>    OPT_SECURE_FILE_PRIV,
>    OPT_MIN_EXAMINED_ROW_LIMIT,
>    OPT_LOG_SLOW_SLAVE_STATEMENTS,
> -  OPT_OLD_MODE
> +  OPT_OLD_MODE,
> +  OPT_SLAVE_EXEC_MODE
>  };
>  
>  
> @@ -5799,8 +5802,11 @@ replicating a LOAD DATA INFILE command."
>     (uchar**) &slave_load_tmpdir, (uchar**) &slave_load_tmpdir, 0,
> GET_STR_ALLOC,
>     REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
>    {"slave-skip-errors", OPT_SLAVE_SKIP_ERRORS,
> -   "Tells the slave thread to continue replication when a query returns an error
> from the provided list.",
> +   "Tells the slave thread to continue replication when a query event returns an
> error from the provided list.",
>     0, 0, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> +  {"slave-exec-mode", OPT_SLAVE_EXEC_MODE,
> +   "Slave can execute a replication event differently depending on the mode. By
> default, the option's value is STRICT, slave stops always if applying an event has been
> failed. When the value is IDEMPOTENT some failures in applying row-based events are
> tolerated. Namely, those failures that could happen when the slave server executes
> repeatedly a binlog with row-based events from the master.",
>   


You don't need to have an explanation of what idempotency is in the 
options comments, that is more appropriate in the documentation.

Suggest something along the lines:

    Replication slave modes for how events should be executed. Legal values are STRICT
(default) and IDEMPOTENT. In IDEMPOTENT mode, replication will not stop for operations
that are idempotent. In STRICT mode, replication will stop on any unexpected difference
between the master and the slave."

      

> +   (uchar**) &slave_exec_mode_str, (uchar**) &slave_exec_mode_str, 0,
> GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
>  #endif
>    {"slow-query-log", OPT_SLOW_LOG,
>     "Enable|disable slow query log", (uchar**) &opt_slow_log,
> @@ -7109,6 +7115,9 @@ static void mysql_init_variables(void)
>  
>    /* Things with default values that are not zero */
>    delay_key_write_options= (uint) DELAY_KEY_WRITE_ON;
> +  slave_exec_mode_options= 0;
> +  slave_exec_mode_options= (uint)
> +    find_bit_type_or_exit(slave_exec_mode_str, &slave_exec_mode_typelib, NULL);
>    opt_specialflag= SPECIAL_ENGLISH;
>    unix_sock= ip_sock= INVALID_SOCKET;
>    mysql_home_ptr= mysql_home;
> @@ -7318,6 +7327,10 @@ mysqld_get_one_option(int optid,
>    case OPT_SLAVE_SKIP_ERRORS:
>      init_slave_skip_errors(argument);
>      break;
> +  case OPT_SLAVE_EXEC_MODE:
> +    slave_exec_mode_options= (uint)
> +      find_bit_type_or_exit(argument, &slave_exec_mode_typelib, "");
> +    break;
>  #endif
>    case OPT_SAFEMALLOC_MEM_LIMIT:
>  #if !defined(DBUG_OFF) && defined(SAFEMALLOC)
> @@ -7863,6 +7876,8 @@ static void get_options(int *argc,char *
>    }
>    /* Set global MyISAM variables from delay_key_write_options */
>    fix_delay_key_write((THD*) 0, OPT_GLOBAL);
> +  /* Set global slave_exec_mode from its option */
> +  fix_slave_exec_mode((THD*) 0, OPT_GLOBAL);
>  
>  #ifndef EMBEDDED_LIBRARY
>    if (mysqld_chroot)
> diff -Nrup a/sql/rpl_rli.cc b/sql/rpl_rli.cc
> --- a/sql/rpl_rli.cc	2007-10-13 23:12:47 +03:00
> +++ b/sql/rpl_rli.cc	2007-11-20 15:15:41 +02:00
> @@ -1160,6 +1160,11 @@ void Relay_log_info::cleanup_context(THD
>    close_thread_tables(thd);
>    clear_tables_to_lock();
>    clear_flag(IN_STMT);
> +  /*
> +    Cleanup for the flags that have been set at do_apply_event.
> +  */
> +  thd->options&= ~OPTION_NO_FOREIGN_KEY_CHECKS;
> +  thd->options&= ~OPTION_RELAXED_UNIQUE_CHECKS;
>    last_event_start_time= 0;
>    DBUG_VOID_RETURN;
>  }
> diff -Nrup a/sql/set_var.cc b/sql/set_var.cc
> --- a/sql/set_var.cc	2007-10-23 18:10:27 +03:00
> +++ b/sql/set_var.cc	2007-11-20 15:15:41 +02:00
> @@ -88,6 +88,16 @@ TYPELIB delay_key_write_typelib=
>    delay_key_write_type_names, NULL
>  };
>  
> +const char *slave_exec_mode_names[]=
> +{ "STRICT", "IDEMPOTENT", NullS };
> +static const unsigned int slave_exec_mode_names_len[]=
> +{ strlen("STRICT"), strlen("IDEMPOTENT"), 0 };
>   

Please don't use function calls (that is, strlen()) when doing static 
initialization. It's unnecessary and it can also cause portability 
problems. It is also contrary to how initialization is done elsewhere. 
Without having delved deeper into how the lengths are computed, there's 
plenty of code to compute the length of these fields when they are being 
used, so I think that you don't have to add the lengths explicitly.

Could you check to see if this definition works?

    TYPELIB slave_exec_mode_typelib=
    {
      array_elements(slave_exec_mode_names)-1, "",  slave_exec_mode_names, NULL
    };

      

If it does, then you can remove the explicit length array. In either 
case, don't use strlen() inside a static initializer.

> +{
> +  array_elements(slave_exec_mode_names)-1, "",
> +  slave_exec_mode_names, (unsigned int *) slave_exec_mode_names_len
> +};
> +
>  static int  sys_check_ftb_syntax(THD *thd,  set_var *var);
>  static bool sys_update_ftb_syntax(THD *thd, set_var * var);
>  static void sys_default_ftb_syntax(THD *thd, enum_var_type type);
> @@ -408,6 +418,11 @@ static sys_var_const_str_ptr sys_secure_
>  static sys_var_long_ptr	sys_server_id(&vars, "server_id", &server_id,
> fix_server_id);
>  static sys_var_bool_ptr	sys_slave_compressed_protocol(&vars,
> "slave_compressed_protocol",
>  						      &opt_slave_compressed_protocol);
> +static sys_var_set_slave_mode slave_exec_mode(&vars,
> +                                              "slave_exec_mode",
> +                                              &slave_exec_mode_options,
> +                                              &slave_exec_mode_typelib,
> +                                              0);
>  static sys_var_long_ptr	sys_slow_launch_time(&vars, "slow_launch_time",
>  					     &slow_launch_time);
>  static sys_var_thd_ulong	sys_sort_buffer(&vars, "sort_buffer_size",
> @@ -981,6 +996,69 @@ extern void fix_delay_key_write(THD *thd
>    }
>  }
>  
> +bool sys_var_set::update(THD *thd, set_var *var)
> +{
> +  *value= var->save_result.ulong_value;
> +  return 0;
> +};
> +
> +uchar *sys_var_set::value_ptr(THD *thd, enum_var_type type,
> +                              LEX_STRING *base)
> +{
> +  char buff[256];
> +  String tmp(buff, sizeof(buff), &my_charset_latin1);
> +  ulong length;
> +  ulong val= *value;
> +
> +  tmp.length(0);
> +  for (uint i= 0; val; val>>= 1, i++)
> +  {
> +    if (val & 1)
> +    {
> +      tmp.append(enum_names->type_names[i],
> +                 enum_names->type_lengths[i]);
> +      tmp.append(',');
> +    }
> +  }
> +
> +  if ((length= tmp.length()))
> +    length--;
> +  return (uchar*) thd->strmake(tmp.ptr(), length);
> +}
> +
> +void sys_var_set_slave_mode::set_default(THD *thd, enum_var_type type)
> +{
> +  slave_exec_mode_options= 0;
> +  do_set_bit(slave_exec_mode_options, SLAVE_MODE_STRICT);
> +}
> +
> +bool sys_var_set_slave_mode::update(THD *thd, set_var *var)
> +{
> +  my_bool rc;
> +  pthread_mutex_lock(&LOCK_global_system_variables);
> +  rc= sys_var_set::update(thd, var);
> +  fix_slave_exec_mode(thd, OPT_GLOBAL);
> +  pthread_mutex_unlock(&LOCK_global_system_variables);
> +  return rc;
> +}
> +
> +extern void fix_slave_exec_mode(THD *thd, enum_var_type type)
> +{
>   

"extern" is implicit for functions, no need to add it.

> +  if (is_bit_set(slave_exec_mode_options, SLAVE_MODE_STRICT) !=0 &&
> +      is_bit_set(slave_exec_mode_options, SLAVE_MODE_IDEMPOTENT) != 0)
> +  {
> +    if (thd)
> +      push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                          ER_SLAVE_AMBIGOUS_EXEC_MODE,
> ER(ER_SLAVE_AMBIGOUS_EXEC_MODE),
> +                          "STRICT will be used");
> +    else
> +      sql_print_warning("Ambiguous slave modes combination."
> +                        " STRICT will be used");
> +    do_clear_bit(slave_exec_mode_options, SLAVE_MODE_IDEMPOTENT);
> +  }
> +  if (is_bit_set(slave_exec_mode_options, SLAVE_MODE_IDEMPOTENT) == 0)
> +    do_set_bit(slave_exec_mode_options, SLAVE_MODE_STRICT);
> +}
>  
>  bool sys_var_thd_binlog_format::is_readonly() const
>  {
> diff -Nrup a/sql/set_var.h b/sql/set_var.h
> --- a/sql/set_var.h	2007-08-13 16:11:12 +03:00
> +++ b/sql/set_var.h	2007-11-20 15:15:41 +02:00
> @@ -30,7 +30,8 @@ class sys_var_pluginvar; /* opaque */
>  typedef struct system_variables SV;
>  typedef struct my_locale_st MY_LOCALE;
>  
> -extern TYPELIB bool_typelib, delay_key_write_typelib, sql_mode_typelib;
> +extern TYPELIB bool_typelib, delay_key_write_typelib, sql_mode_typelib,
> +  slave_exec_mode_typelib;
>  
>  typedef int (*sys_check_func)(THD *,  set_var *);
>  typedef bool (*sys_update_func)(THD *, set_var *);
> @@ -771,6 +772,41 @@ public:
>  };
>  
>  
> +class sys_var_set :public sys_var
> +{
> +protected:
> +  ulong *value;
> +  TYPELIB *enum_names;
> +public:
> +  sys_var_set(sys_var_chain *chain, const char *name_arg, ulong *value_arg,
> +              TYPELIB *typelib, sys_after_update_func func)
> +    :sys_var(name_arg, func), value(value_arg), enum_names(typelib)
> +  { chain_sys_var(chain); }
> +  bool check(THD *thd, set_var *var)
> +  {
> +    return check_set(thd, var, enum_names);
> +  }
> +  virtual void set_default(THD *thd, enum_var_type type)
> +  {  
> +    *value= 0;
> +  }
> +  bool update(THD *thd, set_var *var);
> +  uchar *value_ptr(THD *thd, enum_var_type type, LEX_STRING *base);
> +  bool check_update_type(Item_result type) { return 0; }
> +  SHOW_TYPE show_type() { return SHOW_CHAR; }
> +};
>   

Very good that you introduce a separate class to represent "set" variables.

> +
> +class sys_var_set_slave_mode :public sys_var_set
> +{
> +public:
> +  sys_var_set_slave_mode(sys_var_chain *chain, const char *name_arg,
> +                         ulong *value_arg,
> +                         TYPELIB *typelib, sys_after_update_func func) :
> +    sys_var_set(chain, name_arg, value_arg, typelib, func) {}
> +  void set_default(THD *thd, enum_var_type type);
> +  bool update(THD *thd, set_var *var);
> +};
> +
>  class sys_var_log_output :public sys_var
>  {
>    ulong *value;
> @@ -1189,6 +1225,7 @@ sys_var *find_sys_var(THD *thd, const ch
>  int sql_set_variables(THD *thd, List<set_var_base> *var_list);
>  bool not_all_support_one_shot(List<set_var_base> *var_list);
>  void fix_delay_key_write(THD *thd, enum_var_type type);
> +void fix_slave_exec_mode(THD *thd, enum_var_type type);
>  ulong fix_sql_mode(ulong sql_mode);
>  extern sys_var_const_str sys_charset_system;
>  extern sys_var_str sys_init_connect;
> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt	2007-10-17 11:13:54 +03:00
> +++ b/sql/share/errmsg.txt	2007-11-20 15:15:42 +02:00
> @@ -6112,3 +6112,5 @@ ER_TRG_CANT_OPEN_TABLE
>  
>  ER_CANT_CREATE_SROUTINE
>    eng "Cannot create stored routine `%-.64s`. Check warnings"
> +ER_SLAVE_AMBIGOUS_EXEC_MODE
> +  eng "Ambiguous slave modes combination. %s"
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h	2007-11-06 14:03:54 +02:00
> +++ b/sql/sql_class.h	2007-11-20 15:15:41 +02:00
> @@ -38,6 +38,8 @@ enum enum_ha_read_modes { RFIRST, RNEXT,
>  enum enum_duplicates { DUP_ERROR, DUP_REPLACE, DUP_UPDATE };
>  enum enum_delay_key_write { DELAY_KEY_WRITE_NONE, DELAY_KEY_WRITE_ON,
>  			    DELAY_KEY_WRITE_ALL };
> +enum enum_slave_exec_mode { SLAVE_MODE_STRICT= 0,
>   

0 is implicit for any enum, it is not necessary.

Please consider to use "slave_exec_mode" or "slave_mode" for the 
variable, constant, and enumeration and not mix the ones.

> +                            SLAVE_MODE_IDEMPOTENT };
>  enum enum_mark_columns
>  { MARK_COLUMNS_NONE, MARK_COLUMNS_READ, MARK_COLUMNS_WRITE};
>  
>
>   


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com


Thread
bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Andrei Elkin20 Nov
  • Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Mats Kindahl22 Nov
    • Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Andrei Elkin27 Nov
      • Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Mats Kindahl27 Nov
        • Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Andrei Elkin29 Nov
          • Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552Mats Kindahl30 Nov