Re: bk commit into 5.1 tree (aelkin:1.2610) BUG#31552

[Mats Kindahl]

Hi Andrei!

There are still issues remaining:

    * It is still the case that the bit manipulation macros have
      consistent return values (it should *not* be necessary to have to
      remember which one returns what when you are writing code). I
      strongly suggest to use the existing convention that zero means
      "OK" and anything else means there were an error.
    * You have either not updated the result file or there is something
      wrong with the code to print the handler error. There is still a
      "(null)" in the result file.

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-29 19:22:47+02:00, aelkin@stripped +35 -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-29 19:22:36+02:00, aelkin@stripped +17
> -0
>     basic operations with bits of an integer type are added.
>
>   mysql-test/extra/rpl_tests/rpl_foreign_key.test@stripped, 2007-11-29 19:22:36+02:00,
> aelkin@stripped +31 -0
>     regression test for bug#32468
>
>   mysql-test/extra/rpl_tests/rpl_row_basic.test@stripped, 2007-11-29 19:22: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-29 19:22:37+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-29
> 19:22:37+02:00, aelkin@stripped +13 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_idempotency.result@stripped, 2007-11-29 19:22:37+02:00,
> aelkin@stripped +159 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_ignore_table.result@stripped, 2007-11-29 19:22:37+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_USER.result@stripped, 2007-11-29 19:22:37+02:00,
> aelkin@stripped +3 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_11bugs.result@stripped, 2007-11-29 19:22:37+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result@stripped, 2007-11-29 19:22:38+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result@stripped, 2007-11-29 19:22:38+02:00,
> aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_mystery22.result@stripped, 2007-11-29 19:22:38+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_tabledefs_2myisam.result@stripped, 2007-11-29
> 19:22:38+02:00, aelkin@stripped +4 -2
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_row_tabledefs_3innodb.result@stripped, 2007-11-29
> 19:22:38+02:00, aelkin@stripped +4 -2
>     results changed
>
>   mysql-test/suite/rpl/r/rpl_temporary_errors.result@stripped, 2007-11-29 19:22:38+02:00,
> aelkin@stripped +2 -0
>     results changed
>
>   mysql-test/suite/rpl/t/rpl_idempotency-master.opt@stripped, 2007-11-29 19:22:42+02:00,
> aelkin@stripped +2 -0
>     innodb is necessary
>
>   mysql-test/suite/rpl/t/rpl_idempotency-master.opt@stripped, 2007-11-29 19:22:42+02:00,
> aelkin@stripped +0 -0
>
>   mysql-test/suite/rpl/t/rpl_idempotency-slave.opt@stripped, 2007-11-29 19:22:43+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-29 19:22:43+02:00,
> aelkin@stripped +0 -0
>
>   mysql-test/suite/rpl/t/rpl_idempotency.test@stripped, 2007-11-29 19:22:39+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-29 19:22:39+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-29 19:22:39+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-29 19:22: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-29 19:22: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-29 19:22: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-29
> 19:22:40+02:00, aelkin@stripped +1 -0
>     results changed
>
>   mysql-test/suite/rpl_ndb/r/rpl_row_basic_7ndb.result@stripped, 2007-11-29
> 19:22:40+02:00, aelkin@stripped +4 -0
>     results changed
>
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_dd_advance.test@stripped, 2007-11-29 19:22:40+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-29 19:22:40+02:00, aelkin@stripped +145
> -91
>     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-29 19:22: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-29 19:22:41+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-29 19:22:41+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-29 19:22:41+02:00, aelkin@stripped +5 -0
>     rli post-event-execution cleanup restores the default bits.
>
>   sql/set_var.cc@stripped, 2007-11-29 19:22:41+02:00, aelkin@stripped +84 -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-29 19:22:42+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-29 19:22: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-29 19:22:42+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-29 19:22:36 +02:00
> @@ -159,6 +159,23 @@ static inline my_bool bitmap_cmp(const M
>  #define bitmap_set_all(MAP) \
>    (memset((MAP)->bitmap, 0xFF, 4*no_words_in_map((MAP))))
>  
> +/**
> +   check, set and clear a bit of interest of an integer.
> +
> +   If the bit is outside of the integer's size results are -1.
> +   If the bit is inside then
> +   bit_is_set returns 0 or 1 reflecting the bit is set or not;
> +   bit_do_set returns 1;
> +   bit_do_clear returns 0;
>   

The "return value" of the 'do' macros is still not consistent. Is there 
a special reason that you want to return a different value for "success" 
for these macros? It just complicates the issue. By sticking to the 
"non-zero == error" convention, you simplify the code significantly for 
both yourself and future users of the macros.

> +*/
> +
> +#define bit_is_set(I,B) (int) (sizeof(I) * CHAR_BIT > (B) ?                    \
> +                               (((I) & (ULL(1) << (B))) == 0 ? 0 : 1) :
> -1)
> +#define bit_do_set(I,B) (int) (sizeof(I) * CHAR_BIT > (B) ?                    \
> +                               ((I) |= (ULL(1) << (B)), 1) : -1)
> +#define bit_do_clear(I,B) (int) (sizeof(I) * CHAR_BIT > (B) ?                  \
> +                                 ((I) &= ~(ULL(1) << (B)), 0) : -1)
> +
>  #ifdef	__cplusplus
>  }
>  #endif
>   

[snip]

> 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-29 19:22:38
> +02:00
>   

[snip]

> @@ -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
>   

There is still a "(null)" here. The code below should not print "(null)" 
so I suspect you have not updated the result file.

[snip]

> 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-29 19:22:40 +02:00
> @@ -90,6 +90,27 @@ 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;
> +}
> +
> +/**
> +   macro to call from different branches of Rows_log_event::do_apply_event
> +*/
> +static void inline slave_rows_error_report(enum loglevel level, int ha_error,
> +                                           Relay_log_info const *rli, THD *thd,
> +                                           TABLE *table, const char * type,
> +                                           const char *log_name, ulong pos)
> +{
> +  const char *handler_error=  HA_ERR(ha_error);
> +  rli->report(level, thd->net.last_errno,
> +              "Could not execute %s event on table %s.%s. "
> +              "Error: %s (handler error %s). "
> +              "The event's master log and end_log_pos %s %lu",
> +              type, table->s->db.str,
> +              table->s->table_name.str,
> +              thd->net.last_error ? thd->net.last_error : "",
> +              handler_error == NULL? "<unknown>" : handler_error,
> +              log_name, pos);
>  }
>  #endif
>   

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