Bug#33756 - query cache with concurrent_insert=0 appears broken

When concurrent inserts were disabled, statements after an INSERT
were not put into the query cache. This happened because we do not
save the current data file length at statement start when
concurrent inserts are disabled. But we checked the always zero
local length against the real file length anyway.
  
Fixed by doing the check only if concurrent inserts are not diabled.


mysql-test/r/query_cache.result:
  Bug#33756 - query cache with concurrent_insert=0 appears broken
  Added test result.
mysql-test/t/query_cache.test:
  Bug#33756 - query cache with concurrent_insert=0 appears broken
  Added test.
sql/ha_myisam.cc:
  Bug#33756 - query cache with concurrent_insert=0 appears broken
  Changed code so that file length check is done only when
  concurrent inserts are possible.

3 files changed, 84 insertions, 27 deletions

diff --git a/mysql-test/r/query_cache.result b/mysql-test/r/query_cache.result
index 68f18d1b0b2..fa80a44c177 100644
--- a/mysql-test/r/query_cache.result
+++ b/mysql-test/r/query_cache.result
@@ -1654,3 +1654,30 @@ set GLOBAL query_cache_type=default;
 set GLOBAL query_cache_limit=default;
 set GLOBAL query_cache_min_res_unit=default;
 set GLOBAL query_cache_size=default;
+use test;
+FLUSH STATUS;
+SET GLOBAL query_cache_size=10*1024*1024;
+SET @save_concurrent_insert= @@concurrent_insert;
+SET GLOBAL concurrent_insert= 0;
+CREATE TABLE t1 (c1 INT NOT NULL) ENGINE=MyISAM;
+INSERT INTO t1 (c1) VALUES (1), (2);
+SHOW GLOBAL VARIABLES LIKE 'concurrent_insert';
+Variable_name	Value
+concurrent_insert	0
+SHOW STATUS LIKE 'Qcache_hits';
+Variable_name	Value
+Qcache_hits	0
+SELECT * FROM t1;
+c1
+1
+2
+SELECT * FROM t1;
+c1
+1
+2
+SHOW STATUS LIKE 'Qcache_hits';
+Variable_name	Value
+Qcache_hits	1
+DROP TABLE t1;
+SET GLOBAL concurrent_insert= @save_concurrent_insert;
+SET GLOBAL query_cache_size= default;
diff --git a/mysql-test/t/query_cache.test b/mysql-test/t/query_cache.test
index 44b63df9739..ebd24bf2b89 100644
--- a/mysql-test/t/query_cache.test
+++ b/mysql-test/t/query_cache.test
@@ -1255,5 +1255,25 @@ set GLOBAL query_cache_type=default;
 set GLOBAL query_cache_limit=default;
 set GLOBAL query_cache_min_res_unit=default;
 set GLOBAL query_cache_size=default;
+use test;
+
+#
+# Bug#33756 - query cache with concurrent_insert=0 appears broken
+#
+FLUSH STATUS;
+SET GLOBAL query_cache_size=10*1024*1024;
+SET @save_concurrent_insert= @@concurrent_insert;
+SET GLOBAL concurrent_insert= 0;
+CREATE TABLE t1 (c1 INT NOT NULL) ENGINE=MyISAM;
+INSERT INTO t1 (c1) VALUES (1), (2);
+#
+SHOW GLOBAL VARIABLES LIKE 'concurrent_insert';
+SHOW STATUS LIKE 'Qcache_hits';
+SELECT * FROM t1;
+SELECT * FROM t1;
+SHOW STATUS LIKE 'Qcache_hits';
+DROP TABLE t1;
+SET GLOBAL concurrent_insert= @save_concurrent_insert;
+SET GLOBAL query_cache_size= default;
 
 # End of 5.0 tests
diff --git a/sql/ha_myisam.cc b/sql/ha_myisam.cc
index 46afa7a8f45..8c8f027bb6b 100644
--- a/sql/ha_myisam.cc
+++ b/sql/ha_myisam.cc
@@ -1954,6 +1954,7 @@ my_bool ha_myisam::register_query_cache_table(THD *thd, char *table_name,
                                               *engine_callback,
                                               ulonglong *engine_data)
 {
+  DBUG_ENTER("ha_myisam::register_query_cache_table");
   /*
     No call back function is needed to determine if a cached statement
     is valid or not.
@@ -1965,39 +1966,48 @@ my_bool ha_myisam::register_query_cache_table(THD *thd, char *table_name,
   */
   *engine_data= 0;
 
-  /*
-    If a concurrent INSERT has happened just before the currently processed
-    SELECT statement, the total size of the table is unknown.
+  if (file->s->concurrent_insert)
+  {
+    /*
+      If a concurrent INSERT has happened just before the currently
+      processed SELECT statement, the total size of the table is
+      unknown.
 
-    To determine if the table size is known, the current thread's snap shot of
-    the table size with the actual table size are compared.
+      To determine if the table size is known, the current thread's snap
+      shot of the table size with the actual table size are compared.
 
-    If the table size is unknown the SELECT statement can't be cached.
-  */
-  ulonglong actual_data_file_length;
-  ulonglong current_data_file_length;
+      If the table size is unknown the SELECT statement can't be cached.
 
-  /*
-    POSIX visibility rules specify that "2. Whatever memory values a
-    thread can see when it unlocks a mutex <...> can also be seen by any
-    thread that later locks the same mutex". In this particular case,
-    concurrent insert thread had modified the data_file_length in
-    MYISAM_SHARE before it has unlocked (or even locked)
-    structure_guard_mutex. So, here we're guaranteed to see at least that
-    value after we've locked the same mutex. We can see a later value
-    (modified by some other thread) though, but it's ok, as we only want
-    to know if the variable was changed, the actual new value doesn't matter
-  */
-  actual_data_file_length= file->s->state.state.data_file_length;
-  current_data_file_length= file->save_state.data_file_length;
+      When concurrent inserts are disabled at table open, mi_open()
+      does not assign a get_status() function. In this case the local
+      ("current") status is never updated. We would wrongly think that
+      we cannot cache the statement.
+    */
+    ulonglong actual_data_file_length;
+    ulonglong current_data_file_length;
 
-  if (current_data_file_length != actual_data_file_length)
-  {
-    /* Don't cache current statement. */
-    return FALSE;
+    /*
+      POSIX visibility rules specify that "2. Whatever memory values a
+      thread can see when it unlocks a mutex <...> can also be seen by any
+      thread that later locks the same mutex". In this particular case,
+      concurrent insert thread had modified the data_file_length in
+      MYISAM_SHARE before it has unlocked (or even locked)
+      structure_guard_mutex. So, here we're guaranteed to see at least that
+      value after we've locked the same mutex. We can see a later value
+      (modified by some other thread) though, but it's ok, as we only want
+      to know if the variable was changed, the actual new value doesn't matter
+    */
+    actual_data_file_length= file->s->state.state.data_file_length;
+    current_data_file_length= file->save_state.data_file_length;
+
+    if (current_data_file_length != actual_data_file_length)
+    {
+      /* Don't cache current statement. */
+      DBUG_RETURN(FALSE);
+    }
   }
 
   /* It is ok to try to cache current statement. */
-  return TRUE;
+  DBUG_RETURN(TRUE);
 }
 #endif