From: Dmitry Lenev Date: May 3 2011 2:50pm Subject: Re: bzr commit into mysql-trunk branch (alfranio.correia:3773) Bug#11763126 List-Archive: http://lists.mysql.com/commits/136584 Message-Id: <20110503145035.GC29857@bandersnatch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 [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