List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:March 1 2011 4:11pm
Subject:Re: bzr commit into mysql-trunk branch (anders.song:3457)
Bug#55798 Bug#56184
View as plain text  
Hello, Li-Bing!

Here are my comments about your patch.

Please note that I was mostly looking at Runtime-related parts during
review. I assume that all replication-related changes were reviewed and
approved by someone from Replication team.

* anders.song@stripped <anders.song@stripped> [11/01/05
08:39]:
> #At file:///home/anders/Work/bzrwork/wt3/mysql-trunk/ based on
> revid:magne.mahre@stripped
> 
>  3457 anders.song@stripped	2011-01-05
>       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
>       
>       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. 

...

> === added file 'mysql-test/extra/rpl_tests/rpl_temp_error.test'
> --- a/mysql-test/extra/rpl_tests/rpl_temp_error.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_temp_error.test	2011-01-05 05:21:07 +0000
> @@ -0,0 +1,29 @@
> +# An auxaliary file for causing temporary error on slave SQL thread.
        ^^^^^^^^^
Typo:   auxiliary
...

> === modified file 'mysql-test/suite/rpl/r/rpl_stm_drop_create_temp_table.result'
> --- a/mysql-test/suite/rpl/r/rpl_stm_drop_create_temp_table.result	2010-12-19
> 17:22:30 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_stm_drop_create_temp_table.result	2011-01-05
> 05:21:07 +0000

...

> @@ -667,10 +661,6 @@ Note	1051	Unknown table 'test.tt_xx_1'
>  ROLLBACK;
>  -b-b-b-b-b-b-b-b-b-b-b- >> B T Drop-Temp-If-Xe-Temp R <<
> -b-b-b-b-b-b-b-b-b-b-b-
>  Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> -master-bin.000001	#	Query	#	#	BEGIN
> -master-bin.000001	#	Query	#	#	use `test`; INSERT INTO tt_xx_1() VALUES (1)
> -master-bin.000001	#	Query	#	#	use `test`; DROP TEMPORARY TABLE IF EXISTS `tt_xx_1`
> /* generated by server */
> -master-bin.000001	#	Query	#	#	ROLLBACK
>  -e-e-e-e-e-e-e-e-e-e-e- >> B T Drop-Temp-If-Xe-Temp R <<
> -e-e-e-e-e-e-e-e-e-e-e-
>  
>  SET @commands= 'B T Drop-Temp-If-Xe-Temp N Drop-Temp-If-Xe-Temp R';
> @@ -685,17 +675,12 @@ Warnings:
>  Note	1051	Unknown table 'test.tt_xx_1'
>  ROLLBACK;
>  Warnings:
> -Warning	1196	Some non-transactional changed tables couldn't be rolled back
> +Warning	#	Some non-transactional changed tables couldn't be rolled back
>  -b-b-b-b-b-b-b-b-b-b-b- >> B T Drop-Temp-If-Xe-Temp N Drop-Temp-If-Xe-Temp R
> << -b-b-b-b-b-b-b-b-b-b-b-
>  Log_name	Pos	Event_type	Server_id	End_log_pos	Info
>  master-bin.000001	#	Query	#	#	BEGIN
>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO nt_xx_1() VALUES (1)
>  master-bin.000001	#	Query	#	#	COMMIT
> -master-bin.000001	#	Query	#	#	BEGIN
> -master-bin.000001	#	Query	#	#	use `test`; INSERT INTO tt_xx_1() VALUES (1)
> -master-bin.000001	#	Query	#	#	use `test`; DROP TEMPORARY TABLE IF EXISTS `tt_xx_1`
> /* generated by server */
> -master-bin.000001	#	Query	#	#	use `test`; DROP TEMPORARY TABLE IF EXISTS `tt_xx_1`
> /* generated by server */
> -master-bin.000001	#	Query	#	#	ROLLBACK
>  -e-e-e-e-e-e-e-e-e-e-e- >> B T Drop-Temp-If-Xe-Temp N Drop-Temp-If-Xe-Temp R
> << -e-e-e-e-e-e-e-e-e-e-e-
>  
>  SET @commands= 'B T Drop-Temp-TXe-Temp R';

Hmmm... So after your patch successful DROP TEMPORARY TABLE IF EXISTS
that didn't drop table will no longer stay in binary log in case of ROLLBACK.
OTOH it will present in binary log in case of COMMIT.
Isn't such behavior inconsistent?

