Re: bzr commit into mysql-5.1 branch (aelkin:2656) Bug#36443, Bug#33029

[Mats Kindahl]

Hi Andrei,

You still didn't make the copy assignment operator and the copy
constructor private, which was the reason for replacing it with an
explicit function instead.

I also find your definition of the swap operator quite strange, even
though it is correct.

You also introduced a number of member functions that were not needed
and which violates the encapsuling.

All I asked you to do is make the copy assignment operator and the copy
constructor private and replace that with an explicit method.  I
suggested that you implement a swap() operator instead of a
shallow_copy(), since that avoid the problem with resource leaks.

I'll approve the patch, since it does not contain any obvious flaws, but
I would advice you to make the two functions mentioned above private.

Just my few cents,
Mats Kindahl

Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/bug36443-slave_crash_insert_trigger/
> 
>  2656 Andrei Elkin	2008-06-19
>       Bug#36443 Server crashes when executing insert when insert trigger on table
>                         
>       The crash appeared to be a result of allocating an instance of
> Discrete_interval 
>       automatically that that was referred in out-of-declaration scope.
>                         
>       Fixed with correcting backing up and restoring scheme of
>       auto_inc_intervals_forced, introduced by bug#33029, by means of shallow
> copying;
>       added simulation code that forces executing those fixes of the former bug that
>       targeted at master-and-slave having incompatible bug#33029-prone versions.
> added:
>   mysql-test/suite/bugs/r/rpl_bug33029.result
>   mysql-test/suite/bugs/t/rpl_bug33029.test
> modified:
>   sql/slave.cc
>   sql/sql_class.cc
>   sql/structs.h
> 
> per-file messages:
>   mysql-test/suite/bugs/r/rpl_bug33029.result
>     new results file
>   mysql-test/suite/bugs/t/rpl_bug33029.test
>     test merely checks no crash happens on slave.
>   sql/slave.cc
>     forcing to execute special logics implemented for bug#33029 if
>     simulate_bug33029 the debug option is set.
>   sql/sql_class.cc
>     swaps of backed and the actual auto_inc_intervals_forced basing on shallow
> coping.
>   sql/structs.h
>     Removing the deep _copy() and methods associated with it;
>     adding methods to Discrete_intervals_list:
>     
>     private set_members();
>     public  copy_shallow(), swap(), get_{head, tail, current}();
>     empty_no_free() through set_members().
> === added file 'mysql-test/suite/bugs/r/rpl_bug33029.result'
> --- a/mysql-test/suite/bugs/r/rpl_bug33029.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/bugs/r/rpl_bug33029.result	2008-06-19 14:04:11 +0000
> @@ -0,0 +1,15 @@
> +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;
> +create table `t1` (`id` int not null auto_increment primary key);
> +create trigger `trg` before insert on `t1` for each row begin end;
> +set @@global.debug="+d,simulate_bug33029";
> +stop slave;
> +start slave;
> +insert into `t1` values ();
> +select * from t1;
> +id
> +1
> 
> === added file 'mysql-test/suite/bugs/t/rpl_bug33029.test'
> --- a/mysql-test/suite/bugs/t/rpl_bug33029.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/bugs/t/rpl_bug33029.test	2008-06-19 14:04:11 +0000
> @@ -0,0 +1,25 @@
> +#
> +# Bug #36443 Server crashes when executing insert when insert trigger on table
> +#
> +# Emulating the former bug#33029 situation to see that there is no crash anymore.
> +# 
> +
> +
> +source include/master-slave.inc;
> +
> +create table `t1` (`id` int not null auto_increment primary key);
> +create trigger `trg` before insert on `t1` for each row begin end;
> +
> +sync_slave_with_master;
> +set @@global.debug="+d,simulate_bug33029";
> +
> +stop slave;
> +start slave;
> +
> +connection master;
> +
> +insert into `t1` values ();
> +
> +sync_slave_with_master;
> +select * from t1;
> +
> 
> === modified file 'sql/slave.cc'
> --- a/sql/slave.cc	2008-03-31 08:57:18 +0000
> +++ b/sql/slave.cc	2008-06-19 14:04:11 +0000
> @@ -4136,6 +4136,7 @@ bool rpl_master_erroneous_autoinc(THD *t
>    if (active_mi && active_mi->rli.sql_thd == thd)
>    {
>      Relay_log_info *rli= &active_mi->rli;
> +    DBUG_EXECUTE_IF("simulate_bug33029", return TRUE;);
>      return rpl_master_has_bug(rli, 33029, FALSE);
>    }
>    return FALSE;
> 
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc	2008-05-20 07:38:17 +0000
> +++ b/sql/sql_class.cc	2008-06-19 14:04:11 +0000
> @@ -2882,8 +2882,8 @@ void THD::reset_sub_statement_state(Sub_
>     */
>    if (rpl_master_erroneous_autoinc(this))
>    {
> -    backup->auto_inc_intervals_forced= auto_inc_intervals_forced;
> -    auto_inc_intervals_forced.empty();
> +    DBUG_ASSERT(backup->auto_inc_intervals_forced.nb_elements() == 0);
> +    auto_inc_intervals_forced.swap(&backup->auto_inc_intervals_forced);
>    }
>  #endif
>    
> @@ -2931,8 +2931,8 @@ void THD::restore_sub_statement_state(Su
>     */
>    if (rpl_master_erroneous_autoinc(this))
>    {
> -    auto_inc_intervals_forced= backup->auto_inc_intervals_forced;
> -    backup->auto_inc_intervals_forced.empty();
> +    backup->auto_inc_intervals_forced.swap(&auto_inc_intervals_forced);
> +    DBUG_ASSERT(backup->auto_inc_intervals_forced.nb_elements() == 0);
>    }
>  #endif
>  
> 
> === modified file 'sql/structs.h'
> --- a/sql/structs.h	2008-03-14 03:35:41 +0000
> +++ b/sql/structs.h	2008-06-19 14:04:11 +0000
> @@ -314,31 +314,20 @@ private:
>    */
>    Discrete_interval        *current;
>    uint                  elements; // number of elements
> -
> -  /* helper function for copy construct and assignment operator */
> -  void copy_(const Discrete_intervals_list& from)
> -  {
> -    for (Discrete_interval *i= from.head; i; i= i->next)
> -    {
> -      Discrete_interval j= *i;
> -      append(&j);
> -    }
> +  void set_members(Discrete_interval *h, Discrete_interval *t,
> +                   Discrete_interval *c, uint el)
> +  {  
> +    head= h;
> +    tail= t;
> +    current= c;
> +    elements= el;
>    }
> +    
>  public:
>    Discrete_intervals_list() : head(NULL), current(NULL), elements(0) {};
> -  Discrete_intervals_list(const Discrete_intervals_list& from)
> -  {
> -    copy_(from);
> -  }
> -  void operator=(const Discrete_intervals_list& from)
> -  {
> -    empty();
> -    copy_(from);
> -  }
>    void empty_no_free()
>    {
> -    head= current= NULL;
> -    elements= 0;
> +    set_members(NULL, NULL, NULL, 0);
>    }
>    void empty()
>    {
> @@ -350,7 +339,24 @@ public:
>      }
>      empty_no_free();
>    }
> -
> +  void copy_shallow(const Discrete_intervals_list * dli)
> +  {
> +    head= dli->get_head();
> +    tail= dli->get_tail();
> +    current= dli->get_current();
> +    elements= dli->nb_elements();
> +  }
> +  void swap (Discrete_intervals_list * dli)
> +  {
> +    Discrete_interval *h, *t, *c;
> +    uint el;
> +    h= dli->get_head();
> +    t= dli->get_tail();
> +    c= dli->get_current();
> +    el= dli->nb_elements();
> +    dli->copy_shallow(this);
> +    set_members(h, t, c, el);
> +  }
>    const Discrete_interval* get_next()
>    {
>      Discrete_interval *tmp= current;
> @@ -364,4 +370,7 @@ public:
>    ulonglong minimum()     const { return (head ? head->minimum() : 0); };
>    ulonglong maximum()     const { return (head ? tail->maximum() : 0); };
>    uint      nb_elements() const { return elements; }
> +  Discrete_interval* get_head() const { return head; };
> +  Discrete_interval* get_tail() const { return tail; };
> +  Discrete_interval* get_current() const { return current; };
>  };
> 
> 


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