From 12f91b1d8c45bb84ee5bc7ee4b0b455fbb0e2a90 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Fri, 5 Jun 2009 19:16:54 -0300 Subject: [PATCH 1/2] 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. --- mysql-test/r/xa.result | 6 ++++++ mysql-test/t/xa.test | 11 ++++++++++ sql/handler.cc | 48 ++++++++++++++++++++++++------------------ sql/sql_class.h | 8 +++++++ 4 files changed, 53 insertions(+), 20 deletions(-) diff --git a/mysql-test/r/xa.result b/mysql-test/r/xa.result index 592cf07522b..92f52b402ec 100644 --- a/mysql-test/r/xa.result +++ b/mysql-test/r/xa.result @@ -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'; diff --git a/mysql-test/t/xa.test b/mysql-test/t/xa.test index 04ecf518577..a0d5aa60641 100644 --- a/mysql-test/t/xa.test +++ b/mysql-test/t/xa.test @@ -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 diff --git a/sql/handler.cc b/sql/handler.cc index d22ee220dc1..9246c04ce7d 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -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(). - */ + /* Free resources and perform other cleanup even for 'empty' transactions. */ + else if (is_real_trans) thd->transaction.cleanup(); - } #endif /* USING_TRANSACTIONS */ DBUG_RETURN(error); } @@ -1206,6 +1206,13 @@ int ha_commit_one_phase(THD *thd, bool all) { 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 all) } 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 all) 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(); } #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,18 +1307,13 @@ 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 (is_real_trans && thd->transaction_rollback_request) + thd->transaction.xid_state.rm_error= thd->main_da.sql_errno(); if (all) thd->variables.tx_isolation=thd->session_tx_isolation; } /* Always cleanup. Even if there nht==0. There may be savepoints. */ - if (all) + if (is_real_trans) thd->transaction.cleanup(); #endif /* USING_TRANSACTIONS */ if (all) diff --git a/sql/sql_class.h b/sql/sql_class.h index f4d55917b48..ae7f2a51428 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1464,6 +1464,14 @@ public: { changed_tables= 0; savepoints= 0; + /* + If rm_error is raised, it means that this piece of a distributed + transaction has failed and must be rolled back. But the user must + rollback it explicitly, so don't start a new distributed XA until + then. + */ + if (!xid_state.rm_error) + xid_state.xid.null(); #ifdef USING_TRANSACTIONS free_root(&mem_root,MYF(MY_KEEP_PREALLOC)); #endif From 6e422ce034e5e4a1775c8b28abb8f5deca3f1bd9 Mon Sep 17 00:00:00 2001 From: "Tatiana A. Nurnberg" Date: Sat, 6 Jun 2009 15:05:44 +0200 Subject: [PATCH 2/2] Addendum to Bug #45286: backport macro name form other tree use same (slightly unwieldy) name in all trees; fix before this version goes "public". bless ctype to avoid upmerge conflict, le sigh. --- cmd-line-utils/readline/bind.c | 2 +- cmd-line-utils/readline/chardefs.h | 4 ++-- cmd-line-utils/readline/display.c | 2 +- strings/ctype.c | 5 ++--- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/cmd-line-utils/readline/bind.c b/cmd-line-utils/readline/bind.c index fc7bebfd813..2338f73f6c9 100644 --- a/cmd-line-utils/readline/bind.c +++ b/cmd-line-utils/readline/bind.c @@ -701,7 +701,7 @@ rl_function_of_keyseq (keyseq, map, type) { unsigned char ic = keyseq[i]; - if (META_BYTE (ic) && _rl_convert_meta_chars_to_ascii) + if (META_CHAR_FOR_UCHAR (ic) && _rl_convert_meta_chars_to_ascii) { if (map[ESC].type == ISKMAP) { diff --git a/cmd-line-utils/readline/chardefs.h b/cmd-line-utils/readline/chardefs.h index 36fe7fdd7ac..0787d9943bb 100644 --- a/cmd-line-utils/readline/chardefs.h +++ b/cmd-line-utils/readline/chardefs.h @@ -59,8 +59,8 @@ #define largest_char 255 /* Largest character value. */ #define CTRL_CHAR(c) ((c) < control_character_threshold && (((c) & 0x80) == 0)) -#define META_BYTE(c) ((c) > meta_character_threshold) -#define META_CHAR(c) (META_BYTE(c) && (c) <= largest_char) +#define META_CHAR_FOR_UCHAR(c) ((c) > meta_character_threshold) +#define META_CHAR(c) (META_CHAR_FOR_UCHAR(c) && (c) <= largest_char) #define CTRL(c) ((c) & control_character_mask) #define META(c) ((c) | meta_character_bit) diff --git a/cmd-line-utils/readline/display.c b/cmd-line-utils/readline/display.c index 842a586b76b..a68ddff8c88 100644 --- a/cmd-line-utils/readline/display.c +++ b/cmd-line-utils/readline/display.c @@ -1888,7 +1888,7 @@ rl_character_len (c, pos) uc = (unsigned char)c; - if (META_BYTE (uc)) + if (META_CHAR_FOR_UCHAR (uc)) return ((_rl_output_meta_chars == 0) ? 4 : 1); if (uc == '\t') diff --git a/strings/ctype.c b/strings/ctype.c index 4a54d898337..17ad1256e74 100644 --- a/strings/ctype.c +++ b/strings/ctype.c @@ -328,9 +328,8 @@ my_string_repertoire(CHARSET_INFO *cs, const char *str, ulong length) { my_wc_t wc; int chlen; - for (; (chlen= cs->cset->mb_wc(cs, &wc, - (const unsigned char *) str, - (const unsigned char *) strend)) > 0; + for (; + (chlen= cs->cset->mb_wc(cs, &wc, (uchar*) str, (uchar*) strend)) > 0; str+= chlen) { if (wc > 0x7F)