Bug#43758 Query cache can lock up threads in 'freeing items' state

Early patch submitted for discussion.

It is possible for more than one thread to enter the condition
in query_cache_insert(), but the condition predicate is to
signal one thread each time the cache status changes between
the following states: {NO_FLUSH_IN_PROGRESS,FLUSH_IN_PROGRESS,
TABLE_FLUSH_IN_PROGRESS}

Consider three threads THD1, THD2, THD3

   THD2: select ... => Got a writer in ::store_query
   THD3: select ... => Got a writer in ::store_query
   THD1: flush tables => qc status= FLUSH_IN_PROGRESS;
                      new writers are blocked.
   THD2: select ... => Still got a writer and enters cond in
                       query_cache_insert
   THD3: select ... => Still got a writer and enters cond in
                       query_cache_insert
   THD1: flush tables => finished and signal status change.
   THD2: select ... => Wakes up and completes the insert.
   THD3: select ... => Happily waiting for better times. Why hurry?

This patch is a refactoring of this lock system. It introduces four new methods:
   Query_cache::try_lock()
   Query_cache::lock()
   Query_cache::lock_and_suspend()
   Query_cache::unlock()

This change also deprecates wait_while_table_flush_is_in_progress(). All threads are
queued and put on a conditional wait. On each unlock the queue is signalled. This resolve
the issues with left over threads. To assure that no threads are spending unnecessary
time waiting a signal broadcast is issued every time a lock is taken before a full
cache flush.

mysql-test/r/query_cache_debug.result:
  * Added test case for bug43758
mysql-test/t/query_cache_debug.test:
  * Added test case for bug43758
sql/sql_cache.cc:
  * Replaced calls to wait_while_table_flush_is_in_progress() with
    calls to try_lock(), lock_and_suspend() and unlock().
  * Renamed enumeration Cache_status to Cache_lock_status.
  * Renamed enumeration items to UNLOCKED, LOCKED_NO_WAIT and LOCKED.
    If the LOCKED_NO_WAIT lock type is used to lock the query cache, other
    threads using try_lock() will fail to acquire the lock.
    This is useful if the query cache is temporary disabled due to 
    a full table flush.
sql/sql_cache.h:
  * Replaced calls to wait_while_table_flush_is_in_progress() with
    calls to try_lock(), lock_and_suspend() and unlock().
  * Renamed enumeration Cache_status to Cache_lock_status.
  * Renamed enumeration items to UNLOCKED, LOCKED_NO_WAIT and LOCKED.
    If the LOCKED_NO_WAIT lock type is used to lock the query cache, other
    threads using try_lock() will fail to acquire the lock.
    This is useful if the query cache is temporary disabled due to 
    a full table flush.

mysql-test/r/query_cache_debug.result

@@ -71,3 +71,111 @@ DROP TABLE t1,t2;
 SET GLOBAL concurrent_insert= DEFAULT;
 SET GLOBAL query_cache_size= DEFAULT;
 SET GLOBAL query_cache_type= DEFAULT;
