Hi!
On Nov 14, konstantin@stripped wrote:
> ChangeSet@stripped, 2007-11-14 17:46:40+03:00, kostja@bodhi.(none) +9 -0
> Bug#12713. This patch fixes the bug. No test cases, changeset comments,
> or review comments.
> An intermediate commit for the passing test suite.
I've only looked at handler.cc, handler.h, and sql_class.h changes.
They looked ok to me.
The rest was, apparently, not related to commit/rollback changes, but
consisted of fixes for your earlier changesets.
see a couple of suggestions below, mostly about comments - what they
could clarify better.
> diff -Nrup a/sql/handler.cc b/sql/handler.cc
> --- a/sql/handler.cc 2007-10-30 22:35:11 +03:00
> +++ b/sql/handler.cc 2007-11-14 17:46:37 +03:00
> @@ -608,12 +608,13 @@ void trans_register_ha(THD *thd, bool al
> else
> trans= &thd->transaction.stmt;
>
> - for (ht=trans->ht; *ht; ht++)
> - if (*ht == ht_arg)
> - DBUG_VOID_RETURN; /* already registered, return */
> + ha_info= thd->ha_data[ht_arg->slot].ha_info +
> static_cast<unsigned>(all);
do you get a warning without this explicit cast ?
> +
> + if (ha_info->ht)
> + DBUG_VOID_RETURN; /* already registered, return */
> +
> + ha_info->register_ha(trans, ht_arg);
>
> - trans->ht[trans->nht++]=ht_arg;
> - DBUG_ASSERT(*ht == ht_arg);
> trans->no_2pc|=(ht_arg->prepare==0);
> if (thd->transaction.xid_state.xid.is_null())
> thd->transaction.xid_state.xid.set(thd->query_id);
> @@ -669,9 +671,13 @@ int ha_prepare(THD *thd)
> int ha_commit_trans(THD *thd, bool all)
> {
> int error= 0, cookie= 0;
> + /*
> + 'all' means that this is either an explicit commit issued by
> + user, or an implicit commit issued by a DDL.
you'd better add a comment explaining is_real_trans (a commit of a
transaction that makes the changes persistent, when is_real_trans==0,
it means we're at the end of the statement within transaction, and there
will be a "real" commit or rollback later)
> + */
> THD_TRANS *trans= all ? &thd->transaction.all :
> &thd->transaction.stmt;
> - bool is_real_trans= all || thd->transaction.all.nht == 0;
> - handlerton **ht= trans->ht;
> + bool is_real_trans= all || thd->transaction.all.ha_list == 0;
> + Ha_trx_info *ha_info= trans->ha_list;
> my_xid xid= thd->transaction.xid_state.xid.get_my_xid();
> DBUG_ENTER("ha_commit_trans");
>
> @@ -722,12 +728,26 @@ int ha_commit_trans(THD *thd, bool all)
> if (is_real_trans) /* not a statement commit */
> thd->stmt_map.close_transient_cursors();
>
> - if (!trans->no_2pc && trans->nht > 1)
> + if (!trans->no_2pc && ha_info->next)
> {
> - for (; *ht && !error; ht++)
> + for (; ha_info && !error; ha_info= ha_info->next)
> {
> int err;
> - if ((err= (*(*ht)->prepare)(*ht, thd, all)))
> + handlerton *ht= ha_info->ht;
> + /**
> + Perform a two-phase commit only if:
perform a two-phase commit *in this storage engine* only if:
> + - we're doing a DDL or
> + - we're issuing a DML that has actually updated rows.
actually updated rows *in this storage engine*
> + In other cases, like read-only SELECT statements,
> + we would like to save on two-phase commits and therefore
> + avoid a potential disk sync of the binary log entailed by it.
> + */
> + if (!all &&
> + (sql_command_flags[thd->lex->sql_command] & CF_DML)
> &&
> + ha_info->is_trx_read_only())
> + continue;
> +
> + if ((err= ht->prepare(ht, thd, all)))
> {
> my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
> error= 1;
> diff -Nrup a/sql/handler.h b/sql/handler.h
> --- a/sql/handler.h 2007-09-07 17:41:46 +04:00
> +++ b/sql/handler.h 2007-11-14 17:46:37 +03:00
> @@ -758,7 +758,57 @@ typedef struct st_thd_trans
> saved value.
> */
> bool modified_non_trans_table;
> -} THD_TRANS;
> +
> + void reset() { no_2pc= FALSE; modified_non_trans_table= FALSE; }
> +};
> +
> +
> +/**
> + Either statement- or transaction- local thread-specific
> + storage engine data.
> +
> + If a storage engine participates in a statement/transaction,
> + an instance of this class is present in
> + thd->transaction.{stmt|all}.ha_list. The addition to ha_list
> + is done by trans_register_ha.
> +
> + When it's time to commit or rollback, each element of ha_list
> + is used to access storage engine prepare/commit/rollback
> + methods, and also to evaluate whether a full two phase commit
> + is necessary.
> +*/
> +
> +struct Ha_trx_info
> +{
> + /**
> + Register this storage engine in the given transaction context.
> + */
> + void register_ha(THD_TRANS *trans, handlerton *ht_arg)
> + { next= trans->ha_list; trans->ha_list= this; ht= ht_arg; flags= 0; }
please add a short comment that it's important to prepend to the list,
not to append. e.g. something like
/**
Register this storage engine in the given transaction context.
note that new ha's are added to the *beginning* of the list,
ha_rollback_to_savepoint() relies on it.
*/
> +
> + /** Clear, prepare for reuse. */
> + void reset() { next= NULL; ht= NULL; flags= 0; }
> +
> + Ha_trx_info() { reset(); }
> +
> + void set_trx_read_write() { flags|= (int) TRX_READ_WRITE; }
> + bool is_trx_read_only() const { return !(flags & (int) TRX_READ_WRITE); }
> +public:
> + /** public only to simplify access. Please use methods to assign. */
why not to use getters ?
> + /** Auxiliary, used for ha_list management */
> + Ha_trx_info *next;
> + /**
> + Although a given Ha_trx_info instance is currently always used
> + for the same storage engine, 'ht' is not-NULL only when the
> + corresponding storage is a part of a transaction.
> + */
> + handlerton *ht;
> +private:
> + enum { TRX_READ_ONLY= 0, TRX_READ_WRITE= 1 };
> + uchar flags;
> +};
> +
> +
>
> enum enum_tx_isolation { ISO_READ_UNCOMMITTED, ISO_READ_COMMITTED,
> ISO_REPEATABLE_READ, ISO_SERIALIZABLE};
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Dachauer Str. 37, D-80335 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
