List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:May 12 2011 9:27am
Subject:Re: bzr commit into mysql-trunk branch (alfranio.correia:3773) Bug#11763126
View as plain text  
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.

Thread
bzr commit into mysql-trunk branch (alfranio.correia:3773) Bug#11763126Alfranio Correia8 Apr
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3773)Bug#11763126Dmitry Lenev3 May
    • Re: bzr commit into mysql-trunk branch (alfranio.correia:3773) Bug#11763126Alfranio Correia12 May