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

Konstja, hello.

> Hello Andrei,
>
> Please find my review inline.
>
> * Andrei Elkin <aelkin@mysql.com> [07/06/26 19:30]:
>
>> ChangeSet@1.2439, 2007-06-26 18:21:52+03:00, aelkin@dsl-hkibras1-ff5dc300-70.dhcp.inet.fi +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.
>

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

    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.

Can we help you?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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

Can't find what you're looking for?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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

-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com