bk commit into 5.0 tree (kroki:1.2238) BUG#21051

[kroki]

Below is the list of changes that have just been committed into a local
5.0 repository of tomash. When tomash 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, 2006-07-24 18:37:41+04:00, kroki@stripped +2 -0
  BUG#21051: RESET QUERY CACHE very slow when query_cache_type=0
  
  There were two problems: RESET QUERY CACHE took a long time to complete
  and other threads were blocked during this time.
  
  The patch does three things:
    1 fixes a bug with improper use of test-lock-test_again technique.
        AKA Double-Checked Locking is not applicable here.
    2 Improves performance of RESET QUERY CACHE.
        Do my_hash_reset() instead of deleting elements one by one.
    3 Makes RESET QUERY CACHE non-blocking.
        The patch adjusts the locking protocol of the query cache in the
        following way: it introduces a flag flush_in_progress, which is
        set when Query_cache::flush_cache() is in progress.  This call
        sets the flag on enter, and then releases the lock.  Every other
        call is able to acquire the lock, but does nothing if
        flush_in_progress is set (as if the query cache is disabled).
        The only exception is the concurrent calls to
        Query_cache::flush_cache(), that are blocked until the flush is
        over.  When leaving Query_cache::flush_cache(), the lock is
        acquired and the flag is reset, and other (waiting) calls to
        Query_cache::flush_cache() (if any) are notified that the flush
        has finished.

  sql/sql_cache.cc@stripped, 2006-07-24 18:37:36+04:00, kroki@stripped +162 -131
    Fix bug with testing query_cache_size before acquiring the lock.
    Wherever we check that cache is usable (query_cache_size > 0) we now
    also check that flush_in_progress is false, i.e. we are not in the
    middle of cache flush.
    Add Query_cache::not_in_flush_or_wait() method and use it in
    Query_cache::flush_cache(), so that threads doing cache flush will
    wait it to finish, while other threads will bypass the cache as if
    it is disabled.
    Extract Query_cache::free_query_internal() from Query_cache::free_query(),
    which does not removes elements from the hash, and use it together with
    my_hash_reset() in Query_cache::flush_cache().

  sql/sql_cache.h@stripped, 2006-07-24 18:37:36+04:00, kroki@stripped +13 -0
    Add declarations for new members and methods.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	kroki
# Host:	moonlight.intranet
# Root:	/home/tomash/src/mysql_ab/mysql-5.0-bug21051-2

