Re: bk commit into 5.0 tree (aelkin:1.2439) BUG#27417

[Andrei Elkin]

Konstja, hello.

> Hello Andrei,
>
> Please find my review inline.
>
> * Andrei Elkin <aelkin@stripped> [07/06/26 19:30]:
>
>> ChangeSet@stripped, 2007-06-26 18:21:52+03:00,
> aelkin@stripped +18 -0
>>   Bug #27417 thd->no_trans_update.stmt lost value inside of SF-exec-stack
>
> The new changeset comments are a bit cryptic. Could you please
> take time to convert it to a more cohesive description of the
> problem and the solution?
>

Doing... while Serg is reviewing.

> The patch itself is OK to push. My review comments are
> mainly cosmetic. 
>
> One non-cosmetic matter is the following:
>
> You merge with transaction.stmt with transaction.all in the DML
> code (sql_insert.cc, sql_update.cc, etc). Why not do that where we
> bzero(thd->transaction.stmt)? I think these locations identify
> good candidates for this code.
>

Must agree that the place would be close to the optimal.

> OK, I understand that non_trans_update.all is already updated in
> many places, so your consolidation of the merge will only be
> partial anyway.
>

ack


> Review comment is thus left for your consideration.
>
>>    bool        no_2pc;
>>    /* storage engines that registered themselves for this transaction */
>>    handlerton *ht[MAX_HA];
>> +  bool modified_non_trans_table;
>>  } THD_TRANS;
>>  
>
> Please add a doxygen comment describing the purpose of this
> variable.
>

done. Actually nothing doxygen-specific could i make out.

>> +++ 1.210/sql/log.cc	2007-06-26 18:21:47 +03:00
>> @@ -162,7 +162,7 @@ static int binlog_rollback(THD *thd, boo
>>      table. Such cases should be rare (updating a
>>      non-transactional table inside a transaction...)
>>    */
>> -  if (unlikely(thd->no_trans_update.all))
>> +  if (unlikely(thd->transaction.all.modified_non_trans_table))
>
> Personal taste: do not use unlikely. It's not a
> performance-critical path, and unlikely only clutters the code in
> such places.
>

I inclined to agree `binlog_rollback' is not that critical. I personally
doubt that the rollback of an engine can be done quicker that 1/50 of
the commit. Application is to perform not rollback :)
Anyway, I'd rather to leave the code that does not apparently hurt and
I did not write...



>> +    if (thd->transaction.stmt.modified_non_trans_table)
>> +      thd->transaction.all.modified_non_trans_table= TRUE;
>
> Could you use either |= or 'if' consistently when doing merging
> of two variables please?

Hmm, these are two different kind of merges imo.

   thd->transaction.stmt.modified_non_trans_table |= 
thd->parent_modified_non_trans_table;

at the end of substatement

and

    if (thd->transaction.stmt.modified_non_trans_table)
      thd->transaction.all.modified_non_trans_table= TRUE;

"right away" unioning for `all'.
While in the latter the write operation can be avoided sometimes, in
the former it's impossible as the saved parent's value must be recovered.

This means that the common is `|=' which has the drawback of possible
extra-write.



>
>> +        if (thd->transaction.stmt.modified_non_trans_table)
>> +          thd->transaction.all.modified_non_trans_table= TRUE;
>
>> +  bool changed, transactional_table= table->file->has_transactions();
>
>> @@ -2587,6 +2587,8 @@ mysql_execute_command(THD *thd)
>>      statistic_increment(thd->status_var.com_stat[lex->sql_command],
>>                          &LOCK_status);
>>  
>> +  thd->transaction.stmt.modified_non_trans_table= FALSE;
>> +  
>
> You don't have to reset it here, the entire transaction.stmt is
> bzeroed at the end of each statement, and for sub-statements you
> should perform the reset in reset_lex_and_exec_core.
>

good point. As we agreed I turn that into

DBUG_ASSERT(thd->transaction.stmt.modified_non_trans_table == FALSE);



>> +++ mysql-test/r/stored_routines_side_effect.result	07/06/26 18:21:49
>
> If you need a separate test file, please prefix it with 
> either rpl_ or binlog_.
>
> Please check with the replication team whether this deserves a
> separate test. Perhaps you could append it to
> mix_innodb_myisam_binlog.test (this test does not follow
> rpl test naming conventions :-( )

moved to mix_innodb_myisam_binlog.test. No consultation is needed as
it's close to the best choice. Yes, this time there is no need for the
mixture of the engines but further bug#23333 fix will create them and
continue with the "stem" this patch plants.


>
>> +#
>> +# bug#27417,bug#28960
>> +#
>> +# testing appearence of insert into temp_table
>> +#
>
> Please provide titles of the bugs.

done

>> +
>> +## send_eof() branch

>
> A more elaborate explanation of what is tested is needed.

I added some few words. The objects of testing I always outline in
queries' comments so that it's possible one can read sole result file
to see if the results are expectable.

>> +
>> +# prepare
>> +
>> +create temporary table tt (a int unique);
>> +create table ti (a int) engine=innodb;
>> +reset master;
>> +show master status;
>> +
>> +# action
>> +
>> +begin; 
>> +insert into ti values (1); 
>> +insert into ti values (2) ; 
>> +insert into tt select * from ti;
>> +rollback;
>> +
>> +# check
>> +
>> +select count(*) from tt /* 2 */;
>> +show master status;
>> +--replace_column 2 # 5 #
>> +show binlog events from 98;
>> +select count(*) from ti /* zero */;
>> +insert into ti select * from tt;
>> +select * from ti /* that is what slave would miss - a bug */;
>> +
>> +
>> +## send_error() branch
>> +delete from ti;
>> +delete from tt where a=1;
>> +reset master;
>> +show master status;
>> +
>> +# action
>> +
>> +begin; 
>> +insert into ti values (1); 
>> +insert into ti values (2) /* to make the dup error in the following */; 
>> +--error ER_DUP_ENTRY
>> +insert into tt select * from ti /* one affected and error */;
>> +rollback;
>> +
>> +# check
>> +
>> +show master status;
>> +--replace_column 2 # 5 #
>> +show binlog events from 98;
>> +select count(*) from ti /* zero */;
>> +insert into ti select * from tt;
>> +select * from tt /* that is what slave would miss - the bug */;
>> +
>> +drop table ti;
>> +
>> +--echo end of tests
>> +
>> @@ -3697,7 +3697,7 @@ int ha_ndbcluster::external_lock(THD *th
>>      {
>>        m_transaction_on= FALSE;
>>        /* Would be simpler if has_transactions() didn't always say "yes" */
>> -      thd->no_trans_update.all= thd->no_trans_update.stmt= TRUE;
>> +      thd->transaction.all.modified_non_trans_table=
> thd->transaction.stmt.modified_non_trans_table= TRUE;
>>      }

>
> I have no idea whatsoever what is going on here.
>

Can not afford any more time to explore that, sorry.

>
>> +  /* 
>> +     the flag is saved at the entry to the following substatement.
>> +     It's reset further in the common code part.
>> +     It's merged with the saved parent's value at the exit of this func.
>> +  */
>> +  bool parent_modified_non_trans_table=
> thd->transaction.stmt.modified_non_trans_table;
>
> Please reset modified_non_trans_table to FALSE right here instead
> of mysql_execute_command.

done

>
> The patch is OK to push.
> Thank you for working on this,
>

Always pleasure to do it shoulder to shoulder!


Andrei