Merge lp:~jaypipes/drizzle/explicit-transaction into lp:~drizzle-trunk/drizzle/development

[Jay Pipes]

[Jay Pipes (jaypipes)]

    Completes the work of removing the weirdness around transaction
    boundaries in the storage engine API.

    * Transactional storage engines are now all explicitly notified
      of the start of a new "normal" transaction in the new PSE API
      method plugin::TransactionalStorageEngine::doStartTransaction()
      This new method takes a start_transaction_option_t as one of its
      parameters, and passing this option allows the storage engine
      API to cleanly signal the start of a consistent snapshot (and in
      the future additional transaction attributes). This meant the
      removal of the old start_consistent_snapshot() method.

    * The TransactionServices component now fully manages the transaction
      boundaries, notification of transaction boundaries to participating
      resource managers (transactional storage engines)

    Adds a simple test case (to be expanded with future XA work) for
    transaction behaviour.

[Stewart Smith (stewart)]

explicit transaction start++

(i should look at the code too, but love explicit)

=== modified file 'drizzled/plugin/transactional_storage_engine.cc'
--- drizzled/plugin/transactional_storage_engine.cc	2010-02-14 19:27:57 +0000
+++ drizzled/plugin/transactional_storage_engine.cc	2010-02-25 04:43:15 +0000
@@ -2,6 +2,7 @@
  *  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
  *
  *  Copyright (C) 2008 Sun Microsystems
+ *  Copyright (c) 2009-2010 Jay Pipes <[e-mail]>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -92,12 +93,35 @@
   return 0;
 }
 
