MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:dlenev Date:July 31 2006 7:02pm
Subject:bk commit into 5.1 tree (dlenev:1.2259) BUG#12212
View as plain text  
Below is the list of changes that have just been committed into a local
5.1 repository of dlenev. When dlenev 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-31 23:02:07+04:00, dlenev@stripped +7 -0
  Fix for bug#12212/19403 "Crash that happens during removing of database name
  from cache" and #21216 "Simultaneous DROP TABLE and SHOW OPEN TABLES causes
  server to crash".
  
  Crash happened when one ran DROP DATABASE or SHOW OPEN TABLES statements
  while concurrently doing DROP TABLE (or RENAME TABLE, CREATE TABLE LIKE
  or any other command that takes name-lock) in other connection.
  
  This problem was caused by the fact that table placeholders which were
  added to table cache in order to obtain name-lock on table had
  TABLE_SHARE::db and table_name set to 0. Therefore they broke assumption
  that these members are non-0 for all tables in table cache on which some
  of our code relies.
   
  The fix sets these members for such placeholders to appropriate value making
  this assumption true again. As attempt to avoid such problems in future
  we introduce auxiliary TABLE_SHARE::set_table_cache_key() method which
  should be used when one wants to set TABLE_SHARE::table_cache_key and which
  ensures that TABLE_SHARE::table_name/db are set properly.
  
  Question for reviewer is marked by QQ mark.

  mysql-test/r/drop.result@stripped, 2006-07-31 23:02:04+04:00, dlenev@stripped +13 -0
    Added tests for bug#12212/19403 "Crash that happens during removing of
    database name from cache" and #21216 "Simultaneous DROP TABLE and SHOW
    OPEN TABLES causes server to crash".

  mysql-test/t/drop.test@stripped, 2006-07-31 23:02:04+04:00, dlenev@stripped +41 -0
    Added tests for bug#12212/19403 "Crash that happens during removing of
    database name from cache" and #21216 "Simultaneous DROP TABLE and SHOW
    OPEN TABLES causes server to crash".

  sql/lock.cc@stripped, 2006-07-31 23:02:04+04:00, dlenev@stripped +2 -3
    Our code assumes that TABLE_SHARE::table_name/db for objects in table cache
    is set properly (and is non-NULL). For example look in list_open_tables()
    and remove_db_from_cache(). This was not true for table placeholders that
    were added to table cache for name-locking. Changed lock_table_name() to
    preserve this assumption (now it uses TABLE_SHARE::set_table_cache_key()
    to set all three table_cache_key/db/table_name members at once).

  sql/sql_base.cc@stripped, 2006-07-31 23:02:04+04:00, dlenev@stripped +16 -30
    Now we use TABLE_SHARE::set_table_cache_key() auxiliary method to set
    TABLE_SHARE::table_cache_key/db/table_name members at once.
    Also got rid of unused code in reopen_name_locked_table() function.

  sql/sql_select.cc@stripped, 2006-07-31 23:02:04+04:00, dlenev@stripped +0 -2
    Got rid of redundant code. TABLE_SHARE::db/table_cache_key are both set to
    empty string by the call to init_tmp_table_share() routine.

  sql/table.cc@stripped, 2006-07-31 23:02:05+04:00, dlenev@stripped +2 -8
    Now alloc_table_share() uses auxiliary TABLE_SHARE::set_table_cache_key()
    method to properly set TABLE_SHARE::table_cache_key/db/table_name members.

  sql/table.h@stripped, 2006-07-31 23:02:05+04:00, dlenev@stripped +42 -1
    Added comment about importance of apropriate setting of
    TABLE_SHARE::table_name/db/table_cache_key for tables in table cache.
    Introduced auxiliary TABLE_SHARE::set_table_cache_key() method which allows
    to set these three members at once.

# 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:	dlenev
# Host:	mockturtle.local
# Root:	/home/dlenev/src/mysql-5.1-bg19403

--- 1.95/sql/lock.cc	2006-07-31 23:02:14 +04:00
+++ 1.96/sql/lock.cc	2006-07-31 23:02:14 +04:00
@@ -910,9 +910,8 @@ int lock_table_name(THD *thd, TABLE_LIST
                                   key_length, MYF(MY_WME | MY_ZEROFILL))))
     DBUG_RETURN(-1);
   table->s= (TABLE_SHARE*) (table+1);
-  memcpy((table->s->table_cache_key.str= (char*) (table->s+1)), key,
-         key_length);
-  table->s->table_cache_key.length= key_length;
+  memcpy((char*) (table->s+1), key, key_length);
+  table->s->set_table_cache_key((char*) (table->s+1), key_length);
   table->s->tmp_table= INTERNAL_TMP_TABLE;  // for intern_close_table
   table->in_use= thd;
   table->locked_by_name=1;

