bk commit into 5.1 tree (thek:1.2589) BUG#30887

[kpettersson]

Below is the list of changes that have just been committed into a local
5.1 repository of thek. When thek 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, 2007-09-07 16:11:55+02:00, thek@adventure.(none) +2 -0
  Bug #30887 Server crashes on SET GLOBAL query_cache_size=0
  
  Reseting the query cache by issuing a SET GLOBAL query_cache_size=0 would
  cause the server to crash if a the server was saving a new result set to the
  query cache.
  
  This patch make the invalidation thread wait on the cache result writers to
  finish before the underlying memory is released.

  sql/sql_cache.cc@stripped, 2007-09-07 16:11:51+02:00, thek@adventure.(none) +97 -40
    - Added mutex assertions for extra safety.
    - Added Query_cache methods: drop_writer, wait_until_no_active_writers
    - Refactored query_cache_end_of_result to rely on block level locks instead
      of global lock.
    - Added reference variable counting active writers.
    - Fixed bug for embedded access which doesn't support grant privileges.

  sql/sql_cache.h@stripped, 2007-09-07 16:11:51+02:00, thek@adventure.(none) +22 -0
    - Added mutex assertions for extra safety.
    - Added Query_cache methods: drop_writer, wait_until_no_active_writers
    - Refactored query_cache_end_of_result to rely on block level locks instead
      of global lock.
    - Added reference variable counting active writers.

