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