--- 1.334/sql/sql_base.cc	2006-07-31 23:02:14 +04:00
+++ 1.335/sql/sql_base.cc	2006-07-31 23:02:14 +04:00
@@ -642,20 +642,14 @@ static void close_handle_and_leave_table
     This has to be done to ensure that the table share is removed from
     the table defintion cache as soon as the last instance is removed
   */
-  if ((share= (TABLE_SHARE*) alloc_root(mem_root, sizeof(*share))))
+  if ((share= (TABLE_SHARE*) alloc_root(mem_root, sizeof(*share) +
+                                        old_share->table_cache_key.length)))
   {
     bzero((char*) share, sizeof(*share));
-    share->db.str= memdup_root(mem_root, old_share->db.str,
-                               old_share->db.length+1);
-    share->db.length= old_share->db.length;
-    share->table_name.str= memdup_root(mem_root,
-                                       old_share->table_name.str,
-                                       old_share->table_name.length+1);
-    share->table_name.length= old_share->table_name.length;
-    share->table_cache_key.str= memdup_root(mem_root,
-                                            old_share->table_cache_key.str,
-                                            old_share->table_cache_key.length);
-    share->table_cache_key.length= old_share->table_cache_key.length;
+    memcpy((char*) (share+1), old_share->table_cache_key.str,
+           old_share->table_cache_key.length);
+    share->set_table_cache_key((char*) (share+1),
+                               old_share->table_cache_key.length);
     share->tmp_table= INTERNAL_TMP_TABLE;       // for intern_close_table()
   }
 
@@ -1600,28 +1594,24 @@ bool rename_temporary_table(THD* thd, TA
 			    const char *table_name)
 {
   char *key;
+  uint key_len;
   TABLE_SHARE *share= table->s;
   TABLE_LIST table_list;
-  uint db_length, table_length;
   DBUG_ENTER("rename_temporary_table");
 
-  if (!(key=(char*) alloc_root(&share->mem_root,
-			       (uint) (db_length= strlen(db))+
-			       (uint) (table_length= strlen(table_name))+6+4)))
+  /*
+    QQ: Is there any better approach ? E.g. introducing version
+    of set_table_cache_key that will also accept db and table name
+    lengths as parameters ?
+  */
+
+  if (!(key=(char*) alloc_root(&share->mem_root, MAX_DBKEY_LENGTH)))
     DBUG_RETURN(1);				/* purecov: inspected */
 
   table_list.db= (char*) db;
   table_list.table_name= (char*) table_name;
-  share->db.str= share->table_cache_key.str= key;
-  share->db.length= db_length;
-  share->table_cache_key.length= create_table_def_key(thd, key,
-                                                      &table_list, 1);
-  /*
-    Here we use the fact that table_name is stored as the second component
-    in the 'key' (after db_name), where components are separated with \0
-  */
-  share->table_name.str=    key+db_length+1;
-  share->table_name.length= table_length;
+  key_len= create_table_def_key(thd, key, &table_list, 1);
+  share->set_table_cache_key(key, key_len);
   DBUG_RETURN(0);
 }
 
