List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:March 31 2011 6:39am
Subject:bzr commit into mysql-trunk branch (Dmitry.Lenev:3534) Bug#11746602
View as plain text  
#At file:///home/dlenev/src/bzr/mysql-trunk-bug27480-2/ based on revid:dmitry.lenev@stripped

 3534 Dmitry Lenev	2011-03-31
      Yet another follow-up for the patch for Bug#11746602
      27480: Extend CREATE TEMPORARY TABLES privilege to
      allow temp table operations).
      
      This patch tries to eliminate performance overhead which
      was introduced by one of previous patches for this bug.
      
      One of those patches added pre-opening of temporary tables as
      a separate stage of statement execution, handled by a separate
      call to open_temporary_tables(). As result we started to
      construct table definition key twice per statement almost for
      each table to be open by statement.
      
      This patch addresses the issue by changing code to avoid
      construction of keys and borrowing pre-constructed key from
      TABLE_LIST::mdl_request::key instead, where it is possible.
     @ sql/sql_admin.cc
        Adjusted prepare_for_repair() code to use get_table_def_key()
        instead of create_table_def_key() to get table definition
        cache key (the former piggy-backs on pre-constructed
        TABLE_LIST::mdl_request::key instead of constructing new
        instance of key like the latter).
     @ sql/sql_base.cc
        - Introduced get_table_def_key() function which allows to
          get table definition cache key for the table list element
          cheaply, by piggy-backing on pre-constructed
          TABLE_LIST::mdl_request::key.
          Adjusted code to use this function instead of
          create_table_def_key(), which constructs new instance of
          key, where possible.
        - Made create_table_def_key() private to sql_base.cc as it
          is no longer used outside of this file. Also changed it to
          accept database and table name instead of table list element
          since now it is only used in cases when table list element
          is not easily available.
          Changed some code which specifically constructed temporary
          TABLE_LIST objects to call create_table_def_key() to not
          to do this.
        - Changed find_temporary_table(TABLE_LIST) to use key borrowed
          from TABLE_LIST::mdl_request::key instead of especially
          constructed key. This change allows us to save on key
          construction but adds a small overhead during key comparison.
          Such a trade-off should be acceptable as overhead during key
          comparison is only a few instructions.
        - Signatures of get_table_share/get_table_share_with_discover()
          and tdc_open_view() were changed to accept constant table
          definition key to allow them to be used in cases when keys
          were borrowed from MDL subsystem.
     @ sql/sql_base.h
        - Introduced get_table_def_key() function which allows to
          get table definition cache key for the table list element
          cheaply, by piggy-backing on pre-constructed
          TABLE_LIST::mdl_request::key.
          key, where possible.
        - Made create_table_def_key() private to sql_base.cc as it
          is no longer used outside of this file.
        - Signatures of get_table_share/get_table_share_with_discover()
          and tdc_open_view() were changed to accept constant table
          definition key to allow them to be used in cases when keys
          were borrowed from MDL subsystem.
     @ sql/sql_show.cc
        Adjusted fill_schema_table_from_frm() code to use
        get_table_def_key() instead of create_table_def_key() to get
        table definition cache key (the former piggy-backs on
        pre-constructed TABLE_LIST::mdl_request::key instead of
        constructing new instance of key like the latter).
     @ sql/sql_table.cc
        Changed two places in mysql_alter_table() code where we tried
        to open tables using table list element objects which were not
        fully/correctly initialized and which, therefore, were impossible
        to use to get table definition cache key from them.
     @ sql/sql_view.cc
        Adjusted fill_defined_view_parts() code to use
        get_table_def_key() instead of create_table_def_key() to
        get table definition cache key (the former piggy-backs on
        pre-constructed TABLE_LIST::mdl_request::key instead of
        constructing new instance of key like the latter).
     @ sql/table.cc
        Changed signature of alloc_table_share() to accept constant
        table definition key to allow it to be used in cases when
        key is borrowed from MDL subsystem.
     @ sql/table.h
        Changed signature of alloc_table_share() to accept constant
        table definition key to allow it to be used in cases when
        key is borrowed from MDL subsystem.

    modified:
      sql/sql_admin.cc
      sql/sql_base.cc
      sql/sql_base.h
      sql/sql_show.cc
      sql/sql_table.cc
      sql/sql_view.cc
      sql/table.cc
      sql/table.h
=== modified file 'sql/sql_admin.cc'
--- a/sql/sql_admin.cc	2011-03-09 12:24:36 +0000
+++ b/sql/sql_admin.cc	2011-03-31 06:39:38 +0000
@@ -65,7 +65,7 @@ static int prepare_for_repair(THD *thd, 
 
   if (!(table= table_list->table))
   {
-    char key[MAX_DBKEY_LENGTH];
+    const char *key;
     uint key_length;
     /*
       If the table didn't exist, we have a shared metadata lock
@@ -81,7 +81,6 @@ static int prepare_for_repair(THD *thd, 
     */
     my_hash_value_type hash_value;
 
-    key_length= create_table_def_key(thd, key, table_list, 0);
     table_list->mdl_request.init(MDL_key::TABLE,
                                  table_list->db, table_list->table_name,
                                  MDL_EXCLUSIVE, MDL_TRANSACTION);
@@ -91,6 +90,8 @@ static int prepare_for_repair(THD *thd, 
       DBUG_RETURN(0);
     has_mdl_lock= TRUE;
 
+    key_length= get_table_def_key(table_list, &key);
+
     hash_value= my_calc_hash(&table_def_cache, (uchar*) key, key_length);
     mysql_mutex_lock(&LOCK_open);
     share= get_table_share(thd, table_list, key, key_length, 0,

=== modified file 'sql/sql_base.cc'
--- a/sql/sql_base.cc	2011-03-26 10:56:27 +0000
+++ b/sql/sql_base.cc	2011-03-31 06:39:38 +0000
@@ -222,17 +222,16 @@ static void check_unused(void)
 #endif
 
 
-/*
+/**
   Create a table cache key
 
-  SYNOPSIS
-    create_table_def_key()
-    thd			Thread handler
-    key			Create key here (must be of size MAX_DBKEY_LENGTH)
-    table_list		Table definition
-    tmp_table		Set if table is a tmp table
+  @param thd        Thread context
+  @param key        Create key here (must be of size MAX_DBKEY_LENGTH)
+  @param db         Database name.
+  @param table_name Table name.
+  @param tmp_table  Set if table is a tmp table.
 
- IMPLEMENTATION
+  @note
     The table cache_key is created from:
     db_name + \0
     table_name + \0
@@ -243,16 +242,14 @@ static void check_unused(void)
     4 bytes for master thread id
     4 bytes pseudo thread id
 
-  RETURN
-    Length of key
+  @return Length of key.
 */
 
-uint create_table_def_key(THD *thd, char *key,
-                          const TABLE_LIST *table_list,
-                          bool tmp_table)
+static uint create_table_def_key(THD *thd, char *key,
+                                 const char *db, const char *table_name,
+                                 bool tmp_table)
 {
-  uint key_length= (uint) (strmov(strmov(key, table_list->db)+1,
-                                  table_list->table_name)-key)+1;
+  uint key_length= (uint) (strmov(strmov(key, db)+1, table_name)-key)+1;
   if (tmp_table)
   {
     int4store(key + key_length, thd->server_id);
@@ -263,6 +260,41 @@ uint create_table_def_key(THD *thd, char
 }
 
 
+/**
+  Get table cache key for a table list element.
+
+  @param table_list[in]  Table list element.
+  @param key[out]        On return points to table cache key for the table.
+
+  @note Unlike create_table_def_key() call this function doesn't construct
+        key in a buffer provider by caller. Instead it relies on the fact
+        that table list element for which key is requested has properly
+        initialized MDL_request object and the fact that table defition
+        cache key is suffix of key used in MDL subsystem. So to get table
+        definition key it simply needs to return pointer to appropriate
+        part of MDL_key object nested in this table list element.
+        Indeed, this means that lifetime of key produced by this call is
+        limited by the lifetime of table list element which it got as
+        parameter.
+
+  @return Length of key.
+*/
+
+uint get_table_def_key(const TABLE_LIST *table_list, const char **key)
+{
+  /*
+    This call relies on the fact that TABLE_LIST::mdl_request::key object
+    is properly initialized, so table definition cache can be produced
+    from key used by MDL subsystem.
+  */
+  DBUG_ASSERT(!strcmp(table_list->db, table_list->mdl_request.key.db_name()) &&
+              !strcmp(table_list->table_name, table_list->mdl_request.key.name()));
+
+  *key= (const char*)table_list->mdl_request.key.ptr() + 1;
+  return table_list->mdl_request.key.length() - 1;
+}
+
+
 
 /*****************************************************************************
   Functions to handle table definition cach (TABLE_SHARE)
@@ -494,8 +526,9 @@ static void table_def_unuse_table(TABLE 
    #  Share for table
 */
 
-TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list, char *key,
-                             uint key_length, uint db_flags, int *error,
+TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list,
+                             const char *key, uint key_length,
+                             uint db_flags, int *error,
                              my_hash_value_type hash_value)
 {
   TABLE_SHARE *share;
@@ -610,7 +643,7 @@ found:
 
 static TABLE_SHARE *
 get_table_share_with_discover(THD *thd, TABLE_LIST *table_list,
-                              char *key, uint key_length,
+                              const char *key, uint key_length,
                               uint db_flags, int *error,
                               my_hash_value_type hash_value)
 
@@ -755,14 +788,11 @@ void release_table_share(TABLE_SHARE *sh
 
 TABLE_SHARE *get_cached_table_share(const char *db, const char *table_name)
 {
-  char key[NAME_LEN*2+2];
-  TABLE_LIST table_list;
+  char key[MAX_DBKEY_LENGTH];
   uint key_length;
   mysql_mutex_assert_owner(&LOCK_open);
 
-  table_list.db= (char*) db;
-  table_list.table_name= (char*) table_name;
-  key_length= create_table_def_key((THD*) 0, key, &table_list, 0);
+  key_length= create_table_def_key((THD*) 0, key, db, table_name, 0);
   return (TABLE_SHARE*) my_hash_search(&table_def_cache,
                                        (uchar*) key, key_length);
 }  
@@ -1990,12 +2020,9 @@ void update_non_unique_table_error(TABLE
 
 TABLE *find_temporary_table(THD *thd, const char *db, const char *table_name)
 {
-  TABLE_LIST tl;
-
-  tl.db= (char*) db;
-  tl.table_name= (char*) table_name;
-
-  return find_temporary_table(thd, &tl);
+  char key[MAX_DBKEY_LENGTH];
+  uint key_length= create_table_def_key(thd, key, db, table_name, 1);
+  return find_temporary_table(thd, key, key_length);
 }
 
 
@@ -2008,10 +2035,25 @@ TABLE *find_temporary_table(THD *thd, co
 
 TABLE *find_temporary_table(THD *thd, const TABLE_LIST *tl)
 {
-  char key[MAX_DBKEY_LENGTH];
-  uint key_length= create_table_def_key(thd, key, tl, 1);
+  const char *key;
+  uint key_length;
+  char key_suffix[TMP_TABLE_KEY_EXTRA];
+  TABLE *table;
 
-  return find_temporary_table(thd, key, key_length);
+  key_length= get_table_def_key(tl, &key);
+
+  int4store(key_suffix, thd->server_id);
+  int4store(key_suffix + 4, thd->variables.pseudo_thread_id);
+
+  for (table= thd->temporary_tables; table; table= table->next)
+  {
+    if ((table->s->table_cache_key.length == key_length+TMP_TABLE_KEY_EXTRA) &&
+        !memcmp(table->s->table_cache_key.str, key, key_length) &&
+        !memcmp(table->s->table_cache_key.str + key_length, key_suffix,
+                TMP_TABLE_KEY_EXTRA))
+      return table;
+  }
+  return NULL;
 }
 
 
@@ -2186,15 +2228,12 @@ bool rename_temporary_table(THD* thd, TA
   char *key;
   uint key_length;
   TABLE_SHARE *share= table->s;
-  TABLE_LIST table_list;
   DBUG_ENTER("rename_temporary_table");
 
   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;
-  key_length= create_table_def_key(thd, key, &table_list, 1);
+  key_length= create_table_def_key(thd, key, db, table_name, 1);
   share->set_table_cache_key(key, key_length);
   DBUG_RETURN(0);
 }
@@ -2600,8 +2639,8 @@ bool open_table(THD *thd, TABLE_LIST *ta
                 Open_table_context *ot_ctx)
 {
   reg1	TABLE *table;
-  char	key[MAX_DBKEY_LENGTH];
-  uint	key_length;
+  const char *key;
+  uint key_length;
   char	*alias= table_list->alias;
   uint flags= ot_ctx->get_flags();
   MDL_ticket *mdl_ticket;
@@ -2625,7 +2664,7 @@ bool open_table(THD *thd, TABLE_LIST *ta
   if (thd->killed)
     DBUG_RETURN(TRUE);
 
-  key_length= create_table_def_key(thd, key, table_list, 0);
+  key_length= get_table_def_key(table_list, &key);
 
   /*
     If we're in pre-locked or LOCK TABLES mode, let's try to find the
@@ -3664,7 +3703,7 @@ check_and_update_routine_version(THD *th
 */
 
 bool tdc_open_view(THD *thd, TABLE_LIST *table_list, const char *alias,
-                   char *cache_key, uint cache_key_length,
+                   const char *cache_key, uint cache_key_length,
                    MEM_ROOT *mem_root, uint flags)
 {
   TABLE not_used;
@@ -3765,15 +3804,15 @@ static bool open_table_entry_fini(THD *t
 
 static bool auto_repair_table(THD *thd, TABLE_LIST *table_list)
 {
-  char	cache_key[MAX_DBKEY_LENGTH];
-  uint	cache_key_length;
+  const char *cache_key;
+  uint cache_key_length;
   TABLE_SHARE *share;
   TABLE *entry;
   int not_used;
   bool result= TRUE;
   my_hash_value_type hash_value;
 
-  cache_key_length= create_table_def_key(thd, cache_key, table_list, 0);
+  cache_key_length= get_table_def_key(table_list, &cache_key);
 
   thd->clear_error();
 
@@ -5883,7 +5922,6 @@ TABLE *open_table_uncached(THD *thd, con
   TABLE_SHARE *share;
   char cache_key[MAX_DBKEY_LENGTH], *saved_cache_key, *tmp_path;
   uint key_length;
-  TABLE_LIST table_list;
   DBUG_ENTER("open_table_uncached");
   DBUG_PRINT("enter",
              ("table: '%s'.'%s'  path: '%s'  server_id: %u  "
@@ -5891,10 +5929,8 @@ TABLE *open_table_uncached(THD *thd, con
               db, table_name, path,
               (uint) thd->server_id, (ulong) thd->variables.pseudo_thread_id));
 
-  table_list.db=         (char*) db;
-  table_list.table_name= (char*) table_name;
   /* Create the cache_key for temporary tables */
-  key_length= create_table_def_key(thd, cache_key, &table_list, 1);
+  key_length= create_table_def_key(thd, cache_key, db, table_name, 1);
 
   if (!(tmp_table= (TABLE*) my_malloc(sizeof(*tmp_table) + sizeof(*share) +
                                       strlen(path)+1 + key_length,

=== modified file 'sql/sql_base.h'
--- a/sql/sql_base.h	2011-03-26 10:56:27 +0000
+++ b/sql/sql_base.h	2011-03-31 06:39:38 +0000
@@ -78,11 +78,10 @@ void table_def_start_shutdown(void);
 void assign_new_table_id(TABLE_SHARE *share);
 uint cached_open_tables(void);
 uint cached_table_definitions(void);
-uint create_table_def_key(THD *thd, char *key,
-                          const TABLE_LIST *table_list,
-                          bool tmp_table);
-TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list, char *key,
-                             uint key_length, uint db_flags, int *error,
+uint get_table_def_key(const TABLE_LIST *table_list, const char **key);
+TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list,
+                             const char *key, uint key_length,
+                             uint db_flags, int *error,
                              my_hash_value_type hash_value);
 void release_table_share(TABLE_SHARE *share);
 TABLE_SHARE *get_cached_table_share(const char *db, const char *table_name);
@@ -294,7 +293,7 @@ void tdc_remove_table(THD *thd, enum_tdc
                       const char *db, const char *table_name,
                       bool has_lock);
 bool tdc_open_view(THD *thd, TABLE_LIST *table_list, const char *alias,
-                   char *cache_key, uint cache_key_length,
+                   const char *cache_key, uint cache_key_length,
                    MEM_ROOT *mem_root, uint flags);
 void tdc_flush_unused_tables();
 TABLE *find_table_for_mdl_upgrade(THD *thd, const char *db,

=== modified file 'sql/sql_show.cc'
--- a/sql/sql_show.cc	2011-03-09 12:24:36 +0000
+++ b/sql/sql_show.cc	2011-03-31 06:39:38 +0000
@@ -3272,7 +3272,7 @@ static int fill_schema_table_from_frm(TH
   uint res= 0;
   int not_used;
   my_hash_value_type hash_value;
-  char key[MAX_DBKEY_LENGTH];
+  const char *key;
   uint key_length;
   char db_name_buff[NAME_LEN + 1], table_name_buff[NAME_LEN + 1];
 
@@ -3345,7 +3345,7 @@ static int fill_schema_table_from_frm(TH
     goto end;
   }
 
-  key_length= create_table_def_key(thd, key, &table_list, 0);
+  key_length= get_table_def_key(&table_list, &key);
   hash_value= my_calc_hash(&table_def_cache, (uchar*) key, key_length);
   mysql_mutex_lock(&LOCK_open);
   share= get_table_share(thd, &table_list, key,

=== modified file 'sql/sql_table.cc'
--- a/sql/sql_table.cc	2011-03-26 10:56:27 +0000
+++ b/sql/sql_table.cc	2011-03-31 06:39:38 +0000
@@ -6592,9 +6592,9 @@ bool mysql_alter_table(THD *thd,char *ne
     if (table->s->tmp_table)
     {
       TABLE_LIST tbl;
-      bzero((void*) &tbl, sizeof(tbl));
-      tbl.db= new_db;
-      tbl.table_name= tbl.alias= tmp_name;
+      tbl.init_one_table(new_db, strlen(new_db),
+                         tmp_name, strlen(tmp_name),
+                         tmp_name, TL_READ_NO_INSERT);
       /* Table is in thd->temporary_tables */
       (void) open_temporary_table(thd, &tbl);
       new_table= tbl.table;
@@ -6890,15 +6890,15 @@ bool mysql_alter_table(THD *thd,char *ne
       NO need to tamper with MERGE tables. The real open is done later.
     */
     Open_table_context ot_ctx(thd, MYSQL_OPEN_REOPEN);
-    TABLE *t_table;
+    TABLE_LIST temp_table_list;
+    TABLE_LIST *t_table_list;
     if (new_name != table_name || new_db != db)
     {
-      table_list->alias= new_name;
-      table_list->table_name= new_name;
-      table_list->table_name_length= strlen(new_name);
-      table_list->db= new_db;
-      table_list->db_length= strlen(new_db);
-      table_list->mdl_request.ticket= target_mdl_request.ticket;
+      temp_table_list.init_one_table(new_db, strlen(new_db),
+                                     new_name, strlen(new_name),
+                                     new_name, TL_READ_NO_INSERT);
+      temp_table_list.mdl_request.ticket= target_mdl_request.ticket;
+      t_table_list= &temp_table_list;
     }
     else
     {
@@ -6908,20 +6908,21 @@ bool mysql_alter_table(THD *thd,char *ne
         to request the lock.
       */
       table_list->mdl_request.ticket= mdl_ticket;
+      t_table_list= table_list;
     }
-    if (open_table(thd, table_list, thd->mem_root, &ot_ctx))
+    if (open_table(thd, t_table_list, thd->mem_root, &ot_ctx))
     {
       goto err_with_mdl;
     }
-    t_table= table_list->table;
 
     /* Tell the handler that a new frm file is in place. */
-    error= t_table->file->ha_create_handler_files(path, NULL, CHF_INDEX_FLAG,
-                                               create_info);
+    error= t_table_list->table->file->ha_create_handler_files(path, NULL,
+                                                              CHF_INDEX_FLAG,
+                                                              create_info);
 
-    DBUG_ASSERT(thd->open_tables == t_table);
+    DBUG_ASSERT(thd->open_tables == t_table_list->table);
     close_thread_table(thd, &thd->open_tables);
-    table_list->table= 0;
+    t_table_list->table= 0;
 
     if (error)
       goto err_with_mdl;

=== modified file 'sql/sql_view.cc'
--- a/sql/sql_view.cc	2011-03-09 12:24:36 +0000
+++ b/sql/sql_view.cc	2011-03-31 06:39:38 +0000
@@ -209,13 +209,14 @@ static void make_valid_column_names(List
 static bool
 fill_defined_view_parts (THD *thd, TABLE_LIST *view)
 {
-  char key[MAX_DBKEY_LENGTH];
+  const char *key;
   uint key_length;
   LEX *lex= thd->lex;
   TABLE_LIST decoy;
 
   memcpy (&decoy, view, sizeof (TABLE_LIST));
-  key_length= create_table_def_key(thd, key, view, 0);
+
+  key_length= get_table_def_key(view, &key);
 
   if (tdc_open_view(thd, &decoy, decoy.alias, key, key_length,
                     thd->mem_root, OPEN_VIEW_NO_PARSE))

=== modified file 'sql/table.cc'
--- a/sql/table.cc	2011-03-01 14:47:01 +0000
+++ b/sql/table.cc	2011-03-31 06:39:38 +0000
@@ -311,7 +311,7 @@ TABLE_CATEGORY get_table_category(const 
     #  Share
 */
 
-TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, char *key,
+TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, const char *key,
                                uint key_length)
 {
   MEM_ROOT mem_root;

=== modified file 'sql/table.h'
--- a/sql/table.h	2011-02-02 22:02:29 +0000
+++ b/sql/table.h	2011-03-31 06:39:38 +0000
@@ -2150,7 +2150,7 @@ void init_mdl_requests(TABLE_LIST *table
 int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias,
                           uint db_stat, uint prgflag, uint ha_open_flags,
                           TABLE *outparam, bool is_create_table);
-TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, char *key,
+TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, const char *key,
                                uint key_length);
 void init_tmp_table_share(THD *thd, TABLE_SHARE *share, const char *key,
                           uint key_length,


Attachment: [text/bzr-bundle] bzr/dmitry.lenev@oracle.com-20110331063938-bn2ypz2aumfq5aac.bundle
Thread
bzr commit into mysql-trunk branch (Dmitry.Lenev:3534) Bug#11746602Dmitry Lenev31 Mar
  • Re: bzr commit into mysql-trunk branch (Dmitry.Lenev:3534) Bug#11746602Alexander Nozdrin1 Apr