List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:March 3 2011 9:26am
Subject:Re: bzr commit into mysql-5.5 branch (jon.hauglid:3364)
Bug#11755431
View as plain text  
Hello Jon Olav!

Here are my comments about your patch:

* Jon Olav Hauglid <jon.hauglid@stripped> [11/03/02 20:56]:
> #At file:///export/home/x/mysql-5.5-bug11755431/ based on
> revid:vinay.fisrekar@stripped
> 
>  3364 Jon Olav Hauglid	2011-03-02
>       Bug #11755431 (former 47205)
>       MAP 'REPAIR TABLE' TO RECREATE +ANALYZE FOR ENGINES NOT
>       SUPPORTING NATIVE REPAIR
>       
>       Executing 'mysqlcheck --check-upgrade --auto-repair ...' will first issue
>       'CHECK TABLE FOR UPGRADE' for all tables in the database in order to check if
> the
>       tables are compatible with the current version of MySQL. Any tables that are
>       found incompatible are then upgraded using 'REPAIR TABLE'.
>       
>       The problem was that some engines (e.g. InnoDB) does not support 'REPAIR
> TABLE'.
>       This caused any such tables to be left incompatible. As a result such tables
> were
>       not properly fixed by the mysql_upgrade tool.
>       
>       This patch fixes the problem by first changing 'CHECK TABLE FOR UPGRADE' to
> return
>       a different error message if the engine does not support REPAIR. Instead of
>       "Table upgrade required. Please do "REPAIR TABLE ..." it will report
>       "Table rebuild required. Please do "ALTER TABLE ... FORCE ..."
>       
>       Second, the patch changes mysqlcheck to do 'ALTER TABLE ... FORCE' instead of
>       'REPAIR TABLE' in these cases.
>       
>       Test case added to mysqlcheck.test

IMO it makes sense to note in ChangeSet comment that after this
patch ALTER TABLE FORCE has started to really rebuild table.

Probably we should also ask Documentation Team to add FORCE to
the list of supported ALTER TABLE variants.

> === modified file 'client/mysqlcheck.c'
> --- a/client/mysqlcheck.c	2011-01-16 03:59:05 +0000
> +++ b/client/mysqlcheck.c	2011-03-02 17:06:19 +0000

...

> @@ -626,6 +626,28 @@ static int fix_database_storage_name(con
>    return rc;
>  }
>  
> +static int rebuild_table(char *name)
> +{
> +  char *query, *ptr;
> +  int rc= 0;
> +  if (!(query =(char *) my_malloc((sizeof(char)*(fixed_name_length(name)+20)),
> +                                  MYF(MY_WME))))
> +    return 1;

Could you please add comment explaining the 20 thing (should not it be 19 BTW)?
Alternatively you can rewrite the above sum as:

12 + fixed_name_length(name) + 6 + 1

to make things more obvious...

> +  ptr= strmov(query, "ALTER TABLE ");
> +  ptr= fix_table_name(ptr, name);
> +  ptr= strxmov(ptr, " FORCE", NullS);
> +  if (mysql_real_query(sock, query, (uint)(ptr - query)))
> +  {
> +    fprintf(stderr, "Failed to %s\n", query);
> +    fprintf(stderr, "Error: %s\n", mysql_error(sock));
> +    rc= 1;
> +  }
> +  if (verbose)
> +    printf("%-50s %s\n", name, rc ? "FAILED" : "OK");

I think it is better to be consistent with behavior of code which
handles case with automatical REPAIR TABLE and not to print anything
here.

Or are you trying to mimic print_result() behavior? Then why this
printing happens only in verbose mode?

...

> @@ -770,6 +798,8 @@ static void print_result()
>        printf("%s\n%-9s: %s", row[0], row[2], row[3]);
>        if (strcmp(row[2],"note"))
>  	found_error=1;
> +      if (opt_auto_repair && strstr(row[3], "ALTER TABLE") != NULL)
> +        table_rebuild=1;

Maybe it makes sense to set table_rebuild flag only if we get an error and
not a note? I.e. maybe we should consider changing the above code to:

     if (strcmp(row[2],"note"))
+    {
       found_error=1;
+      if (opt_auto_repair && strstr(row[3], "ALTER TABLE") != NULL)
+        table_rebuild=1;
+    }

What do you think?

...

> === modified file 'mysql-test/t/mysqlcheck.test'
> --- a/mysql-test/t/mysqlcheck.test	2010-10-08 07:09:47 +0000
> +++ b/mysql-test/t/mysqlcheck.test	2011-03-02 17:06:19 +0000
> @@ -229,3 +229,78 @@ drop table `t1-1`;
>  --error 1
>  --exec $MYSQL_CHECK -aoc test "#mysql50#t1-1"
>  
> +
> +--echo #
> +--echo # Bug#11755431 47205: MAP 'REPAIR TABLE' TO RECREATE +ANALYZE FOR
> +--echo #              ENGINES NOT SUPPORTING NATIVE
> +--echo #
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS bug47205;
> +--enable_warnings
> +
> +--echo #
> +--echo # Test 1: Check that ALTER TABLE ... upgrades the table
> +
> +CREATE TABLE bug47205(a VARCHAR(20) PRIMARY KEY)
> +  DEFAULT CHARACTER SET utf8 COLLATE utf8_general_ci engine=innodb;
> +
> +FLUSH TABLE bug47205;
> +
> +--echo # Replace the FRM with a 5.0 FRM that will require upgrade
> +let $MYSQLD_DATADIR= `select @@datadir`;
> +--remove_file $MYSQLD_DATADIR/test/bug47205.frm
> +--copy_file std_data/bug47205.frm $MYSQLD_DATADIR/test/bug47205.frm
> +
> +--echo # Should indicate that ALTER TABLE ... FORCE is needed
> +CHECK TABLE bug47205 FOR UPGRADE;
> +
> +ALTER TABLE bug47205 FORCE;

IMO it also makes sense to check here that ALTER TABLE FORCE fully rebuilds
the table and not only installs new .FRM. This can be done by inserting
a row into the table and looking at the number of records affected by
ALTER (see how --enable_info/--disable_info is used in alter_table.test).

...

Otherwise I am OK with your patch and think that it can be pushed
after considering/addressing the above points and getting approval
from the second reviewer.

-- 
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 branch (jon.hauglid:3364) Bug#11755431Jon Olav Hauglid2 Mar
  • Re: bzr commit into mysql-5.5 branch (jon.hauglid:3364)Bug#11755431Dmitry Lenev3 Mar