Re: bk commit into 5.1 tree (kostja:1.2653) BUG#12713

[Sergei Golubchik]

Hi!

On Dec 29, konstantin@stripped wrote:
> ChangeSet@stripped, 2007-12-29 22:18:31+03:00, kostja@bodhi.(none) +27 -0
>   A fix and a test case for Bug#12713 "Error in a stored function called from 
>   a SELECT doesn't cause ROLLBACK of statem"
>   The latest changeset for a code review. 

See my questions below.
Thanks for solving this very complicated problem!

> diff -Nrup a/mysql-test/include/commit.inc b/mysql-test/include/commit.inc
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/include/commit.inc	2007-12-29 22:18:25 +03:00
> @@ -0,0 +1,750 @@
>  .....
> +
> +insert into t2 (a) values (1001);
> +--error ER_DUP_ENTRY
> +insert into t1 (a) values (f2(1));
> +select * from t2;
> +rollback;
> +select * from t2;
> +commit;

why a commit ?

> +
>   .....
> +
> +--echo # We need at least one procedure to make sure the WHERE clause is
> +--echo # evaluated
> +create procedure dummy() begin end;

you probably need 'drop procedure if exists dummy' in the beginning

> +insert into t2 (a) values (1020);
> +--error ER_DUP_ENTRY
> +show procedure status where (f2(20) = 20);
> +select * from t2;
> +rollback;
> +select * from t2;
> +commit;
> +drop procedure dummy;
>  .....
> +--echo # 4. Read-write statement: UPDATE, update 0 rows, 1 row matches WHERE 
> +--echo #
> +update t1 set a=2;
> +call p_verify_status_increment(2, 2, 2, 2);
> +commit;
> +call p_verify_status_increment(2, 2, 2, 2);

why this one isn't optimized ?

> +
> +--echo # 5. Read-write statement: UPDATE, update 0 rows, 0 rows match WHERE 
> +--echo #
> +update t1 set a=3 where a=1;
> +call p_verify_status_increment(2, 0, 1, 0);
> +commit;
> +call p_verify_status_increment(2, 0, 1, 0);

why different results for mixed/row ?

>  .....
> +--echo # 8. Read-write statement: unqualified DELETE
> +--echo #
> +delete from t1;
> +call p_verify_status_increment(2, 2, 1, 0);
> +commit;
> +call p_verify_status_increment(2, 2, 1, 0);

oh, row/mixed are even optimized differently. why ?

> +--echo #
> +--echo # Replace the non-transactional table with a temporary
> +--echo # transactional table. Demonstrate that a change to a temporary
> +--echo # transactional table does not provoke 2-phase commit, although

good idea!

> +--echo # does trigger a commit and a binlog write (in statement mode).
> +--echo #
>  .....
> diff -Nrup a/mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result
> b/mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result
> --- a/mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result	2007-10-14
> 00:12:46 +04:00
> +++ b/mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result	2007-12-29
> 22:18:20 +03:00
> @@ -349,11 +348,10 @@ master-bin.000001	#	Query	#	#	use `test`
>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 values (8,8)
>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 values (9,9)
>  master-bin.000001	#	Query	#	#	use `test`; TRUNCATE table t2
> -master-bin.000001	#	Xid	#	#	COMMIT /* XID */
>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 values (10,10)
>  master-bin.000001	#	Query	#	#	use `test`; BEGIN
>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t2 values (100,100)
> -master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +master-bin.000001	#	Query	#	#	use `test`; COMMIT
>  master-bin.000001	#	Query	#	#	use `test`; DROP TABLE t1,t2

ok, I understand why "COMMIT /* XID */" can be changed to "use `test`; COMMIT".
but how it can simlpy dissapear ?

>  reset master;
>  create table t1 (a int) engine=innodb;
> diff -Nrup a/mysql-test/suite/rpl_ndb/t/disabled.def
> b/mysql-test/suite/rpl_ndb/t/disabled.def
> --- a/mysql-test/suite/rpl_ndb/t/disabled.def	2007-12-11 13:51:00 +03:00
> +++ b/mysql-test/suite/rpl_ndb/t/disabled.def	2007-12-29 22:18:20 +03:00
> @@ -21,3 +21,5 @@ rpl_ndb_mix_innodb       : Bug #32720   
>  # the below testcase have been reworked to avoid the bug, test contains comment,
> keep bug open
>  
>  #rpl_ndb_dd_advance	 : Bug#25913 rpl_ndb_dd_advance fails randomly
> +rpl_ndb_circular         : DBUG ASSERT fails
> +rpl_ndb_circular_simplex : DBUG ASSERT fails

