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

[Andrei Elkin]

Sergei, hello!

> Hi!
>
> On Jun 26, Andrei Elkin wrote:
>> 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
>>   
>>   Once had been set the flag might later got reset inside of a stored routine
> execution stack.
>>   The reason was in that there was no check if a new statement started at time of
> resetting.
>>   The artifact affects most of binlogable DML queries. Notice, that multi-update
> is wrapped up within
>>   bug@27716 fix, multi-delete bug@29136.
>>   
>>   Fixed with saving parent's statement flag of whether the statement modified
> non-transactional table, 
>>   and unioning (merging) the value with that was gained in
> mysql_execute_command.
>>   
>>   Resettling thd->no_trans_update members into thd->transaction.`member`;
>>   Asserting code;
>>   Effectively the following properties are held.
>>   
>>   1. At the end of a substatement
> thd->transaction.stmt.modified_non_trans_table
>>      reflects the fact if such a table got modified by the substatement.
>>      That also respects THD::really_abort_on_warnin() requirements.
>>   2. Eventually thd->transaction.stmt.modified_non_trans_table will be
> computed as
>>      the union of the values of all invoked sub-statements.
>>      That fixes this bug#27417;
>>   
>>   Computing of thd->transaction.all.modified_non_trans_table is refined to
> base to the stmt's 
>>   value for all the case including insert .. select statement which before the
> patch had an 
>>   extra issue bug#28960.
>>   Minor issues are covered with mysql_load, mysql_delete, and binloggin of insert
> into temp_table select. 
>>   
>>   The supplied test verifies limitely, mostly asserts. The ultimate testing is
> defered
>>   for bug_13270, bug_23333.
>> 
>> --- 1.197/sql/sql_delete.cc	2007-04-17 16:51:56 +03:00
>> +++ 1.198/sql/sql_delete.cc	2007-06-26 18:21:48 +03:00
>> @@ -91,7 +91,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>>    }
>>  
>>    select_lex->no_error= thd->lex->ignore;
>> -
>> +  transactional_table= table->file->has_transactions();
>>    /*
>>      Test if the user wants to delete all rows and deletion doesn't have
>>      any side-effects (because of triggers), so we can use optimized
>> @@ -109,8 +109,11 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>>      if (!(error=table->file->delete_all_rows()))
>>      {
>>        error= -1;                                // ok
>> +      if (!transactional_table && deleted > 0)
>> +        thd->transaction.stmt.modified_non_trans_table= TRUE;
>>        goto cleanup;
>>      }
>> +    deleted= 0;
>
> Why ? 'deleted' is reset later, there's no need to do it here.

I was thinking about possibility to skip per-row loop 

    deleted= table->file->records;
    ...

    +    deleted= 0;

    if (error != HA_ERR_WRONG_COMMAND)
    {
      table->file->print_error(error,MYF(0));
      error=0;
      goto cleanup;
    }

with having deleted assigned with an estimated whereas 
there could be no rows deleted as by that point

         error=table->file->delete_all_rows()

executed with error != 0.

However, I am removing the reset because it's better play safe expecting that actual
deletion might happened nevertheless.

>
>>      if (error != HA_ERR_WRONG_COMMAND)
>>      {
>>        table->file->print_error(error,MYF(0));
>> @@ -280,6 +283,9 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>>      else
>>        table->file->unlock_row();  // Row failed selection, release lock on
> it
>>    }
>> +  if (!transactional_table && deleted > 0)
>> +    thd->transaction.stmt.modified_non_trans_table= TRUE;
>
> Why not to do it under the 'cleanup' label, as it was done before ?
>
>>    if (thd->killed && !error)
>>      error= 1;                                   // Aborted
>>    thd->proc_info="end";
>> @@ -314,7 +320,6 @@ cleanup:
>>    }
>>  
>>    delete select;
>> -  transactional_table= table->file->has_transactions();
>
> if you'd set thd->transaction.stmt.modified_non_trans_table here you
> wouldn't need to move this assignment either.

Good point! Indeed could be done better. Changed to have only one hunk
in the end.

>
>>    /* See similar binlogging code in sql_update.cc, for comments */
>>    if ((error < 0) || (deleted && !transactional_table))
>>    {
>> --- 1.617/sql/sql_parse.cc	2007-04-03 14:11:32 +03:00
>> +++ 1.618/sql/sql_parse.cc	2007-06-26 18:21:48 +03:00
>> @@ -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;
>
> Hm, I don't think it's a good idea.
> transaction.stmt describes a "statement transaction", that's BDB-ism,
> but still it refers to the statement, as a whole, the unit of work that
> will be rolled back on, for example, unique constraint violation.
>
> You're using thd->transaction.stmt.modified_non_trans_table, in fact, as
> a *sub-statement* flag, not a *statement*. Having a sub-stmt flag in
> transaction.stmt is VERY confusing. It's absolutely against
> expectations.
>
> This unfortunaly means, that 'modified_non_trans_table' does not belong
> in thd->transaction :(
>
> I actually liked that change, it's quite logical to have transaction
> flag in thd->transaction.all and stmt flag in thd->transaction.stmt, but
> now I see that you have sub-stmt flag in thd->transaction.stmt. And this
> is bad.
>

This was discussed over. We need to improve the comments. I am
attaching the latest changeset which is for Kostja's, who offered me
help, check.




regards,

Andrei


Attachment: [text/x-patch]