-int TransactionalStorageEngine::startConsistentSnapshot(Session *session)
-{
-  for_each(vector_of_transactional_engines.begin(), vector_of_transactional_engines.end(),
-           bind2nd(mem_fun(&TransactionalStorageEngine::doStartConsistentSnapshot),
-                   session));
-  return 0;
+struct StartTransactionFunc :public unary_function<TransactionalStorageEngine *, int>
+{
+  Session *session;
+  start_transaction_option_t options;
+  StartTransactionFunc(Session *in_session, start_transaction_option_t in_options) :
+    session(in_session),
+    options(in_options)
+  {}
+  result_type operator()(argument_type engine) const
+  {
+    return engine->startTransaction(session, options);
+  }
+};
+
+int TransactionalStorageEngine::notifyStartTransaction(Session *session, start_transaction_option_t options)
+{
+  if (vector_of_transactional_engines.empty())
+    return 0;
+  else
+  {
+    StartTransactionFunc functor(session, options);
+    vector<int> results;
+    results.reserve(vector_of_transactional_engines.size());
+    transform(vector_of_transactional_engines.begin(),
+              vector_of_transactional_engines.end(),
+              results.begin(),
+              functor);
+    return *max_element(results.begin(), results.end());
+  }
 }
 
 bool TransactionalStorageEngine::addPlugin(TransactionalStorageEngine *engine)

=== modified file 'drizzled/plugin/transactional_storage_engine.h'
--- drizzled/plugin/transactional_storage_engine.h	2010-02-25 04:43:14 +0000
+++ drizzled/plugin/transactional_storage_engine.h	2010-02-25 04:43:15 +0000
@@ -2,6 +2,7 @@
  *  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
  *
  *  Copyright (C) 2008 Sun Microsystems
+ *  Copyright (c) 2009-2010 Jay Pipes <[e-mail]>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -20,6 +21,7 @@
 #ifndef DRIZZLED_PLUGIN_TRANSACTIONAL_STORAGE_ENGINE_H
 #define DRIZZLED_PLUGIN_TRANSACTIONAL_STORAGE_ENGINE_H
 
+#include "drizzled/definitions.h" /* for start_transaction_option_t */
 #include "drizzled/plugin/storage_engine.h"
 #include "drizzled/transaction_services.h"
 
@@ -63,6 +65,13 @@
 
   virtual ~TransactionalStorageEngine();
 
+  int startTransaction(Session *session, start_transaction_option_t options)
+  {
+    TransactionServices &transaction_services= TransactionServices::singleton();
+    transaction_services.registerResourceForTransaction(session, this);
+    return doStartTransaction(session, options);
+  }
+
   void startStatement(Session *session)
   {
     TransactionServices &transaction_services= TransactionServices::singleton();
@@ -103,14 +112,12 @@
   /** 
    * The below static class methods wrap the interaction
    * of the vector of transactional storage engines.
-   *
-   * @todo kill these. they belong in TransactionServices.
    */
+  static int notifyStartTransaction(Session *session, start_transaction_option_t options);
   /**
    * @todo Kill this one entirely.  It's implementation, not interface...
    */
   static int releaseTemporaryLatches(Session *session);
-  static int startConsistentSnapshot(Session *session);
 
   /* Class Methods for operating on plugin */
   static bool addPlugin(plugin::TransactionalStorageEngine *engine);
@@ -121,6 +128,30 @@
 
   /*
    * Indicates to a storage engine the start of a
+   * new SQL transaction.  This is called ONLY in the following
+   * scenarios:
+   *
+   * 1) An explicit BEGIN WORK/START TRANSACTION is called
+   * 2) After an explicit COMMIT AND CHAIN is called
+   * 3) After an explicit ROLLBACK AND RELEASE is called
+   * 4) When in AUTOCOMMIT mode and directly before a new
+   *    SQL statement is started.
+   *
+   * Engines should typically use the doStartStatement()
+   * and doEndStatement() methods to manage transaction state,
+   * since the kernel ALWAYS notifies engines at the start
+   * and end of statement transactions and at the end of the
+   * normal transaction by calling doCommit() or doRollback().
+   */
+  virtual int doStartTransaction(Session *session, start_transaction_option_t options)
+  {
+    (void) session;
+    (void) options;
+    return 0;
+  }
+
+  /*
+   * Indicates to a storage engine the start of a
    * new SQL statement.
    */
   virtual void doStartStatement(Session *session)

=== modified file 'drizzled/session.cc'
--- drizzled/session.cc	2010-02-22 16:14:47 +0000
+++ drizzled/session.cc	2010-02-25 04:43:15 +0000
@@ -849,12 +849,9 @@
     options|= OPTION_BEGIN;
     server_status|= SERVER_STATUS_IN_TRANS;
 
-    if (opt == START_TRANS_OPT_WITH_CONS_SNAPSHOT)
+    if (plugin::TransactionalStorageEngine::notifyStartTransaction(this, opt))
     {
-      if (plugin::TransactionalStorageEngine::startConsistentSnapshot(this))
-      {
-        result= false;
-      }
+      result= false;
     }
   }
 

=== modified file 'drizzled/session.h'
--- drizzled/session.h	2010-02-22 16:14:47 +0000
+++ drizzled/session.h	2010-02-25 04:43:15 +0000
@@ -312,15 +312,23 @@
   */
   void *ha_ptr;
   /**
-    0: Life time: one statement within a transaction. If @@autocommit is
-    on, also represents the entire transaction.
-    @sa trans_register_ha()
-
-    1: Life time: one transaction within a connection.
-    If the storage engine does not participate in a transaction,
-    this should not be used.
-    @sa trans_register_ha()
-  */
+   * Resource contexts for both the "statement" and "normal"
+   * transactions.
+   *
+   * Resource context at index 0:
+   *
+   * Life time: one statement within a transaction. If @@autocommit is
+   * on, also represents the entire transaction.
+   *
+   * Resource context at index 1:
+   *
+   * Life time: one transaction within a connection. 
+   *
+   * @note
+   *
+   * If the storage engine does not participate in a transaction, 
+   * there will not be a resource context.
+   */
   drizzled::ResourceContext resource_context[2];
 
   Ha_data() :ha_ptr(NULL) {}

=== modified file 'drizzled/transaction_services.cc'
--- drizzled/transaction_services.cc	2010-02-25 04:43:14 +0000
+++ drizzled/transaction_services.cc	2010-02-25 04:43:15 +0000
@@ -2,6 +2,7 @@
  *  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
  *
  *  Copyright (C) 2008 Sun Microsystems
+ *  Copyright (c) Jay Pipes <[e-mail]>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -245,39 +246,80 @@
   session->transaction.all list is cleared.
 
   When a connection is closed, the current normal transaction, if
-  any, is rolled back.
+  any is currently active, is rolled back.
 
   Roles and responsibilities
   --------------------------
 
-  The server has no way to know that an engine participates in
-  the statement and a transaction has been started
-  in it 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 Cursor::external_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 list and in the normal transaction
-  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 Cursor::external_lock(), some
-  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
-  Cursor::external_lock() is never called during CREATE TABLE. But
-  CREATE TABLE ... SELECT does, since Cursor::external_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.
+  Beginning of SQL Statement (and Statement Transaction)
+  ------------------------------------------------------
+
+  At the start of each SQL statement, for each storage engine
+  <strong>that is involved in the SQL statement</strong>, the kernel 
+  calls the engine's plugin::StoragEngine::startStatement() method.  If the
+  engine needs to track some data for the statement, it should use
+  this method invocation to initialize this data.  This is the
+  beginning of what is called the "statement transaction".
+
+  <strong>For transaction storage engines (those storage engines
+  that inherit from plugin::TransactionalStorageEngine)</strong>, the
+  kernel automatically determines if the start of the SQL statement 
+  transaction should <em>also</em> begin the normal SQL transaction.
+  This occurs when the connection is in NOT in autocommit mode. If
+  the kernel detects this, then the kernel automatically starts the
+  normal transaction w/ plugin::TransactionalStorageEngine::startTransaction()
+  method and then calls plugin::StorageEngine::startStatement()
+  afterwards.
+
+  Beginning of an SQL "Normal" Transaction
+  ----------------------------------------
+
+  As noted above, a "normal SQL transaction" may be started when
+  an SQL statement is started in a connection and the connection is
+  NOT in AUTOCOMMIT mode.  This is automatically done by the kernel.
+
+  In addition, when a user executes a START TRANSACTION or
+  BEGIN WORK statement in a connection, the kernel explicitly
+  calls each transactional storage engine's startTransaction() method.
+
+  Ending of an SQL Statement (and Statement Transaction)
+  ------------------------------------------------------
+
+  At the end of each SQL statement, for each of the aforementioned
+  involved storage engines, the kernel calls the engine's
+  plugin::StorageEngine::endStatement() method.  If the engine
+  has initialized or modified some internal data about the
+  statement transaction, it should use this method to reset or destroy
+  this data appropriately.
+
+  Ending of an SQL "Normal" Transaction
+  -------------------------------------
+  
+  The end of a normal transaction is either a ROLLBACK or a COMMIT, 
+  depending on the success or failure of the statement transaction(s) 
+  it encloses.
+  
+  The end of a "normal transaction" occurs when any of the following
+  occurs:
+
+  1) If a statement transaction has completed and AUTOCOMMIT is ON,
+     then the normal transaction which encloses the statement
+     transaction ends
+  2) If a COMMIT or ROLLBACK statement occurs on the connection
+  3) Just before a DDL operation occurs, the kernel will implicitly
+     commit the active normal transaction
+  
+  Transactions and Non-transactional Storage Engines
+  --------------------------------------------------
+  
+  For non-transactional engines, this call can be safely ignored, and
+  the kernel tracks whether a non-transactional engine has changed
+  any data state, and warns the user appropriately if a transaction
+  (statement or normal) is rolled back after such non-transactional
+  data changes have been made.
+
+  XA Two-phase Commit Protocol
+  ----------------------------
 
   During statement execution, whenever any of data-modifying
   PSEA API methods is used, e.g. Cursor::write_row() or
