From: Alexander Nozdrin
Date: April 1 2011 5:01pm
Subject: Re: bzr commit into mysql-trunk branch (Dmitry.Lenev:3534) Bug#11746602
List-Archive: http://lists.mysql.com/commits/134488
Message-Id: <4D9604D5.2050209@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Hi Dmitry,

thank you for the patch!

Generally, the patch is Ok to push, but please consider
a few minor notes inline.

On 03/31/11 10:39, Dmitry Lenev wrote:
> === 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);
> +

I guess, our coding style does not require putting declaration
of variables in the beginning of the block.

Please consider moving declarations of 'key' and 'key_length' variables
right before their initialization/use.

> === 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 [out] key Placeholder for the created key
                 (must be of size MAX_DBKEY_LENGTH)
?

> +  @param db         Database name.

db_name (to be consistent with table_name) ?

> +  @param table_name Table name.
> +  @param tmp_table  Set if table is a 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;

Please put spaces before and after operations '+', '-'.

> +/**
> +  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

s/defition/definition/

> @@ -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)&&

Missing spaces around '+'.


> === 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);

Please consider moving declarations right before use.

> === 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
> @@ -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))

s/,&/, &/

> -    table_list->table= 0;
> +    t_table_list->table= 0;

may be NULL instead of 0 ?