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

[Sergei Golubchik]

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.

>      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.

>    /* 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.

>    switch (lex->sql_command) {
>    case SQLCOM_SELECT:
>    {

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Radlkoferstr. 2, D-81373 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140