List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:May 3 2011 2:50pm
Subject:Re: bzr commit into mysql-trunk branch (alfranio.correia:3773)
Bug#11763126
View as plain text  
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... :)

...

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

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

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
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