diff -Nrup a/sql/sql_cache.cc b/sql/sql_cache.cc
--- a/sql/sql_cache.cc	2007-09-04 17:43:26 +02:00
+++ b/sql/sql_cache.cc	2007-09-07 16:11:51 +02:00
@@ -653,7 +653,7 @@ void query_cache_insert(NET *net, const 
   if (interrupt)
   {
     STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
-    return;
+    DBUG_VOID_RETURN;
   }
 
   Query_cache_block *query_block= (Query_cache_block*)net->query_cache_query;
@@ -671,6 +671,7 @@ void query_cache_insert(NET *net, const 
   Query_cache_block *result= header->result();
 
   DUMP(&query_cache);
+  safe_mutex_assert_owner(&query_cache.structure_guard_mutex);
   BLOCK_LOCK_WR(query_block);
   DBUG_PRINT("qcache", ("insert packet %lu bytes long",length));
 
@@ -686,6 +687,7 @@ void query_cache_insert(NET *net, const 
     header->result(result);
     DBUG_PRINT("qcache", ("free query 0x%lx", (ulong) query_block));
     // The following call will remove the lock on query_block
+    query_cache.drop_writer(query_block);
     query_cache.free_query(query_block);
     // append_result_data no success => we need unlock
     STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
@@ -727,15 +729,16 @@ void query_cache_abort(NET *net)
   if (query_block)
   {
     DUMP(&query_cache);
+    safe_mutex_assert_owner(&query_cache.structure_guard_mutex);
     BLOCK_LOCK_WR(query_block);
     // The following call will remove the lock on query_block
+    query_cache.drop_writer(query_block);
     query_cache.free_query(query_block);
     net->query_cache_query= 0;
     DBUG_EXECUTE("check_querycache",query_cache.check_integrity(1););
   }
 
   STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
-
   DBUG_VOID_RETURN;
 }
 
@@ -762,14 +765,6 @@ void query_cache_end_of_result(THD *thd)
 
   STRUCT_LOCK(&query_cache.structure_guard_mutex);
 
-  bool interrupt;
-  query_cache.wait_while_table_flush_is_in_progress(&interrupt);
-  if (interrupt)
-  {
-    STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
-    DBUG_VOID_RETURN;
-  }
-
   query_block= ((Query_cache_block*) thd->net.query_cache_query);
   if (query_block)
   {
@@ -787,14 +782,15 @@ void query_cache_end_of_result(THD *thd)
 
     if (header->result() == 0)
     {
-      DBUG_PRINT("error", ("End of data with no result blocks; "
-                           "Query '%s' removed from cache.", header->query()));
       /*
-        Extra safety: empty result should not happen in the normal call
-        to this function. In the release version that query should be ignored
-        and removed from QC.
+        This can happen if query cache is being invalidated under
+        m_cache_status= Query_cache::FLUSH_IN_PROGRESS or
+        Query_cache::TABLE_FLUSH_IN_PROGRESS
       */
-      DBUG_ASSERT(0);
+      DBUG_PRINT("error", ("End of data with no result blocks; "
+                           "Query '%s' removed from cache.", header->query()));
+      /* Drop the writer. */
+      query_cache.drop_writer(query_block);
       query_cache.free_query(query_block);
       STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
       DBUG_VOID_RETURN;
@@ -810,15 +806,16 @@ void query_cache_end_of_result(THD *thd)
     header->result()->type= Query_cache_block::RESULT;
 
     /* Drop the writer. */
-    header->writer(0);
-    thd->net.query_cache_query= 0;
+    query_cache.drop_writer(query_block);
 
     BLOCK_UNLOCK_WR(query_block);
+
     DBUG_EXECUTE("check_querycache",query_cache.check_integrity(1););
 
   }
 
   STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
+
   DBUG_VOID_RETURN;
 }
 
@@ -881,6 +878,8 @@ ulong Query_cache::resize(ulong query_ca
   while (is_flushing())
     pthread_cond_wait(&COND_cache_status_changed, &structure_guard_mutex);
   m_cache_status= Query_cache::FLUSH_IN_PROGRESS;
+  /* Wait until there is no active writers */
+  wait_until_no_active_writers();
   STRUCT_UNLOCK(&structure_guard_mutex);
 
   free_cache();
@@ -1067,7 +1066,11 @@ def_week_frmt: %lu",                    
 	inserts++;
 	queries_in_cache++;
 	net->query_cache_query= (uchar*) query_block;
-	header->writer(net);
+
+        /* Register result writer */
+        m_number_of_active_writers++;
+        header->writer(net);
+
 	header->tables_type(tables_type);
 
 	STRUCT_UNLOCK(&structure_guard_mutex);
@@ -1259,6 +1262,7 @@ def_week_frmt: %lu",                    
   DBUG_PRINT("qcache", ("Query in query hash 0x%lx", (ulong)query_block));
 
   /* Now lock and test that nothing changed while blocks was unlocked */
+  safe_mutex_assert_owner(&structure_guard_mutex);
   BLOCK_LOCK_RD(query_block);
 
   query = query_block->query();
@@ -1565,6 +1569,21 @@ void Query_cache::wait_while_table_flush
 
 
 /**
+  @brief Wait on the predicate m_number_of_active_writers==0 and then locks
+    structure_guard_mutex.
+
+  @pre Query_cache:m_status has to be FLUSH_IN_PROGRESS to be able to prevent
+    new writers from being created and structure_guard_mutex has to be locked.
+*/
+void Query_cache::wait_until_no_active_writers(void)
+{
+  safe_mutex_assert_owner(&structure_guard_mutex);
+  while (m_number_of_active_writers > 0)
+    pthread_cond_wait(&COND_no_active_writers, &structure_guard_mutex);
+}
+
+
+/**
    @brief Remove all cached queries that uses the given database
 */
 
@@ -1581,6 +1600,8 @@ void Query_cache::invalidate(char *db)
     STRUCT_UNLOCK(&structure_guard_mutex);
     return;
   }
+  m_cache_status= Query_cache::FLUSH_IN_PROGRESS;
+  wait_until_no_active_writers();
 
   THD *thd= current_thd;
 
@@ -1636,6 +1657,8 @@ void Query_cache::invalidate(char *db)
       } while (restart);
     } // end if( tables_blocks )
   }
+
+  m_cache_status= Query_cache::NO_FLUSH_IN_PROGRESS;
   STRUCT_UNLOCK(&structure_guard_mutex);
 
   DBUG_VOID_RETURN;
@@ -1697,6 +1720,9 @@ void Query_cache::pack(ulong join_limit,
     DBUG_VOID_RETURN;
   }
 
+  m_cache_status= Query_cache::FLUSH_IN_PROGRESS;
+  wait_until_no_active_writers();
+
   if (query_cache_size == 0)
   {
     STRUCT_UNLOCK(&structure_guard_mutex);
@@ -1709,6 +1735,7 @@ void Query_cache::pack(ulong join_limit,
     pack_cache();
   } while ((++i < iteration_limit) && join_results(join_limit));
 
+  m_cache_status= Query_cache::NO_FLUSH_IN_PROGRESS;
   STRUCT_UNLOCK(&structure_guard_mutex);
   DBUG_VOID_RETURN;
 }
