Re: bzr commit into mysql-5.1-bugteam branch (ramil:2765) Bug#33094

[Dmitry Lenev]

Hello Ramil!

Sorry for delay with review, but unlike you expected it required
more than 2-3 minutes:

* Ramil Kalimullin <ramil@stripped> [08/12/19 13:56]:
> #At file:///home/ram/mysql/b33094-5.1-bugteam/ based on
> revid:timothy.smith@stripped
> 
>  2765 Ramil Kalimullin	2008-12-19
>       Fix for
>       bug#33094: Error in upgrading from 5.0 to 5.1 when table contains
>       triggers
>       and
>       #41385: Crash when attempting to repair a #mysql50# upgraded table
>       #with
>       triggers.
>       
>       Problem:
>       1. trigger code didn't assume a table name may have
>       a "#mysql50#" prefix, that may lead to a failing ASSERT().

Actually original problem which is described in bug #33094, i.e. failure to
fix database names due to "Trigger in wrong schema" errors while running
"mysqlcheck --check-upgrade --fix-db-names --fix-table-names --all-databases",
is caused by the fact that trigger code doesn't allow moving tables between
different databases. "ALTER DATABASE `#mysql50#somename` UPGRADE DATA
DIRECTORY NAME" which is invoked by this command internally tries to move
tables between `#mysql50#somename` and `somename` databases which fails
for tables with triggers due to the following code in Table_triggers_list::
change_table_name() method:

    /*
      Since triggers should be in the same schema as their subject tables
      moving table with them between two schemas raises too many questions.
      (E.g. what should happen if in new schema we already have trigger
       with same name ?).
    */
    if (my_strcasecmp(table_alias_charset, db, new_db))
    {
      my_error(ER_TRG_IN_WRONG_SCHEMA, MYF(0));
      result= 1;
      goto end;
    }

Since your fix doesn't change the above code it, unfortunately, fails
to solve problem originally reported by user... Please adjust the above
code to allow renaming between '#mysql50#somename' and 'somename'
databases in order to fix this problem and add appropriate test coverage.
(Actually the former might require more changes as I am not sure how .TRN
files will be handled by current code for such rename).

>       2. mysqlcheck --fix-table-name doesn't use UTF8 as a default
>       character set that results in (parsing) errors for tables with
>       non-latin symbols in their names.

I think it is better to clarify here that those parsing errors occur
while parsing definitions of triggers when server tries to open table
with triggers from 5.0 and assumes that it is ok to use default
character set as charset in which trigger's create statement is stored
(BTW 5.1 stores this charset along with trigger definition in .TRG file).

>       Fix:
>       1. properly handle table names with "#mysql50#" prefix.
>       2. handle --default-character-set mysqlcheck option;
>       if mysqlcheck is launched with --fix-table-name or --fix-db-name
>       set default character set to UTF8 if no --default-character-set 
>       option given.
>       
>       Note: if given --fix-table-name or --fix-db-name option,
>       without --default-character-set mysqlcheck option
>       default character set is UTF8.
>       
>       There's no test case from the bug report as it may fail on
>       some filesystems.

But at least you can have coverage for problems with renaming tables and
moving tables between databases by using table and database with safe from
filesystem POV but nevertheless encoded characters such as '@' and '-'.

...

> === modified file 'mysql-test/r/mysqlcheck.result'
> --- a/mysql-test/r/mysqlcheck.result	2008-11-14 09:48:01 +0000
> +++ b/mysql-test/r/mysqlcheck.result	2008-12-19 10:45:19 +0000
> @@ -130,3 +130,27 @@ v1
>  v-1
>  drop view v1, `v-1`;
>  drop table t1;
> +SET NAMES utf8;
> +CREATE TABLE `#mysql50#@` (a INT);
> +SHOW TABLES;
> +Tables_in_test
> +#mysql50#@
> +SET NAMES DEFAULT;
> +mysqlcheck --fix-table-names --databases test
> +SET NAMES utf8;
> +SHOW TABLES;
> +Tables_in_test
> +@
> +DROP TABLE `@`;
> +CREATE TABLE `я` (a INT);
> +SET NAMES DEFAULT;
> +mysqlcheck --default-character-set="latin1" --databases test
> +test.?
> +Error    : Table 'test.?' doesn't exist
> +error    : Corrupt
> +mysqlcheck --default-character-set="utf8" --databases test
> +test.я                                            OK
> +SET NAMES utf8;
> +DROP TABLE `я`;
> +SET NAMES DEFAULT;
> +End of 5.1 tests
> 
> === modified file 'mysql-test/t/mysqlcheck.test'
> --- a/mysql-test/t/mysqlcheck.test	2008-11-14 09:48:01 +0000> +++
> b/mysql-test/t/mysqlcheck.test	2008-12-19 10:45:19 +0000
> @@ -112,3 +112,30 @@ show tables;
>  show tables;
>  drop view v1, `v-1`;
>  drop table t1;
> +
> +#
> +# Bug #33094: Error in upgrading from 5.0 to 5.1 when table contains triggers
> +# Bug #41385: Crash when attempting to repair a #mysql50# upgraded table with
> +#             triggers
> +#
> +SET NAMES utf8;
> +CREATE TABLE `#mysql50#@` (a INT);
> +SHOW TABLES;
> +SET NAMES DEFAULT;
> +--echo mysqlcheck --fix-table-names --databases test
> +--exec $MYSQL_CHECK --fix-table-names --databases test
> +SET NAMES utf8;
> +SHOW TABLES;
> +DROP TABLE `@`;

