bzr commit into mysql-5.1-bugteam branch (davi:2935) Bug#44672

[Davi Arnaut]

# At a local mysql-5.1-bugteam repository of davi

 2935 Davi Arnaut	2009-06-05
      Bug#44672: Assertion failed: thd->transaction.xid_state.xid.is_null()
      
      The problem is that when a optimization of read-only transactions
      (bypass 2-phase commit) was implemented, it removed the code that
      reseted the XID once a transaction wasn't active anymore:
      
      sql/sql_parse.cc:
      
      -  bzero(&thd->transaction.stmt, sizeof(thd->transaction.stmt));
      -  if (!thd->active_transaction())
      -    thd->transaction.xid_state.xid.null();
      +  thd->transaction.stmt.reset();
      
      This mostly worked fine as the transaction commit and rollback
      functions (in handler.cc) reset the XID once the transaction is
      ended. But those functions wouldn't reset the XID in case of
      a empty transaction, leading to a assertion when a new starting
      a new XA transaction.
      
      The solution is to ensure that the XID state is reset when empty
      transactions are ended (by either commit or rollback). This is
      achieved by reorganizing the code so that the transaction cleanup
      routine is invoked whenever a transaction is ended.
     @ mysql-test/r/xa.result
        Add test case result for Bug#44672
     @ mysql-test/t/xa.test
        Add test case for Bug#44672
     @ sql/handler.cc
        Invoke transaction cleanup function whenever a transaction is
        ended. Move XID state reset logic to the transaction cleanup
        function.
     @ sql/sql_class.h
        Add XID state reset logic.

    modified:
      mysql-test/r/xa.result
      mysql-test/t/xa.test
      sql/handler.cc
      sql/sql_class.h
=== modified file 'mysql-test/r/xa.result'
--- a/mysql-test/r/xa.result	2008-10-23 20:56:03 +0000
+++ b/mysql-test/r/xa.result	2009-06-05 21:43:09 +0000
@@ -75,3 +75,9 @@ xa rollback 'a','c';
 xa start 'a','c';
 drop table t1;
 End of 5.0 tests
+xa start 'a';
+xa end 'a';
+xa rollback 'a';
+xa start 'a';
+xa end 'a';
+xa rollback 'a';

=== modified file 'mysql-test/t/xa.test'
--- a/mysql-test/t/xa.test	2009-03-03 20:34:18 +0000
+++ b/mysql-test/t/xa.test	2009-06-05 21:43:09 +0000
@@ -124,6 +124,17 @@ drop table t1;
 
 --echo End of 5.0 tests
 
+#
+# Bug#44672: Assertion failed: thd->transaction.xid_state.xid.is_null()
+#
+
+xa start 'a';
+xa end 'a';
+xa rollback 'a';
+xa start 'a';
+xa end 'a';
+xa rollback 'a';
+
 # Wait till all disconnects are completed
 --source include/wait_until_count_sessions.inc
 

=== modified file 'sql/handler.cc'
--- a/sql/handler.cc	2009-03-27 09:34:24 +0000
+++ b/sql/handler.cc	2009-06-05 21:43:09 +0000
@@ -1073,6 +1073,13 @@ int ha_commit_trans(THD *thd, bool all)
     user, or an implicit commit issued by a DDL.
   */
   THD_TRANS *trans= all ? &thd->transaction.all : &thd->transaction.stmt;
+  /*
+    "real" is a nick name for a transaction for which a commit will
+    make persistent changes. E.g. a 'stmt' transaction inside a 'all'
+    transation is not 'real': even though it's possible to commit it,
+    the changes are not durable as they might be rolled back if the
+    enclosing 'all' transaction is rolled back.
+  */
   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();
@@ -1184,16 +1191,9 @@ end:
     if (rw_trans)
       start_waiting_global_read_lock(thd);
   }
-  else if (all)
-  {
-    /*
-      A COMMIT of an empty transaction. There may be savepoints.
-      Destroy them. If the transaction is not empty
-      savepoints are cleared in ha_commit_one_phase()
-      or ha_rollback_trans().
-    */
-    thd->transaction.cleanup();
-  }
+  /* Free resources and perform other cleanup even for 'empty' transactions. */
+  else if (is_real_trans)
+    thd->transaction.cleanup(thd);
 #endif /* USING_TRANSACTIONS */
   DBUG_RETURN(error);
 }
