List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:December 2 2010 4:22pm
Subject:Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798
Bug#56184
View as plain text  
Hi Libing,

Great work. You are in the right direction to simplify the binlog.
Please, see some comments in what follows.

STATUS
------

Patch not approved.


REQUESTS
--------

1 - Remove the disabled line in /sql/sql_priv.h

2 - Can you explain the additional expressions? I can understand the
format but I don't think it is necessary in_multi_stmt_transaction_mode?

   /* So that CREATE TEMPORARY TABLE gets to binlog at commit/rollback */
     if (!res && (create_info.options & HA_LEX_CREATE_TMP_TABLE) &&
         thd->in_multi_stmt_transaction_mode() &&
         !thd->is_current_stmt_binlog_format_row())
       thd->transaction.stmt.modified_non_trans_table= TRUE;

3 - There are other entry points where you need to put 
set_non_trans_table_in_trx_cache(). For instance, 
flush_and_set_pending_rows_event().

4 - There are several failures after your patch:

rpl.rpl_mixed_binlog_max_cache_size
rpl.rpl_mixed_drop_create_temp_table
rpl.rpl_mixed_mixing_engines
rpl.rpl_non_direct_mixed_mixing_engines
rpl.rpl_stm_innodb
rpl.rpl_stop_slave
rpl.rpl_non_direct_row_mixing_engines
rpl.rpl_row_binlog_max_cache_size
rpl.rpl_row_drop_create_temp_table
rpl.rpl_row_mixing_engines
rpl.rpl_non_direct_stm_mixing_engines
rpl.rpl_stm_binlog_max_cache_size
rpl.rpl_stm_drop_create_temp_table
rpl.rpl_stm_mixing_engines
binlog.binlog_stm_mix_innodb_myisam
binlog.binlog_row_mix_innodb_myisam
binlog.binlog_innodb_row
main.flush_read_lock main.mysql

5 - Please, remove the function stmt_accessed_non_trans_temp_table and 
any other function that is not necessary after your patch.

6 - You don't need a test for BUG#56184.


SUGGESTIONS
-----------

1 - Please, move this

+  void set_non_trans_table_in_trx_cache()
+  {
+    non_trans_table_in_trx_cache= TRUE;
+  }
+
+  bool is_non_trans_table_in_trx_cache()
+  {
+    return non_trans_table_in_trx_cache;
    }

to binlog_cache_data.

Cheers.


On 11/29/2010 05:36 AM, Li-Bing.Song@stripped wrote:
> #At file:///home/anders/Work/bzrwork/wt3/mysql-trunk-bugfixing/ based on
> revid:dao-gang.qu@stripped
>
>   3350 Li-Bing.Song@stripped	2010-11-29
>        Bug#56184 Rolled back transaction without non-trasactional table updated was
> binlogged
>        Bug#55798 Slave SQL retry on transaction inserts extra data into
> non-transaction table
>
>        Bug#56184
>        The transaction modified non-transactional table will be binlogged with
> ROLLBACK if it
>        rolls back on master. It includes the case that all statements which modified
>        non-transactional table are binlogged outside(before) the transaction.
>        Example:
>          BEGIN
>          INSERT INTO trans-table;
>          INSERT INOT non-trans-table;
>          ROLLBACK
>        it will be binlogged as:
>          BEGIN
>          INSERT INTO non-trans-table;
>          COMMIT
>          BEGIN
>          INSERT INTO trans-table;
>          ROLLBACK;
>        All statements in the second binlogged transaction modify only transactional
> tables and
>        are rolled back safely on master. So the second transaction should not be
> binlogged.
>
>        After 5.5, there are two caches for binary logs. A transactional cache and a
> statement cache.
>        When executing a transaction, statements modified only transactional tables
> are always put in
>        transactional cache. statements modified non-transactional tables can be put
> in both caches.
>        It depends on different situations. In this patch, a flag is added to mark if
> there is any
>        statement modified non-transactional table is in transactional cache. When
> rolling back a
>        transaction on master, transactional cache should not be flushed to binary
> log, if there is
>        no any statement in it modified non-transactional table, else it should be
> flushed into binary
>        log followed by 'ROLLBACK' statement.

Some suggestions to improve the comments.

