From: Davi Arnaut
Date: June 5 2009 9:43pm
Subject: bzr commit into mysql-5.1-bugteam branch (davi:2935) Bug#44672
List-Archive: http://lists.mysql.com/commits/75755
X-Bug: 44672
Message-Id: <20090605214315.DC3D7444268@skynet>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Boundary_(ID_IztXkjFJ7WMV0VMH6wdjdg)"

--Boundary_(ID_IztXkjFJ7WMV0VMH6wdjdg)
MIME-version: 1.0
Content-type: text/plain; CHARSET=US-ASCII
Content-transfer-encoding: 7BIT
Content-disposition: inline

# 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


--Boundary_(ID_IztXkjFJ7WMV0VMH6wdjdg)
MIME-version: 1.0
Content-type: text/bzr-bundle; CHARSET=US-ASCII;
 name="bzr/davi.arnaut@stripped"
Content-transfer-encoding: 7BIT
Content-disposition: inline;
 filename="bzr/davi.arnaut@stripped"

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: davi.arnaut@stripped
# target_branch: file:///home/davi/bzr/work/44672-5.1/
# testament_sha1: b2e73dc7a0e3e8b9ab3acd4ad70ffdb2ceba8bf5
# timestamp: 2009-06-05 18:43:15 -0300
# base_revision_id: davi.arnaut@stripped\
#   okvrebutqv443m6k
# 
# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWRr+yM8ABX9fgFQQef///39i
jEC////wYAul94mc77s8MkCUb2dNOd3ZD3t7Xa9vLuXncvJ7ryG1IITFMGkn6ZTNIT2hEybU2oyZ
MgDTQJKaIyNNNTApogepjKnpqZDQGmhoaBkOYExNBhMmTJkYTBNNMjEwBDAJEhAjVPU2jTJmqfpJ
oxqNHqNNBoANABtSBNTKn5qYlP1T1DeqeptHqQ9QeUADQAAJImpiaIaIp+1T001PRMU9PSCG0npN
MgGmggZWQLTp0OxZnW8jWctkWZsTpmgl9fn5dy/P315LNJ4GPAYVylxz5NcVNWXl9dMmDDlohW6s
kEyT2ayeyV8Ynj9J3oLsWVzhKA3UY8fgzx2MaX6uhO+MGl/UJ3/LMQ2HJIQOI8GQhmZvC/IYOIOf
Nc6+lykts9vNIMVv8YDqR9Q3A20m02J933Fh3p5GbKceucD8rqZgwhlsqSJOKRR42Ge6qWNtVJ3V
TdjbcwuiNxpObQG5n86Ea2pLq6lp4k7kVaZuIBh4LRTT2RW6JpyjXQPeoooia7ayuRdfXuyUysww
k9mTrr85WIxdrlhXjNVgcdNRkzL6N6taxTsaFXBOXhkiFYLpkJTykFJK5O7YzeN6rHkQtcRvC6xW
rFiAbUUSQFhrLPX73UVbnHNpPlc5NopkbFZzYKr1pmGGzdzP451Qm4uMeSUfHWc9vVrh2F0ztVCh
m2607bgyJR3U1w6NSQ7dlugkbxkjrNSWfz7EX5ysY0MY0iCBQTWI76EpxJ66ZWiF44y58FbLl24T
m9EVpqrrC1GWolRzx6NO+WEQ4AleuOc0YXvqeeDec5ElUuxEdcwJiSZXkucBcNlvJydt5ly7o60j
3wcAYI0c9GZgHwV4ZXYNmjFFrrTRAQt4bEEcswye5FjtCRK957QDWwOlok3seuYumEC2ExSFHcf5
oIFHjukKWIpX7ylnFNxVSU5ubySSmXl5N6NKpcmBQIMXFCJzvgkdft4XkFtd7IkzjFYfEmL5bMjU
QKZyaGsxZhGlkQYZYzt07qg2OmTKENhqBOQLsxi0BzmsnPAZf9cVMxtR79dpnZrVxsLQhQLtEMUL
MzxBhyFQtfoP68qZZbx4CFgW7aFC3gCs6Gr5CZ0q28HM60ZglUVDdoKI2ZFtAO5PYTKkREJVPriU
LjmBdKkSWghtNYNtQlQe0sHKApCoROc9hO5haGuLiZaMNN0XPJQkPfOCaE7CpMoTME8rid6othvW
uGRwK70fSiZhYEc0W55jyr5PQnptFge78gmTiSk4GQ6h2vIQsGFuYR8pAxRSVR+R1riR/43Tpvur
TejoDm37DrgPFUvXGfbfOK3CgwKCuHaJ7bnyGIJ5GPBV7+N+xljMWHUWlp8+BqRKFpyLCcyJ5LvA
FirZ43766WD60lVJQOcbxoHJQNCFvZ1v7cL1qXSwOrIhEe1rCgvL10v8aLoMNq9azDVtv1ZiyggT
nUbz8alOO8rzEFCZj0GBNaDUWPOXZh6NnflAr/sNRw4ayw3+heK5ry8Y6ZtsM3ke90DYm2RS0852
Euji12eehiYrHFxyWlOkmWLiBr4mdewFWlx0GRQUC1l6DTA5gRvQmofBay+jLnxtSLlAWnOQOic5
CgGd0aGiT5SSpWx94DCOxdLI2+4yo6D1MvXEVwqySibjPuBkhoGtHYj0+KU4k6pxOPMK7xvnpAHr
zG2plHKkyGXB6zMo/v+Iw+MSZvWlFP9h6enlMoMyXMja3jzBJc4OCzLpOo+bvnBuYW9DUVbCMPgD
OhwGF29YKSKsiVhFvE9JB1nIy7VkuRM9/RXlxkyJOAuQBl520ThSS+w4hBB924uKCej67JYm1C+M
kkFgI8vc4bAXpAGvR5QGrDxUT4ondsZMVAuDnXmjdziD+DqAPL3HUeJ5dZM8idSLvVqgswswzDMR
UqsM54S8bzfUUFB008jnbBBWNzA/oHNIZGHeYnMOVBUwWjijzDEy8qQfKvospsWF5acUyCAZH/Iu
RoVFdSCEgYRpWa1Aq1agG02kNtheuEkFVGlcyRIhTFzBDBcRYySrCgQV6evToKwoKrOygqE5FEBn
GTj3DCcGx7zIF8VJaEwy6lReVD+S9NotPVsUDFFhEzkGS87Sp08hEUmN6TtgyjuiP7SWCqhvWNzU
xwUF7D0vQ4RlMzddx4dxcavXWM69R9x3nQeBiQMdN8eAdQsDaVrlq7VBYlxvjBkDgOYytPauuwUd
+M3vaxbDFlpmr8NGuLPJvtP/IWEWPbeqh8FBHIaBslQM6JwLvM5IkWFzXU7O7qjkQiSYM94u00Cd
jSKysyTtIVQO1e+usOomkMUQa1fWiG6nT9SFuCBzsN/E7g7DUdY52IcxRmfuHhxD1BeLBcNpJBmy
kuM5UG1+youkPYn3Lv4LRP1A5hKQPOBsM1oltVvqCigt1p1aGg7FJ/SlAg9c4jDTet9EttoAfAYZ
MH7EbZihV1G9TDP31tSTyGQDQuHLYKh3ApLldxDhw3B4ADIsuhaC1h7utxEV5W3MhLWrCclO2GwJ
I2crsRRTjoKOjA9TIsaQDEMUAcA0GJWsEipcCKNNNrkgsVw6It1u2CUvTUOlOFxOkdbIsLnqDp1n
uCZI3bzulxMt2nl2FBzkOGSxSUUTjvgEjAVQXmOh899F6FcDTEtloExSMadWYUX0TFaGdLUmKKGS
RarTWqiQuLYLXX3p9o6d0OJ49xdfWCWqQ/bwHL2bIJMH1eHE9C4qOKRDTVanRZiI5HovrAqifAgq
9S4xXgpLDzq07dRkuHmRDG1DEoTXFtgL2mEDJesEyJsjWgyEHBdkHNguCsDhIwslIJCcvpBYhBck
60PdOBZ2PFQXBNMwILFaCbA3HOaFQCizr5RXPhv5EVuMvM1RYkdC2joGKszW4yJCLZiCgJE1pJ4p
BC5DMOoF2H12BWxeFU9RyFf1eaKvyp8bs/MDnBYgGeeKmjp07hefowZSREMhZHGHatbVwgNSicQu
sWamUvUwMu4Z8jbkJLUdGCsOasroh6VjksnowZPbAcGXPgAShYYEnSV8D5FJzHq6MyQzmFfwcnC0
velkYfkpZS4x/tNA6djOu9cSSXfq6lTaclmRGNkM9EtV5YZGmDG13mDXXm3J8sKkmoqYtaaIBzqY
WK1FnqVkKjoobnJAH03CZV5AoLwXjQqzGfdesQGRqh6vWCuVUiStDbuWeYs1jVWle5HpdkzRERER
uNag1zmqYuouFYLEjhUgWQDgRHCp7FZaUrhewQPt8FhtzCylbDkugvU1tuWgYo5Uszwrq5qSiTyZ
NKFCyhzn49/KjCsYIDSFfABtyYlQCV9LKBhqNQWlCJARaEB3e1f1/OZ2N+9xNpQmLk1Qr2HaTwJG
7RLoBjQZk1KIkxIliFSGDYQqOYiJJmXsL0mZndTayZYyNdt0Ew4mez7VyBfj3/qHt2CgNsJl82ED
weA2M1a+8hNhJQz9J0LeY77ChyjqknOZK4ZV3DcsbkJRBSFbL4JPWXisAnJ2x2pG9x0ucOrtN5uS
/zJmx4TEJWl7anj09sSaDJn1HAiXmCN2k0lIuchs0RqZE8YnKqNsB5g87PkKf8XckU4UJAa/sjPA


--Boundary_(ID_IztXkjFJ7WMV0VMH6wdjdg)--