+#
+# Bug43758 Query cache can lock up threads in 'freeing items' state
+#
+FLUSH STATUS;
+SET GLOBAL query_cache_type=DEMAND;
+SET GLOBAL query_cache_size= 1024*768;
+DROP TABLE IF EXISTS t1,t2,t3,t4,t5;
+CREATE TABLE t1 (a VARCHAR(100));
+CREATE TABLE t2 (a VARCHAR(100));
+CREATE TABLE t3 (a VARCHAR(100));
+CREATE TABLE t4 (a VARCHAR(100));
+CREATE TABLE t5 (a VARCHAR(100));
+INSERT INTO t1 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t2 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t3 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t4 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t5 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+=================================== Connection thd1
+**
+** Load Query Cache with a result set and one table.
+**
+SELECT SQL_CACHE * FROM t1;
+a
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+*************************************************************************
+** We want to accomplish the following state:
+**  - Query cache status: TABLE_FLUSH_IN_PROGRESS
+**  - THD1: invalidate_table_internal (iterating query blocks)
+**  - THD2: query_cache_insert (cond_wait)
+**  - THD3: query_cache_insert (cond_wait)
+**  - No thread should be holding the structure_guard_mutex.
+**
+** First step is to place a DELETE-statement on the debug hook just
+** before the mutex lock in invalidate_table_internal.
+** This will allow new result sets to be written into the QC.
+** 
+SET SESSION debug='+d,wait_in_query_cache_invalidate1';
+SET SESSION debug='+d,wait_in_query_cache_invalidate2';
+DELETE FROM t1 WHERE a like '%a%';;
+=================================== Connection default
+** Assert that the expect process status is obtained.
+**
+=================================== Connection thd2
+** On THD2: Insert a result into the cache. This attempt will be blocked
+** because of a debug hook placed just before the mutex lock after which
+** the first part of the result set is written.
+SET SESSION debug='+d,wait_in_query_cache_insert';
+SELECT SQL_CACHE * FROM t2 UNION SELECT * FROM t3;
+=================================== Connection thd3
+** On THD3: Insert another result into the cache and block on the same
+** debug hook.
+SET SESSION debug='+d,wait_in_query_cache_insert';
+SELECT SQL_CACHE * FROM t4 UNION SELECT * FROM t5;;
+=================================== Connection default
+** Assert that the two SELECT-stmt threads to reach the hook.
+**
+**
+** Signal the DELETE thread, THD1, to continue. It will enter the mutex
+** lock and set query cache status to TABLE_FLUSH_IN_PROGRESS and then
+** unlock the mutex before stopping on the next debug hook.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_invalidate1' LIMIT 1 INTO @flush_thread_id;
+KILL QUERY @flush_thread_id;
+** Assert that we reach the next debug hook.
+**
+** Signal the remaining debug hooks blocking THD2 and THD3.
+** The threads will grab the guard mutex enter the wait condition and
+** and finally release the mutex. The threads will continue to wait
+** until a broadcast signal reaches them causing both threads to 
+** come alive and check the condition.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_insert' LIMIT 1 INTO @thread_id;
+KILL QUERY @thread_id;
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_insert' LIMIT 1 INTO @thread_id;
+KILL QUERY @thread_id;
+**
+** Finally signal the DELETE statement on THD1 one last time.
+** The stmt will complete the query cache invalidation and return 
+** cache status to NO_FLUSH_IN_PROGRESS. On the status change
+** One signal will be sent to the thread group waiting for executing
+** invalidations and a broadcast signal will be sent to the thread 
+** group holding result set writers.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_invalidate2' LIMIT 1 INTO @flush_thread_id;
+KILL QUERY @flush_thread_id;
+**
+*************************************************************************
+** No tables should be locked
+=================================== Connection thd2
+a
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+DELETE FROM t1;
+DELETE FROM t2;
+DELETE FROM t3;
+=================================== Connection thd3
+a
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+DELETE FROM t4;
+DELETE FROM t5;
+=================================== Connection thd1
+** Done.
+SET GLOBAL query_cache_size= 0;
+# Restore defaults
+RESET QUERY CACHE;
+FLUSH STATUS;
+DROP TABLE t1,t2,t3,t4,t5;
+SET GLOBAL query_cache_size= DEFAULT;
+SET GLOBAL query_cache_type= DEFAULT;

mysql-test/t/query_cache_debug.test

@@ -112,3 +112,147 @@ DROP TABLE t1,t2;
 SET GLOBAL concurrent_insert= DEFAULT;
 SET GLOBAL query_cache_size= DEFAULT;
 SET GLOBAL query_cache_type= DEFAULT;