@@ -305,47 +347,49 @@
   Additional notes on DDL and the normal transaction.
   ---------------------------------------------------
 
-  DDLs and operations with non-transactional engines
-  do not "register" in session->transaction lists, and thus do not
-  modify the transaction state. Besides, each DDL in
-  MySQL is prefixed with an implicit normal transaction commit
-  (a call to Session::endActiveTransaction()), and thus leaves nothing
-  to modify.
-  However, as it has been pointed out with CREATE TABLE .. SELECT,
-  some DDL statements can start a *new* transaction.
+  CREATE TABLE .. SELECT can start a *new* normal transaction
+  because of the fact that SELECTs on a transactional storage
+  engine participate in the normal SQL transaction (due to
+  isolation level issues and consistent read views).
 
   Behaviour of the server in this case is currently badly
   defined.
+
   DDL statements use a form of "semantic" logging
   to maintain atomicity: if CREATE TABLE .. SELECT failed,
   the newly created table is deleted.
+
   In addition, some DDL statements issue interim transaction
-  commits: e.g. ALTER Table issues a commit after data is copied
+  commits: e.g. ALTER TABLE issues a COMMIT after data is copied
   from the original table to the internal temporary table. Other
   statements, e.g. CREATE TABLE ... SELECT do not always commit
   after itself.
