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

[Konstantin Osipov]

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?

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.

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.

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.

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

> +    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?

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

> +++ 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 :-( )

> +#
> +# bug#27417,bug#28960
> +#
> +# testing appearence of insert into temp_table
> +#

Please provide titles of the bugs.
> +
> +## send_eof() branch

A more elaborate explanation of what is tested is needed.
> +
> +# 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.


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

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

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY