bk commit into 6.0 tree (istruewing:1.2594) BUG#33756

[Ingo Struewing]

Below is the list of changes that have just been committed into a local
6.0 repository of istruewing.  When istruewing does a push these changes
will be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2008-03-12 18:02:59+01:00, istruewing@stripped +3 -0
  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@stripped, 2008-03-12 18:02:57+01:00,
istruewing@stripped +39 -0
    Bug#33756 - query cache with concurrent_insert=0 appears broken
    Added test result.

  mysql-test/t/query_cache.test@stripped, 2008-03-12 18:02:57+01:00, istruewing@stripped
+18 -0
    Bug#33756 - query cache with concurrent_insert=0 appears broken
    Added test.

  storage/myisam/ha_myisam.cc@stripped, 2008-03-12 18:02:57+01:00, istruewing@stripped
+40 -30
    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.

diff -Nrup a/mysql-test/r/query_cache.result b/mysql-test/r/query_cache.result
--- a/mysql-test/r/query_cache.result	2008-01-07 21:11:41 +01:00
+++ b/mysql-test/r/query_cache.result	2008-03-12 18:02:57 +01:00
@@ -1669,4 +1669,43 @@ SELECT 1 FROM t1 GROUP BY
 1
 1
 DROP TABLE t1;
+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%';
+Variable_name	Value
+concurrent_insert	0
+SHOW STATUS LIKE 'Qc%';
+Variable_name	Value
+Qcache_free_blocks	1
+Qcache_free_memory	10468032
+Qcache_hits	0
+Qcache_inserts	0
+Qcache_lowmem_prunes	0
+Qcache_not_cached	0
+Qcache_queries_in_cache	0
+Qcache_total_blocks	1
+SELECT * FROM t1;
+c1
+1
+2
+SELECT * FROM t1;
+c1
+1
+2
+SHOW STATUS LIKE 'Qc%';
+Variable_name	Value
+Qcache_free_blocks	1
+Qcache_free_memory	10466496
+Qcache_hits	1
+Qcache_inserts	1
+Qcache_lowmem_prunes	0
+Qcache_not_cached	0
+Qcache_queries_in_cache	1
+Qcache_total_blocks	4
+SET GLOBAL concurrent_insert= @save_concurrent_insert;
+SET GLOBAL query_cache_size= default;
 End of 5.1 tests
diff -Nrup a/mysql-test/t/query_cache.test b/mysql-test/t/query_cache.test
--- a/mysql-test/t/query_cache.test	2008-01-07 21:11:41 +01:00
+++ b/mysql-test/t/query_cache.test	2008-03-12 18:02:57 +01:00
@@ -1316,5 +1316,23 @@ SELECT 1 FROM t1 GROUP BY
   (SELECT LAST_INSERT_ID() FROM t1 ORDER BY MIN(a) ASC LIMIT 1);
 DROP TABLE t1;
 
+#
+# 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%';
+SHOW STATUS LIKE 'Qc%';
+SELECT * FROM t1;
+SELECT * FROM t1;
+SHOW STATUS LIKE 'Qc%';
+SET GLOBAL concurrent_insert= @save_concurrent_insert;
+SET GLOBAL query_cache_size= default;
+
 --echo End of 5.1 tests
 
diff -Nrup a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
--- a/storage/myisam/ha_myisam.cc	2008-03-11 15:01:28 +01:00
+++ b/storage/myisam/ha_myisam.cc	2008-03-12 18:02:57 +01:00
@@ -2083,6 +2083,7 @@ my_bool ha_myisam::register_query_cache_
                                               *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.
@@ -2094,39 +2095,48 @@ my_bool ha_myisam::register_query_cache_
   */
   *engine_data= 0;
 
-  /*
-    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.
-
-    If the table size is unknown the SELECT statement can't be cached.
-  */
-  ulonglong actual_data_file_length;
-  ulonglong current_data_file_length;
-
-  /*
-    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)
+  if (file->s->concurrent_insert)
   {
-    /* Don't cache current statement. */
-    return FALSE;
+    /*
+      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.
+
+      If the table size is unknown the SELECT statement can't be cached.
+
+      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;
+
+    /*
+      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