+
   And finally there is a group of DDL statements such as
-  RENAME/DROP Table that doesn't start a new transaction
+  RENAME/DROP TABLE that doesn't start a new transaction
   and doesn't commit.
 
-  This diversity makes it hard to say what will happen if
-  by chance a stored function is invoked during a DDL --
-  whether any modifications it makes will be committed or not
-  is not clear. Fortunately, SQL grammar of few DDLs allows
-  invocation of a stored function.
-
   A consistent behaviour is perhaps to always commit the normal
   transaction after all DDLs, just like the statement transaction
   is always committed at the end of all statements.
 */
-
 void TransactionServices::registerResourceForStatement(Session *session,
                                                        plugin::TransactionalStorageEngine *engine)
 {
+  if (session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
+  {
+    /* 
+     * Now we automatically register this resource manager for the
+     * normal transaction.  This is fine because a statement
+     * transaction registration should always enlist the resource
+     * in the normal transaction which contains the statement
+     * transaction.
+     */
+    registerResourceForTransaction(session, engine);
+  }
+
   TransactionContext *trans= &session->transaction.stmt;
-  ResourceContext *resource_context;
-
-  resource_context= session->getResourceContext(engine, 0);
+  ResourceContext *resource_context= session->getResourceContext(engine, 0);
 
   if (resource_context->isStarted())
     return; /* already registered, return */
@@ -356,38 +400,28 @@
   trans->no_2pc|= not engine->hasTwoPhaseCommit();
 }
 
-/**
-  Register a storage engine for a transaction.
-
-  Every storage engine MUST call this function when it starts
-  a transaction or a statement (that is it must be called both for the
-  "beginning of transaction" and "beginning of statement").
-  Only storage engines registered for the transaction/statement
-  will know when to commit/rollback it.
-
-  @note
-    trans_register_ha is idempotent - storage engine may register many
-    times per transaction.
-
-*/
-void TransactionServices::trans_register_ha(Session *session, plugin::TransactionalStorageEngine *engine)
+void TransactionServices::registerResourceForTransaction(Session *session,
+                                                         plugin::TransactionalStorageEngine *engine)
 {
   TransactionContext *trans= &session->transaction.all;
-  ResourceContext *resource_context;
+  ResourceContext *resource_context= session->getResourceContext(engine, 1);
+
+  if (resource_context->isStarted())
+    return; /* already registered, return */
 
   session->server_status|= SERVER_STATUS_IN_TRANS;
 
-  resource_context= session->getResourceContext(engine, 1);
-
-  if (resource_context->isStarted())
-    return; /* already registered, return */
-
   resource_context->setResource(engine);
   trans->registerResource(resource_context);
 
   trans->no_2pc|= not engine->hasTwoPhaseCommit();
+
   if (session->transaction.xid_state.xid.is_null())
     session->transaction.xid_state.xid.set(session->getQueryId());
+
+  /* Only true if user is executing a BEGIN WORK/START TRANSACTION */
+  if (! session->getResourceContext(engine, 0)->isStarted())
+    registerResourceForStatement(session, engine);
 }
 
 /**
@@ -555,6 +589,7 @@
     {
       int err;
       ResourceContext *resource_context= *it;
+
       plugin::TransactionalStorageEngine *engine= static_cast<plugin::TransactionalStorageEngine *>(resource_context->getResource());
       if ((err= engine->commit(session, normal_transaction)))
       {
@@ -564,7 +599,6 @@
       status_var_increment(session->status_var.ha_commit_count);
       resource_context->reset(); /* keep it conveniently zero-filled */
     }
