List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 16 2010 8:22am
Subject:Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3137)
Bug#56595
View as plain text  
Hello Jon Olav!

* Jon Olav Hauglid <jon.hauglid@stripped> [10/09/15 13:17]:
> #At file:///export/home/x/mysql-5.5-runtime-bug56595/ based on
> revid:jon.hauglid@stripped
> 
>  3137 Jon Olav Hauglid	2010-09-15
>       Bug #56595 RENAME TABLE causes assert on OS X
>       
>       The problem was that RENAME TABLE caused an assert if the system variable
>       lower_case_table_names was 2 (default on Mac OS X) and the old table name
>       was given in upper case. This caused lowercase_table2.test to fail.
>       
>       The assert checks that an exclusive metadata lock is held by the connection
>       trying to do RENAME TABLE - specificially during updates of table triggers.
>       The assert was triggered since the check is case sensitive and the lock
>       was held on the normalized (lower case) version of the table name.
>       
>       This patch fixes the problem by making sure a normalized version of the
>       table name is used for the metadata lock check, while using a non-normalized
>       version of the table name for the rename of trigger files. The same is done
>       for ALTER TABLE ... RENAME.
>       
>       No test case added since this problem is already covered by 
>       lowercase_table2.test
> 

...

> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc	2010-09-03 07:42:51 +0000
> +++ b/sql/sql_table.cc	2010-09-15 09:08:18 +0000
> @@ -6555,8 +6556,8 @@ bool mysql_alter_table(THD *thd,char *ne
>             (need_copy_table != ALTER_TABLE_METADATA_ONLY ||
>              mysql_rename_table(save_old_db_type, db, table_name, new_db,
>                                 new_alias, NO_FRM_RENAME)) &&
> -           Table_triggers_list::change_table_name(thd, db, table_name,
> -                                                  new_db, new_alias)))
> +            Table_triggers_list::change_table_name(thd, db, alias, table_name,
> +                                                   new_db, new_alias)))

I suspect that alignment of the above code is wrong.

Please check.

>    {
>      /* Try to get everything back. */
>      error=1;
> 
> === modified file 'sql/sql_trigger.cc'
> --- a/sql/sql_trigger.cc	2010-08-31 09:52:56 +0000
> +++ b/sql/sql_trigger.cc	2010-09-15 09:08:18 +0000
> @@ -1874,6 +1874,7 @@ Table_triggers_list::change_table_name_i
>  
>    @param[in,out] thd Thread context
>    @param[in] db Old database of subject table
> +  @param[in] old_alias Old alias of subject table
>    @param[in] old_table Old name of subject table
>    @param[in] new_db New database for subject table
>    @param[in] new_table New name of subject table
> @@ -1890,6 +1891,7 @@ Table_triggers_list::change_table_name_i
>  */
>  
>  bool Table_triggers_list::change_table_name(THD *thd, const char *db,
> +                                            const char *old_alias,
>                                              const char *old_table,
>                                              const char *new_db,
>                                              const char *new_table)
> @@ -1910,8 +1912,11 @@ bool Table_triggers_list::change_table_n
>    DBUG_ASSERT(thd->mdl_context.is_lock_owner(MDL_key::TABLE, db, old_table,
>                                               MDL_EXCLUSIVE));
>  
> -  DBUG_ASSERT(my_strcasecmp(table_alias_charset, db, new_db) ||
> -              my_strcasecmp(table_alias_charset, old_table, new_table));
> +  DBUG_ASSERT((lower_case_table_names &&
> +               (my_strcasecmp(table_alias_charset, db, new_db) ||
> +                my_strcasecmp(table_alias_charset, old_table, new_table))) ||
> +              (!lower_case_table_names &&
> +               (strcmp(db, new_db) || strcmp(old_alias, new_table))));

Sorry. It seems that I have misled you with this assert. When
lower_case_table_name is 0 table_alias_charset points to
my_charset_bin and thus in the original assert case-sensitive
comparison is performed. So original assert is correct.

So I think you should revert this change.

Also probably it makes sense to add test case which will do
"RENAME TABLE t1 TO T1" to lowercase_fs_off.test.
What do you think?

...

Otherwise I am OK with this patch and think that it can be
pushed after addressing/considering above points.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.5-runtime branch (jon.hauglid:3137) Bug#56595Jon Olav Hauglid15 Sep
  • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3137)Bug#56595Dmitry Lenev16 Sep
    • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3137) Bug#56595Jon Olav Hauglid16 Sep