Re: bk commit into 5.1 tree (hezx:1.2534) BUG#33029

[Andrei Elkin]

Zhen Xing, hello.

The patch for 5.0 addresses its half of the total problem.
The idea - 

@@ -622,6 +622,7 @@ void THD::cleanup_after_query()
+    insert_id_used= 0;

must be correct.

In the following I am leaving my comments along the 5.1 patch to mean
in some places a remark belongs to both csets.


> Below is the list of changes that have just been committed into a local
> 5.1 repository of hezx.  When hezx 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, 2008-03-11 13:25:26+08:00, hezx@stripped +11 -0
>   BUG#33029 5.0 to 5.1 replication fails on dup key when inserting
>   using a trig in SP
>   
>   Prior to MySQL 5.1.12 (including 5.0.x), when a stored routine or

I'd put it this way: 

    For all 5.0 and up to 5.1.12 exclusive  \footnote{i judge basing on the
    following references in the test, please correct if wrong}
    
    when a stored function  or trigger ...

Would be nice to refer to a fix or wl that made 5.1.12 so advanced.

>   trigger caused an INSERT into an AUTO_INCREMENT column, the
>   generated AUTO_INCREMENT value should not be written into the binary
>   log, which means if a statement does not generate AUTO_INCREMENT

>   valule itself,

"itself" would be better be said as `by the top level statement' to
contrast to a sf() or a trigger() that comprise a substatement rooted
from the top level.

>   there will be no Intvar event (SET INSERT_ID)
>   associated with it even if one of the stored routine or trigger
>   caused generation of such a value. And meanwhile, when executing a
>   stored routine or trigger, it would ignore the INSERT_ID value even
>   if there is a INSERT_ID value available set by a SET INSERT_ID
>   statement.
>   


>   After MySQL 5.1.12, the generated AUTO_INCREMENT value is written

I'd put it 

    Starting from 5.1.12, ...

>   into the binary log, and the value will be used if availabe when
>   executing the stored routine or trigger.
>   
>   Prior fix of this bug in MySQL 5.0 and prior MySQL 5.1.12
>   (referenced as the buggy versions in the text below), when one
>   statement that generates AUTO_INCREMENT value by itself was executed
>   in the body of a SP, all statements in the SP after this statement
>   would be treated as if they had generated AUTO_INCREMENT by itself.
>   When a statement that did not generate AUTO_INCREMENT value by
>   itself but by a function/trigger called by it, an erroenous Intvar
>   event would be associated with the statement, this erroenous
>   INSERT_ID value wouldn't cause problem when replicating between
>   masters and slaves of 5.0.x or prior 5.1.12, because the erroneous
>   INSERT_ID value was not used when executing functions/triggers. But
>   when replicating from buggy versions to 5.1.12 or newer, which will
>   use the INSERT_ID value in functions/triggers, the erronous value
>   will be used.

Please add here - that the errornous value caused the observed duplicate
error in the bug description.
>   
>   The patch for 5.1 fixed it to ignore the SET INSERT_ID value when
>   executing functions/triggers if it is replicating from a master
>   of buggy versions, another patch for 5.0 fixed it not to generate
>   the erroneous Intvar event.

Okay.

Please make sure, I'd put a todo item in these comments, to document
as `CHANGES FOR DOCUMENTATION PROPOSAL' the 5.0 limitation applies to
5.1.x where x < 12.


>
>   mysql-test/include/show_binlog_events.inc@stripped, 2008-03-11 13:25:22+08:00,
> hezx@stripped +8 -3
>     add $_binlog_start parameter to show binlog events from a given position
>
>   mysql-test/std_data/bug33029-slave-relay-bin.000001@stripped, 2008-03-11 13:25:23+08:00,
> hezx@stripped +89 -0
>     relay logs from a buggy 5.0 master for test case of BUG#33029
>
>   mysql-test/std_data/bug33029-slave-relay-bin.000001@stripped, 2008-03-11 13:25:23+08:00,
> hezx@stripped +0 -0
>
>   mysql-test/suite/binlog/r/binlog_auto_increment_bug33029.result@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +33 -0
>     Test if the slave can process relay logs from a buggy master of BUG#33029
>
>   mysql-test/suite/binlog/r/binlog_auto_increment_bug33029.result@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +0 -0
>
>   mysql-test/suite/binlog/t/binlog_auto_increment_bug33029-master.opt@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +1 -0
>     Test if the slave can process relay logs from a buggy master of BUG#33029
>
>   mysql-test/suite/binlog/t/binlog_auto_increment_bug33029-master.opt@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +0 -0
>
>   mysql-test/suite/binlog/t/binlog_auto_increment_bug33029.test@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +24 -0
>     Test if the slave can process relay logs from a buggy master of BUG#33029
>
>   mysql-test/suite/binlog/t/binlog_auto_increment_bug33029.test@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +0 -0
>
>   mysql-test/suite/rpl/r/rpl_auto_increment_bug33029.result@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +168 -0
>     Add test for BUG#33029
>
>   mysql-test/suite/rpl/r/rpl_auto_increment_bug33029.result@stripped, 2008-03-11
> 13:25:23+08:00, hezx@stripped +0 -0
>
>   sql/slave.cc@stripped, 2008-03-11 13:25:22+08:00, hezx@stripped +22 -0
>     Add function to check for bug#33029
>
>   sql/slave.h@stripped, 2008-03-11 13:25:22+08:00, hezx@stripped +1 -0
>     Add function to check for bug#33029
>
>   sql/sql_class.cc@stripped, 2008-03-11 13:25:22+08:00, hezx@stripped +11 -0
>     if master has bug#33029, reset auto_inc_intervals_forced for sub statements
>
>   sql/sql_class.h@stripped, 2008-03-11 13:25:23+08:00, hezx@stripped +1 -0
>     Add member to save and restore auto_inc_intervals_forced
>
>   sql/structs.h@stripped, 2008-03-11 13:25:23+08:00, hezx@stripped +22 -0
>     Add functions to save and restore Discrete_intervals_list object
>