-    trans->reset();
 
     if (is_real_trans)
       session->transaction.xid_state.xid.null();
@@ -575,6 +609,7 @@
       session->transaction.cleanup();
     }
   }
+  trans->reset();
   if (error == 0)
   {
     if (is_real_trans)
@@ -613,16 +648,16 @@
     {
       int err;
       ResourceContext *resource_context= *it;
+
       plugin::TransactionalStorageEngine *engine= static_cast<plugin::TransactionalStorageEngine *>(resource_context->getResource());
       if ((err= engine->rollback(session, normal_transaction)))
-      { // cannot happen
+      {
         my_error(ER_ERROR_DURING_ROLLBACK, MYF(0), err);
         error=1;
       }
       status_var_increment(session->status_var.ha_rollback_count);
       resource_context->reset(); /* keep it conveniently zero-filled */
     }
-    trans->reset();
     
     /* 
      * We need to signal the ROLLBACK to ReplicationServices here
@@ -646,14 +681,8 @@
     session->transaction_rollback_request= false;
 
   /*
-    If a non-transactional table was updated, warn; don't warn if this is a
-    slave thread (because when a slave thread executes a ROLLBACK, it has
-    been read from the binary log, so it's 100% sure and normal to produce
-    error ER_WARNING_NOT_COMPLETE_ROLLBACK. If we sent the warning to the
-    slave SQL thread, it would not stop the thread but just be printed in
-    the error log; but we don't want users to wonder why they have this
-    message in the error log, so we don't send it.
-  */
+   * If a non-transactional table was updated, warn the user
+   */
   if (is_real_trans &&
       session->transaction.all.hasModifiedNonTransData() &&
       session->killed != Session::KILL_CONNECTION)
@@ -662,6 +691,7 @@
                  ER_WARNING_NOT_COMPLETE_ROLLBACK,
                  ER(ER_WARNING_NOT_COMPLETE_ROLLBACK));
   }
+  trans->reset();
   return error;
 }
 

=== modified file 'drizzled/transaction_services.h'
--- drizzled/transaction_services.h	2010-02-25 04:43:14 +0000
+++ drizzled/transaction_services.h	2010-02-25 04:43:15 +0000
@@ -2,6 +2,7 @@
  *  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
  *
  *  Copyright (C) 2008 Sun Microsystems
+ *  Copyright (c) Jay Pipes <[e-mail]>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -91,8 +92,32 @@
   void registerResourceForStatement(Session *session,
                                     plugin::TransactionalStorageEngine *engine);
 
-  /* these are called by storage engines */
-  void trans_register_ha(Session *session, plugin::TransactionalStorageEngine *engine);
+  /**
+   * Registers a resource manager in the "normal" transaction.
+   *
+   * @note
+   *
+   * This method is idempotent and must be idempotent
+   * because it can be called both by the above 
+   * TransactionServices::registerResourceForStatement(),
+   * which occurs at the beginning of each SQL statement,
+   * and also manually when a BEGIN WORK/START TRANSACTION
+   * statement is executed. If the latter case (BEGIN WORK)
+   * is called, then subsequent contained statement transactions
+   * will call this method as well.
+   *
+   * @note
+   *
+   * This method checks to see if the supplied resource
+   * is also registered in the statement transaction, and
+   * if not, registers the resource in the statement
+   * transaction.  This happens ONLY when the user has
+   * called BEGIN WORK/START TRANSACTION, which is the only
+   * time when this method is called except from the
+   * TransactionServices::registerResourceForStatement method.
+   */
+  void registerResourceForTransaction(Session *session,
+                                      plugin::TransactionalStorageEngine *engine);
 };
 
 } /* namespace drizzled */

