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

[He Zhenxing]

Hi Andrei

Thank you!

On 2008-03-11 Tue 17:39 +0200，Andrei Elkin wrote：
> 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 ...
> 

good

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

ok

> >   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, ...
> 

nice!

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

right

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

I have already reported a bug to request the change, and Jon has already
updated the document. Please check:

http://dev.mysql.com/doc/refman/5.1/en/replication-features-autoincid.html


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

Good that $binlog_start will become a pre-defined global variable, I'll
change it back.

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

Sorry, I don't understand, maybe I forget something we discussed...

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

Yes, you're right, shamed about this, will be fixed.

> 
> > +
> >  #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.
> 

There is no copy constructor, only a defalut constructor that takes no
argument, correct me if I am wrong.

I had thought of use the code you mentioned, the problem is I think a
simple copy construct that just copy all the members is not safe, I mean
after:

   backup->auto_inc_intervals_forced= auto_inc_intervals_forced;

We will have two instance with shared list of elements, if we use empty
on any of the instance, the other will be in a dangling state, all the
elements are actuall freed. In the current case, this is not a problem,
because we can call empty_no_free to empty the original instance without
free. But when we add a new interface, it can be use later by other
code. So this would be an unsafe interface.

If we do need the copy construct, we have to add a reference count to
record how many copies we have, and only free the list when the count is
zero. Maybe we should try this.

BTW: I think empty_no_free should be private instead of public.



> 
> cheers,
> 
> Andrei