that's a good idea in below!

> diff -Nrup a/mysql-test/include/show_binlog_events.inc
> b/mysql-test/include/show_binlog_events.inc
> --- a/mysql-test/include/show_binlog_events.inc	2008-01-29 21:43:03 +08:00
> +++ b/mysql-test/include/show_binlog_events.inc	2008-03-11 13:25:22 +08:00
> @@ -1,5 +1,10 @@

> ---let $binlog_start=106
still, i'd leave the constant

   let $binlog_start=106;

to use in the following assignement.
In future, we may consider to have a list of mysql-test global variables/constants that
are
sensitive to mysql version and $binlog_start would one of them.



> ---replace_result $binlog_start <binlog_start>
> +# $_binlog_start can be set by caller or take a default value
> +
> +if (!$_binlog_start)
> +{
> +  let $_binlog_start=106;
> +}
> +--replace_result $_binlog_start <binlog_start>
>  --replace_column 2 # 4 # 5 #
>  --replace_regex /\/\* xid=.* \*\//\/* XID *\// /table_id: [0-9]+/table_id: #/
> /file_id=[0-9]+/file_id=#/
> ---eval show binlog events from $binlog_start
> +--eval show binlog events from $_binlog_start
> Binary files a/mysql-test/std_data/bug33029-slave-relay-bin.000001 and
> b/mysql-test/std_data/bug33029-slave-relay-bin.000001 differ
> diff -Nrup a/mysql-test/suite/binlog/r/binlog_auto_increment_bug33029.result
> b/mysql-test/suite/binlog/r/binlog_auto_increment_bug33029.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/r/binlog_auto_increment_bug33029.result	2008-03-11
> 13:25:23 +08:00
> @@ -0,0 +1,33 @@
> +change master to
> +MASTER_HOST='dummy.localdomain',
> +RELAY_LOG_FILE='slave-relay-bin.000001',
> +RELAY_LOG_POS=4;
> +start slave sql_thread;
> +select MASTER_POS_WAIT('master-bin.000001', 3776);
> +# Result on slave
> +SELECT * FROM t1;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +SELECT * FROM t2;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +12
> +13
> +14
> +15
> +16
> +17
> +18
> +19
> diff -Nrup a/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029-master.opt
> b/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029-master.opt
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029-master.opt	2008-03-11
> 13:25:23 +08:00
> @@ -0,0 +1 @@
> +--replicate-same-server-id --relay-log=slave-relay-bin --skip-slave-start
> diff -Nrup a/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029.test
> b/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/t/binlog_auto_increment_bug33029.test	2008-03-11
> 13:25:23 +08:00
> @@ -0,0 +1,24 @@
> +# BUG#33029 5.0 to 5.1 replication fails on dup key when inserting
> +# using a trig in SP
> +
> +# Test if the slave can replicate from a buggy master
> +
> +copy_file std_data/bug33029-slave-relay-bin.000001
> var/master-data/slave-relay-bin.000001;
> +
> +write_file var/master-data/slave-relay-bin.index;
> +slave-relay-bin.000001
> +EOF
> +
> +change master to
> +  MASTER_HOST='dummy.localdomain',
> +  RELAY_LOG_FILE='slave-relay-bin.000001',
> +  RELAY_LOG_POS=4;
> +
> +start slave sql_thread;
> +disable_result_log;
> +select MASTER_POS_WAIT('master-bin.000001', 3776);
> +enable_result_log;
> +
> +echo # Result on slave;
> +SELECT * FROM t1;
> +SELECT * FROM t2;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_auto_increment_bug33029.result
> b/mysql-test/suite/rpl/r/rpl_auto_increment_bug33029.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/r/rpl_auto_increment_bug33029.result	2008-03-11 13:25:23
> +08:00
> @@ -0,0 +1,168 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +DROP TABLE IF EXISTS t1, t2;
> +DROP PROCEDURE IF EXISTS p1;
> +DROP PROCEDURE IF EXISTS p2;
> +DROP TRIGGER IF EXISTS tr1;
> +DROP FUNCTION IF EXISTS f1;
> +CREATE TABLE t1 (id INT AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t2 (id INT AUTO_INCREMENT PRIMARY KEY);
> +CREATE PROCEDURE p1()
> +BEGIN
> +DECLARE ins_count INT DEFAULT 10; 
> +WHILE ins_count > 0 DO
> +INSERT INTO t1 VALUES (NULL);
> +SET ins_count = ins_count - 1;
> +END WHILE;
> +DELETE FROM t1 WHERE id = 1;
> +DELETE FROM t1 WHERE id = 2;
> +DELETE FROM t2 WHERE id = 1;
> +DELETE FROM t2 WHERE id = 2;
> +END//
> +CREATE PROCEDURE p2()
> +BEGIN
> +INSERT INTO t1 VALUES (NULL);
> +DELETE FROM t1 WHERE id = f1(3);
> +DELETE FROM t1 WHERE id = f1(4);
> +DELETE FROM t2 WHERE id = 3;
> +DELETE FROM t2 WHERE id = 4;
> +END//
> +CREATE TRIGGER tr1 BEFORE DELETE
> +ON t1 FOR EACH ROW 
> +BEGIN
> +INSERT INTO t2  VALUES (NULL);
> +END//
> +CREATE FUNCTION f1 (i int) RETURNS int
> +BEGIN
> +INSERT INTO t2 VALUES (NULL);
> +RETURN i;
> +END//
> +CALL p1();
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=1
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=2
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=3
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=4
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=5
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=6
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=7
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=8
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=9
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=10
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=1
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t1 WHERE id = 1
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=2
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t1 WHERE id = 2
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t2 WHERE id = 1
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t2 WHERE id = 2
> +# Result on master
> +SELECT * FROM t1;
> +id
> +3
> +4
> +5
> +6
> +7
> +8
> +9
> +10
> +SELECT * FROM t2;
> +id
> +# Result on slave
> +SELECT * FROM t1;
> +id
> +3
> +4
> +5
> +6
> +7
> +8
> +9
> +10
> +SELECT * FROM t2;
> +id
> +DROP TRIGGER tr1;
> +CALL p2();
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=11
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (NULL)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=3
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t1 WHERE id = f1(3)
> +master-bin.000001	#	Intvar	#	#	INSERT_ID=12
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t1 WHERE id = f1(4)
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t2 WHERE id = 3
> +master-bin.000001	#	Query	#	#	use `test`; DELETE FROM t2 WHERE id = 4
> +# Result on master
> +SELECT * FROM t1;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +SELECT * FROM t2;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +12
> +13
> +14
> +15
> +16
> +17
> +18
> +19
> +# Result on slave
> +SELECT * FROM t1;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +SELECT * FROM t2;
> +id
> +5
> +6
> +7
> +8
> +9
> +10
> +11
> +12
> +13
> +14
> +15
> +16
> +17
> +18
> +19
> +DROP TABLE IF EXISTS t1, t2;
> +DROP PROCEDURE IF EXISTS p1;
> +DROP PROCEDURE IF EXISTS p2;
> +DROP FUNCTION IF EXISTS f1;

Have not you forgotten to update the check function - find your TODO?

> diff -Nrup a/sql/slave.cc b/sql/slave.cc
> --- a/sql/slave.cc	2008-02-21 05:24:58 +08:00
> +++ b/sql/slave.cc	2008-03-11 13:25:22 +08:00
> @@ -4085,6 +4085,28 @@ bool rpl_master_has_bug(Relay_log_info *
>    return FALSE;
>  }
>  
> +/**
> +   Detect if the master affected by BUG#33029, which does not support
> +   using SET INSERT to set auto_increment field value for
> +   sub-statements.
> + */
> +bool rpl_master_bug33029(THD *thd)
> +{
> +  if (active_mi && active_mi->rli.sql_thd == thd)
> +  {
> +    Relay_log_info *rli= &active_mi->rli;
> +    const uchar *master_ver=
> +      rli->relay_log.description_event_for_exec->server_version_split;
> +    
> +    /* TODO: currently for all 5.0 versions, there may be other
> +       affected versions that should be checked here.
> +     */
> +    if (master_ver[0] == 5 && master_ver[1] == 0)
> +      return TRUE;
> +  }
> +  return FALSE;
> +}

I thought we agreed that you will enforce the test with checking the
minor number that is master_ver[2]. Otherwise I don't get how it
is supposed to distinguish 5.1.[0-11] + 5.0.*  from  the rest 
5.1.12+ correct insert_id generating master.


> +
>  #ifdef HAVE_EXPLICIT_TEMPLATE_INSTANTIATION
>  template class I_List_iterator<i_string>;
>  template class I_List_iterator<i_string_pair>;
> diff -Nrup a/sql/slave.h b/sql/slave.h
> --- a/sql/slave.h	2008-02-07 04:07:43 +08:00
> +++ b/sql/slave.h	2008-03-11 13:25:22 +08:00
> @@ -166,6 +166,7 @@ int fetch_master_table(THD* thd, const c
>  bool show_master_info(THD* thd, Master_info* mi);
>  bool show_binlog_info(THD* thd);
>  bool rpl_master_has_bug(Relay_log_info *rli, uint bug_id);
> +bool rpl_master_bug33029(THD *thd);
>  
>  const char *print_slave_db_safe(const char *db);
>  int check_expected_error(THD* thd, Relay_log_info const *rli, int error_code);
> diff -Nrup a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc	2008-02-13 01:21:07 +08:00
> +++ b/sql/sql_class.cc	2008-03-11 13:25:22 +08:00
> @@ -28,6 +28,7 @@
>  #include "mysql_priv.h"
>  #include "rpl_rli.h"
>  #include "rpl_record.h"
> +#include "slave.h"
>  #include <my_bitmap.h>
>  #include "log_event.h"
>  #include <m_ctype.h>
> @@ -2827,6 +2828,11 @@ extern "C" void thd_mark_transaction_to_
>  void THD::reset_sub_statement_state(Sub_statement_state *backup,
>                                      uint new_state)
>  {
> +#ifndef EMBEDDED_LIBRARY
> +  if (rpl_master_bug33029(this))
> +    auto_inc_intervals_forced.save(&backup->auto_inc_intervals_forced);
> +#endif
> +  
>    backup->options=         options;
>    backup->in_sub_stmt=     in_sub_stmt;
>    backup->enable_slow_log= enable_slow_log;
> @@ -2864,6 +2870,11 @@ void THD::reset_sub_statement_state(Sub_
>  
>  void THD::restore_sub_statement_state(Sub_statement_state *backup)
>  {
> +#ifndef EMBEDDED_LIBRARY
> +  if (rpl_master_bug33029(this))
> +    auto_inc_intervals_forced.restore(&backup->auto_inc_intervals_forced);
> +#endif
> +
>    /*
>      To save resources we want to release savepoints which were created
>      during execution of function or trigger before leaving their savepoint
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h	2008-02-20 22:22:13 +08:00
> +++ b/sql/sql_class.h	2008-03-11 13:25:23 +08:00
> @@ -911,6 +911,7 @@ public:
>    ulonglong first_successful_insert_id_in_prev_stmt;
>    ulonglong first_successful_insert_id_in_cur_stmt, insert_id_for_cur_row;
>    Discrete_interval auto_inc_interval_for_cur_row;
> +  Discrete_intervals_list auto_inc_intervals_forced;
>    ulonglong limit_found_rows;
>    ha_rows    cuted_fields, sent_row_count, examined_row_count;
>    ulong client_capabilities;
> diff -Nrup a/sql/structs.h b/sql/structs.h
> --- a/sql/structs.h	2007-12-15 01:01:47 +08:00
> +++ b/sql/structs.h	2008-03-11 13:25:23 +08:00


> @@ -331,6 +331,28 @@ public:
>      }
>      empty_no_free();
>    }
> +
> +  /* Transfer elements from one list to an empty list */
> +  static void transfer(Discrete_intervals_list *from, Discrete_intervals_list *to)
> +  {
> +    DBUG_ASSERT(to->elements == 0);
> +    to->head = from->head;
> +    to->tail = from->tail;
> +    to->current = from->current;
> +    to->elements = from->elements;
> +    from->empty_no_free();
> +  }
> +
> +  void save(Discrete_intervals_list *to)
> +  {
> +    transfer(this, to);
> +  }
> +
> +  void restore(Discrete_intervals_list *from)
> +  {
> +    transfer(from, this);
> +  }
> +
>    const Discrete_interval* get_next()
>    {
>      Discrete_interval *tmp= current;

Why not to be content with the existing default copy constructor
instead of your new methods?

E.g

  auto_inc_intervals_forced.save(&backup->auto_inc_intervals_forced);

turns to be 2 instuctions

  backup->auto_inc_intervals_forced= auto_inc_intervals_forced;
  auto_inc_intervals_forced.empty_no_free();

but i'd rather to go with them as empty_no_free() is a public.


cheers,

Andrei