After 5.5, there are two caches for binary logs, a transactional cache 
and a statement cache. When executing a transaction, statements that 
modified only transactional tables are always put in transactional 
cache. Statements that modified non-transactional tables can be put in 
either transactional or non-transactional cache depending on different 
situations. In this patch, a flag is added to mark if there is any 
statement that modified non-transactional table in transactional cache. 
When rolling back a transaction on master, transactional cache should 
not be flushed to binary log, if there is no statement in it that 
modified a non-transactional table. Otherwise, it should be flushed into 
binary log followed by 'ROLLBACK' statement.


>
>        BUG#55798
>        When a temporary error(eg. Lock timeout) happens, Slave SQL thread will
> rollback the
>        transaction and retry it again. But it is possible that the transaction cannot
> be
>        rolled back safely. For example a non-transactional table has been modified by
> the
>        transaction. It will make master and slave diversely.

s/diversely/to diverge/

>
>        After this patch, SQL thread will not retry to execute a transaction which can
> not be rolled
>        back safely if temporary error is encountered.
>       @ mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test
>          Add test to verify this patch.
>       @ sql/binlog.cc
>          Refactor reset_cache, it is divided into reset_trx_cache() and
> reset_stmt_cache().
>          Add code to fix this bug.
>       @ sql/rpl_slave.cc
>          OPTION_KEEP_LOG is replaced by stmt.modified_non_trans_table.
>          Remove OPTION_KEEP_LOG from all code.
>       @ sql/sql_parse.cc
>          OPTION_KEEP_LOG is replaced by stmt.modified_non_trans_table.
>          Remove OPTION_KEEP_LOG from all code.
>       @ sql/sql_table.cc
>          Set stmt.modified_non_trans_table after a temporary table has been dropped.
>       @ sql/sys_vars.cc
>          OPTION_KEEP_LOG is replaced by stmt.modified_non_trans_table.
>          Remove OPTION_KEEP_LOG from all code.
>       @ sql/transaction.cc
>          OPTION_KEEP_LOG is replaced by stmt.modified_non_trans_table.
>          Remove OPTION_KEEP_LOG from all code.
>

ok.