what DBUG_ASSERT ?
are you going to do anything about it ?

> diff -Nrup a/sql/handler.cc b/sql/handler.cc
> --- a/sql/handler.cc	2007-12-20 21:16:51 +03:00
> +++ b/sql/handler.cc	2007-12-29 22:18:20 +03:00
> @@ -577,6 +577,218 @@ void ha_close_connection(THD* thd)
>  
>  /* ========================================================================
>   ======================= TRANSACTIONS ===================================*/
> +/**
> +  Transaction handling in the server
> +  ==================================
> +
> +  In each client connection, MySQL maintains two transactions:
> +  - a statement transaction,

I'd explain here what a "statement transaction" is, because the term is
non-standard, it came from BDB - it was the first transactional engine,
and it used nested transactions for every statement. InnoDB uses
savepoints, and Heikki objects to the term "statement commit", for
example.  And he's right, in a sense, as it's not a commit strictly
speaking, it doesn't make any persistent changes - it simply marks end
of statement.  So, I would define here what is "statement transaction",
and "statement commit" in convensional language, without using BDB-isms.

> +  - a standard, also called "global", transaction.

global transaction is most often used in the context of distributed
transaction processing, and it means a distributed transaction managed
by the transaction coordinator, as opposite to the local transaction in
every particular resource manager.

Best is to avoid saying "global transaction" when you simply
mean a normal, local to one MySQL instance, transaction.

> +
> +  The statement transaction is started for each statement that
> +  accesses transactional tables or uses the binary log.
> +  If the statement succeeds, the statement transaction
> +  is committed. If the statement fails, the transaction
> +  is rolled back. Commits of statement transactions are not
> +  durable -- each such transaction is nested in the global
> +  transaction, and if the global transaction is rolled back, the
> +  effects of all enclosed statement transactions are undone as well.
> +  Technically, a statement transaction can be viewed as a savepoint
> +  maintained automatically in order to make effects of one
> +  statement atomic.
> +
> +  The global transaction is started by the user and is ended
> +  usually upon a user request as well. The global transaction
> +  encloses transactions of all statements issued between
> +  its beginning and its end.
> +  In autocommit mode, the global transaction is equivalent
> +  to the statement transaction.
> +
> +  Since MySQL supports PSEA (pluggable storage engine
> +  architecture), more than one transactional engine can be
> +  active at a time. Hence transactions, from the server
> +  point of view, are always distributed. In particular,
> +  transactional state is maintained independently for each
> +  engine. In order to commit a transaction two phase
> +  commit protocol is employed.
> +
> +  Finally, not all statements are executed in context
> +  of a transaction. Administrative and status information
> +  statements do not modify engine data, and thus do not start
> +  a statement transaction and have no effect on the global
> +  transaction. Examples of such statements are SHOW STATUS
> +  and RESET SLAVE.
> +  DDL statements, historically, were not transactional, and
> +  therefore a transaction is [almost] never started for a DDL
> +  statement.
> +  At last, SQL statements that work with non-transactional
> +  engines also have no effect on the transaction state of the
> +  connection. Even though they are written to the binary log,
> +  and the binary log is, overall, transactional, the writes
> +  are done in "write-through" mode, directly to the binlog
> +  file, followed with a OS cache sync, in other words,
> +  bypassing the binlog undo log (translog).
> +
> +  Data layout
> +  -----------
> +
> +  The server stores its transaction-related data in
> +  thd->transaction. This structure has two members of type
> +  THD_TRANS. These members correspond to the statement and
> +  global transactions respectively:
> +
> +  - thd->transaction.stmt contains a list of engines
> +  that participate in the given statement
> +  - thd->transaction.all contains a list of engines that
> +  participated in any of the statement transactions started
> +  within the context of the global transaction.
> +  Each element of the list contains a pointer to the storage
> +  engine, engine-specific transactional data, and engine-specific
> +  transaction flags.
> +
> +  In autocommit mode thd->transaction.all is not
> +  maintained. Instead data of thd->transaction.stmt is

"not maintained" or "is empty" ?

> +  used to commit/rollback the global transaction.
> +
> +  The list of registered engines has a few important properties:
> +  - no engine is registered in the list twice
> +  - the list has a reverse temporal order -- engines are
> +  always added to the beginning of the list.
> +
> +  Transaction life cycle
> +  ----------------------
> +
> +  When a new connection is established, thd->transaction
> +  members are initialized to an empty state.
> +  If a statement uses any tables, all affected engines
> +  are registered in the statement engine list. In
> +  non-autocommit mode, the same engines are registered in
> +  the global transaction list.
> +  At the end of the statement, the server issues a commit
> +  or a roll back for all engines in the statement list.
> +  At this point transaction flags of an engine, if any, are
> +  propagated from the statement list to the global transaction
> +  list.
> +  When commit/rollback is finished, the statement list is
> +  cleared. It will be filled in again by the next statement,
> +  and emptied again at the next statement's end.
> +
> +  The global transaction is committed in a similar way
> +  (by going over all engines in thd->transaction.all list)
> +  but at different times:
> +  - if COMMIT SQL statement is issued by the user
> +  - implicitly, by the server, at the beginning of a DDL statement
> +  or SET AUTOCOMMIT={0|1} statement.
> +
> +  It can be rolled back as well:
> +  - if the user has requested so, by issuing ROLLBACK SQL
> +  statement
> +  - if one of the storage engines requested a rollback
> +  by setting thd->transaction_rollback_request. In this case

add "(for example in case of internal deadlock)" or something else you
may find appropriate to explain why we need
transaction_rollback_request and how it can ever happen.

> +  the rollback is accompanied by an error sent to the user.
> +
> +  When committing a statement or a global transaction, the server
> +  either uses the two-phase commit protocol, or commits a transaction
> +  in each engine independently. The two-phase commit protocol
> +  is used only if:
> +  - all participating engines support two-phase commit (provide
> +    handlerton::prepare PSEA API call) and
> +  - transactions in at least two engines modify data (i.e. are
> +  not read-only).
> +
> +  When a connection is closed, the current global transaction, if
> +  any, is rolled back.
> +
> +  Roles and responsibilities
> +  --------------------------
> +
> +  The server has no way to know that a transaction has been started
> +  in the engine unless the engine says so. Thus, in order to be
> +  a part of a transaction, the engine must "register" itself.
> +  This is done by invoking trans_register_ha() server call.
> +  Normally the engine registers itself whenever handler::store_lock()

external_lock, not store_lock

> +  is called. trans_register_ha() can be invoked many times: if
> +  an engine is already registered, the call does nothing.
> +  In case autocommit is not set, the engine must register itself
> +  twice -- both in the statement lits and in the global list.
> +  In which list to register is a parameter of trans_register_ha().
> +
> +  Note, that although the registration interface in itself is
> +  fairly clear, the current usage practice often leads to undesired
> +  effects: e.g. since a call to trans_register_ha in most engines
> +  is embedded into implementation of handler::store_lock(), some

external_lock here and below

> +  DDL statements start a transaction (at least from the server
> +  point of view) even though they are not expected to. E.g.
> +  CREATE TABLE does not start a transaction, since
> +  handler::store_lock() is never called during CREATE TABLE. But
> +  CREATE TABLE ... SELECT does, since handler::store_lock() is
> +  called for the table that is being selected from. This has no
> +  practical effects currently, but must be kept in mind
> +  nevertheless.
> +
> +  Once an engine is registered, the server will do the rest
> +  of the work.
> +
.....
> @@ -722,12 +1006,25 @@ 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)
> +    must_2pc= ha_check_and_coalesce_trx_read_only(thd, ha_info, all);

do you need a call to ha_check_and_coalesce_trx_read_only() here ?
can you check and coalesce in the for() loop below
that does ht->prepare() ?

> +
> +    if (!trans->no_2pc && must_2pc)
>      {
> -      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();
> +        /*
> +          Call two-phase commit even if this particular
> +          transaction is read-only, just in case. It should not
> +          produce any overhead, and normally should be a no-op,
> +          but gives the engine a chance to abort the distributed
> +          commit.

why it could be useful (I mean "chance to abort ...") ?

> +
> +          Sic: we know that prepare() is not NULL since otherwise
> +          trans->no_2pc would have been set.
> +        */
> +        if ((err= ht->prepare(ht, thd, all)))
>          {
>            my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
>            error= 1;
> @@ -805,8 +1104,17 @@ int ha_rollback_trans(THD *thd, bool all
>  {
>    int error=0;
>    THD_TRANS *trans=all ? &thd->transaction.all :
> &thd->transaction.stmt;
> -  bool is_real_trans=all || thd->transaction.all.nht == 0;
> +  Ha_trx_info *ha_info= trans->ha_list, *ha_info_next;
> +  bool is_real_trans=all || thd->transaction.all.ha_list == 0;
>    DBUG_ENTER("ha_rollback_trans");
> +
> +  /*
> +    We must not commit a global transaction if statement

s/commit/rollback/

> +    transaction is pending.
> +  */
> +  DBUG_ASSERT(thd->transaction.stmt.ha_list == NULL ||
> +              trans == &thd->transaction.stmt);
> +
>    if (thd->in_sub_stmt)
>    {
>      /*
> @@ -2500,6 +2824,33 @@ int handler::ha_check(THD *thd, HA_CHECK
>    return update_frm_version(table);
>  }
>  
> +/**
> +  A helper function to mark a transaction read-write,
> +  if it is started.
> +*/
> +
> +inline
> +void
> +handler::mark_trx_read_write()
> +{
> +  Ha_trx_info *ha_info= &ha_thd()->ha_data[ht->slot].ha_info[0];
> +  /*
> +    When a storage engine method is called, the transaction must
> +    have been started, unless it's a DDL call, for which the
> +    storage engine starts the transaction internally, and commits
> +    it internally, without registering in the ha_list.
> +    Unfortunately here we can't know know for sure if the engine
> +    has registered the transaction or not, so we must check.
> +  */
> +
> +  if (ha_info->is_started())
> +  {
> +    DBUG_ASSERT(has_transactions());
> +    if (table_share == NULL || table_share->tmp_table == NO_TMP_TABLE)

when can table_share be NULL ?

> +      ha_info->set_trx_read_write();
> +  }
> +}
> +
>  
>  /**
>    Repair table: public interface.
> diff -Nrup a/sql/handler.h b/sql/handler.h
> --- a/sql/handler.h	2007-12-20 21:16:51 +03:00
> +++ b/sql/handler.h	2007-12-29 22:18:20 +03:00
> @@ -758,7 +758,106 @@ 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 global- transaction- related
> +  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
> +  {stmt|all}.ha_list is made by trans_register_ha().
> +
> +  When it's time to commit or rollback, each element of ha_list
> +  is used to access storage engine's prepare()/commit()/rollback()
> +  methods, and also to evaluate if a full two phase commit is
> +  necessary.
> +
> +  @sa General description of transaction handling in handler.cc.
> +*/
> +
> +class Ha_trx_info
> +{
> +public:
> +  /** Register this storage engine in the given transaction context. */
> +  void register_ha(THD_TRANS *trans, handlerton *ht_arg)
> +  {
> +    DBUG_ASSERT(m_flags == 0);
> +    DBUG_ASSERT(m_ht == NULL);
> +    DBUG_ASSERT(m_next == NULL);
> +
> +    m_ht= ht_arg;
> +    m_flags= (int) TRX_READ_ONLY; /* Assume read-only at start. */
> +
> +    m_next= trans->ha_list;
> +    trans->ha_list= this;
> +  }
> +
> +  /** Clear, prepare for reuse. */
> +  void reset()
> +  {
> +    m_next= NULL;
> +    m_ht= NULL;
> +    m_flags= 0;
> +  }
> +
> +  Ha_trx_info() { reset(); }
> +
> +  void set_trx_read_write()
> +  {
> +    DBUG_ASSERT(is_started());
> +    m_flags|= (int) TRX_READ_WRITE;
> +  }
> +  bool is_trx_read_write() const
> +  {
> +    DBUG_ASSERT(is_started());
> +    return m_flags & (int) TRX_READ_WRITE;
> +  }
> +  bool is_started() const { return m_ht != NULL; }
> +  /** Mark this transaction read-write if the argument is read-write. */
> +  void coalesce_trx_with(const Ha_trx_info *stmt_trx)
> +  {
> +    /*
> +      Must be called only after the transaction has been started.
> +      Can be called twice, e.g. when we have two read-write

better (less confusing): twice -> many times, two -> many

> +      statements in a global transaction.
> +    */
> +    DBUG_ASSERT(is_started());
> +    if (stmt_trx->is_trx_read_write())
> +      set_trx_read_write();
> +  }
> +  Ha_trx_info *next() const
> +  {
> +    DBUG_ASSERT(is_started());
> +    return m_next;
> +  }
> +  handlerton *ht() const
> +  {
> +    DBUG_ASSERT(is_started());
> +    return m_ht;
> +  }
> +private:
> +  enum { TRX_READ_ONLY= 0, TRX_READ_WRITE= 1 };
> +  /** Auxiliary, used for ha_list management */
> +  Ha_trx_info *m_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 *m_ht;
> +  /**
> +    Transaction flags related to this engine.
> +    Not-null only if this instance is a part of transaction.
> +    May assume a combination of enum values above.
> +  */
> +  uchar       m_flags;
> +};
> +
>  
>  enum enum_tx_isolation { ISO_READ_UNCOMMITTED, ISO_READ_COMMITTED,
>  			 ISO_REPEATABLE_READ, ISO_SERIALIZABLE};
> @@ -1640,8 +1739,15 @@ protected:
>      provide useful functionality.
>    */
>    virtual int rename_table(const char *from, const char *to);
> +  /**
> +    Delete a table in the engine. Called for temporary tables, intermediate
> +    tables of CREATE TABLE ... SELECT.

do you mean it's not called for normal tables ?

> +  */
>    virtual int delete_table(const char *name);
>  private:
> +  /* Private helpers */
> +  inline void mark_trx_read_write();
> +private:

why private: twice ?

>    /*
>      Low-level primitives for storage engines.  These should be
>      overridden by the storage engine class. To call these methods, use
> diff -Nrup a/sql/log.cc b/sql/log.cc
> --- a/sql/log.cc	2007-12-13 23:58:53 +03:00
> +++ b/sql/log.cc	2007-12-29 22:18:21 +03:00
> @@ -3321,6 +3321,16 @@ THD::binlog_start_trans_and_stmt()
>      if (options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
>        trans_register_ha(this, TRUE, binlog_hton);
>      trans_register_ha(this, FALSE, binlog_hton);
> +    /*
> +      Mark statement transaction as read/write. We never start
> +      a binary log transaction and keep it read-only,
> +      therefore it's best to mark the transaction read/write just
> +      at the same time we start it.

why it's best ? shouldn't it be marked read-write automatically
on the first ha_write/delete/update_row ?

I agree that it will be marked read-write sooner or later, and
that forcing read-write here doesn't hurt. But why it's necessary ?

> +      Not necessary to mark the global transaction read/write
> +      since the statement-level flag will be propagated automatically
> +      inside ha_commit_trans.
> +    */
> +    ha_data[binlog_hton->slot].ha_info[0].set_trx_read_write();
>    }
>    DBUG_VOID_RETURN;
>  }
> diff -Nrup a/sql/rpl_injector.cc b/sql/rpl_injector.cc
> --- a/sql/rpl_injector.cc	2007-04-12 18:13:46 +04:00
> +++ b/sql/rpl_injector.cc	2007-12-29 22:18:23 +03:00
> @@ -61,7 +61,9 @@ injector::transaction::~transaction()
>  int injector::transaction::commit()
>  {
>     DBUG_ENTER("injector::transaction::commit()");
> +   printf("\n\n\n\n\nHERE WE GO\n\n\n\n");

:)

>     m_thd->binlog_flush_pending_rows_event(true);
> +   ha_autocommit_or_rollback(m_thd, 0);

ok, you've added it, I guess, to ensure that a statement
transaction is committed before main transaction.

but why did you need it ? every insert/update/delete ends
with ha_autocommit_or_rollback(), and a normal statements don't
need an extra ha_autocommit_or_rollback() right before end_trans().
do you mean that in rbr this invariant broken ?

>     end_trans(m_thd, COMMIT);
>     DBUG_RETURN(0);
>  }
> diff -Nrup a/sql/sp.cc b/sql/sp.cc
> --- a/sql/sp.cc	2007-10-30 20:08:11 +03:00
> +++ b/sql/sp.cc	2007-12-29 22:18:23 +03:00
> @@ -669,6 +669,13 @@ sp_returns_type(THD *thd, String &result
>                (TYPE_ENUM_PROCEDURE or TYPE_ENUM_FUNCTION).
>    @param sp   Stored routine object to store.
>  
> +  @note Opens and closes the thread tables. Therefore assumes
> +  that there are no locked tables in this thread on invokation.
> +  Unlike some other DDL statements, does close the tables
> +  in the end, since the caller normally issues the implicit
> +  grant to the definer of the created routine, which leads
> +  to open/modify/close of mysql.procs_priv table.
> +

Why does it need to close ? Shouldn't close_thread_tables() that is
called after mysql_parse() be enough ?

>    @return Error code. SP_OK is returned on success. Other SP_ constants are
>    used to indicate about errors.
>  */
> diff -Nrup a/sql/sql_acl.cc b/sql/sql_acl.cc
> --- a/sql/sql_acl.cc	2007-12-12 18:36:03 +03:00
> +++ b/sql/sql_acl.cc	2007-12-29 22:18:23 +03:00
> @@ -3862,7 +3862,7 @@ bool check_grant(THD *thd, ulong want_ac
>      of other queries). For simple queries first_not_own_table is 0.
>    */
>    for (i= 0, table= tables;
> -       table != first_not_own_table && i < number;
> +       table && table != first_not_own_table && i < number;

can that happen (table==0, I mean) ?
shouldn't "i < number" take care of that ?

>         table= table->next_global, i++)
>    {
>      /* Remove SHOW_VIEW_ACL, because it will be checked during making view */
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc	2007-12-20 21:16:52 +03:00
> +++ b/sql/sql_base.cc	2007-12-29 22:18:23 +03:00
> @@ -1324,29 +1324,45 @@ void close_thread_tables(THD *thd)
>      Mark all temporary tables used by this statement as free for reuse.
>    */
>    mark_temp_tables_as_free_for_reuse(thd);
> -
> -  if (thd->locked_tables || prelocked_mode)
> -  {
>      /*
>        Let us commit transaction for statement. Since in 5.0 we only have
>        one statement transaction and don't allow several nested statement
>        transactions this call will do nothing if we are inside of stored
>        function or trigger (i.e. statement transaction is already active and
>        does not belong to statement for which we do close_thread_tables()).
>        TODO: This should be fixed in later releases.
>      */
> -    ha_commit_stmt(thd);
> +  if (!(thd->state_flags & Open_tables_state::BACKUPS_AVAIL))
> +  {
> +    thd->main_da.can_overwrite_status= TRUE;
> +    ha_autocommit_or_rollback(thd, thd->is_error());
> +    thd->main_da.can_overwrite_status= FALSE;
>  
> +    /*
> +      Reset transaction state, but only if we're not inside a
> +      sub-statement of a prelocked statement.
> +    */

how can we get here if we are inside a sub-statement of a prelocked
statement ?

> +    if (! prelocked_mode || thd->lex->requires_prelocking())
> +      thd->transaction.stmt.reset();
> +  }
> +
> +  if (thd->locked_tables || prelocked_mode)
> +  {
> +
>      /* Ensure we are calling ha_reset() for all used tables */
>      mark_used_tables_as_free_for_reuse(thd, thd->open_tables);
>  
> -    /* We are under simple LOCK TABLES so should not do anything else. */
> +    /*
> +      We are under simple LOCK TABLES or we're inside a sub-statement
> +      of a prelocked statement, so should not do anything else.
> +    */
>      if (!prelocked_mode || !thd->lex->requires_prelocking())
>        DBUG_VOID_RETURN;
>  
>      /*
> -      We are in prelocked mode, so we have to leave it now with doing
> -      implicit UNLOCK TABLES if need.
> +      We are in the top-level statement of a prelocked statement,
> +      so we have to leave the prelocked mode now with doing implicit
> +      UNLOCK TABLES if needed.
>      */
>      DBUG_PRINT("info",("thd->prelocked_mode= NON_PRELOCKED"));
>      thd->prelocked_mode= NON_PRELOCKED;
> diff -Nrup a/sql/sql_delete.cc b/sql/sql_delete.cc
> --- a/sql/sql_delete.cc	2007-12-20 21:16:52 +03:00
> +++ b/sql/sql_delete.cc	2007-12-29 22:18:24 +03:00
> @@ -380,17 +388,6 @@ cleanup:
>    }
>    DBUG_ASSERT(transactional_table || !deleted ||
> thd->transaction.stmt.modified_non_trans_table);
>    free_underlaid_joins(thd, select_lex);
> -  if (transactional_table)
> -  {
> -    if (ha_autocommit_or_rollback(thd,error >= 0))
> -      error=1;
> -  }
> -
> -  if (thd->lock)
> -  {
> -    mysql_unlock_tables(thd, thd->lock);
> -    thd->lock=0;
> -  }

Hmm, if here (and below, and for insert/update/load) you don't call
ha_autocommit_or_rollback, then how can you ensure that
commit will succeed before sending an ok packet ?

>    if (error < 0 || (thd->lex->ignore && !thd->is_fatal_error))
>    {
>      thd->row_count_func= deleted;
 
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