=== modified file 'plugin/innobase/handler/ha_innodb.cc'
--- plugin/innobase/handler/ha_innodb.cc	2010-02-25 04:43:14 +0000
+++ plugin/innobase/handler/ha_innodb.cc	2010-02-25 04:43:15 +0000
@@ -270,6 +270,7 @@
     addAlias("INNOBASE");
   }
 private:
+  virtual int doStartTransaction(Session *session, start_transaction_option_t options);
   virtual void doStartStatement(Session *session);
   virtual void doEndStatement(Session *session);
 public:
@@ -348,18 +349,6 @@
   			the database name: for example, in 'mysql/data/test'
   			the database name is 'test' */
 
-  /*********************************************************************
-  Creates an InnoDB transaction struct for the session if it does not yet have one.
-  Starts a new InnoDB transaction if a transaction is not yet started. And
-  assigns a new snapshot for a consistent read if the transaction does not yet
-  have one. */
-  virtual
-  int
-  doStartConsistentSnapshot(
-  /*====================================*/
-  			/* out: 0 */
-  	Session*	session);	/* in: MySQL thread handle of the user for whom
-  			the transaction should be committed */
   /********************************************************************
   Flushes InnoDB logs to disk and makes a checkpoint. Really, a commit flushes
   the logs, and the name of this function should be innobase_checkpoint. */
@@ -1506,27 +1495,6 @@
 	update_session(session);
 }
 
-/*********************************************************************//**
-Registers an InnoDB transaction in MySQL, so that the MySQL XA code knows
-to call the InnoDB prepare and commit, or rollback for the transaction. This
-MUST be called for every transaction for which the user may call commit or
-rollback. Calling this several times to register the same transaction is
-allowed, too.
-This function also registers the current SQL statement. */
-static inline
-void
-innobase_register_trx_and_stmt(
-/*===========================*/
-        plugin::TransactionalStorageEngine *engine, /*!< in: Innobase StorageEngine */
-	Session*	session)	/*!< in: MySQL thd (connection) object */
-{
-	if (session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) {
-		/* No autocommit mode, register for a transaction */
-    TransactionServices &transaction_services= TransactionServices::singleton();
-    transaction_services.trans_register_ha(session, engine);
-	}
-}
-
 /*****************************************************************//**
 Convert an SQL identifier to the MySQL system_charset_info (UTF-8)
 and quote it if needed.
@@ -2089,41 +2057,35 @@
 have one.
 @return	0 */
 int
-InnobaseEngine::doStartConsistentSnapshot(
+InnobaseEngine::doStartTransaction(
 /*====================================*/
-	Session*	session)	/*!< in: MySQL thread handle of the user for whom
-			the transaction should be committed */
+	Session*	session,	/*!< in: MySQL thread handle of the user for whom
+			                         the transaction should be committed */
+  start_transaction_option_t options)
 {
-	trx_t*	trx;
-
 	assert(this == innodb_engine_ptr);
 
 	/* Create a new trx struct for session, if it does not yet have one */
-
-	trx = check_trx_exists(session);
+	trx_t *trx = check_trx_exists(session);
 
 	/* This is just to play safe: release a possible FIFO ticket and
 	search latch. Since we will reserve the kernel mutex, we have to
 	release the search system latch first to obey the latching order. */
-
 	innobase_release_stat_resources(trx);
 
 	/* If the transaction is not started yet, start it */
-
 	trx_start_if_not_started(trx);
 
 	/* Assign a read view if the transaction does not have it yet */
-
-	trx_assign_read_view(trx);
-
-	/* Set the MySQL flag to mark that there is an active transaction */
-
+  if (options == START_TRANS_OPT_WITH_CONS_SNAPSHOT)
+	  trx_assign_read_view(trx);
+
+	/* Set the Drizzle flag to mark that there is an active transaction */
 	if (trx->active_trans == 0) {
-		innobase_register_trx_and_stmt(this, current_session);
-		trx->active_trans = 1;
+		trx->active_trans= 1;
 	}
 
-	return(0);
+	return 0;
 }
 
 /*****************************************************************//**
@@ -2153,9 +2115,10 @@
 
 	/* The flag trx->active_trans is set to 1 in
 
-	1. ::external_lock(),
-	4. InnobaseEngine::setSavepoint(),
-	6. InnobaseEngine::doStartConsistentSnapshot(),
+	1. ::external_lock()
+  2  InnobaseEngine::doStartStatement()
+	3. InnobaseEngine::setSavepoint()
+	4. InnobaseEngine::doStartTransaction()
 
 	and it is only set to 0 in a commit or a rollback. If it is 0 we know
 	there cannot be resources to be freed and we could return immediately.
@@ -8031,7 +7994,6 @@
    * @todo this should go away
    */
   if (trx->active_trans == 0) {
-    innobase_register_trx_and_stmt(innodb_engine_ptr, session);
     trx->active_trans= 1;
   }
 }