@@ -1725,10 +1752,13 @@ void Query_cache::destroy()
   {
     /* Underlying code expects the lock. */
     STRUCT_LOCK(&structure_guard_mutex);
+
     free_cache();
+
     STRUCT_UNLOCK(&structure_guard_mutex);
 
     pthread_cond_destroy(&COND_cache_status_changed);
+    pthread_cond_destroy(&COND_no_active_writers);
     pthread_mutex_destroy(&structure_guard_mutex);
     initialized = 0;
   }
@@ -1746,7 +1776,8 @@ void Query_cache::init()
   pthread_mutex_init(&structure_guard_mutex,MY_MUTEX_INIT_FAST);
   pthread_cond_init(&COND_cache_status_changed, NULL);
   m_cache_status= Query_cache::NO_FLUSH_IN_PROGRESS;
-  initialized = 1;
+  initialized= 1;
+  m_number_of_active_writers= 0;
   DBUG_VOID_RETURN;
 }
 
@@ -2034,6 +2065,7 @@ my_bool Query_cache::free_old_query()
     {
       Query_cache_block *block = queries_blocks;
       /* Search until we find first query that we can remove */
+      safe_mutex_assert_owner(&structure_guard_mutex);
       do
       {
 	Query_cache_query *header = block->query();
@@ -2058,21 +2090,17 @@ my_bool Query_cache::free_old_query()
 }
 
 
-/*
-  free_query_internal() - free query from query cache.
+/**
+  @brief Free query from cache and decreases the cached queries count.
 
-  SYNOPSIS
-    free_query_internal()
-      query_block           Query_cache_block representing the query
+  This function will remove the query from the cache, and place its
+  memory blocks to the list of free blocks.  'query_block' must be
+  locked for writing, this function will release (and destroy) this
+  lock.
 
-  DESCRIPTION
-    This function will remove the query from a cache, and place its
-    memory blocks to the list of free blocks.  'query_block' must be
-    locked for writing, this function will release (and destroy) this
-    lock.
+  @param[in,out] query_block Query_cache_block representing the query
 
-  NOTE
-    'query_block' should be removed from 'queries' hash _before_
+  @note 'query_block' should be removed from 'queries' hash _before_
     calling this method, as the lock will be destroyed here.
 */
 
@@ -2087,12 +2115,8 @@ void Query_cache::free_query_internal(Qu
 
   Query_cache_query *query= query_block->query();
 
-  if (query->writer() != 0)
-  {
-    /* Tell MySQL that this query should not be cached anymore */
-    query->writer()->query_cache_query= 0;
-    query->writer(0);
-  }
+  DBUG_ASSERT (query->writer() == 0);
+
   double_linked_list_exclude(query_block, &queries_blocks);
   Query_cache_block_table *table= query_block->table(0);
 
@@ -2478,6 +2502,7 @@ void Query_cache::invalidate_table(THD *
     to wait for the flush to finish.
   */
   m_cache_status= Query_cache::TABLE_FLUSH_IN_PROGRESS;
+  wait_until_no_active_writers();
   STRUCT_UNLOCK(&structure_guard_mutex);
 
   if (query_cache_size > 0)
@@ -2534,10 +2559,36 @@ Query_cache::invalidate_query_block_list
   while (list_root->next != list_root)
   {
     Query_cache_block *query_block= list_root->next->block();
-    BLOCK_LOCK_WR(query_block);
+    //BLOCK_LOCK_WR(query_block);
     free_query(query_block);
     DBUG_EXECUTE_IF("debug_cache_locks", sleep(10););
   }
+
+}
+
+
+/**
+  @brief Unregister any writer associated with the query block.
+
+  @param block The query cache block which might be associated with an active
+    writer.
+
+  @pre This function should be called under structure_guard_mutex protection.
+*/
+
+void Query_cache::drop_writer(Query_cache_block *block)
+{
+  safe_mutex_assert_owner(&structure_guard_mutex);
+  Query_cache_query *query= block->query();
+  if (query != 0 && query->writer() != 0)
+  {
+    DBUG_ASSERT(m_number_of_active_writers >0);
+    query->writer()->query_cache_query= 0;
+    query->writer(0);
+    --m_number_of_active_writers;
+    if (m_number_of_active_writers == 0)
+      pthread_cond_broadcast(&COND_no_active_writers);
+  }
 }
 
 /*
@@ -3075,6 +3126,8 @@ void Query_cache::insert_into_free_memor
 uint Query_cache::find_bin(ulong size)
 {
   DBUG_ENTER("Query_cache::find_bin");
+  if (size < min_allocation_unit)
+    DBUG_RETURN(mem_bin_steps);
   // Binary search
   int left = 0, right = mem_bin_steps;
   do
@@ -3240,7 +3293,7 @@ Query_cache::process_and_count_tables(TH
   for (; tables_used; tables_used= tables_used->next_global)
   {
     table_count++;
-#ifdef HAVE_QUERY_CACHE
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
     /*
       Disable any attempt to store this statement if there are
       column level grants on any referenced tables.
@@ -3551,6 +3604,7 @@ my_bool Query_cache::move_by_type(uchar 
     DBUG_PRINT("qcache", ("block 0x%lx QUERY", (ulong) block));
     if (*border == 0)
       break;
+    safe_mutex_assert_owner(&structure_guard_mutex);
     BLOCK_LOCK_WR(block);
     ulong len = block->length, used = block->used;
     TABLE_COUNTER_TYPE n_tables = block->n_tables;
@@ -3651,6 +3705,7 @@ my_bool Query_cache::move_by_type(uchar 
 		      *next = block->next,
 		      *prev = block->prev;
     Query_cache_block::block_type type = block->type;
+    safe_mutex_assert_owner(&structure_guard_mutex);
     BLOCK_LOCK_WR(query_block);
     ulong len = block->length, used = block->used;
     Query_cache_block *pprev = block->pprev,
@@ -3754,6 +3809,7 @@ my_bool Query_cache::join_results(ulong 
 	  if (new_result_block->length >
 	      ALIGN_SIZE(new_len) + min_allocation_unit)
 	    split_block(new_result_block, ALIGN_SIZE(new_len));
+          safe_mutex_assert_owner(&structure_guard_mutex);
 	  BLOCK_LOCK_WR(block);
 	  header->result(new_result_block);
 	  new_result_block->type = Query_cache_block::RESULT;
@@ -4176,6 +4232,7 @@ my_bool Query_cache::check_integrity(boo
       }
       else
       {
+        safe_mutex_assert_owner(&structure_guard_mutex);
 	BLOCK_LOCK_RD(query_block);
 	if (in_list(queries_blocks, query_block, "query from results"))
 	  result = 1;
diff -Nrup a/sql/sql_cache.h b/sql/sql_cache.h
--- a/sql/sql_cache.h	2007-08-17 17:26:35 +02:00
+++ b/sql/sql_cache.h	2007-09-07 16:11:51 +02:00
@@ -274,13 +274,35 @@ public:
 private:
   pthread_cond_t COND_cache_status_changed;
 
+  /* m_number_of_active_writers == 0 */
+  pthread_cond_t COND_no_active_writers;
+
   enum Cache_status { NO_FLUSH_IN_PROGRESS, FLUSH_IN_PROGRESS,
                       TABLE_FLUSH_IN_PROGRESS };
 
   Cache_status m_cache_status;
 
+
+  /**
+    The number of active result writers.
+
+    This value is updated in four places:
+      1) Query_cache::store_query()  -  When the writer is first registered.
+      2) query_cache_end_of_result() -  When ever the current thread passes
+                                        this function regardless if there still
+                                        is an active result writer or not.
+      3) query_cache_abort  -  Drop the writer if the query is aborted.
+      4) query_cache_insert -  Drop the writer if an error occurs while
+                               inserting
+
+  */
+ 
+  int m_number_of_active_writers;
+
   void free_query_internal(Query_cache_block *point);
   void invalidate_table_internal(THD *thd, uchar *key, uint32 key_length);
+  void wait_until_no_active_writers(void);
+  void drop_writer(Query_cache_block *block);
 
 protected:
   /*