List:Commits« Previous MessageNext Message »
From:Libing Song Date:December 9 2010 2:45pm
Subject:Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798
Bug#56184
View as plain text  
Hi Alfranio and Daogang,
Thanks for your review.

A new patch was committed, please review it again.
Please find my comments below.

CHANGE LOGS
===========
Added test file rpl_DML_error.test for this patch.
Added more test case in rpl_begin_commit_rollback.test for this patch.
Rename cannot_safe_rollback to cannot_safely_rollback.

Moved
> +  THD::st_transactions &trans= thd->transaction;
> +
trans.all.add_unsafe_rollback_flags(trans.stmt.get_unsafe_rollback_flags());
> 
into
>     if (! thd->in_sub_stmt)
>     {



On Tue, 2010-12-07 at 14:11 +0000, Alfranio Correia wrote:
> Hi Libing,
> 
> Really great work.
> 
> STATUS
> ------
> 
> Not approved.
> 
> REQUIRED
> --------
> 
> 1 - Can you create a new patch based on the latest branch?
> 
Done.
> 2 - I think it is all instead of stmt:
> 
> @@ -1663,7 +1661,7 @@ multi_update::~multi_update()
>       delete [] copy_field;
>     thd->count_cuted_fields= CHECK_FIELD_IGNORE;		// Restore this setting
>     DBUG_ASSERT(trans_safe || !updated ||
> -              thd->transaction.all.modified_non_trans_table);
> +              thd->transaction.stmt.cannot_safe_rollback());
>   }
> 
As the following two lines were removed from multi_update,
all.modified_non_trans_table() is never called here.
So the assertion will cause server crash.

-  if (thd->transaction.stmt.modified_non_trans_table)
-      thd->transaction.all.modified_non_trans_table= TRUE;
> 
> 3 - Can you check if a failing statement that update a non-transactional 
> engine is correctly logged? Two cases with
> multi-row insert: (1) fails before inserting anything, (2) fails after
> inserting at least a row.
OK.
> 
> @@ -850,9 +850,6 @@ int mysql_update(THD *thd,
>       query_cache_invalidate3(thd, table_list, 1);
>     }
> 
> -  if (thd->transaction.stmt.modified_non_trans_table)
> -      thd->transaction.all.modified_non_trans_table= TRUE;
> 
> 
> 4 - Wouldn't be better s/cannot_safe_rollback/cannot_safely_rollback/ ?
Yes, I changed it.
> 
> 
> REQUESTS
> --------
> 
> 
> 1 - Why don't you use thd->transaction.stmt.created_temp_table();
> at the points where OPTION_KEEP_LOG was set? For example,
> 
The principle is that stmt.unsafe_rollback_flags has to be set before
the statement is binlogged. As
+ if (is_trans_cache && stmt_cannot_safely_rollback(thd))
+   cache_manager->set_trx_cache_cannot_rollback()
are called when the statement is being binlogged.
>       }
>       else
>       {
> -      /* So that CREATE TEMPORARY TABLE gets to binlog at 
> commit/rollback */
> -      if (create_info.options & HA_LEX_CREATE_TMP_TABLE)
> -        thd->variables.option_bits|= OPTION_KEEP_LOG;
>         /* regular create */
>         if (create_info.options & HA_LEX_CREATE_TABLE_LIKE)
>         {
> 
> 
> 
> 2 - I cannot understand why a copy at this point of the code.
> What does it happen if the statement fails and still a non-transactional
> table is updated? Can you write a comment on this? See Item 3, above.
> 
I removed the code that set all.unsafe_rollback_flags from update.cc,
delete.cc, insert.cc and load.cc.

I think they just need to know stmt.unsafe_rollback_flags and the
all.unsafe_rollback_flags can be handled at the end of the statement.
So the reasonable logic is that:
a. stmt.reset_unsafe_rollback_flags();
b. enter a statement
c. set stmt.unsafe_rollback_flags as soon as something has been done and
can not be rolled back. The flags will never be cleared in the
statement.
d. exit the statement.
e. Merge stmt.unsafe_rollback_flags to all.unsafe_rollback_flags, no
matter the statement succeed or not.

The only change against the original logic is that merging to all.unsafe_rollback_flags
is move out statement handlers.

In fact, That if trx_cache should be binlogged or truncated depends 
only on stmt.unsafe_rollback_flags. The only caller of all.unsafe_rollback_flags
is SQL thread. When a temporary error happens, SQL thread use all.unsafe_rollback_flags
to decide whether the current transaction can be rolled back and retry again.

> @@ -4378,6 +4366,8 @@ finish:
>     DBUG_ASSERT(!thd->in_active_multi_stmt_transaction() ||
>                  thd->in_multi_stmt_transaction_mode());
> 
> +  THD::st_transactions &trans= thd->transaction;
> + 
> trans.all.add_unsafe_rollback_flags(trans.stmt.get_unsafe_rollback_flags());
> 
>     if (! thd->in_sub_stmt)
>     {
> 
> 
> Cheers.
> 


-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================

Thread
bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798 Bug#56184Li-Bing.Song4 Dec
  • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798Bug#56184Alfranio Correia7 Dec
    • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798Bug#56184Libing Song9 Dec