OK. But why you don't add trigger here ?

Even if you can't create trigger on `#mysql50#@` table you still can store
.TRG and .TRN files from 5.0 in mysql_test/std_data sub-directory and creating
this trigger by copying this files using something like:

--copy_file --copy_file $MYSQL_TEST_DIR/std_data/@.TRG
$MYSQLTEST_VARDIR/master-data/test/@.TRG

(You will also have to add those files to section which handles std_data in
mysql-test/Makefile.am).

Also please add test coverage for case when we upgrading database with tricky
name (e.g. `a-b-c`). From my first comment is fairly clear that having such
test case is important for this patch. You can create such database by using
--mkdir command (see example in upgrade.test).

> +
> +CREATE TABLE `я` (a INT);
> +SET NAMES DEFAULT;
> +--echo mysqlcheck --default-character-set="latin1" --databases test
> +--exec $MYSQL_CHECK --default-character-set="latin1" --databases test
> +--echo mysqlcheck --default-character-set="utf8" --databases test
> +--exec $MYSQL_CHECK --default-character-set="utf8" --databases test
> +SET NAMES utf8;
> +DROP TABLE `я`;
> +SET NAMES DEFAULT;
> +
> +--echo End of 5.1 tests
> 
> === modified file 'sql/sql_trigger.cc'
> --- a/sql/sql_trigger.cc	2008-11-14 17:37:27 +0000
> +++ b/sql/sql_trigger.cc	2008-12-19 10:45:19 +0000
> @@ -1368,15 +1368,23 @@ bool Table_triggers_list::check_n_load(T
>  
>          if (triggers->on_table_names_list.push_back(on_table_name,
> &table->mem_root))
>            goto err_with_lex_cleanup;
> -
> +#ifndef DBUG_OFF
>          /*
>            Let us check that we correctly update trigger definitions when we
>            rename tables with triggers.
> +          
> +          There's a special case when given a table name with "#mysql50#" prefix
> +          (e.g. processing "RENAME TABLE #mysql50#somename TO somename"),
> +          we have to take it away, a tablename_to_filename() call does the trick.
>          */

How about something like:

In special cases like "RENAME TABLE `#mysql50#somename` TO `somename`"
or "ALTER DATABASE `#mysql50#somename` UPGRADE DATA DIRECTORY NAME"
we might be given table or database name with "#mysql50#" prefix (and
trigger's definiton contains un-prefixed version of the same name).
To remove this prefix we use tablename_to_filename().

?


> -        DBUG_ASSERT(!my_strcasecmp(table_alias_charset, lex.query_tables->db, db)
> &&
> -                    !my_strcasecmp(table_alias_charset,
> lex.query_tables->table_name,
> -                                   table_name));
>  
> +        char fname[NAME_LEN + 1];
> +        DBUG_ASSERT(!my_strcasecmp(table_alias_charset, lex.query_tables->db, db)
> &&
> +                    (!my_strcasecmp(table_alias_charset,
> lex.query_tables->table_name,
> +                                    table_name) ||
> +                     (tablename_to_filename(table_name, fname, sizeof(fname))
> &&
> +                      !my_strcasecmp(table_alias_charset,
> lex.query_tables->table_name, fname))));
> +#endif
>          if (names_only)
>          {
>            lex_end(&lex);
>

As it is obvious from the above comment I think that you need to extend
this assertion to support situation in which it is database name what
is prefixed.


I think the above issues should be fixed/discussed before pushing this
patch. Ideally I would like to take a look at new version of your patch
before you will push it.

Best regards,
-- 
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification