List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 21 2010 1:49pm
Subject:Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3141)
Bug#56422 Bug#56494
View as plain text  
Hello Jon Olav!

Here are my comments for the new version of your patch:

* Jon Olav Hauglid <jon.hauglid@stripped> [10/09/21 13:53]:
> #At file:///export/home/x/mysql-5.5-runtime-bug56494/ based on
> revid:jon.hauglid@stripped
> 
>  3141 Jon Olav Hauglid	2010-09-21
>       Bug #56494 Segfault in upgrade_shared_lock_to_exclusive() for
>                  REPAIR of merge table
>       Bug #56422 CHECK TABLE run when the table is locked reports
>                  corruption along with timeout
>       
>       The crash happened if a table maintenance statement (ANALYZE TABLE,
>       REPAIR TABLE, etc.) was executed on a MERGE table and opening and 
>       locking a child table failed. This could for example happen if a child
>       table did not exist or if a lock timeout happened while waiting for
>       a conflicting metadata lock to disappear.
>       
>       Since opening and locking the MERGE table and its children failed,
>       the tables would be closed and the metadata locks released.
>       However, TABLE_LIST::table for the MERGE table would still be set,
>       with its value invalid since the tables had been closed.
>       This caused the table maintenance statement to try to continue
>       and upgrade the metadata lock on the MERGE table. But since the lock
>       already had been released, this caused a segfault.
>       
>       This patch fixes the problem by setting TABLE_LIST::table to NULL 
>       if open_and_lock_tables() fails. This prevents maintenance
>       statements from continuing and trying to upgrade the metadata lock.
>       
>       The patch also includes a 5.5 version of the fix for
>       Bug #46339 crash on REPAIR TABLE merge table USE_FRM.
>       This bug caused REPAIR TABLE ... USE_FRM to give an assert 
>       when used on merge tables.
>       
>       Finally, the patch changes the error message from "Corrupt" to
>       "Operation failed" for a number of issues not related to table
>       corruption. For example "Lock wait timeout exceeded" and 
>       "Deadlock found trying to get lock".
>       
>       Test cases added to merge.test and check.test.

I think it makes sense to mention in the ChangeSet comment that this
patch also enables CHECK TABLES statement for log tables. We should
explain why this step was necessary and why it is OK to do so.

...

> === modified file 'sql/sql_admin.cc'
> --- a/sql/sql_admin.cc	2010-08-18 11:29:04 +0000
> +++ b/sql/sql_admin.cc	2010-09-21 09:45:56 +0000
> @@ -112,7 +112,8 @@ static int prepare_for_repair(THD *thd, 
>    }
>  
>    /* A MERGE table must not come here. */
> -  DBUG_ASSERT(table->file->ht->db_type != DB_TYPE_MRG_MYISAM);
> +  if (table->file->ht->db_type == DB_TYPE_MRG_MYISAM)
> +    goto end;
>  
>    /*
>      REPAIR TABLE ... USE_FRM for temporary tables makes little sense.

I don't think the above if-statement is necessary.
A few lines below we do:

  /*
    Check if this is a table type that stores index and data separately,
    like ISAM or MyISAM. We assume fixed order of engine file name
    extentions array. First element of engine file name extentions array
    is meta/index file extention. Second element - data file extention. 
  */
  ext= table->file->bas_ext();
  if (!ext[0] || !ext[1])
    goto end;                                   // No data file

And since for MERGE tables ext[1] is NULL this check will have
similar effect to the above if-statement.

IMO it is better to rely on this generic check rather than add yet
another MERGE-specific check. To ensure that it won't break if MERGE
engine bas_ext() method is changed we can simply move the above assert
after this generic check.

> @@ -418,9 +453,7 @@ static bool mysql_admin_table(THD* thd, 
>          push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
>                       ER_VIEW_CHECKSUM, ER(ER_VIEW_CHECKSUM));
>        if (thd->stmt_da->is_error() &&
> -          (thd->stmt_da->sql_errno() == ER_NO_SUCH_TABLE ||
> -           thd->stmt_da->sql_errno() == ER_FILE_NOT_FOUND))
> -        /* A missing table is just issued as a failed command */
> +          table_not_corrupt_error(thd->stmt_da->sql_errno()))
>          result_code= HA_ADMIN_FAILED;
>        else
>          /* Default failure code is corrupt table */
> 

Otherwise I am OK with your patch and think that it can be pushed
after considering the 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:3141) Bug#56422Bug#56494Jon Olav Hauglid21 Sep
  • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3141)Bug#56422 Bug#56494Dmitry Lenev21 Sep