I don't think this change will have any practical consequences and,
ultimately, it is you, replication guys, who decide what is the correct/
acceptable behavior in such cases...

...

> === added file 'mysql-test/suite/rpl/t/rpl_DML_error.test'
> --- a/mysql-test/suite/rpl/t/rpl_DML_error.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_DML_error.test	2011-01-05 05:21:07 +0000
> @@ -0,0 +1,75 @@
> +source include/master-slave.inc;
> +source include/have_innodb.inc;
> +
> +--echo # Verify the statments can be binlogged correctly when error happens
> +--echo # ------------------------------------------------------------------
                       ^^^^^^^^^^
Typo:                  statements
...

> === modified file 'mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test'
> --- a/mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test	2010-12-19 17:22:30
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_begin_commit_rollback.test	2011-01-05 05:21:07
> +0000
> @@ -171,6 +171,160 @@ 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#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 master ]
> +connection master;
> +USE db1;
> +DROP TABLE t1, t2;
> +CREATE TABLE t1(c1 INT KEY, c2 CHAR(100)) ENGINE=InnoDB;
> +CREATE TABLE t2(c1 INT) ENGINE=MyISAM;
> +CREATE TABLE t3(c1 INT) ENGINE=InnoDB;
> +INSERT INTO t1 VALUES(1, "master");
> +
> +SET @@session.binlog_direct_non_transactional_updates= FALSE;
> +
> +sync_slave_with_master;
> +
> +--echo # [ on slave ]
> +USE db1;
> +
> +let $timeout_old= `SELECT @@GLOBAL.innodb_lock_wait_timeout`;

I think it is better to use user variable for saving/restoring value
of @@global.innodb_lock_wait_timeout instead of mysqltest variable
(e.g.:
"SET @timeout_old= @@global.innodb_lock_wait_timeout;
 ... 
 SET @@global.innodb_lock_wait_timeout= @timeout_old;"
).
This way we won't need to update .result file if default value of
innodb_lock_wait_timeout changes.
...

> +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;
> +
> +--echo # Verify the SQL thread don't retry the transaction when different
> +--echo # sort of statements has modified a non-transactional table.
> +--echo # ----------------------------------------------------------------

IMO it is better to change the above comment to something like:

--echo # Verify the SQL thread doesn't retry the transaction when one of
--echo # its statements has modified a non-transactional table.
--echo # ----------------------------------------------------------------
...

> +--echo
> +--echo # Verify the SQL thread don't retry the transaction if one of the
                                  ^^^^^^^
Typo:                             doesn't

> +--echo # sub-statement has modified a non-transactional table.
            ^^^^^^^^^^^^^^
Typo:       sub-statements
...

> === modified file 'sql/binlog.cc'
> --- a/sql/binlog.cc	2010-12-17 02:01:32 +0000
> +++ b/sql/binlog.cc	2011-01-05 05:21:07 +0000
> @@ -775,55 +806,23 @@ 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 if (!error)
> -  {  
> -    /*
> -      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)))
> -      error= binlog_rollback_flush_trx_cache(thd, cache_mngr);
>  
> -    /*
> -      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)))
> -      error= binlog_truncate_trx_cache(thd, cache_mngr, all);
> +  if (cache_mngr->trx_cache_cannot_rollback())
> +  {
> +    if (ending_trans(thd, all))
> +    {
> +      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);
> +    }
>    }
> -  /* 
> -    This is part of the stmt rollback.
> -  */
> -  if (!all)
> -    cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
> +  else
> +    error= binlog_truncate_trx_cache(thd, cache_mngr, all);
>  
>    DBUG_RETURN(error);
>  }

To my shame I wasn't able to understand fully the above changes. So
please ensure that they are properly reviewed by one of replication
experts. Please also consider adding/keeping comments explaining
logic of the above code.

...

> @@ -3287,9 +3272,7 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>  {
>    THD *thd= event_info->thd;
>    bool error= 1;
> -  bool is_trans_cache= FALSE;
>    DBUG_ENTER("MYSQL_BIN_LOG::write(Log_event *)");
> -  binlog_cache_data *cache_data= 0;
>  
>    if (thd->binlog_evt_union.do_union)
>    {
> @@ -3336,6 +3319,9 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>  #endif /* HAVE_REPLICATION */
>  
>      IO_CACHE *file= NULL;
> +    binlog_cache_mngr *cache_mngr= NULL;
> +    binlog_cache_data *cache_data= 0;
> +    bool is_trans_cache= FALSE;
>  
>      if (event_info->use_direct_logging())
>      {
> @@ -3347,16 +3333,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);
> -
> +      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()));
> @@ -3439,6 +3420,8 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>  
>      error= 0;
>  
> +    if (is_trans_cache && stmt_cannot_safely_rollback(thd))
> +      cache_mngr->set_trx_cache_cannot_rollback();