+
+
+--echo #
+--echo # Bug43758 Query cache can lock up threads in 'freeing items' state
+--echo #
+FLUSH STATUS;
+SET GLOBAL query_cache_type=DEMAND;
+SET GLOBAL query_cache_size= 1024*768;
+--disable_warnings
+DROP TABLE IF EXISTS t1,t2,t3,t4,t5;
+--enable_warnings
+CREATE TABLE t1 (a VARCHAR(100));
+CREATE TABLE t2 (a VARCHAR(100));
+CREATE TABLE t3 (a VARCHAR(100));
+CREATE TABLE t4 (a VARCHAR(100));
+CREATE TABLE t5 (a VARCHAR(100));
+
+INSERT INTO t1 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t2 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t3 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t4 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+INSERT INTO t5 VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),('bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb');
+
+connect (thd2, localhost, root, ,test);
+connect (thd3, localhost, root, ,test);
+connect (thd1, localhost, root, ,test);
+
+connection thd1;
+--echo =================================== Connection thd1
+--echo **
+--echo ** Load Query Cache with a result set and one table.
+--echo **
+SELECT SQL_CACHE * FROM t1;
+--echo *************************************************************************
+--echo ** We want to accomplish the following state:
+--echo **  - Query cache status: TABLE_FLUSH_IN_PROGRESS
+--echo **  - THD1: invalidate_table_internal (iterating query blocks)
+--echo **  - THD2: query_cache_insert (cond_wait)
+--echo **  - THD3: query_cache_insert (cond_wait)
+--echo **  - No thread should be holding the structure_guard_mutex.
+--echo **
+--echo ** First step is to place a DELETE-statement on the debug hook just
+--echo ** before the mutex lock in invalidate_table_internal.
+--echo ** This will allow new result sets to be written into the QC.
+--echo ** 
+SET SESSION debug='+d,wait_in_query_cache_invalidate1';
+SET SESSION debug='+d,wait_in_query_cache_invalidate2';
+--send DELETE FROM t1 WHERE a like '%a%';
+
+connection default;
+--echo =================================== Connection default
+--echo ** Assert that the expect process status is obtained.
+LET $wait_condition= SELECT SQL_NO_CACHE COUNT(*)= 1 FROM information_schema.processlist WHERE state= 'wait_in_query_cache_invalidate1';
+--source include/wait_condition.inc
+-- echo **
+
+connection thd2;
+--echo =================================== Connection thd2
+--echo ** On THD2: Insert a result into the cache. This attempt will be blocked
+--echo ** because of a debug hook placed just before the mutex lock after which
+--echo ** the first part of the result set is written.
+SET SESSION debug='+d,wait_in_query_cache_insert';
+--send SELECT SQL_CACHE * FROM t2 UNION SELECT * FROM t3
+
+connection thd3;
+--echo =================================== Connection thd3
+--echo ** On THD3: Insert another result into the cache and block on the same
+--echo ** debug hook.
+SET SESSION debug='+d,wait_in_query_cache_insert';
+--send SELECT SQL_CACHE * FROM t4 UNION SELECT * FROM t5;
+
+connection default;
+--echo =================================== Connection default
+--echo ** Assert that the two SELECT-stmt threads to reach the hook.
+LET $wait_condition= SELECT SQL_NO_CACHE COUNT(*)= 2 FROM information_schema.processlist WHERE state='wait_in_query_cache_insert';
+--source include/wait_condition.inc
+--echo **
+--echo **
+
+--echo ** Signal the DELETE thread, THD1, to continue. It will enter the mutex
+--echo ** lock and set query cache status to TABLE_FLUSH_IN_PROGRESS and then
+--echo ** unlock the mutex before stopping on the next debug hook.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_invalidate1' LIMIT 1 INTO @flush_thread_id;
+KILL QUERY @flush_thread_id;
+--echo ** Assert that we reach the next debug hook.
+LET $wait_condition= SELECT SQL_NO_CACHE COUNT(*)= 1 FROM information_schema.processlist WHERE state='wait_in_query_cache_invalidate2';
+--source include/wait_condition.inc
+
+--echo **
+--echo ** Signal the remaining debug hooks blocking THD2 and THD3.
+--echo ** The threads will grab the guard mutex enter the wait condition and
+--echo ** and finally release the mutex. The threads will continue to wait
+--echo ** until a broadcast signal reaches them causing both threads to 
+--echo ** come alive and check the condition.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_insert' LIMIT 1 INTO @thread_id;
+KILL QUERY @thread_id;
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_insert' LIMIT 1 INTO @thread_id;
+KILL QUERY @thread_id;
+--echo **
+--echo ** Finally signal the DELETE statement on THD1 one last time.
+--echo ** The stmt will complete the query cache invalidation and return 
+--echo ** cache status to NO_FLUSH_IN_PROGRESS. On the status change
+--echo ** One signal will be sent to the thread group waiting for executing
+--echo ** invalidations and a broadcast signal will be sent to the thread 
+--echo ** group holding result set writers.
+SELECT SQL_NO_CACHE id FROM information_schema.processlist WHERE state='wait_in_query_cache_invalidate2' LIMIT 1 INTO @flush_thread_id;
+KILL QUERY @flush_thread_id;
+
+--echo **
+--echo *************************************************************************
+--echo ** No tables should be locked
+connection thd2;
+--echo =================================== Connection thd2
+reap;
+DELETE FROM t1;
+DELETE FROM t2;
+DELETE FROM t3;
+
+connection thd3;
+--echo =================================== Connection thd3
+reap;
+DELETE FROM t4;
+DELETE FROM t5;
+
+connection thd1;
+--echo =================================== Connection thd1
+reap;
+
+--echo ** Done.
+
+connection default;
+disconnect thd1;
+disconnect thd2;
+disconnect thd3;
+SET GLOBAL query_cache_size= 0;
+
+connection default;
+--echo # Restore defaults
+RESET QUERY CACHE;
+FLUSH STATUS;
+DROP TABLE t1,t2,t3,t4,t5;
+SET GLOBAL query_cache_size= DEFAULT;
+SET GLOBAL query_cache_type= DEFAULT;
+exit;

