From 653f14c2659160b1fda7f3fceb0988fa90731d4d Mon Sep 17 00:00:00 2001 From: Magne Mahre Date: Wed, 6 Oct 2010 11:01:24 +0200 Subject: Bug#56452 Assertion failed: thd->transaction.stmt.is_empty() || thd->in_sub_stmt In a precursor patch for Bug#52044 (revid:bzr/kostja@stripped), a number of reorganizations of code was made. In addition some assertions were added to ensure the correct transactional state. The reorganization had a small glitch so statements that was active in the query cache was not followed by a statement commit/rollback (this code was removed). A section in the trans_commit_stmt/trans_rollback_stmt code is to clear the thd->transaction.stmt list of affected storage engines. When a new statement is initiated, an assert introduced by the 523044 patch checks if this list is cleared. When the query cache is accessed, this list may be populated, and since it's not committed it will not be cleared. This fix adds explicit statement commit or rollback for statements that is contained in the query cache. --- sql/sql_cache.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'sql/sql_cache.cc') diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index b57c851edf4..a10e470afd4 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -341,6 +341,7 @@ TODO list: #include "../storage/myisammrg/ha_myisammrg.h" #include "../storage/myisammrg/myrg_def.h" #include "probes_mysql.h" +#include "transaction.h" #ifdef EMBEDDED_LIBRARY #include "emb_qcache.h" @@ -1683,6 +1684,8 @@ def_week_frmt: %lu, in_trans: %d, autocommit: %d", } else thd->lex->safe_to_cache_query= 0; // Don't try to cache this + /* End the statement transaction potentially started by engine. */ + trans_rollback_stmt(thd); goto err_unlock; // Parse query } else @@ -1724,6 +1727,13 @@ def_week_frmt: %lu, in_trans: %d, autocommit: %d", thd->limit_found_rows = query->found_rows(); thd->status_var.last_query_cost= 0.0; + /* + End the statement transaction potentially started by an + engine callback. We ignore the return value for now, + since as long as EOF packet is part of the query cache + response, we can't handle it anyway. + */ + (void) trans_commit_stmt(thd); if (!thd->stmt_da->is_set()) thd->stmt_da->disable_status(); -- cgit v1.2.1 From 36051b0106544a5650326cf69895b61163ad72c2 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Thu, 7 Oct 2010 19:51:37 -0300 Subject: Bug#56822: Add a thread state for sessions waiting on the query cache lock The problem was that threads waiting on the query cache lock are not easily seen due to the lack of a state indicating that the thread is waiting on the said lock. This made it difficult for users to quickly spot (for example, via SHOW PROCESSLIST) a query cache contention problem. The solution is to update the thread state when the query cache lock needs to be acquired. Whenever the lock is to be acquired, the thread state is updated to "Waiting for query cache lock" and is reset once the lock is granted or the wait is interrupted. The intention is to make query cache related hangs more evident. To further investigate query cache related locking problems, one may use PERFORMANCE_SCHEMA to track the overhead associated with the locking bits and determine which particular lock is being a contention point. sql/sql_cache.cc: Set and reset the thread state whenever a attempt to lock the query cache is made. Use DEBUG_SYNC instead of the now unnecessary wait_for_kill hack. --- sql/sql_cache.cc | 74 +++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 39 deletions(-) (limited to 'sql/sql_cache.cc') diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index a10e470afd4..b68355206b5 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -334,6 +334,7 @@ TODO list: #include "tztime.h" // struct Time_zone #include "sql_acl.h" // SELECT_ACL #include "sql_base.h" // TMP_TABLE_KEY_EXTRA +#include "debug_sync.h" // DEBUG_SYNC #ifdef HAVE_QUERY_CACHE #include #include @@ -372,30 +373,32 @@ TODO list: #define DUMP(C) DBUG_EXECUTE("qcache", {\ (C)->cache_dump(); (C)->queries_dump();(C)->tables_dump();}) - /** - Causes the thread to wait in a spin lock for a query kill signal. - This function is used by the test frame work to identify race conditions. - - The signal is caught and ignored and the thread is not killed. + Thread state to be used when the query cache lock needs to be acquired. + Sets the thread state name in the constructor, resets on destructor. */ -static void debug_wait_for_kill(const char *info) +struct Query_cache_wait_state { - DBUG_ENTER("debug_wait_for_kill"); - const char *prev_info; - THD *thd; - thd= current_thd; - prev_info= thd->proc_info; - thd->proc_info= info; - sql_print_information("%s", info); - while(!thd->killed) - my_sleep(1000); - thd->killed= THD::NOT_KILLED; - sql_print_information("Exit debug_wait_for_kill"); - thd->proc_info= prev_info; - DBUG_VOID_RETURN; -} + THD *m_thd; + const char *m_proc_info; + + Query_cache_wait_state(THD *thd, const char *func, + const char *file, unsigned int line) + : m_thd(thd) + { + if (m_thd) + m_proc_info= set_thd_proc_info(m_thd, + "Waiting for query cache lock", + func, file, line); + } + + ~Query_cache_wait_state() + { + if (m_thd) + set_thd_proc_info(m_thd, m_proc_info, NULL, NULL, 0); + } +}; #else #define RW_WLOCK(M) mysql_rwlock_wrlock(M) @@ -429,6 +432,8 @@ static void debug_wait_for_kill(const char *info) bool Query_cache::try_lock(bool use_timeout) { bool interrupt= FALSE; + THD *thd= current_thd; + Query_cache_wait_state wait_state(thd, __func__, __FILE__, __LINE__); DBUG_ENTER("Query_cache::try_lock"); mysql_mutex_lock(&structure_guard_mutex); @@ -438,7 +443,6 @@ bool Query_cache::try_lock(bool use_timeout) { m_cache_lock_status= Query_cache::LOCKED; #ifndef DBUG_OFF - THD *thd= current_thd; if (thd) m_cache_lock_thread_id= thd->thread_id; #endif @@ -497,6 +501,8 @@ bool Query_cache::try_lock(bool use_timeout) void Query_cache::lock_and_suspend(void) { + THD *thd= current_thd; + Query_cache_wait_state wait_state(thd, __func__, __FILE__, __LINE__); DBUG_ENTER("Query_cache::lock_and_suspend"); mysql_mutex_lock(&structure_guard_mutex); @@ -504,7 +510,6 @@ void Query_cache::lock_and_suspend(void) mysql_cond_wait(&COND_cache_status_changed, &structure_guard_mutex); m_cache_lock_status= Query_cache::LOCKED_NO_WAIT; #ifndef DBUG_OFF - THD *thd= current_thd; if (thd) m_cache_lock_thread_id= thd->thread_id; #endif @@ -525,6 +530,8 @@ void Query_cache::lock_and_suspend(void) void Query_cache::lock(void) { + THD *thd= current_thd; + Query_cache_wait_state wait_state(thd, __func__, __FILE__, __LINE__); DBUG_ENTER("Query_cache::lock"); mysql_mutex_lock(&structure_guard_mutex); @@ -532,7 +539,6 @@ void Query_cache::lock(void) mysql_cond_wait(&COND_cache_status_changed, &structure_guard_mutex); m_cache_lock_status= Query_cache::LOCKED; #ifndef DBUG_OFF - THD *thd= current_thd; if (thd) m_cache_lock_thread_id= thd->thread_id; #endif @@ -872,9 +878,7 @@ Query_cache::insert(Query_cache_tls *query_cache_tls, if (is_disabled() || query_cache_tls->first_query_block == NULL) DBUG_VOID_RETURN; - DBUG_EXECUTE_IF("wait_in_query_cache_insert", - debug_wait_for_kill("wait_in_query_cache_insert"); ); - + DEBUG_SYNC(current_thd, "wait_in_query_cache_insert"); if (try_lock()) DBUG_VOID_RETURN; @@ -1779,8 +1783,7 @@ void Query_cache::invalidate(THD *thd, TABLE_LIST *tables_used, invalidate_table(thd, tables_used); } - DBUG_EXECUTE_IF("wait_after_query_cache_invalidate", - debug_wait_for_kill("wait_after_query_cache_invalidate");); + DEBUG_SYNC(thd, "wait_after_query_cache_invalidate"); DBUG_VOID_RETURN; } @@ -1971,8 +1974,7 @@ void Query_cache::flush() if (is_disabled()) DBUG_VOID_RETURN; - DBUG_EXECUTE_IF("wait_in_query_cache_flush1", - debug_wait_for_kill("wait_in_query_cache_flush1");); + DEBUG_SYNC(current_thd, "wait_in_query_cache_flush1"); lock_and_suspend(); if (query_cache_size > 0) @@ -2312,9 +2314,7 @@ void Query_cache::free_cache() void Query_cache::flush_cache() { - - DBUG_EXECUTE_IF("wait_in_query_cache_flush2", - debug_wait_for_kill("wait_in_query_cache_flush2");); + DEBUG_SYNC(current_thd, "wait_in_query_cache_flush2"); my_hash_reset(&queries); while (queries_blocks != 0) @@ -2760,8 +2760,7 @@ void Query_cache::invalidate_table(THD *thd, TABLE *table) void Query_cache::invalidate_table(THD *thd, uchar * key, uint32 key_length) { - DBUG_EXECUTE_IF("wait_in_query_cache_invalidate1", - debug_wait_for_kill("wait_in_query_cache_invalidate1"); ); + DEBUG_SYNC(thd, "wait_in_query_cache_invalidate1"); /* Lock the query cache and queue all invalidation attempts to avoid @@ -2769,9 +2768,7 @@ void Query_cache::invalidate_table(THD *thd, uchar * key, uint32 key_length) */ lock(); - DBUG_EXECUTE_IF("wait_in_query_cache_invalidate2", - debug_wait_for_kill("wait_in_query_cache_invalidate2"); ); - + DEBUG_SYNC(thd, "wait_in_query_cache_invalidate2"); if (query_cache_size > 0) invalidate_table_internal(thd, key, key_length); @@ -2821,7 +2818,6 @@ Query_cache::invalidate_query_block_list(THD *thd, Query_cache_block *query_block= list_root->next->block(); BLOCK_LOCK_WR(query_block); free_query(query_block); - DBUG_EXECUTE_IF("debug_cache_locks", sleep(10);); } } -- cgit v1.2.1 From 28be8f919f768542465b07099f711632e3334f6e Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Thu, 7 Oct 2010 21:05:23 -0300 Subject: Bug#56822: Add a thread state for sessions waiting on the query cache lock Move Query_cache_wait_state declaration out of a debug block. --- sql/sql_cache.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'sql/sql_cache.cc') diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index b68355206b5..8c3abdbf1b0 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -372,6 +372,17 @@ TODO list: __LINE__,(ulong)(B)));B->query()->unlock_reading();} #define DUMP(C) DBUG_EXECUTE("qcache", {\ (C)->cache_dump(); (C)->queries_dump();(C)->tables_dump();}) +#else +#define RW_WLOCK(M) mysql_rwlock_wrlock(M) +#define RW_RLOCK(M) mysql_rwlock_rdlock(M) +#define RW_UNLOCK(M) mysql_rwlock_unlock(M) +#define BLOCK_LOCK_WR(B) B->query()->lock_writing() +#define BLOCK_LOCK_RD(B) B->query()->lock_reading() +#define BLOCK_UNLOCK_WR(B) B->query()->unlock_writing() +#define BLOCK_UNLOCK_RD(B) B->query()->unlock_reading() +#define DUMP(C) +#endif + /** Thread state to be used when the query cache lock needs to be acquired. @@ -400,16 +411,6 @@ struct Query_cache_wait_state } }; -#else -#define RW_WLOCK(M) mysql_rwlock_wrlock(M) -#define RW_RLOCK(M) mysql_rwlock_rdlock(M) -#define RW_UNLOCK(M) mysql_rwlock_unlock(M) -#define BLOCK_LOCK_WR(B) B->query()->lock_writing() -#define BLOCK_LOCK_RD(B) B->query()->lock_reading() -#define BLOCK_UNLOCK_WR(B) B->query()->unlock_writing() -#define BLOCK_UNLOCK_RD(B) B->query()->unlock_reading() -#define DUMP(C) -#endif /** Serialize access to the query cache. -- cgit v1.2.1 From 15ccca1d5520eca44fd63925fdd2020428965985 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Fri, 8 Oct 2010 09:16:20 -0300 Subject: Bug#56822: Add a thread state for sessions waiting on the query cache lock Only wait for a single debug signal at a time as the signal state is global. Also, do not activate the query cache debug sync points if the thread has no associated THD session. mysql-test/t/query_cache_debug.test: Only wait for a single debug signal at a time as the signal state is global. sql/sql_cache.cc: Do not execute the debug sync point if the thread has no associated THD session. This scenario happens for federated threads. --- sql/sql_cache.cc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'sql/sql_cache.cc') diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index 8c3abdbf1b0..9876c46bbb3 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -384,6 +384,22 @@ TODO list: #endif +/** + Macro that executes the requested action at a synchronization point + only if the thread has a associated THD session. +*/ +#if defined(ENABLED_DEBUG_SYNC) +#define QC_DEBUG_SYNC(name) \ + do { \ + THD *thd= current_thd; \ + if (thd) \ + DEBUG_SYNC(thd, name); \ + } while (0) +#else +#define QC_DEBUG_SYNC(name) +#endif + + /** Thread state to be used when the query cache lock needs to be acquired. Sets the thread state name in the constructor, resets on destructor. @@ -879,7 +895,7 @@ Query_cache::insert(Query_cache_tls *query_cache_tls, if (is_disabled() || query_cache_tls->first_query_block == NULL) DBUG_VOID_RETURN; - DEBUG_SYNC(current_thd, "wait_in_query_cache_insert"); + QC_DEBUG_SYNC("wait_in_query_cache_insert"); if (try_lock()) DBUG_VOID_RETURN; @@ -1975,7 +1991,7 @@ void Query_cache::flush() if (is_disabled()) DBUG_VOID_RETURN; - DEBUG_SYNC(current_thd, "wait_in_query_cache_flush1"); + QC_DEBUG_SYNC("wait_in_query_cache_flush1"); lock_and_suspend(); if (query_cache_size > 0) @@ -2315,7 +2331,7 @@ void Query_cache::free_cache() void Query_cache::flush_cache() { - DEBUG_SYNC(current_thd, "wait_in_query_cache_flush2"); + QC_DEBUG_SYNC("wait_in_query_cache_flush2"); my_hash_reset(&queries); while (queries_blocks != 0) -- cgit v1.2.1 From de3d279372527ed028524933817269218e102a21 Mon Sep 17 00:00:00 2001 From: Alexander Nozdrin Date: Tue, 12 Oct 2010 14:07:13 +0400 Subject: Fix compilation warnings. --- sql/sql_cache.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'sql/sql_cache.cc') diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index 9876c46bbb3..0dadc0f0cd4 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -412,7 +412,8 @@ struct Query_cache_wait_state Query_cache_wait_state(THD *thd, const char *func, const char *file, unsigned int line) - : m_thd(thd) + : m_thd(thd), + m_proc_info(NULL) { if (m_thd) m_proc_info= set_thd_proc_info(m_thd, -- cgit v1.2.1