The above if-statement makes an assumption that in cases when 
is_trans_cache == TRUE the cache_mngr is also non-NULL.
This is a correct assumption at this point, but maybe it makes
sense to move this if-statement to the block which sets 
is_trans_cache and cache_mngr instead?
IMO this way code is going to be more future-proof/easier to support.

Or do I miss something?

>  err:
>      if (event_info->use_direct_logging())
>      {

...

> === modified file 'sql/ha_ndbcluster.cc'
> --- a/sql/ha_ndbcluster.cc	2010-11-18 16:34:56 +0000
> +++ b/sql/ha_ndbcluster.cc	2011-01-05 05:21:07 +0000
> @@ -4609,8 +4609,8 @@ void ha_ndbcluster::transaction_checks(T
>    {
>      m_transaction_on= FALSE;
>      /* Would be simpler if has_transactions() didn't always say "yes" */
> -    thd->transaction.all.modified_non_trans_table=
> -      thd->transaction.stmt.modified_non_trans_table= TRUE;
> +    thd->transaction.all.modified_non_trans_table();
> +    thd->transaction.stmt.modified_non_trans_table();
>    }
>    else if (!thd->transaction.on)
>      m_transaction_on= FALSE;
>

Taking into account that the above code works only in cases when
trans_commit_stmt() is going to be called before transaction commit/
rollback does it makes sense to do:

thd->transaction.all.modified_non_trans_table();

?

Maybe instead we should rely on trans_commit/rollback_stmt doing its
job and propagating flags from statement to "normal" transaction?

...

> === modified file 'sql/handler.h'
> --- a/sql/handler.h	2010-11-18 16:34:56 +0000
> +++ b/sql/handler.h	2011-01-05 05:21:07 +0000
> @@ -830,36 +830,93 @@ struct THD_TRANS
>    /* storage engines that registered in this transaction */
>    Ha_trx_info *ha_list;
>    /* 
> -    The purpose of this flag is to keep track of non-transactional
> -    tables that were modified in scope of:
> -    - transaction, when the variable is a member of
> -    THD::transaction.all
> +    The purpose of this flag is to keep track of statements which cannot be
> +    rolled back safely(completely). For example, statements that modified
> +    non-transactional tables, 'DROP TEMPORARY TABLE' and
> +    'CREATE TEMPORARY TABLE' statements. The tracked statements are
> +    modified in scope of:
> +    - transaction, when the variable is a member of THD::transaction.all
>      - top-level statement or sub-statement, when the variable is a
>      member of THD::transaction.stmt
>      This member has the following life cycle:
> -    * stmt.modified_non_trans_table is used to keep track of
> -    modified non-transactional tables of top-level statements. At
> -    the end of the previous statement and at the beginning of the session,
> -    it is reset to FALSE.  If such functions
> -    as mysql_insert, mysql_update, mysql_delete etc modify a
> -    non-transactional table, they set this flag to TRUE.  At the
> -    end of the statement, the value of stmt.modified_non_trans_table 
> -    is merged with all.modified_non_trans_table and gets reset.
> -    * all.modified_non_trans_table is reset at the end of transaction
> -    
> -    * Since we do not have a dedicated context for execution of a
> -    sub-statement, to keep track of non-transactional changes in a
> -    sub-statement, we re-use stmt.modified_non_trans_table. 
> -    At entrance into a sub-statement, a copy of the value of
> -    stmt.modified_non_trans_table (containing the changes of the
> -    outer statement) is saved on stack. Then 
> -    stmt.modified_non_trans_table is reset to FALSE and the
> -    substatement is executed. Then the new value is merged with the
> -    saved value.
> +    * stmt.unsafe_rollback_flags is used to keep track of top-level statements
> +    which cannot be rolled back safely. At the end of the previous statement
> +    and at the beginning of the session, it is reset to 0.  If such functions
> +    as mysql_insert, mysql_update, mysql_delete etc modify a non-transactional
> +    table, flag MODIFIED_NON_TRANS_TABLE is set.  After 'CREATE TEMPORARY TABLE'
> +    creates a table successfully, flag CREATED_TEMP_TABLE is set. After
> +    'DROP TEMPORARY TABLE' drops a temporary table, flag DROPPED_TEMP_TABLE is
> +    set.  At the end of the statement, the value of stmt.unsafe_rollback_flags
> +    is merged with all.unsafe_rollback_flags and gets reset.
> +    * all.cannot_safely_rollback is reset at the end of transaction
> +
> +    * Since we do not have a dedicated context for execution of a sub-statement,
> +    to keep track of non-transactional changes in a sub-statement, we re-use
> +    stmt.unsafe_rollback_flags.  At entrance into a sub-statement, a copy of the
> +    value of stmt.unsafe_rollback_flags (containing the changes of the outer
> +    statement) is saved on stack.  Then stmt.unsafe_rollback_flags is reset
> +    to FALSE and the substatement is executed. Then the new value is merged with
> +    the saved value.
> +  */
> +private:

I think it makes sense to make struct THD_TRANS a class to
emphasize that it no longer contains only public members
(and, yes, this is purely syntax sugar).

> +  unsigned int unsafe_rollback_flags;

I suggest to rename this member to m_unsafe_rollback_flags
to reflect the fact that it is private.

> +  /*
> +    Define the type of statemens which cannot be rolled back safely.
> +    Each type occupies one bit in unsafe_rollback_flags.
>    */
> -  bool modified_non_trans_table;
> +  enum
> +  {
> +    MODIFIED_NON_TRANS_TABLE= 0x01,
> +    CREATED_TEMP_TABLE= 0x02,
> +    DROPPED_TEMP_TABLE= 0x04
> +  };
> +public:
> +  bool cannot_safely_rollback() const
> +  {
> +    return unsafe_rollback_flags > 0;
> +  }
> +  unsigned int get_unsafe_rollback_flags() const
> +  {
> +    return unsafe_rollback_flags;
> +  }
> +  void set_unsafe_rollback_flags(unsigned int flags)
> +  {
> +    unsafe_rollback_flags= flags;
> +  }
> +  void add_unsafe_rollback_flags(unsigned int flags)
> +  {
> +    unsafe_rollback_flags|= flags;
> +  }
> +  void reset_unsafe_rollback_flags()
> +  {
> +    unsafe_rollback_flags= 0;
> +  }
> +  void modified_non_trans_table()
> +  {
> +    unsafe_rollback_flags|= MODIFIED_NON_TRANS_TABLE;
> +  }

In my opinion it makes sense to rename the above method
in such way that it begins with a verb. For example, to
mark_modified_non_trans_table().

> +  bool has_modified_non_trans_table() const
> +  {
> +    return unsafe_rollback_flags & MODIFIED_NON_TRANS_TABLE;
> +  }
> +  void created_temp_table()
> +  {
> +    unsafe_rollback_flags|= CREATED_TEMP_TABLE;
> +  }

Same comment here. One of possible names is - mark_created_temp_table().

> +  bool has_created_temp_table() const
> +  {
> +    return unsafe_rollback_flags & CREATED_TEMP_TABLE;
> +  }
> +  void dropped_temp_table()
> +  {
> +    unsafe_rollback_flags|= DROPPED_TEMP_TABLE;
> +  }

Same comment here. One of possible names is - mark_dropped_temp_table().

> +  bool has_dropped_temp_table() const
> +  {
> +    return unsafe_rollback_flags & DROPPED_TEMP_TABLE;
> +  }

OTOH, maybe it makes sense not to introduce
mark/has_modified_non_trans_table(), mark/has_created_temp_table(),
mark/has_dropped_temp_table() helper methods and try to get by
making enum public and using add_unsafe_rollback_flags() and
has_unsafe_rollback_flags() instead? What do you think?

>  
> -  void reset() { no_2pc= FALSE; modified_non_trans_table= FALSE; }
> +  void reset() { no_2pc= FALSE; reset_unsafe_rollback_flags(); }
>    bool is_empty() const { return ha_list == NULL; }
>  };

My biggest concern about this patch that it extends THD_TRANS class which
is defined in handler.h header. This file is supposed to describe SE API
and is included by handler implementation in various storage engines and
the information/methods that you add to this class should not be part of
SQL-layer/SE API.

OTOH if we look a bit closer we will see that THD_TRANS is not really
necessary in handler.h and can be moved to sql_class.h after moving
definition of Ha_trx_info::register_ha() to handler.cc (it can still
be inline).

IMO we should do such a step as part of your refactoring (since it
does significant change in this area anyway) in order to reduce
dependencies in server and to make it more clear what is or is not
part of SQL-layer/SE interface.

...

> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc	2010-12-29 05:35:31 +0000
> +++ b/sql/log_event.cc	2011-01-05 05:21:07 +0000
> @@ -8161,8 +8161,10 @@ int Rows_log_event::do_apply_event(Relay
>        m_curr_row= m_curr_row_end;
>   
>        if (error == 0 && !transactional_table)
> -        thd->transaction.all.modified_non_trans_table=
> -          thd->transaction.stmt.modified_non_trans_table= TRUE;
> +      {
> +        thd->transaction.all.modified_non_trans_table();
> +        thd->transaction.stmt.modified_non_trans_table();
> +      }
>  
>        if (m_curr_row == m_rows_end)
>          break;

Hmm... Is there any reason why we can't rely on trans_commit_stmt()
to propagate unsafe_rollback_flags from statement level to transaction
level? If no, then probably it makes sense to remove
thd->transaction.all.modified_non_trans_table(). If yes then it
would be nice to have a comment explaining these reasons.

> === modified file 'sql/share/errmsg-utf8.txt'
> --- a/sql/share/errmsg-utf8.txt	2010-12-05 22:51:49 +0000
> +++ b/sql/share/errmsg-utf8.txt	2011-01-05 05:21:07 +0000
> @@ -6439,6 +6439,7 @@ ER_SLAVE_HEARTBEAT_VALUE_OUT_OF_RANGE_MA
>  ER_PARTITION_EXCHANGE_FOREIGN_KEY
>    eng "Table to exchange with partition has foreign key references: '%-.64s'"
>    swe "Tabellen att byta ut mot partition har foreign key referenser: '%-.64s'"
> +
>  ER_NO_SUCH_KEY_VALUE
>    eng "Key value '%-.192s' was not found in table '%-.192s.%-.192s'"
>  ER_RPL_INFO_DATA_TOO_LONG

Please revert the above unnecessary change, which adds an extra line.
It might complicate merges.

> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc	2010-12-17 11:28:59 +0000
> +++ b/sql/sp_head.cc	2011-01-05 05:21:07 +0000
> @@ -376,8 +376,8 @@ sp_eval_expr(THD *thd, Field *result_fie
>    Item *expr_item;
>    enum_check_fields save_count_cuted_fields= thd->count_cuted_fields;
>    bool save_abort_on_warning= thd->abort_on_warning;
> -  bool save_stmt_modified_non_trans_table= 
> -    thd->transaction.stmt.modified_non_trans_table;
> +  unsigned int stmt_unsafe_rollback_flags=
> +    thd->transaction.stmt.get_unsafe_rollback_flags();
>  
>    DBUG_ENTER("sp_eval_expr");

How about adding short comment explaining what we are trying
to achieve here? Particularly I am interested in why here we don't
merge unsafe_rollback_flags accumulated during item evaluation in
the statement's flags like we do in another place in this file.

...

> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2010-12-29 00:38:59 +0000
> +++ b/sql/sql_class.h	2011-01-05 05:21:07 +0000
> @@ -1770,6 +1770,32 @@ public:
>        xid_state.xid.null();
>        init_sql_alloc(&mem_root, ALLOC_ROOT_MIN_BLOCK_SIZE, 0);
>      }
> +    void push_unsafe_rollback_warnings(THD *thd)
> +    {
> +      if (all.has_modified_non_trans_table())
> +        push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                     ER_WARNING_NOT_COMPLETE_ROLLBACK,
> +                     ER(ER_WARNING_NOT_COMPLETE_ROLLBACK));
> +
> +      if (all.has_created_temp_table())
> +        push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                     ER_WARNING_NOT_COMPLETE_ROLLBACK_WITH_CREATED_TEMP_TABLE,
> +                     ER(ER_WARNING_NOT_COMPLETE_ROLLBACK_WITH_CREATED_TEMP_TABLE));
> +
> +      if (all.has_dropped_temp_table())
> +        push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                     ER_WARNING_NOT_COMPLETE_ROLLBACK_WITH_DROPPED_TEMP_TABLE,
> +                     ER(ER_WARNING_NOT_COMPLETE_ROLLBACK_WITH_DROPPED_TEMP_TABLE));
> +    }