>      modified:
>        mysql-test/suite/rpl/r/rpl_begin_commit_rollback.result
>        mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test
>        sql/binlog.cc
>        sql/rpl_slave.cc
>        sql/sql_parse.cc
>        sql/sql_priv.h
>        sql/sql_table.cc
>        sql/sys_vars.cc
>        sql/transaction.cc
> === modified file 'mysql-test/suite/rpl/r/rpl_begin_commit_rollback.result'
> --- a/mysql-test/suite/rpl/r/rpl_begin_commit_rollback.result	2010-09-27 13:20:24
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_begin_commit_rollback.result	2010-11-29 05:36:21
> +0000
> @@ -173,6 +173,64 @@ in savepoint mixed_cases
>   # Verify INSERT statements on the Innodb table are rolled back;
>   SELECT * FROM db1.t1 WHERE a IN (30, 40);
>   a
> +
> +# Bug#56184 Rolled back transaction without non-trasactional table
> +# updated was binlogged
> +# ----------------------------------------------------------------
> +# Transactional cache should not be binlogged if no statement in it
> +# updates a non-trasactional table.
> +# [ on master ]
> +USE db1;
> +DROP TABLE t1, t2;
> +CREATE TABLE t1(c1 INT KEY) ENGINE=MyISAM;
> +CREATE TABLE t2(c1 INT KEY) ENGINE=InnoDB;
> +USE db1;
> +# [ on slave ]
> +# This statement is used to verify that the transaction which is rolled
> +# back is not binlogged. The duplicate keys will cause slave to stop if
> +# the transaction is binlogged.
> +INSERT INTO t2 VALUES(1),(2);
> +# [ on master ]
> +SET @@session.binlog_direct_non_transactional_updates= TRUE;
> +BEGIN;
> +INSERT INTO t2 VALUES(1);
> +# It will not be put into transaction cache.
> +INSERT INTO t1 VALUES(1);
> +INSERT INTO t2 VALUES(2);
> +ROLLBACK;
> +Warnings:
> +Warning	1196	Some non-transactional changed tables couldn't be rolled back
> +Comparing tables master:db1.t1 and slave:db1.t1
> +
> +# BUG#55798 Slave SQL retry on transaction inserts extra data into
> +# non-transaction table
> +# ----------------------------------------------------------------
> +# To verify that SQL thread does not retry a transaction which can
> +# not be rolled back safely, even though only a temporary error is
> +# encountered.
> +
> +# [ on slave ]
> +SET GLOBAL innodb_lock_wait_timeout= 1;
> +STOP SLAVE SQL_THREAD;
> +START SLAVE SQL_THREAD;
> +BEGIN;
> +# It will lock table t2 on row in which c1 is 1 until COMMIT or ROLLBACK
> +UPDATE t2 SET c1=10 WHERE c1=1;
> +# [ on master ]
> +SET @@session.binlog_direct_non_transactional_updates= FALSE;
> +BEGIN;
> +INSERT INTO t2 VALUES(3);
> +INSERT INTO t1 VALUES(4);
> +Warnings:
> +Note	1592	Unsafe statement written to the binary log using statement format since
> BINLOG_FORMAT = STATEMENT. Statement is unsafe because it accesses a non-transactional
> table after accessing a transactional table within the same transaction.
> +UPDATE t2 SET c1=11 WHERE c1=1;
> +COMMIT;
> +# [ on slave ]
> +Comparing tables master:db1.t1 and slave:db1.t1
> +ROLLBACK;
> +DELETE FROM t1 WHERE c1=4;
> +SET GLOBAL innodb_lock_wait_timeout= 50;
> +START SLAVE SQL_THREAD;
>   #
>   # Clean up
>   #
>
> === modified file 'mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test'
> --- a/mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test	2010-06-30 13:09:43
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test	2010-11-29 05:36:21
> +0000
> @@ -171,10 +171,90 @@ SELECT * FROM db1.t2 WHERE s LIKE '% sav
>   --echo # Verify INSERT statements on the Innodb table are rolled back;
>   SELECT * FROM db1.t1 WHERE a IN (30, 40);
>
> +--echo
> +--echo # Bug#56184 Rolled back transaction without non-trasactional table
> +--echo # updated was binlogged
> +--echo # ----------------------------------------------------------------
> +--echo # Transactional cache should not be binlogged if no statement in it
> +--echo # updates a non-trasactional table.
> +--echo # [ on master ]
> +connection master;
> +USE db1;
> +DROP TABLE t1, t2;
> +CREATE TABLE t1(c1 INT KEY) ENGINE=MyISAM;
> +CREATE TABLE t2(c1 INT KEY) ENGINE=InnoDB;
> +sync_slave_with_master;
> +
> +USE db1;
> +--echo # [ on slave ]
> +--echo # This statement is used to verify that the transaction which is rolled
> +--echo # back is not binlogged. The duplicate keys will cause slave to stop if
> +--echo # the transaction is binlogged.
> +INSERT INTO t2 VALUES(1),(2);
> +
> +--echo # [ on master ]
> +connection master;
> +SET @@session.binlog_direct_non_transactional_updates= TRUE;
> +BEGIN;
> +INSERT INTO t2 VALUES(1);
> +--echo # It will not be put into transaction cache.
> +INSERT INTO t1 VALUES(1);
> +INSERT INTO t2 VALUES(2);
> +ROLLBACK;
> +
> +sync_slave_with_master;
> +let $diff_table=db1.t1;
> +source include/rpl_diff_tables.inc;
> +
> +--echo
> +--echo # BUG#55798 Slave SQL retry on transaction inserts extra data into
> +--echo # non-transaction table
> +--echo # ----------------------------------------------------------------
> +--echo # To verify that SQL thread does not retry a transaction which can
> +--echo # not be rolled back safely, even though only a temporary error is
> +--echo # encountered.
> +--echo
> +--echo # [ on slave ]
> +
> +let $timeout_old= `SELECT @@GLOBAL.innodb_lock_wait_timeout`;
> +SET GLOBAL innodb_lock_wait_timeout= 1;
> +
> +STOP SLAVE SQL_THREAD;
> +source include/wait_for_slave_sql_to_stop.inc;
> +START SLAVE SQL_THREAD;
> +source include/wait_for_slave_sql_to_start.inc;
> +
> +BEGIN;
> +--echo # It will lock table t2 on row in which c1 is 1 until COMMIT or ROLLBACK
> +UPDATE t2 SET c1=10 WHERE c1=1;
> +
> +connection master;
> +--echo # [ on master ]
> +SET @@session.binlog_direct_non_transactional_updates= FALSE;
> +BEGIN;
> +INSERT INTO t2 VALUES(3);
> +INSERT INTO t1 VALUES(4);
> +UPDATE t2 SET c1=11 WHERE c1=1;
> +COMMIT;
> +			
> +connection slave;
> +--echo # [ on slave ]
> +let $slave_sql_errno= 1205;
> +source include/wait_for_slave_sql_to_stop.inc;
> +let $diff_table_1=master:db1.t1;
> +let $diff_table_2=slave:db1.t1;
> +source include/diff_tables.inc;
> +ROLLBACK;
> +DELETE FROM t1 WHERE c1=4;
> +
> +eval SET GLOBAL innodb_lock_wait_timeout= $timeout_old;
> +START SLAVE SQL_THREAD;
> +source include/wait_for_slave_sql_to_start.inc;
> +
>   --echo #
>   --echo # Clean up
>   --echo #
>   connection master;
>   DROP DATABASE db1;
>   DROP DATABASE db2;
> -source include/master-slave-end.inc;
> +source include/master-slave-end.inc;
> \ No newline at end of file
>
> === modified file 'sql/binlog.cc'
> --- a/sql/binlog.cc	2010-11-16 12:38:17 +0000
> +++ b/sql/binlog.cc	2010-11-29 05:36:21 +0000
> @@ -121,16 +121,6 @@ public:
>       return(incident);
>     }
>
> -  void set_changes_to_non_trans_temp_table()
> -  {
> -    changes_to_non_trans_temp_table_flag= TRUE;
> -  }
> -
> -  bool changes_to_non_trans_temp_table()
> -  {
> -    return (changes_to_non_trans_temp_table_flag);
> -  }
> -
>     void reset()
>     {
>       truncate(0);
> @@ -219,11 +209,27 @@ private:
>
>   class binlog_cache_mngr {
>   public:
> -  binlog_cache_mngr() {}
> +  binlog_cache_mngr() : non_trans_table_in_trx_cache(FALSE) {}
>
> -  void reset_cache(binlog_cache_data* cache_data)
> +  void reset_stmt_cache()
>     {
> -    cache_data->reset();
> +    stmt_cache.reset();
> +  }
> +
> +  void reset_trx_cache()
> +  {
> +    trx_cache.reset();
> +    non_trans_table_in_trx_cache= FALSE;
> +  }
> +
> +  void set_non_trans_table_in_trx_cache()
> +  {
> +    non_trans_table_in_trx_cache= TRUE;
> +  }
> +
> +  bool is_non_trans_table_in_trx_cache()
> +  {
> +    return non_trans_table_in_trx_cache;
>     }
>
>     binlog_cache_data* get_binlog_cache_data(bool is_transactional)
> @@ -244,6 +250,12 @@ private:
>
>     binlog_cache_mngr&  operator=(const binlog_cache_mngr&  info);
>     binlog_cache_mngr(const binlog_cache_mngr&  info);
> +
> +  /*
> +    It will be set TRUE if any statement which updates a non-transactional
> +    table is put in trx_cache.
> +  */
> +  bool non_trans_table_in_trx_cache;
>   };
>
>   /**
> @@ -406,7 +418,7 @@ binlog_flush_trx_cache(THD *thd, binlog_
>     */
>     error= mysql_bin_log.write(thd,&cache_mngr->trx_cache.cache_log, end_ev,
>                                cache_mngr->trx_cache.has_incident());
> -  cache_mngr->reset_cache(&cache_mngr->trx_cache);
> +  cache_mngr->reset_trx_cache();
>
>     statistic_increment(binlog_cache_use,&LOCK_status);
>     if (cache_log->disk_writes != 0)
> @@ -456,7 +468,7 @@ binlog_truncate_trx_cache(THD *thd, binl
>       if (cache_mngr->trx_cache.has_incident())
>         error= mysql_bin_log.write_incident(thd, TRUE);
>
> -    cache_mngr->reset_cache(&cache_mngr->trx_cache);
> +    cache_mngr->reset_trx_cache();
>
>       thd->clear_binlog_table_maps();
>     }
> @@ -516,7 +528,7 @@ binlog_flush_stmt_cache(THD *thd, binlog
>     if ((error= mysql_bin_log.write(thd, cache_log,&qev,
>                                     cache_mngr->stmt_cache.has_incident())))
>       DBUG_RETURN(error);
> -  cache_mngr->reset_cache(&cache_mngr->stmt_cache);
> +  cache_mngr->reset_stmt_cache();
>
>     statistic_increment(binlog_cache_use,&LOCK_status);
>     if (cache_log->disk_writes != 0)
> @@ -563,7 +575,7 @@ static int binlog_commit(handlerton *hto
>       /*
>         we're here because cache_log was flushed in MYSQL_BIN_LOG::log_xid()
>       */
> -    cache_mngr->reset_cache(&cache_mngr->trx_cache);
> +    cache_mngr->reset_trx_cache();
>       DBUG_RETURN(0);
>     }
>
> @@ -616,7 +628,7 @@ static int binlog_rollback(handlerton *h
>     if (cache_mngr->stmt_cache.has_incident())
>     {
>       error= mysql_bin_log.write_incident(thd, TRUE);
> -    cache_mngr->reset_cache(&cache_mngr->stmt_cache);
> +    cache_mngr->reset_stmt_cache();
>     }
>     else if (!cache_mngr->stmt_cache.empty())
>     {
> @@ -628,7 +640,7 @@ static int binlog_rollback(handlerton *h
>       /*
>         we're here because cache_log was flushed in MYSQL_BIN_LOG::log_xid()
>       */
> -    cache_mngr->reset_cache(&cache_mngr->trx_cache);
> +    cache_mngr->reset_trx_cache();
>       DBUG_RETURN(0);
>     }
>
> @@ -646,57 +658,21 @@ static int binlog_rollback(handlerton *h
>         a cache and need to be rolled back.
>       */
>       error= binlog_truncate_trx_cache(thd, cache_mngr, all);
> +    DBUG_RETURN(error);
>     }
> -  else
> -  {
> -    /*
> -      We flush the cache wrapped in a beging/rollback if:
> -        . aborting a single or multi-statement transaction and;
> -        . the format is STMT and non-trans engines were updated or;
> -        . the OPTION_KEEP_LOG is activate.
> -        . the OPTION_KEEP_LOG is active or;
> -        . the format is STMT and a non-trans table was updated or;
> -        . the format is MIXED and a temporary non-trans table was
> -          updated or;
> -        . the format is MIXED, non-trans table was updated and
> -          aborting a single statement transaction;
> -     */
> -     if (ending_trans(thd, all)&&
> -         ((thd->variables.option_bits&  OPTION_KEEP_LOG) ||
> -          (trans_has_updated_non_trans_table(thd)&&
> -          thd->variables.binlog_format == BINLOG_FORMAT_STMT) ||
> -         (cache_mngr->trx_cache.changes_to_non_trans_temp_table()&&
> -          thd->variables.binlog_format == BINLOG_FORMAT_MIXED) ||
> -         (trans_has_updated_non_trans_table(thd)&&
> -          ending_single_stmt_trans(thd,all)&&
> -          thd->variables.binlog_format == BINLOG_FORMAT_MIXED)))
> +
> +  if (ending_trans(thd, all))
> +  {
> +    if (cache_mngr->is_non_trans_table_in_trx_cache())
>       {
>         Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, FALSE, TRUE, 0);
>         error= binlog_flush_trx_cache(thd, cache_mngr,&qev);
>       }
> -    /*
> -      Truncate the cache if:
> -        . aborting a single or multi-statement transaction or;
> -        . the OPTION_KEEP_LOG is not activate and;
> -        . the format is not STMT or no non-trans were updated.
> -        . the OPTION_KEEP_LOG is not active and;
> -        . the format is not STMT or no non-trans was updated and;
> -        . the format is not MIXED or no temporary non-trans was
> -          updated.
> -     */
> -     else if (ending_trans(thd, all) ||
> -              (!(thd->variables.option_bits&  OPTION_KEEP_LOG)&&
> -              (!stmt_has_updated_non_trans_table(thd) ||
> -               thd->variables.binlog_format != BINLOG_FORMAT_STMT)&&
> -              (!cache_mngr->trx_cache.changes_to_non_trans_temp_table() ||
> -               thd->variables.binlog_format != BINLOG_FORMAT_MIXED)))
> +    else
>         error= binlog_truncate_trx_cache(thd, cache_mngr, all);
>     }
> -  /*
> -    This is part of the stmt rollback.
> -  */
> -  if (!all)
> -    cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
> +  else
> +    cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
>     DBUG_RETURN(error);
>   }
>
> @@ -767,8 +743,10 @@ static int binlog_savepoint_rollback(han
>       non-transactional table. Otherwise, truncate the binlog cache starting
>       from the SAVEPOINT command.
>     */
> -  if (unlikely(trans_has_updated_non_trans_table(thd) ||
> -               (thd->variables.option_bits&  OPTION_KEEP_LOG)))
> +  binlog_cache_mngr *const cache_mngr=
> +    (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
> +
> +  if (unlikely(cache_mngr->is_non_trans_table_in_trx_cache()))
>     {
>       String log_query;
>       if (log_query.append(STRING_WITH_LEN("ROLLBACK TO ")) ||
> @@ -3090,6 +3068,8 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>   #endif /* HAVE_REPLICATION */
>
>       IO_CACHE *file= NULL;
> +    bool is_trans_cache= FALSE;
> +    binlog_cache_mngr *cache_mngr= NULL;
>
>       if (event_info->use_direct_logging())
>       {
> @@ -3101,16 +3081,11 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>         if (thd->binlog_setup_trx_data())
>           goto err;
>
> -      binlog_cache_mngr *const cache_mngr=
> -        (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
> -
> -      bool is_trans_cache= use_trans_cache(thd, event_info->use_trans_cache());
> +      cache_mngr= (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
> +      is_trans_cache= use_trans_cache(thd, event_info->use_trans_cache());
>         file= cache_mngr->get_binlog_cache_log(is_trans_cache);
>         cache_data= cache_mngr->get_binlog_cache_data(is_trans_cache);
>
> -      if (thd->lex->stmt_accessed_non_trans_temp_table())
> -        cache_data->set_changes_to_non_trans_temp_table();
> -
>         thd->binlog_start_trans_and_stmt();
>       }
>       DBUG_PRINT("info",("event type: %d",event_info->get_type_code()));
> @@ -3185,7 +3160,8 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>         goto err;
>
>       error= 0;
> -
> +    if (is_trans_cache&& 
> thd->transaction.stmt.modified_non_trans_table)
> +      cache_mngr->set_non_trans_table_in_trx_cache();
>   err:
>       if (event_info->use_direct_logging())
>       {
>
> === modified file 'sql/rpl_slave.cc'
> --- a/sql/rpl_slave.cc	2010-11-16 12:38:17 +0000
> +++ b/sql/rpl_slave.cc	2010-11-29 05:36:21 +0000
> @@ -1008,17 +1008,7 @@ static bool sql_slave_killed(THD* thd, R
>     DBUG_ASSERT(rli->slave_running == 1);// tracking buffer overrun
>     if (abort_loop || thd->killed || rli->abort_slave)
>     {
> -    /*
> -      The transaction should always be binlogged if OPTION_KEEP_LOG is set
> -      (it implies that something can not be rolled back). And such case
> -      should be regarded similarly as modifing a non-transactional table
> -      because retrying of the transaction will lead to an error or inconsistency
> -      as well.
> -      Example: OPTION_KEEP_LOG is set if a temporary table is created or dropped.
> -    */
> -    if ((thd->transaction.all.modified_non_trans_table ||
> -         (thd->variables.option_bits&  OPTION_KEEP_LOG))
> -&&  rli->is_in_group())
> +    if (thd->transaction.all.modified_non_trans_table&& 
> rli->is_in_group())
>       {
>         char msg_stopped[]=
>           "... The slave SQL is stopped, leaving the current group "
> @@ -2796,7 +2786,8 @@ static int exec_relay_log_event(THD* thd
>       if (slave_trans_retries)
>       {
>         int UNINIT_VAR(temp_err);
> -      if (exec_res&&  (temp_err= has_temporary_error(thd)))
> +      if (exec_res&&  (temp_err= has_temporary_error(thd))&&
> +          !thd->transaction.all.modified_non_trans_table)
>         {
>           const char *errmsg;
>           /*
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-10-27 14:46:44 +0000
> +++ b/sql/sql_parse.cc	2010-11-29 05:36:21 +0000
> @@ -2463,10 +2463,6 @@ case SQLCOM_PREPARE:
>           */
>           lex->unlink_first_table(&link_to_local);
>
> -        /* 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;
> -
>           /*
>             select_create is currently not re-execution friendly and
>             needs to be created for every execution of a PS/SP.
> @@ -2492,9 +2488,6 @@ case SQLCOM_PREPARE:
>       }
>       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)
>         {
> @@ -2511,6 +2504,11 @@ case SQLCOM_PREPARE:
>         if (!res)
>           my_ok(thd);
>       }
> +    /* So that CREATE TEMPORARY TABLE gets to binlog at commit/rollback */
> +    if (!res&&  (create_info.options& 
> HA_LEX_CREATE_TMP_TABLE)&&
> +        thd->in_multi_stmt_transaction_mode()&&
> +        !thd->is_current_stmt_binlog_format_row())
> +      thd->transaction.stmt.modified_non_trans_table= TRUE;
>
>   end_with_restore_list:
>       break;
> @@ -3015,11 +3013,6 @@ end_with_restore_list:
>         if (check_table_access(thd, DROP_ACL, all_tables, FALSE, UINT_MAX, FALSE))
>   	goto error;				/* purecov: inspected */
>       }
> -    else
> -    {
> -      /* So that DROP TEMPORARY TABLE gets to binlog at commit/rollback */
> -      thd->variables.option_bits|= OPTION_KEEP_LOG;
> -    }
>       /* DDL and binlog write order are protected by metadata locks. */
>       res= mysql_rm_table(thd, first_table, lex->drop_if_exists,
>   			lex->drop_temporary);
> @@ -5248,14 +5241,8 @@ void THD::reset_for_next_command()
>       beginning of each SQL statement.
>     */
>     thd->server_status&= ~SERVER_STATUS_CLEAR_SET;
> -  /*
> -    If in autocommit mode and not in a transaction, reset
> -    OPTION_STATUS_NO_TRANS_UPDATE | OPTION_KEEP_LOG to not get warnings
> -    in ha_rollback_trans() about some tables couldn't be rolled back.
> -  */
>     if (!thd->in_multi_stmt_transaction_mode())
>     {
> -    thd->variables.option_bits&= ~OPTION_KEEP_LOG;
>       thd->transaction.all.modified_non_trans_table= FALSE;
>     }
>     DBUG_ASSERT(thd->security_ctx==&thd->main_security_ctx);
>
> === modified file 'sql/sql_priv.h'
> --- a/sql/sql_priv.h	2010-11-05 16:23:32 +0000
> +++ b/sql/sql_priv.h	2010-11-29 05:36:21 +0000
> @@ -101,7 +101,7 @@
>   #define OPTION_BEGIN            (1ULL<<  20)    // THD, intern
>   #define OPTION_TABLE_LOCK       (1ULL<<  21)    // THD, intern
>   #define OPTION_QUICK            (1ULL<<  22)    // SELECT (for DELETE)
> -#define OPTION_KEEP_LOG         (1ULL<<  23)    // THD, user
> +//#define OPTION_KEEP_LOG         (1ULL<<  23)    // THD, user
>
>   /* The following is used to detect a conflict with DISTINCT */
>   #define SELECT_ALL              (1ULL<<  24)    // SELECT, user, parser
>
> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc	2010-11-11 09:40:06 +0000
> +++ b/sql/sql_table.cc	2010-11-29 05:36:21 +0000
> @@ -2244,6 +2244,14 @@ int mysql_rm_table_part2(THD *thd, TABLE
>         goto err;
>       }
>
> +    /*
> +      DROP TEMPORARY TABLE doesn't terminate a transaction. Setting
> +      modified_non_trans_table TRUE guarantees the transaction can be binlogged
> +      scorrectly.
> +    */
> +    if (drop_temporary&&  error == 0)
> +      thd->transaction.stmt.modified_non_trans_table= TRUE;
> +
>       if ((drop_temporary&&  if_exists) || !error)
>       {
>         /*
>
> === modified file 'sql/sys_vars.cc'
> --- a/sql/sys_vars.cc	2010-11-16 09:38:43 +0000
> +++ b/sql/sys_vars.cc	2010-11-29 05:36:21 +0000
> @@ -2260,7 +2260,7 @@ static bool fix_autocommit(sys_var *self
>         transaction implicitly at the end (@sa stmt_causes_implicitcommit()).
>       */
>       thd->variables.option_bits&=
> -                 ~(OPTION_BEGIN | OPTION_KEEP_LOG | OPTION_NOT_AUTOCOMMIT);
> +                 ~(OPTION_BEGIN | OPTION_NOT_AUTOCOMMIT);
>       thd->transaction.all.modified_non_trans_table= false;
>       thd->server_status|= SERVER_STATUS_AUTOCOMMIT;
>       return false;
>
> === modified file 'sql/transaction.cc'
> --- a/sql/transaction.cc	2010-10-01 10:23:16 +0000
> +++ b/sql/transaction.cc	2010-11-29 05:36:21 +0000
> @@ -110,7 +110,7 @@ bool trans_begin(THD *thd, uint flags)
>       res= test(ha_commit_trans(thd, TRUE));
>     }
>
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>
>     if (res)
> @@ -159,7 +159,7 @@ bool trans_commit(THD *thd)
>       RUN_HOOK(transaction, after_rollback, (thd, FALSE));
>     else
>       RUN_HOOK(transaction, after_commit, (thd, FALSE));
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>     thd->lex->start_transaction_opt= 0;
>
> @@ -196,7 +196,7 @@ bool trans_commit_implicit(THD *thd)
>       res= test(ha_commit_trans(thd, TRUE));
>     }
>
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>
>     /*
> @@ -231,7 +231,7 @@ bool trans_rollback(THD *thd)
>     thd->server_status&= ~SERVER_STATUS_IN_TRANS;
>     res= ha_rollback_trans(thd, TRUE);
>     RUN_HOOK(transaction, after_rollback, (thd, FALSE));
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>     thd->lex->start_transaction_opt= 0;
>
> @@ -436,9 +436,7 @@ bool trans_rollback_to_savepoint(THD *th
>
>     if (ha_rollback_to_savepoint(thd, sv))
>       res= TRUE;
> -  else if (((thd->variables.option_bits&  OPTION_KEEP_LOG) ||
> -            thd->transaction.all.modified_non_trans_table)&&
> -           !thd->slave_thread)
> +  else if (thd->transaction.all.modified_non_trans_table&& 
> !thd->slave_thread)
>       push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
>                    ER_WARNING_NOT_COMPLETE_ROLLBACK,
>                    ER(ER_WARNING_NOT_COMPLETE_ROLLBACK));
> @@ -664,7 +662,7 @@ bool trans_xa_commit(THD *thd)
>       DBUG_RETURN(TRUE);
>     }
>
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>     thd->server_status&= ~SERVER_STATUS_IN_TRANS;
>     xid_cache_delete(&thd->transaction.xid_state);
> @@ -719,7 +717,7 @@ bool trans_xa_rollback(THD *thd)
>     if ((res= test(ha_rollback_trans(thd, TRUE))))
>       my_error(ER_XAER_RMERR, MYF(0));
>
> -  thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
> +  thd->variables.option_bits&= ~OPTION_BEGIN;
>     thd->transaction.all.modified_non_trans_table= FALSE;
>     thd->server_status&= ~SERVER_STATUS_IN_TRANS;
>     xid_cache_delete(&thd->transaction.xid_state);
>
>
>
>
>

Thread
bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798 Bug#56184Li-Bing.Song29 Nov
  • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#55798Bug#56184Alfranio Correia2 Dec