Hi Dmitry,
Thank you again for the great review.
See some comments in-line.
Cheers.
On 05/03/2011 03:50 PM, Dmitry Lenev wrote:
> Hello Alfranio!
>
> I see that all important issues that I have note in my review for
> Li-Bing's main patch are addressed by this follow-up patch.
>
> Thanks a lot!
>
> Still I have a few comments:
>
> * Alfranio Correia <alfranio.correia@stripped> [11/04/08 21:27]:
>> #At
> file:///home/acorreia/workspace.oracle/repository.mysql/bzrwork/bug-11763126/mysql-trunk/
> based on revid:alfranio.correia@stripped
>>
>> 3773 Alfranio Correia 2011-04-08
>> BUG#11763126
>> @ mysql-test/extra/rpl_tests/rpl_drop_create_temp_table.inc
>> Improved the test case to cover the following cases:
>
> ...
>
>> === modified file 'sql/binlog.cc'
>> --- a/sql/binlog.cc 2011-03-16 09:37:35 +0000
>> +++ b/sql/binlog.cc 2011-04-08 17:20:14 +0000
>> @@ -288,7 +288,7 @@ public:
>> void restore_savepoint(my_off_t pos)
>> {
>> truncate(pos);
>> - if (pos < before_stmt_pos)
>> + if (pos <= before_stmt_pos)
>> before_stmt_pos= MY_OFF_T_UNDEF;
>> }
>>
>> @@ -809,20 +809,55 @@ static int binlog_rollback(handlerton *h
>> DBUG_RETURN(error);
>> }
>>
>> - if (cache_mngr->trx_cache_cannot_rollback())
>> + if (ending_trans(thd, all))
>> {
>> - if (ending_trans(thd, all))
>> + if (trans_cannot_safely_rollback(thd))
>> {
>> + /*
>> + If the transaction is being rolled back and contains changes that
>> + cannot be rolled back, the trx-cache's content is flushed.
>> + */
>> error= binlog_rollback_flush_trx_cache(thd, cache_mngr);
>> }
>> else
>> {
>> - /* The statement should be kept in trx_cache. */
>> - cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
>> + /*
>> + If the transaction is being rolled back and its changes can be
>> + rolled back, the trx-cache's content is truncated.
>> + */
>> + error= binlog_truncate_trx_cache(thd, cache_mngr, all);
>> }
>> }
>> else
>> - error= binlog_truncate_trx_cache(thd, cache_mngr, all);
>> + {
>> + /*
>> + If a statement is being rolled back, it is necessary to know
>> + exactly why a statement may not be safely rolled back as in
>> + some specific situations the statement can be rolled back.
>> + */
>> +
>> + if (thd->transaction.stmt.has_dropped_temp_table() ||
>> + thd->transaction.stmt.has_created_temp_table() ||
>> + (thd->transaction.stmt.has_modified_non_trans_table() &&
>> + thd->variables.binlog_format == BINLOG_FORMAT_STMT))
>> + {
>> + /*
>> + If the statement is being rolled back and dropped or created a
>> + temporary table or modified a non-transactional table and the
>> + statement-based replication is in use, the statement's changes
>> + in the trx-cache are preserved.
>> + */
>> + cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
>> + }
>> + else
>> + {
>> + /*
>> + Otherwise, the statement's changes in the trx-cache are
>> + truncated.
>> + */
>> + error= binlog_truncate_trx_cache(thd, cache_mngr, all);
>> + }
>> + }
>>
>> DBUG_RETURN(error);
>> }
>
> Thanks to new comments this code is much more clear now.
> Still I assume that both the above changes were reviewed
> by some expert from Replication team, I can't say that
> I have fully understood it... :)
>
I agree that this code is not easy to understand and with
Libing's patch we managed to simplify it a lot.
I will ask someone in the rep team to take a look at this and see
if I missed anything else this time.
> ...
>
>> === modified file 'sql/sql_class.h'
>> --- a/sql/sql_class.h 2011-03-16 09:37:35 +0000
>> +++ b/sql/sql_class.h 2011-04-08 17:20:14 +0000
>> @@ -1465,9 +1667,10 @@ private:
>> MDL_ticket *m_mdl_blocks_commits_lock;
>> };
>>
>> -
>> extern "C" void my_message_sql(uint error, const char *str, myf MyFlags);
>>
>> +
>> +
>> /**
>> @class THD
>> For each client connection we create a separate thread with THD serving as
>
> ...
>
> In my opinion it is better to remove unnecessary formatting changes,
> as they can complicate future merges.
I agree.... I did not bother with this now but before pushing the patch I will
remove any unnecessary change Libing or I have introduced to the code.
>
>> === modified file 'sql/sql_table.cc'
>> --- a/sql/sql_table.cc 2011-03-16 09:37:35 +0000
>> +++ b/sql/sql_table.cc 2011-04-08 17:20:14 +0000
>> @@ -2250,14 +2250,6 @@ int mysql_rm_table_no_locks(THD *thd, TA
>> goto err;
>> }
>>
>> - /*
>> - DROP TEMPORARY TABLE doesn't terminate a transaction. Calling
>> - stmt.dropped_temp_table() guarantees the transaction can be binlogged
>> - correctly.
>> - */
>> - if (!error && drop_temporary)
>> - thd->transaction.stmt.dropped_temp_table();
>> -
>> if ((drop_temporary && if_exists) || !error)
>> {
>> /*
>> @@ -2486,6 +2478,7 @@ err:
>> {
>> if (non_trans_tmp_table_deleted)
>> {
>> + thd->transaction.stmt.mark_dropped_temp_table();
>> /* Chop of the last comma */
>> built_non_trans_tmp_query.chop();
>> built_non_trans_tmp_query.append(" /* generated by server */");
>> @@ -2496,6 +2489,7 @@ err:
>> }
>> if (trans_tmp_table_deleted)
>> {
>> + thd->transaction.stmt.mark_dropped_temp_table();
>> /* Chop of the last comma */
>> built_trans_tmp_query.chop();
>> built_trans_tmp_query.append(" /* generated by server */");
>
> Hmm... So now transaction will be marked as unsafe to rollback due to
> fact that it has dropped temporary table only if binary log is open.
>
> This seems wrong to me as this flag now not only responsible for
> controlling how events are flushed into log but also ensures that
> on ROLLBACK an appropriate warning emitted.
>
> I think it is better to fix this issue by moving call to
> mark_dropped_temp_table() out of if-statement which has
> mysql_bin_log.is_open() as part of condition. I.e. change
> the above code to something like:
>
> if (non_trans_tmp_table_deleted ||
> trans_tmp_table_deleted || non_tmp_table_deleted)
> {
> query_cache_invalidate3(thd, tables, 0);
>
> + if (non_trans_tmp_table_deleted || trans_tmp_table_deleted)
> + thd->transaction.stmt.mark_dropped_temp_table();
>
> if (!dont_log_query && mysql_bin_log.is_open())
> {
>
> ...
>
> Otherwise I am OK with combination of Li-Bing's and your patches and
> think that it is OK to push it after addressing/discussing the above
> issues and getting approval for replication-related changes from
> an expert from Replication Team.
>
I agree with you.
I will do what you have suggested.
