MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Struewing Date:March 13 2008 3:39pm
Subject:bk commit into 5.0 tree (istruewing:1.2596) BUG#33756
View as plain text  
Below is the list of changes that have just been committed into a local
5.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-13 16:39:27+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-13 16:39:26+01:00, istruewing@stripped +27 -0
    Bug#33756 - query cache with concurrent_insert=0 appears broken
    Added test result.

  mysql-test/t/query_cache.test@stripped, 2008-03-13 16:39:26+01:00, istruewing@stripped +20 -0
    Bug#33756 - query cache with concurrent_insert=0 appears broken
    Added test.

  sql/ha_myisam.cc@stripped, 2008-03-13 16:39:26+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	2007-08-21 17:47:03 +02:00
+++ b/mysql-test/r/query_cache.result	2008-03-13 16:39:26 +01:00
@@ -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 -Nrup a/mysql-test/t/query_cache.test b/mysql-test/t/query_cache.test
--- a/mysql-test/t/query_cache.test	2007-08-21 17:47:03 +02:00
+++ b/mysql-test/t/query_cache.test	2008-03-13 16:39:26 +01:00
@@ -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 -Nrup a/sql/ha_myisam.cc b/sql/ha_myisam.cc
--- a/sql/ha_myisam.cc	2008-01-23 16:01:27 +01:00
+++ b/sql/ha_myisam.cc	2008-03-13 16:39:26 +01:00
@@ -1954,6 +1954,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.
@@ -1965,39 +1966,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
Thread
bk commit into 5.0 tree (istruewing:1.2596) BUG#33756Ingo Struewing13 Mar