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

[Andrei Elkin]

Mats,

> Hi Andrei!
>
> I have a few comments, that you can see below.
>
> 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-17
>>       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 whereas it meant to be dynamical.
>>       
>>       Fixed with correcting allocation; added simulation code that forces those
> fixes of bug@33029
>>       that targeted at master-and-slave having incompatible bug33029-prone
> versions. 
>
> There is no explanation what BUG#33029 has to do with this. Is this bug
> a regression or is it just related to that bug in some other way. Could
> you please add a sentence saying how they are related.
>

I thought it's pretty much apparent.
Considering the hunk

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

where the number of the bug is --------^

Albeit, comments can be added.

>> added:
>>   mysql-test/suite/bugs/r/rpl_bug33029.result
>>   mysql-test/suite/bugs/t/rpl_bug33029.test
>> modified:
>>   sql/slave.cc
>>   sql/structs.h
>>   support-files/build-tags
>> 
>> per-file messages:
>>   mysql-test/suite/bugs/r/rpl_bug33029.result
>>     new result 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/structs.h
>>     converting automatic allocation of a being appended item to Discrete_interval
> to dynamic.
>> === 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-17 13:45:05 +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-17 13:45:05 +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-17 13:45:05 +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/structs.h'
>> --- a/sql/structs.h	2008-03-14 03:35:41 +0000
>> +++ b/sql/structs.h	2008-06-17 13:45:05 +0000
>> @@ -320,8 +320,8 @@ private:
>>    {
>>      for (Discrete_interval *i= from.head; i; i= i->next)
>>      {
>> -      Discrete_interval j= *i;
>> -      append(&j);
>> +      Discrete_interval *j= new Discrete_interval(*i);
>> +      append(j);
>
> Here new can return NULL, causing a partial list to be constructed
> without any indication that is the case. A better option would be to
> release the memory that was allocated in the copying and mark the list
> as "bad". That way, it is at least possible for the caller to check if
> the list was copied successfully.
>

I had to agree to nullify in that case. It's the last resort solution
and the only sound approach is stop the server so in the current as well
as in zillions other places of the server code.
However I succumb as it's not clear to me how to implement that and naturally gracefully.

>>      }
>>    }
>>  public:
>> 
>> === modified file 'support-files/build-tags'
>> --- a/support-files/build-tags	2002-01-20 02:16:52 +0000
>> +++ b/support-files/build-tags	2008-06-17 13:45:05 +0000
>> @@ -2,7 +2,7 @@
>>  
>>  rm -f TAGS
>>  filter='\.cc$\|\.c$\|\.h$\|\.yy$'
>> -files=`bk -r sfiles -gU | grep $filter `
>> +files=`bzr ls | grep $filter `
>>  for f in $files ;
>>  do
>>  	 etags -o TAGS --append $f

Just spoke to #bzr, this make tags fix is not needed anymore.

regards,

Andrei