@@ -1746,10 +1736,7 @@ bool reopen_name_locked_table(THD* thd, 
 {
   TABLE *table= table_list->table;
   TABLE_SHARE *share;
-  char *db= table_list->db;
   char *table_name= table_list->table_name;
-  char key[MAX_DBKEY_LENGTH];
-  uint key_length;
   TABLE orig_table;
   DBUG_ENTER("reopen_name_locked_table");
 
@@ -1759,7 +1746,6 @@ bool reopen_name_locked_table(THD* thd, 
     DBUG_RETURN(TRUE);
 
   orig_table= *table;
-  key_length=(uint) (strmov(strmov(key,db)+1,table_name)-key)+1;
 
   if (open_unireg_entry(thd, table, table_list, table_name,
                         table->s->table_cache_key.str,

--- 1.422/sql/sql_select.cc	2006-07-31 23:02:14 +04:00
+++ 1.423/sql/sql_select.cc	2006-07-31 23:02:14 +04:00
@@ -8542,8 +8542,6 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
   share->primary_key= MAX_KEY;               // Indicate no primary key
   share->keys_for_keyread.init();
   share->keys_in_use.init();
-  /* For easier error reporting */
-  share->table_cache_key= share->db;
 
   /* Calculate which type of fields we will store in the temporary table */
 

--- 1.232/sql/table.cc	2006-07-31 23:02:14 +04:00
+++ 1.233/sql/table.cc	2006-07-31 23:02:14 +04:00
@@ -105,15 +105,9 @@ TABLE_SHARE *alloc_table_share(TABLE_LIS
                                         path_length +1)))
   {
     bzero((char*) share, sizeof(*share));
-    share->table_cache_key.str=    (char*) (share+1);
-    share->table_cache_key.length= key_length;
-    memcpy(share->table_cache_key.str, key, key_length);
 
-    /* Use the fact the key is db/0/table_name/0 */
-    share->db.str=            share->table_cache_key.str;
-    share->db.length=         strlen(share->db.str);
-    share->table_name.str=    share->db.str + share->db.length + 1;
-    share->table_name.length= strlen(share->table_name.str);
+    memcpy((char*) (share+1), key, key_length);
+    share->set_table_cache_key((char*) (share+1), key_length);
 
     share->path.str= share->table_cache_key.str+ key_length;
     share->path.length= path_length;

--- 1.143/sql/table.h	2006-07-31 23:02:14 +04:00
+++ 1.144/sql/table.h	2006-07-31 23:02:14 +04:00
@@ -138,7 +138,16 @@ typedef struct st_table_share
   CHARSET_INFO *table_charset;		/* Default charset of string fields */
 
   MY_BITMAP all_set;
-  /* A pair "database_name\0table_name\0", widely used as simply a db name */
+  /*
+    Key which is used for looking-up table in table cache and in the list
+    of thread's temporary tables. Has the form of:
+      "database_name\0table_name\0" + optional part for temporary tables.
+
+    Note that all three 'table_cache_key', 'db' and 'table_name' members
+    must be set (and be non-zero) for tables in table cache. They also
+    should correspond to each other.
+    To ensure this one can use set_table_cache() method.
+  */
   LEX_STRING table_cache_key;
   LEX_STRING db;                        /* Pointer to db */
   LEX_STRING table_name;                /* Table name (for open) */
@@ -223,6 +232,38 @@ typedef struct st_table_share
   uint part_state_len;
   handlerton *default_part_db_type;
 #endif
+
+
+  /*
+    Set share's key for table cache and database and table names (taking
+    values for the latter from table cache key).
+
+    SYNOPSIS
+      set_table_cache_key()
+        key         Key for table cache
+        key_length  Key length
+
+    NOTE
+      This method should be used in most situations when one wants
+      add table for which table_cache_key is set to table cache
+      since it automatically ensures that TABLE_SHARE::table_name/db
+      have appropriate values.
+  */
+
+  void set_table_cache_key(char *key, uint key_length)
+  {
+    table_cache_key.str= key;
+    table_cache_key.length= key_length;
+    /*
+      Let us use the fact that the key is "db/0/table_name/0" + optional
+      part for temporary tables.
+    */
+    db.str=            table_cache_key.str;
+    db.length=         strlen(db.str);
+    table_name.str=    db.str + db.length + 1;
+    table_name.length= strlen(table_name.str);
+  }
+
 } TABLE_SHARE;
 
 

--- 1.30/mysql-test/r/drop.result	2006-07-31 23:02:14 +04:00
+++ 1.31/mysql-test/r/drop.result	2006-07-31 23:02:14 +04:00
@@ -74,3 +74,16 @@ show tables;
 Tables_in_test
 t1
 drop table t1;
+drop database if exists mysqltest;
+drop table if exists t1;
+create table t1 (i int);
+lock tables t1 read;
+create database mysqltest;
+ drop table t1;
+show open tables;
+ drop database mysqltest;
+select 1;
+1
+1
+unlock tables;
+End of 5.0 tests

--- 1.22/mysql-test/t/drop.test	2006-07-31 23:02:14 +04:00
+++ 1.23/mysql-test/t/drop.test	2006-07-31 23:02:14 +04:00
@@ -81,3 +81,44 @@ show tables;
 drop table t1;
 
 # End of 4.1 tests
+
+
+#
+# Test for bug#21216 "Simultaneous DROP TABLE and SHOW OPEN TABLES causes
+# server to crash". Crash (caused by failed assertion in 5.0 or by null
+# pointer dereference in 5.1) happened when one ran SHOW OPEN TABLES
+# while concurrently doing DROP TABLE (or RENAME TABLE, CREATE TABLE LIKE
+# or any other command that takes name-lock) in other connection.
+# 
+# Also includes test for similar bug#12212 "Crash that happens during
+# removing of database name from cache" reappeared in 5.1 as bug#19403
+# In its case crash happened when one concurrently executed DROP DATABASE
+# and one of name-locking command.
+# 
+--disable_warnings
+drop database if exists mysqltest;
+drop table if exists t1;
+--enable_warnings
+create table t1 (i int);
+lock tables t1 read;
+create database mysqltest;
+connect (addconroot1, localhost, root,,);
+--send drop table t1
+connect (addconroot2, localhost, root,,);
+# Server should not crash in any of the following statements
+--disable_result_log
+show open tables;
+--enable_result_log
+--send drop database mysqltest
+connection default;
+select 1;
+unlock tables;
+connection addconroot1;
+--reap
+connection addconroot2;
+--reap
+disconnect addconroot1;
+disconnect addconroot2;
+connection default;
+
+--echo End of 5.0 tests
Thread
bk commit into 5.1 tree (dlenev:1.2259) BUG#12212dlenev31 Jul