--- 1.92/sql/sql_cache.cc	2006-07-24 18:37:47 +04:00
+++ 1.93/sql/sql_cache.cc	2006-07-24 18:37:47 +04:00
@@ -574,12 +574,9 @@ void query_cache_insert(NET *net, const 
   DBUG_ENTER("query_cache_insert");
 
   STRUCT_LOCK(&query_cache.structure_guard_mutex);
-  /*
-    It is very unlikely that following condition is TRUE (it is possible
-    only if other thread is resizing cache), so we check it only after guard
-    mutex lock
-  */
-  if (unlikely(query_cache.query_cache_size == 0))
+
+  if (unlikely(query_cache.query_cache_size == 0 ||
+               query_cache.flush_in_progress))
   {
     STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
     DBUG_VOID_RETURN;
@@ -631,12 +628,9 @@ void query_cache_abort(NET *net)
   if (net->query_cache_query != 0)	// Quick check on unlocked structure
   {
     STRUCT_LOCK(&query_cache.structure_guard_mutex);
-    /*
-      It is very unlikely that following condition is TRUE (it is possible
-      only if other thread is resizing cache), so we check it only after guard
-      mutex lock
-    */
-    if (unlikely(query_cache.query_cache_size == 0))
+
+    if (unlikely(query_cache.query_cache_size == 0 ||
+                 query_cache.flush_in_progress))
     {
       STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
       DBUG_VOID_RETURN;
@@ -675,7 +669,8 @@ void query_cache_end_of_result(THD *thd)
       only if other thread is resizing cache), so we check it only after guard
       mutex lock
     */
-    if (unlikely(query_cache.query_cache_size == 0))
+    if (unlikely(query_cache.query_cache_size == 0 ||
+                 query_cache.flush_in_progress))
     {
       STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
       DBUG_VOID_RETURN;
@@ -762,8 +757,7 @@ ulong Query_cache::resize(ulong query_ca
 			query_cache_size_arg));
   DBUG_ASSERT(initialized);
   STRUCT_LOCK(&structure_guard_mutex);
-  if (query_cache_size > 0)
-    free_cache();
+  free_cache();
   query_cache_size= query_cache_size_arg;
   ::query_cache_size= init_cache();
   STRUCT_UNLOCK(&structure_guard_mutex);
@@ -784,7 +778,7 @@ void Query_cache::store_query(THD *thd, 
   TABLE_COUNTER_TYPE local_tables;
   ulong tot_length;
   DBUG_ENTER("Query_cache::store_query");
-  if (query_cache_size == 0 || thd->locked_tables)
+  if (thd->locked_tables)
     DBUG_VOID_RETURN;
   uint8 tables_type= 0;
 
@@ -838,7 +832,7 @@ sql mode: 0x%lx, sort len: %lu, conncat 
     ha_release_temporary_latches(thd);
     STRUCT_LOCK(&structure_guard_mutex);
 
-    if (query_cache_size == 0)
+    if (query_cache_size == 0 || flush_in_progress)
     {
       STRUCT_UNLOCK(&structure_guard_mutex);
       DBUG_VOID_RETURN;
@@ -970,8 +964,7 @@ Query_cache::send_result_to_client(THD *
   Query_cache_query_flags flags;
   DBUG_ENTER("Query_cache::send_result_to_client");
 
-  if (query_cache_size == 0 || thd->locked_tables ||
-      thd->variables.query_cache_type == 0)
+  if (thd->locked_tables || thd->variables.query_cache_type == 0)
     goto err;
 
   /* Check that we haven't forgot to reset the query cache variables */
@@ -1011,11 +1004,12 @@ Query_cache::send_result_to_client(THD *
   }
 
   STRUCT_LOCK(&structure_guard_mutex);
-  if (query_cache_size == 0)
+  if (query_cache_size == 0 || flush_in_progress)
   {
     DBUG_PRINT("qcache", ("query cache disabled"));
     goto err_unlock;
   }
+
   Query_cache_block *query_block;
 
   tot_length= query_length + thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE;
@@ -1241,45 +1235,43 @@ void Query_cache::invalidate(THD *thd, T
 			     my_bool using_transactions)
 {
   DBUG_ENTER("Query_cache::invalidate (table list)");
-  if (query_cache_size > 0)
+  STRUCT_LOCK(&structure_guard_mutex);
+  if (query_cache_size > 0 && !flush_in_progress)
   {
-    STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)
-    {
-      DUMP(this);
+    DUMP(this);
 
-      using_transactions = using_transactions &&
-	(thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
-      for (; tables_used; tables_used= tables_used->next_local)
-      {
-	DBUG_ASSERT(!using_transactions || tables_used->table!=0);
-	if (tables_used->derived)
-	  continue;
-	if (using_transactions &&
-	   (tables_used->table->file->table_cache_type() ==
-	    HA_CACHE_TBL_TRANSACT))
-	  /*
-	     Tables_used->table can't be 0 in transaction.
-	     Only 'drop' invalidate not opened table, but 'drop'
-	     force transaction finish.
-	  */
-	  thd->add_changed_table(tables_used->table);
-	else
-	  invalidate_table(tables_used);
-      }
+    using_transactions = using_transactions &&
+      (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
+    for (; tables_used; tables_used= tables_used->next_local)
+    {
+      DBUG_ASSERT(!using_transactions || tables_used->table!=0);
+      if (tables_used->derived)
+        continue;
+      if (using_transactions &&
+         (tables_used->table->file->table_cache_type() ==
+          HA_CACHE_TBL_TRANSACT))
+        /*
+           Tables_used->table can't be 0 in transaction.
+           Only 'drop' invalidate not opened table, but 'drop'
+           force transaction finish.
+        */
+        thd->add_changed_table(tables_used->table);
+      else
+        invalidate_table(tables_used);
     }
-    STRUCT_UNLOCK(&structure_guard_mutex);
   }
+  STRUCT_UNLOCK(&structure_guard_mutex);
+
   DBUG_VOID_RETURN;
 }
 
 void Query_cache::invalidate(CHANGED_TABLE_LIST *tables_used)
 {
   DBUG_ENTER("Query_cache::invalidate (changed table list)");
-  if (query_cache_size > 0 && tables_used)
+  if (tables_used)
   {
     STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)
+    if (query_cache_size > 0 && !flush_in_progress)
     {
       DUMP(this);
       for (; tables_used; tables_used= tables_used->next)
@@ -1309,10 +1301,10 @@ void Query_cache::invalidate(CHANGED_TAB
 void Query_cache::invalidate_locked_for_write(TABLE_LIST *tables_used)
 {
   DBUG_ENTER("Query_cache::invalidate_locked_for_write");
-  if (query_cache_size > 0 && tables_used)
+  if (tables_used)
   {
     STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)
+    if (query_cache_size > 0 && !flush_in_progress)
     {
       DUMP(this);
       for (; tables_used; tables_used= tables_used->next_local)
@@ -1336,21 +1328,19 @@ void Query_cache::invalidate(THD *thd, T
 {
   DBUG_ENTER("Query_cache::invalidate (table)");
   
-  if (query_cache_size > 0)
+  STRUCT_LOCK(&structure_guard_mutex);
+  if (query_cache_size > 0 && !flush_in_progress)
   {
-    STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)
-    {
-      using_transactions = using_transactions &&
-	(thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
-      if (using_transactions && 
-	  (table->file->table_cache_type() == HA_CACHE_TBL_TRANSACT))
-	thd->add_changed_table(table);
-      else
-	invalidate_table(table);
-    }
-    STRUCT_UNLOCK(&structure_guard_mutex);
+    using_transactions = using_transactions &&
+      (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
+    if (using_transactions && 
+        (table->file->table_cache_type() == HA_CACHE_TBL_TRANSACT))
+      thd->add_changed_table(table);
+    else
+      invalidate_table(table);
   }
+  STRUCT_UNLOCK(&structure_guard_mutex);
+
   DBUG_VOID_RETURN;
 }
 
@@ -1359,20 +1349,18 @@ void Query_cache::invalidate(THD *thd, c
 {
   DBUG_ENTER("Query_cache::invalidate (key)");
   
-  if (query_cache_size > 0)
+  STRUCT_LOCK(&structure_guard_mutex);
+  if (query_cache_size > 0 && !flush_in_progress)
   {
     using_transactions = using_transactions &&
       (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
     if (using_transactions) // used for innodb => has_transactions() is TRUE
       thd->add_changed_table(key, key_length);
     else
-    {
-      STRUCT_LOCK(&structure_guard_mutex);
-      if (query_cache_size > 0)
-	invalidate_table((byte*)key, key_length);
-      STRUCT_UNLOCK(&structure_guard_mutex);  
-    }
+      invalidate_table((byte*)key, key_length);
   }
+  STRUCT_UNLOCK(&structure_guard_mutex);  
+
   DBUG_VOID_RETURN;
 }
 
@@ -1383,38 +1371,36 @@ void Query_cache::invalidate(THD *thd, c
 void Query_cache::invalidate(char *db)
 {
   DBUG_ENTER("Query_cache::invalidate (db)");
-  if (query_cache_size > 0)
+  STRUCT_LOCK(&structure_guard_mutex);
+  if (query_cache_size > 0 && !flush_in_progress)
   {
-    STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)
-    {
-      DUMP(this);
+    DUMP(this);
   restart_search:
-      if (tables_blocks)
+    if (tables_blocks)
+    {
+      Query_cache_block *curr= tables_blocks;
+      Query_cache_block *next;
+      do
       {
-	Query_cache_block *curr= tables_blocks;
-	Query_cache_block *next;
-	do
-	{
-	  next= curr->next;
-	  if (strcmp(db, (char*)(curr->table()->db())) == 0)
-	    invalidate_table(curr);
-	  /*
-	    invalidate_table can freed block on which point 'next' (if
-	    table of this block used only in queries which was deleted
-	    by invalidate_table). As far as we do not allocate new blocks
-	    and mark all headers of freed blocks as 'FREE' (even if they are
-	    merged with other blocks) we can just test type of block
-	    to be sure that block is not deleted
-	  */
-	  if (next->type == Query_cache_block::FREE)
-	    goto restart_search;
-	  curr= next;
-	} while (curr != tables_blocks);
-      }
+        next= curr->next;
+        if (strcmp(db, (char*)(curr->table()->db())) == 0)
+          invalidate_table(curr);
+        /*
+          invalidate_table can freed block on which point 'next' (if
+          table of this block used only in queries which was deleted
+          by invalidate_table). As far as we do not allocate new blocks
+          and mark all headers of freed blocks as 'FREE' (even if they are
+          merged with other blocks) we can just test type of block
+          to be sure that block is not deleted
+        */
+        if (next->type == Query_cache_block::FREE)
+          goto restart_search;
+        curr= next;
+      } while (curr != tables_blocks);
     }
-    STRUCT_UNLOCK(&structure_guard_mutex);
   }
+  STRUCT_UNLOCK(&structure_guard_mutex);
+
   DBUG_VOID_RETURN;
 }
 
@@ -1422,23 +1408,22 @@ void Query_cache::invalidate(char *db)
 void Query_cache::invalidate_by_MyISAM_filename(const char *filename)
 {
   DBUG_ENTER("Query_cache::invalidate_by_MyISAM_filename");
-  if (query_cache_size > 0)
+
+  STRUCT_LOCK(&structure_guard_mutex);
+  if (query_cache_size > 0 && !flush_in_progress)
   {
     /* Calculate the key outside the lock to make the lock shorter */
     char key[MAX_DBKEY_LENGTH];
     uint32 db_length;
     uint key_length= filename_2_table_key(key, filename, &db_length);
-    STRUCT_LOCK(&structure_guard_mutex);
-    if (query_cache_size > 0)			// Safety if cache removed
-    {
-      Query_cache_block *table_block;
-      if ((table_block = (Query_cache_block*) hash_search(&tables,
-							  (byte*) key,
-							  key_length)))
-	invalidate_table(table_block);
-    }
-    STRUCT_UNLOCK(&structure_guard_mutex);
+    Query_cache_block *table_block;
+    if ((table_block = (Query_cache_block*) hash_search(&tables,
+      						  (byte*) key,
+      						  key_length)))
+      invalidate_table(table_block);
   }
+  STRUCT_UNLOCK(&structure_guard_mutex);
+
   DBUG_VOID_RETURN;
 }
 
@@ -1483,7 +1468,12 @@ void Query_cache::destroy()
   }
   else
   {
+    /* Underlying code expects the lock. */
+    STRUCT_LOCK(&structure_guard_mutex);
     free_cache();
+    STRUCT_UNLOCK(&structure_guard_mutex);
+
+    pthread_cond_destroy(&flush_finished_cond);
     pthread_mutex_destroy(&structure_guard_mutex);
     initialized = 0;
   }
@@ -1499,6 +1489,8 @@ void Query_cache::init()
 {
   DBUG_ENTER("Query_cache::init");
   pthread_mutex_init(&structure_guard_mutex,MY_MUTEX_INIT_FAST);
+  pthread_cond_init(&flush_finished_cond, NULL);
+  flush_in_progress= FALSE;
   initialized = 1;
   DBUG_VOID_RETURN;
 }
@@ -1728,16 +1720,48 @@ void Query_cache::free_cache()
   Free block data
 *****************************************************************************/
 
+inline
+bool
+Query_cache::not_in_flush_or_wait()
+{
+  if (flush_in_progress)
+  {
+    while (flush_in_progress)
+      pthread_cond_wait(&flush_finished_cond, &structure_guard_mutex);
+
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+
 /*
   The following assumes we have a lock on the cache
 */
 
 void Query_cache::flush_cache()
 {
-  while (queries_blocks != 0)
+  if (not_in_flush_or_wait())
   {
-    BLOCK_LOCK_WR(queries_blocks);
-    free_query(queries_blocks);
+    /*
+      Setting 'flush_in_progress' will prevent other threads from
+      using the cache while we are in the middle of the flush, and we
+      release the lock so that other threads won't block.
+    */
+    flush_in_progress= TRUE;
+    STRUCT_UNLOCK(&structure_guard_mutex);
+
+    my_hash_reset(&queries);
+    while (queries_blocks != 0)
+    {
+      BLOCK_LOCK_WR(queries_blocks);
+      free_query_internal(queries_blocks);
+    }
+
+    STRUCT_LOCK(&structure_guard_mutex);
+    flush_in_progress= FALSE;
+    pthread_cond_broadcast(&flush_finished_cond);
   }
 }
 
@@ -1788,17 +1812,19 @@ my_bool Query_cache::free_old_query()
   Free query from query cache.
   query_block must be locked for writing.
   This function will remove (and destroy) the lock for the query.
+
+  NOTE: 'query_block' should be removed from 'queries' hash _before_
+  calling this method, as the lock will be destroyed here.
 */
 
-void Query_cache::free_query(Query_cache_block *query_block)
+void Query_cache::free_query_internal(Query_cache_block *query_block)
 {
-  DBUG_ENTER("Query_cache::free_query");
+  DBUG_ENTER("Query_cache::free_query_internal");
   DBUG_PRINT("qcache", ("free query 0x%lx %lu bytes result",
 		      (ulong) query_block,
 		      query_block->query()->length() ));
 
   queries_in_cache--;
-  hash_delete(&queries,(byte *) query_block);
 
   Query_cache_query *query = query_block->query();
 
@@ -1848,6 +1874,19 @@ void Query_cache::free_query(Query_cache
   DBUG_VOID_RETURN;
 }
 
+void Query_cache::free_query(Query_cache_block *query_block)
+{
+  DBUG_ENTER("Query_cache::free_query");
+  DBUG_PRINT("qcache", ("free query 0x%lx %lu bytes result",
+		      (ulong) query_block,
+		      query_block->query()->length() ));
+
+  hash_delete(&queries,(byte *) query_block);
+  free_query_internal(query_block);
+
+  DBUG_VOID_RETURN;
+}
+
 /*****************************************************************************
  Query data creation
 *****************************************************************************/
@@ -2436,7 +2475,7 @@ Query_cache::allocate_block(ulong len, m
       only if other thread is resizing cache), so we check it only after
       guard mutex lock
     */
-    if (unlikely(query_cache.query_cache_size == 0))
+    if (unlikely(query_cache.query_cache_size == 0 || flush_in_progress))
     {
       STRUCT_UNLOCK(&structure_guard_mutex);
       DBUG_RETURN(0);
@@ -2985,7 +3024,7 @@ void Query_cache::pack_cache()
     only if other thread is resizing cache), so we check it only after
     guard mutex lock
   */
-  if (unlikely(query_cache_size == 0))
+  if (unlikely(query_cache_size == 0 || flush_in_progress))
   {
     STRUCT_UNLOCK(&structure_guard_mutex);
     DBUG_VOID_RETURN;
@@ -3300,7 +3339,7 @@ my_bool Query_cache::join_results(ulong 
   DBUG_ENTER("Query_cache::join_results");
 
   STRUCT_LOCK(&structure_guard_mutex);
-  if (queries_blocks != 0)
+  if (queries_blocks != 0 && !flush_in_progress)
   {
     DBUG_ASSERT(query_cache_size > 0);
     Query_cache_block *block = queries_blocks;
@@ -3593,24 +3632,16 @@ my_bool Query_cache::check_integrity(boo
   uint i;
   DBUG_ENTER("check_integrity");
 
-  if (query_cache_size == 0)
-  {
-    DBUG_PRINT("qcache", ("Query Cache not initialized"));
-    DBUG_RETURN(0);
-  }
   if (!not_locked)
-  {
     STRUCT_LOCK(&structure_guard_mutex);
-    /*
-      It is very unlikely that following condition is TRUE (it is possible
-      only if other thread is resizing cache), so we check it only after
-      guard mutex lock
-    */
-    if (unlikely(query_cache_size == 0))
-    {
+
+  if (unlikely(query_cache_size == 0 || flush_in_progress))
+  {
+    if (!not_locked)
       STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
-      DBUG_RETURN(0);
-    }
+
+    DBUG_PRINT("qcache", ("Query Cache not initialized"));
+    DBUG_RETURN(0);
   }
 
   if (hash_check(&queries))

--- 1.32/sql/sql_cache.h	2006-07-24 18:37:47 +04:00
+++ 1.33/sql/sql_cache.h	2006-07-24 18:37:47 +04:00
@@ -241,6 +241,13 @@ public:
   ulong free_memory, queries_in_cache, hits, inserts, refused,
     free_memory_blocks, total_blocks, lowmem_prunes;
 
+private:
+  pthread_cond_t flush_finished_cond;
+  bool flush_in_progress;
+
+  void free_query_internal(Query_cache_block *point);
+  bool not_in_flush_or_wait();
+
 protected:
   /*
     The following mutex is locked when searching or changing global
@@ -249,6 +256,12 @@ protected:
     LOCK SEQUENCE (to prevent deadlocks):
       1. structure_guard_mutex
       2. query block (for operation inside query (query block/results))
+
+    Thread doing cache flush releases the mutex once it sets
+    flush_in_progress flag, so other threads may bypass the cache as
+    if it is disabled, not waiting for reset to finish.  The exception
+    is other threads that were going to do cache flush---they'll wait
+    till the end of a flush operation.
   */
   pthread_mutex_t structure_guard_mutex;
   byte *cache;					// cache memory