@@ -1206,6 +1206,13 @@ int ha_commit_one_phase(THD *thd, bool a
 {
   int error=0;
   THD_TRANS *trans=all ? &thd->transaction.all : &thd->transaction.stmt;
+  /*
+    "real" is a nick name for a transaction for which a commit will
+    make persistent changes. E.g. a 'stmt' transaction inside a 'all'
+    transation is not 'real': even though it's possible to commit it,
+    the changes are not durable as they might be rolled back if the
+    enclosing 'all' transaction is rolled back.
+  */
   bool is_real_trans=all || thd->transaction.all.ha_list == 0;
   Ha_trx_info *ha_info= trans->ha_list, *ha_info_next;
   DBUG_ENTER("ha_commit_one_phase");
@@ -1227,8 +1234,6 @@ int ha_commit_one_phase(THD *thd, bool a
     }
     trans->ha_list= 0;
     trans->no_2pc=0;
-    if (is_real_trans)
-      thd->transaction.xid_state.xid.null();
     if (all)
     {
 #ifdef HAVE_QUERY_CACHE
@@ -1236,8 +1241,9 @@ int ha_commit_one_phase(THD *thd, bool a
         query_cache.invalidate(thd->transaction.changed_tables);
 #endif
       thd->variables.tx_isolation=thd->session_tx_isolation;
-      thd->transaction.cleanup();
     }
+    if (is_real_trans)
+      thd->transaction.cleanup(thd);
   }
 #endif /* USING_TRANSACTIONS */
   DBUG_RETURN(error);
@@ -1249,6 +1255,13 @@ int ha_rollback_trans(THD *thd, bool all
   int error=0;
   THD_TRANS *trans=all ? &thd->transaction.all : &thd->transaction.stmt;
   Ha_trx_info *ha_info= trans->ha_list, *ha_info_next;
+  /*
+    "real" is a nick name for a transaction for which a commit will
+    make persistent changes. E.g. a 'stmt' transaction inside a 'all'
+    transation is not 'real': even though it's possible to commit it,
+    the changes are not durable as they might be rolled back if the
+    enclosing 'all' transaction is rolled back.
+  */
   bool is_real_trans=all || thd->transaction.all.ha_list == 0;
   DBUG_ENTER("ha_rollback_trans");
 
@@ -1294,19 +1307,12 @@ int ha_rollback_trans(THD *thd, bool all
     }
     trans->ha_list= 0;
     trans->no_2pc=0;
-    if (is_real_trans)
-    {
-      if (thd->transaction_rollback_request)
-        thd->transaction.xid_state.rm_error= thd->main_da.sql_errno();
-      else
-        thd->transaction.xid_state.xid.null();
-    }
     if (all)
       thd->variables.tx_isolation=thd->session_tx_isolation;
   }
   /* Always cleanup. Even if there nht==0. There may be savepoints. */
-  if (all)
-    thd->transaction.cleanup();
+  if (is_real_trans)
+    thd->transaction.cleanup(thd);
 #endif /* USING_TRANSACTIONS */
   if (all)
     thd->transaction_rollback_request= FALSE;

=== modified file 'sql/sql_class.h'
--- a/sql/sql_class.h	2009-05-30 13:32:28 +0000
+++ b/sql/sql_class.h	2009-06-05 21:43:09 +0000
@@ -1460,10 +1460,14 @@ public:
     */
     CHANGED_TABLE_LIST* changed_tables;
     MEM_ROOT mem_root; // Transaction-life memory allocation pool
-    void cleanup()
+    void cleanup(THD *thd)
     {
       changed_tables= 0;
       savepoints= 0;
+      if (thd->transaction_rollback_request)
+        xid_state.rm_error= thd->main_da.sql_errno();
+      else
+        xid_state.xid.null();
 #ifdef USING_TRANSACTIONS
       free_root(&mem_root,MYF(MY_KEEP_PREALLOC));
 #endif


Attachment: [text/bzr-bundle] bzr/[e-mail]