bk commit into 5.1 tree (thek:1.2529) BUG#28211

[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-06-11 17:16:15+02:00, thek@adventure.(none) +4 -0
  Bug#28211 RENAME DATABASE and query cache don't play nicely together
  When all table blocks were removed from the query cache the client session
  hung in a tight loop waiting on an impossible condition while consuming a lot
  of CPU.
  
  This patch also corrects an error which caused valid tables to sometimes be
  removed from the query cache.

  mysql-test/r/query_cache.result@stripped, 2007-06-11 17:16:12+02:00, thek@adventure.(none)
+19 -0
    Added test case to make sure server doesn't hang in a tight loop if last
    table block is removed from the cache.

  mysql-test/t/query_cache.test@stripped, 2007-06-11 17:16:13+02:00, thek@adventure.(none) +22
-0
    Added test case to make sure server doesn't hang in a tight loop if last
    table block is removed from the cache.

  sql/sql_cache.cc@stripped, 2007-06-11 17:16:13+02:00, thek@adventure.(none) +38 -23
    - Refactored loop over table blocks. The invalidate_table() function effects
      the elements over which we iterate. The previous stop condition was broken
      due to a compiler optimization error probably caused by the goto-statement
      pointing out of the loop. The effect being that tables_blocks was never
      checked for null values and thus the loop never terminated.
    - The new implementation uses two while loops instead of a goto-statement.
      The tables_blocks is a circular list which becomes null if the last table
      block is removed from the list.

  sql/sql_cache.h@stripped, 2007-06-11 17:16:13+02:00, thek@adventure.(none) +1 -1
    Refactored name of 'tables_blocks' to 'first_table_block'. 

# 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:	thek
# Host:	adventure.(none)
# Root:	/home/thek/Development/cpp/bug28211/my51-bug28211

--- 1.109/sql/sql_cache.cc	2007-05-15 15:44:38 +02:00
+++ 1.110/sql/sql_cache.cc	2007-06-11 17:16:13 +02:00
@@ -1456,28 +1456,43 @@ void Query_cache::invalidate(char *db)
   if (query_cache_size > 0 && !flush_in_progress)
   {
     DUMP(this);
-  restart_search:
     if (tables_blocks)
     {
-      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);
+     Query_cache_block *table_block = tables_blocks;
+     do
+     {
+        bool found= FALSE;
+        do
+        {
+          Query_cache_table *table = table_block->table();
+          if (strcmp(table->db(),db) == 0)
+            found= TRUE;
+          else
+            table_block= table_block->next;
+        } while(!found && table_block != tables_blocks);
+
+        if (found)
+        {
+          /*
+            Invalidating this table will also mean that all cached queries using
+            this table also will be invalidated. This will in turn change the
+            list of tables associated with these queries and the linked list of
+            used table will be changed. Because of this we need to restart the search
+            after each invalidated table.
+            Note: If last table block is removed the tables_blocks pointer will be 0
+          */
+          Query_cache_block *next= table_block->next;
+          invalidate_table(table_block);
+          /*
+            If the next query cache block is marked as FREE this is an indication
+            that we need to restart the search.
+          */
+          if( next->type == Query_cache_block::FREE )
+            table_block= tables_blocks;
+          else
+            table_block= next;
+        }
+     } while (tables_blocks != 0 && table_block != tables_blocks );
     }
   }
   STRUCT_UNLOCK(&structure_guard_mutex);
@@ -2412,6 +2427,7 @@ Query_cache::register_tables_from_list(T
                   (ulong) tables_used->table,
                   tables_used->table->s->table_cache_key.length,
                   (ulong) tables_used->table->s->table_cache_key.str));
+
       if (!insert_table(tables_used->table->s->table_cache_key.length,
                         tables_used->table->s->table_cache_key.str,
                         block_table,
@@ -2478,9 +2494,8 @@ my_bool Query_cache::register_all_tables
 
   n= register_tables_from_list(tables_used, 0, block_table);
 
-  if (n)
+  if (n==0)
   {
-    DBUG_PRINT("qcache", ("failed at table %d", (int) n));
     /* Unlink the tables we allocated above */
     for (Query_cache_block_table *tmp = block->table(0) ;
 	 tmp != block_table;
@@ -3769,7 +3784,7 @@ void Query_cache::tables_dump()
       Query_cache_table *table = table_block->table();
       DBUG_PRINT("qcache", ("'%s' '%s'", table->db(), table->table()));
       table_block = table_block->next;
-    } while ( table_block != tables_blocks);
+    } while (table_block != tables_blocks);
   }
   else
     DBUG_PRINT("qcache", ("no tables in list"));

--- 1.89/mysql-test/r/query_cache.result	2007-05-10 14:27:52 +02:00
+++ 1.90/mysql-test/r/query_cache.result	2007-06-11 17:16:12 +02:00
@@ -1437,3 +1437,22 @@ 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;
+drop database if exists db1;
+drop database if exists db2;
+set GLOBAL query_cache_size=15*1024*1024;
+create database db1;
+use db1;
+create table t1(c1 int)engine=myisam;
+insert into t1(c1) values (1);
+select * from db1.t1 f;
+c1
+1
+show status like 'Qcache_queries_in_cache';
+Variable_name	Value
+Qcache_queries_in_cache	1
+rename schema db1 to db2;
+show status like 'Qcache_queries_in_cache';
+Variable_name	Value
+Qcache_queries_in_cache	0
+drop database db2;
+set global query_cache_size=default;

--- 1.69/mysql-test/t/query_cache.test	2007-05-08 11:56:45 +02:00
+++ 1.70/mysql-test/t/query_cache.test	2007-06-11 17:16:13 +02:00
@@ -1000,3 +1000,25 @@ set GLOBAL query_cache_min_res_unit=defa
 set GLOBAL query_cache_size= default;
 
 # End of 5.0 tests
+
+
+#
+# Bug #28211 RENAME DATABASE and query cache don't play nicely together
+#
+--disable_warnings
+drop database if exists db1;
+drop database if exists db2;
+--enable_warnings
+set GLOBAL query_cache_size=15*1024*1024;
+create database db1;
+use db1;
+create table t1(c1 int)engine=myisam;
+insert into t1(c1) values (1);
+select * from db1.t1 f;
+show status like 'Qcache_queries_in_cache';
+rename schema db1 to db2;
+show status like 'Qcache_queries_in_cache';
+drop database db2;
+set global query_cache_size=default;
+
+# End of 5.1 tests

--- 1.37/sql/sql_cache.h	2007-01-24 18:57:00 +01:00
+++ 1.38/sql/sql_cache.h	2007-06-11 17:16:13 +02:00
@@ -264,7 +264,7 @@ protected:
   byte *cache;					// cache memory
   Query_cache_block *first_block;		// physical location block list
   Query_cache_block *queries_blocks;		// query list (LIFO)
-  Query_cache_block *tables_blocks;
+  Query_cache_block *tables_blocks;         // list of used tables
 
   Query_cache_memory_bin *bins;			// free block lists
   Query_cache_memory_bin_step *steps;		// bins spacing info