Hi Dmitry,
I've worked on your requests and committed a new patch.
Please, can you take a look at it?
http://lists.mysql.com/commits/135103
This is just a diff highlighting your requests. If you
want to I can commit a full patch.
Cheers.
On 03/01/2011 04:11 PM, Dmitry Lenev wrote:
> 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.
>