sql/sql_cache.h

@@ -272,12 +272,12 @@ public:
 
 
 private:
+#ifndef DBUG_OFF
+  my_thread_id m_cache_lock_thread_id;
+#endif
   pthread_cond_t COND_cache_status_changed;
-
-  enum Cache_status { NO_FLUSH_IN_PROGRESS, FLUSH_IN_PROGRESS,
-                      TABLE_FLUSH_IN_PROGRESS };
-
-  Cache_status m_cache_status;
+  enum Cache_lock_status { UNLOCKED, LOCKED_NO_WAIT, LOCKED };
+  Cache_lock_status m_cache_lock_status;
 
   void free_query_internal(Query_cache_block *point);
   void invalidate_table_internal(THD *thd, uchar *key, uint32 key_length);
@@ -380,8 +380,6 @@ protected:
 	      Query_cache_block *pprev);
   my_bool join_results(ulong join_limit);
 
-  void wait_while_table_flush_is_in_progress(bool *interrupt);
-
   /*
     Following function control structure_guard_mutex
     by themself or don't need structure_guard_mutex
@@ -469,11 +467,6 @@ protected:
   friend void query_cache_end_of_result(THD *thd);
   friend void query_cache_abort(NET *net);
 
-  bool is_flushing(void) 
-  { 
-    return (m_cache_status != Query_cache::NO_FLUSH_IN_PROGRESS);
-  }
-
   /*
     The following functions are only used when debugging
     We don't protect these with ifndef DBUG_OFF to not have to recompile
@@ -491,6 +484,11 @@ protected:
 			Query_cache_block_table * point,
 			const char *name);
   my_bool in_blocks(Query_cache_block * point);
+
+  bool try_lock(void);
+  void lock(void);
+  void lock_and_suspend(void);
+  void unlock(void);
 };
 
 extern Query_cache query_cache;