From a168cfb39698d98a9906160a8f60b9522372c5e9 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Sun, 21 Apr 2019 13:27:27 +0400 Subject: [PATCH] Move XID_state::xa_state handing inside xa.cc Let xid_cache_insert()/xid_cache_delete() handle xa_state. Let session tracker use is_explicit_XA() rather than xa_state != XA_NOTR. Fixed open_tables() to refuse data access in XA_ROLLBACK_ONLY state. Removed dead code from THD::cleanup(). It was supposed to be a reminder, but it got messed up over time. spider_internal_start_trx() is called either with XA_NOTR or XA_ACTIVE, which is guarded by server callers. Thus is_explicit_XA() is acceptable replacement for XA_ACTIVE check (which was likely wrong anyway). Setting xa_state to XA_PREPARED in spider_internal_xa_prepare() isn't meaningful, as this value is never accessed later. It can't be accessed by current thread and it can't be recovered either. It can only be accessed by spider internally, which never happens. Make spider_xa_lock()/spider_xa_unlock() static. Part of MDEV-7974 - backport fix for mysql bug#12161 (XA and binlog) --- sql/handler.cc | 2 +- sql/session_tracker.cc | 2 +- sql/sql_base.cc | 8 ++--- sql/sql_class.cc | 9 +----- sql/transaction.cc | 5 ++-- sql/xa.cc | 61 +++++++++++++++++++++++++++------------ sql/xa.h | 28 ++---------------- storage/spider/spd_trx.cc | 13 ++------- storage/spider/spd_trx.h | 8 ----- 9 files changed, 56 insertions(+), 80 deletions(-) diff --git a/sql/handler.cc b/sql/handler.cc index f275b5ca247..e46dcf98052 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -2109,7 +2109,7 @@ static my_bool xarecover_handlerton(THD *unused, plugin_ref plugin, char buf[XIDDATASIZE*4+6]; // see xid_to_str DBUG_PRINT("info", ("ignore xid %s", xid_to_str(buf, info->list+i))); #endif - xid_cache_insert(info->list+i, XA_PREPARED); + xid_cache_insert(info->list + i); info->found_foreign_xids++; continue; } diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index b59475645fa..d6cc0552df6 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -1108,7 +1108,7 @@ bool Transaction_state_tracker::store(THD *thd, String *buf) if ((thd->variables.session_track_transaction_info == TX_TRACK_CHISTICS) && (tx_changed & TX_CHG_CHISTICS)) { - bool is_xa= (thd->transaction.xid_state.xa_state != XA_NOTR); + bool is_xa= thd->transaction.xid_state.is_explicit_XA(); size_t start; /* 2 length by 1 byte and code */ diff --git a/sql/sql_base.cc b/sql/sql_base.cc index dd8d890c301..0e7059918c2 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4190,13 +4190,9 @@ bool open_tables(THD *thd, const DDL_options_st &options, bool has_prelocking_list; DBUG_ENTER("open_tables"); - /* Accessing data in XA_IDLE or XA_PREPARED is not allowed. */ - enum xa_states xa_state= thd->transaction.xid_state.xa_state; - if (*start && (xa_state == XA_IDLE || xa_state == XA_PREPARED)) - { - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); + /* Data access in XA transaction is only allowed when it is active. */ + if (*start && thd->transaction.xid_state.check_has_uncommitted_xa()) DBUG_RETURN(true); - } thd->current_tablenr= 0; restart: diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 4cbbbaa9f01..8243c7377e6 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1509,12 +1509,6 @@ void THD::cleanup(void) DBUG_ASSERT(cleanup_done == 0); set_killed(KILL_CONNECTION); -#ifdef ENABLE_WHEN_BINLOG_WILL_BE_ABLE_TO_PREPARE - if (transaction.xid_state.xa_state == XA_PREPARED) - { -#error xid_state in the cache should be replaced by the allocated value - } -#endif #ifdef WITH_WSREP if (wsrep_cs().state() != wsrep::client_state::s_none) { @@ -1529,9 +1523,8 @@ void THD::cleanup(void) delete_dynamic(&user_var_events); close_temporary_tables(); - transaction.xid_state.xa_state= XA_NOTR; - trans_rollback(this); xid_cache_delete(this, &transaction.xid_state); + trans_rollback(this); DBUG_ASSERT(open_tables == NULL); /* diff --git a/sql/transaction.cc b/sql/transaction.cc index 15c45425528..bb8aaabbcb9 100644 --- a/sql/transaction.cc +++ b/sql/transaction.cc @@ -66,7 +66,6 @@ void trans_reset_one_shot_chistics(THD *thd) /* Conditions under which the transaction state must not change. */ static bool trans_check(THD *thd) { - enum xa_states xa_state= thd->transaction.xid_state.xa_state; DBUG_ENTER("trans_check"); /* @@ -77,8 +76,8 @@ static bool trans_check(THD *thd) if (unlikely(thd->in_sub_stmt)) my_error(ER_COMMIT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0)); - if (xa_state != XA_NOTR) - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); + if (thd->transaction.xid_state.is_explicit_XA()) + thd->transaction.xid_state.er_xaer_rmfail(); else DBUG_RETURN(FALSE); diff --git a/sql/xa.cc b/sql/xa.cc index a01c4b3ca87..2abbb69a031 100644 --- a/sql/xa.cc +++ b/sql/xa.cc @@ -137,9 +137,6 @@ public: static LF_HASH xid_cache; static bool xid_cache_inited; -const char *xa_state_names[]= { - "NON-EXISTING", "ACTIVE", "IDLE", "PREPARED", "ROLLBACK ONLY" -}; bool THD::fix_xid_hash_pins() @@ -156,6 +153,37 @@ void XID_STATE::set_error(uint error) xid_cache_element->rm_error= error; } +void XID_STATE::er_xaer_rmfail() const +{ + static const char *xa_state_names[]= + { "NON-EXISTING", "ACTIVE", "IDLE", "PREPARED", "ROLLBACK ONLY" }; + my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); +} + + +/** + Check that XA transaction has an uncommitted work. Report an error + to the user in case when there is an uncommitted work for XA transaction. + + @return result of check + @retval false XA transaction is NOT in state IDLE, PREPARED + or ROLLBACK_ONLY. + @retval true XA transaction is in state IDLE or PREPARED + or ROLLBACK_ONLY. +*/ + +bool XID_STATE::check_has_uncommitted_xa() const +{ + if (xa_state == XA_IDLE || + xa_state == XA_PREPARED || + xa_state == XA_ROLLBACK_ONLY) + { + er_xaer_rmfail(); + return true; + } + return false; +} + void xid_cache_init() { @@ -201,7 +229,7 @@ static XID_STATE *xid_cache_search(THD *thd, XID *xid) } -bool xid_cache_insert(XID *xid, enum xa_states xa_state) +bool xid_cache_insert(XID *xid) { XID_STATE *xs; LF_PINS *pins; @@ -212,13 +240,15 @@ bool xid_cache_insert(XID *xid, enum xa_states xa_state) if ((xs= (XID_STATE*) my_malloc(sizeof(*xs), MYF(MY_WME)))) { - xs->xa_state=xa_state; xs->xid.set(xid); if ((res= lf_hash_insert(&xid_cache, pins, xs))) my_free(xs); else + { + xs->xa_state= XA_PREPARED; xs->xid_cache_element->set(XID_cache_element::RECOVERED); + } if (res == 1) res= 0; } @@ -236,6 +266,7 @@ bool xid_cache_insert(THD *thd, XID_STATE *xid_state) switch (res) { case 0: + xid_state->xa_state= XA_ACTIVE; xid_state->xid_cache_element->set(XID_cache_element::ACQUIRED); break; case 1: @@ -262,6 +293,7 @@ void xid_cache_delete(THD *thd, XID_STATE *xid_state) else { xid_state->xid_cache_element= 0; + xid_state->xa_state= XA_NOTR; xid_state->xid.null(); } } @@ -373,18 +405,16 @@ bool trans_xa_start(THD *thd) /* TODO: JOIN is not supported yet. */ if (thd->lex->xa_opt != XA_NONE) my_error(ER_XAER_INVAL, MYF(0)); - else if (xa_state != XA_NOTR) - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); + else if (thd->transaction.xid_state.is_explicit_XA()) + thd->transaction.xid_state.er_xaer_rmfail(); else if (thd->locked_tables_mode || thd->in_active_multi_stmt_transaction()) my_error(ER_XAER_OUTSIDE, MYF(0)); else if (!trans_begin(thd)) { DBUG_ASSERT(thd->transaction.xid_state.xid.is_null()); - thd->transaction.xid_state.xa_state= XA_ACTIVE; thd->transaction.xid_state.xid.set(thd->lex->xid); if (xid_cache_insert(thd, &thd->transaction.xid_state)) { - thd->transaction.xid_state.xa_state= XA_NOTR; thd->transaction.xid_state.xid.null(); trans_rollback(thd); DBUG_RETURN(true); @@ -413,8 +443,7 @@ bool trans_xa_end(THD *thd) if (thd->lex->xa_opt != XA_NONE) my_error(ER_XAER_INVAL, MYF(0)); else if (thd->transaction.xid_state.xa_state != XA_ACTIVE) - my_error(ER_XAER_RMFAIL, MYF(0), - xa_state_names[thd->transaction.xid_state.xa_state]); + thd->transaction.xid_state.er_xaer_rmfail(); else if (!thd->transaction.xid_state.xid.eq(thd->lex->xid)) my_error(ER_XAER_NOTA, MYF(0)); else if (!xa_trans_rolled_back(&thd->transaction.xid_state)) @@ -439,14 +468,12 @@ bool trans_xa_prepare(THD *thd) DBUG_ENTER("trans_xa_prepare"); if (thd->transaction.xid_state.xa_state != XA_IDLE) - my_error(ER_XAER_RMFAIL, MYF(0), - xa_state_names[thd->transaction.xid_state.xa_state]); + thd->transaction.xid_state.er_xaer_rmfail(); else if (!thd->transaction.xid_state.xid.eq(thd->lex->xid)) my_error(ER_XAER_NOTA, MYF(0)); else if (ha_prepare(thd)) { xid_cache_delete(thd, &thd->transaction.xid_state); - thd->transaction.xid_state.xa_state= XA_NOTR; my_error(ER_XA_RBROLLBACK, MYF(0)); } else @@ -535,7 +562,7 @@ bool trans_xa_commit(THD *thd) } else { - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); + thd->transaction.xid_state.er_xaer_rmfail(); DBUG_RETURN(TRUE); } @@ -545,7 +572,6 @@ bool trans_xa_commit(THD *thd) ~(SERVER_STATUS_IN_TRANS | SERVER_STATUS_IN_TRANS_READONLY); DBUG_PRINT("info", ("clearing SERVER_STATUS_IN_TRANS")); xid_cache_delete(thd, &thd->transaction.xid_state); - thd->transaction.xid_state.xa_state= XA_NOTR; trans_track_end_trx(thd); @@ -590,7 +616,7 @@ bool trans_xa_rollback(THD *thd) if (xa_state != XA_IDLE && xa_state != XA_PREPARED && xa_state != XA_ROLLBACK_ONLY) { - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); + thd->transaction.xid_state.er_xaer_rmfail(); DBUG_RETURN(TRUE); } @@ -602,7 +628,6 @@ bool trans_xa_rollback(THD *thd) ~(SERVER_STATUS_IN_TRANS | SERVER_STATUS_IN_TRANS_READONLY); DBUG_PRINT("info", ("clearing SERVER_STATUS_IN_TRANS")); xid_cache_delete(thd, &thd->transaction.xid_state); - thd->transaction.xid_state.xa_state= XA_NOTR; trans_track_end_trx(thd); diff --git a/sql/xa.h b/sql/xa.h index 0cd28f02ae3..efe7db4b950 100644 --- a/sql/xa.h +++ b/sql/xa.h @@ -18,7 +18,6 @@ enum xa_states {XA_NOTR=0, XA_ACTIVE, XA_IDLE, XA_PREPARED, XA_ROLLBACK_ONLY}; -extern const char *xa_state_names[]; class XID_cache_element; struct XID_STATE { @@ -27,36 +26,15 @@ struct XID_STATE { enum xa_states xa_state; // used by external XA only XID_cache_element *xid_cache_element; - /** - Check that XA transaction has an uncommitted work. Report an error - to the user in case when there is an uncommitted work for XA transaction. - - @return result of check - @retval false XA transaction is NOT in state IDLE, PREPARED - or ROLLBACK_ONLY. - @retval true XA transaction is in state IDLE or PREPARED - or ROLLBACK_ONLY. - */ - - bool check_has_uncommitted_xa() const - { - if (xa_state == XA_IDLE || - xa_state == XA_PREPARED || - xa_state == XA_ROLLBACK_ONLY) - { - my_error(ER_XAER_RMFAIL, MYF(0), xa_state_names[xa_state]); - return true; - } - return false; - } - + bool check_has_uncommitted_xa() const; bool is_explicit_XA() const { return xid_cache_element != 0; } void set_error(uint error); + void er_xaer_rmfail() const; }; void xid_cache_init(void); void xid_cache_free(void); -bool xid_cache_insert(XID *xid, enum xa_states xa_state); +bool xid_cache_insert(XID *xid); bool xid_cache_insert(THD *thd, XID_STATE *xid_state); void xid_cache_delete(THD *thd, XID_STATE *xid_state); diff --git a/storage/spider/spd_trx.cc b/storage/spider/spd_trx.cc index 7c0efda4a7d..a41db0f2b31 100644 --- a/storage/spider/spd_trx.cc +++ b/storage/spider/spd_trx.cc @@ -1707,7 +1707,7 @@ int spider_check_and_set_time_zone( DBUG_RETURN(0); } -int spider_xa_lock( +static int spider_xa_lock( XID_STATE *xid_state ) { THD *thd = current_thd; @@ -1791,7 +1791,7 @@ error: DBUG_RETURN(error_num); } -int spider_xa_unlock( +static int spider_xa_unlock( XID_STATE *xid_state ) { THD *thd = current_thd; @@ -1910,7 +1910,7 @@ int spider_internal_start_trx( if (!trx->trx_start) { if ( - thd->transaction.xid_state.xa_state == XA_ACTIVE && + thd->transaction.xid_state.is_explicit_XA() && spider_param_support_xa() ) { trx->trx_xa = TRUE; @@ -1948,7 +1948,6 @@ int spider_internal_start_trx( thd->server_id)); #endif - trx->internal_xid_state.xa_state = XA_ACTIVE; trx->internal_xid_state.xid.set(&trx->xid); #ifdef SPIDER_XID_STATE_HAS_in_thd trx->internal_xid_state.in_thd = 1; @@ -2217,7 +2216,6 @@ int spider_internal_xa_commit( table_xa_opened = FALSE; } spider_xa_unlock(&trx->internal_xid_state); - trx->internal_xid_state.xa_state = XA_NOTR; DBUG_RETURN(0); error: @@ -2228,7 +2226,6 @@ error: error_in_commit: error_open_table: spider_xa_unlock(&trx->internal_xid_state); - trx->internal_xid_state.xa_state = XA_NOTR; DBUG_RETURN(error_num); } @@ -2455,7 +2452,6 @@ int spider_internal_xa_rollback( table_xa_opened = FALSE; } spider_xa_unlock(&trx->internal_xid_state); - trx->internal_xid_state.xa_state = XA_NOTR; DBUG_RETURN(0); error: @@ -2466,7 +2462,6 @@ error: error_in_rollback: error_open_table: spider_xa_unlock(&trx->internal_xid_state); - trx->internal_xid_state.xa_state = XA_NOTR; DBUG_RETURN(error_num); } @@ -2635,8 +2630,6 @@ int spider_internal_xa_prepare( spider_close_sys_table(thd, table_xa, &open_tables_backup, TRUE); table_xa_opened = FALSE; } - if (internal_xa) - trx->internal_xid_state.xa_state = XA_PREPARED; DBUG_RETURN(0); error: diff --git a/storage/spider/spd_trx.h b/storage/spider/spd_trx.h index e24f93598ed..8e7822e12e1 100644 --- a/storage/spider/spd_trx.h +++ b/storage/spider/spd_trx.h @@ -112,14 +112,6 @@ int spider_check_and_set_time_zone( int *need_mon ); -int spider_xa_lock( - XID_STATE *xid_state -); - -int spider_xa_unlock( - XID_STATE *xid_state -); - int spider_start_internal_consistent_snapshot( SPIDER_TRX *trx, SPIDER_CONN *conn,