Could you please elaborate why do you think this method should
be part of THD::st_transactions class and not, for example,
part of THD itself?

> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2010-12-22 13:23:59 +0000
> +++ b/sql/sql_insert.cc	2011-01-05 05:21:07 +0000
> @@ -3621,11 +3615,8 @@ void select_insert::abort_result_set() {
>      */
>      changed= (info.copied || info.deleted || info.updated);
>      transactional_table= table->file->has_transactions();
> -    if (thd->transaction.stmt.modified_non_trans_table)
> +    if (thd->transaction.stmt.cannot_safely_rollback())
>      {
> -        if (!can_rollback_data())
> -          thd->transaction.all.modified_non_trans_table= TRUE;
> -
>          if (mysql_bin_log.is_open())
>          {
>            int errcode= query_error_code(thd, thd->killed == THD::NOT_KILLED);

After your changes select_insert/create::can_rollback_data() becomes unused.
Please remove them.

...

> @@ -4058,6 +4049,9 @@ void select_create::send_error(uint errc
>  
>  bool select_create::send_eof()
>  {
> +  if (create_info->options & HA_LEX_CREATE_TMP_TABLE)
> +    thd->transaction.stmt.created_temp_table();
> +
>    bool tmp=select_insert::send_eof();
>    if (tmp)
>      abort_result_set();

Should not we call thd->transaction.stmt.created_temp_table() only
if select_insert::send_eof() has succeeded? Or do I miss something?

...

> === modified file 'sql/sql_load.cc'
> --- a/sql/sql_load.cc	2010-12-10 16:55:50 +0000
> +++ b/sql/sql_load.cc	2011-01-05 05:21:07 +0000
> @@ -1263,7 +1261,8 @@ read_xml_field(THD *thd, COPY_INFO &info
>        We don't need to reset auto-increment field since we are restoring
>        its default value at the beginning of each loop iteration.
>      */
> -    thd->transaction.stmt.modified_non_trans_table= no_trans_update_stmt;
> +    if (no_trans_update_stmt)
> +      thd->transaction.stmt.modified_non_trans_table();
>      thd->warning_info->inc_current_row_for_warning();
>      continue_loop:;
>    }

Is the above if-statement necessary? AFAIU code in write_record()
should correctly update THD::transaction::stmt object, so it can
be removed. Do I miss something?

...

> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-12-29 00:38:59 +0000
> +++ b/sql/sql_parse.cc	2011-01-05 05:21:07 +0000
> @@ -4388,7 +4376,6 @@ finish:
>    DBUG_ASSERT(!thd->in_active_multi_stmt_transaction() ||
>                 thd->in_multi_stmt_transaction_mode());
>  
> -
>    if (! thd->in_sub_stmt)
>    {
>      /* report error issued during command execution */

Please avoid unnecessary formatting changes. They complicate merges.

...

> @@ -5267,15 +5254,9 @@ 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;
> +    thd->transaction.all.reset_unsafe_rollback_flags();
>    }

Please consider keeping the above comment (after rephrasing).
IMO it clarifies things a bit.

...

>    DBUG_ASSERT(thd->security_ctx== &thd->main_security_ctx);
>    thd->thread_specific_used= FALSE;
> 
> === modified file 'sql/sql_priv.h'
> --- a/sql/sql_priv.h	2011-01-03 14:50:58 +0000
> +++ b/sql/sql_priv.h	2011-01-05 05:21:07 +0000
> @@ -101,7 +101,6 @@
>  #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
>  

I suggest to add a short comment explaining that this bit is unused.
Something like:

/* 23rd bit is unused. It was occupied by OPTION_KEEP_LOG. */

...


Otherwise I am OK with your patch and think that it can be pushed
after considering/addressing/discussing the above issues and getting
review 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 (anders.song:3457) Bug#55798 Bug#56184anders.song5 Jan
  • Re: bzr commit into mysql-trunk branch (anders.song:3457)Bug#55798 Bug#56184Dmitry Lenev1 Mar
    • Re: bzr commit into mysql-trunk branch (anders.song:3457)Bug#55798 Bug#56184anders4 Mar
    • Re: bzr commit into mysql-trunk branch (anders.song:3457) Bug#55798Bug#56184Alfranio Correia8 Apr