@@ -8053,6 +8015,7 @@
     if (trx->active_trans != 0)
     {
       commit(session, TRUE);
+      trx->active_trans= 0;
     }
   }
   else
@@ -8065,8 +8028,6 @@
       read_view_close_for_mysql(trx);
     }
   }
-
-  trx->active_trans= 0;
 }
 
 /*******************************************************************//**

=== added file 'tests/r/transaction.result'
--- tests/r/transaction.result	1970-01-01 00:00:00 +0000
+++ tests/r/transaction.result	2010-02-25 04:43:15 +0000
@@ -0,0 +1,43 @@
+DROP TABLE IF EXISTS t1_trx, t1_non_trx;
+SET AUTOCOMMIT= 0;
+CREATE TABLE t1_trx (
+k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=InnoDB;
+CREATE TEMPORARY TABLE t1_non_trx (
+k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=MyISAM;
+START TRANSACTION;
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+INSERT INTO t1_non_trx VALUES ('key1','value1');
+INSERT INTO t1_non_trx VALUES ('key2','value2');
+ROLLBACK;
+Warnings:
+Warning	1196	Some non-transactional changed tables couldn't be rolled back
+Expected warning about non-trx data changes not being rolled back
+SELECT * FROM t1_trx;
+k	v
+SELECT * FROM t1_non_trx;
+k	v
+key1	value1
+key2	value2
+START TRANSACTION;
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+k	v
+key1	value1
+key2	value2
+ROLLBACK;
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+k	v
+DROP TABLE t1_trx;
+DROP TABLE t1_non_trx;

=== added file 'tests/t/transaction.test'
--- tests/t/transaction.test	1970-01-01 00:00:00 +0000
+++ tests/t/transaction.test	2010-02-25 04:43:15 +0000
@@ -0,0 +1,56 @@
+# Tests a number of things related to transactions:
+# 
+# 1. Interaction of more than one engine in a transaction
+# 2. Correct commit and rollback behaviour
+# 3. XA protocol communication and recovery
+
+--disable_warnings
+DROP TABLE IF EXISTS t1_trx, t1_non_trx;
+--enable_warnings
+
+SET AUTOCOMMIT= 0;
+
+CREATE TABLE t1_trx (
+  k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=InnoDB;
+
+CREATE TEMPORARY TABLE t1_non_trx (
+  k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=MyISAM;
+
+START TRANSACTION;
+
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+
+INSERT INTO t1_non_trx VALUES ('key1','value1');
+INSERT INTO t1_non_trx VALUES ('key2','value2');
+
+ROLLBACK;
+
+--echo Expected warning about non-trx data changes not being rolled back
+
+SELECT * FROM t1_trx;
+SELECT * FROM t1_non_trx;
+
+START TRANSACTION;
+
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+
+ROLLBACK;
+
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+
+DROP TABLE t1_trx;
+DROP TABLE t1_non_trx;