Andrei Elkin wrote:
> Mats,
>
>> 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.
>
> That's my omission. Recommitting.
OK.
>> 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.
>
> But there is `swap' as you are confirming above, it's not instead.
> Why can it be defined through copy_shallow()?
>
That's not the problem. I'm talking about the member functions to read
the fields of the structure.
>> 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.
>>
>
> You mean the = op and the copy constructor, don't you?
> It's been done.
Yes, thanks.
Best wishes,
Mats Kindahl
>
> cheers,
>
> Andrei
>
>
>> 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
