From: Sergey Vojtovich
Date: December 9 2010 10:06am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3511)
 Bug#54486
List-Archive: http://lists.mysql.com/commits/126398
Message-Id: <20101209100635.GA1978@june>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hi Dmitry,

ok to push, but there are a few minor suggestions inline.

On Fri, Dec 03, 2010 at 02:08:08PM +0000, Dmitry Shulga wrote:
> #At file:///Users/shulga/projects/mysql/5.1-bugteam-bug54486/ based on revid:vasil.dimov@stripped
> 
>  3511 Dmitry Shulga	2010-12-03
>       Fixed bug#54486 - assert in my_seek, concurrent
>       DROP/CREATE SCHEMA, CREATE TABLE, REPAIR.
>       
>       The cause of assert was concurrent execution of
>       DROP DATABASE and REPAIR TABLE where first statement
>       deleted table's file .TMD at the same time when
>       REPAIR TABLE statement tried replace
>       old file by temporary file that has just been removed.
>       
>       Additionally was fixed trouble when DROP TABLE try delete
>       all files belong to table being dropped at the same time
>       when REPAIR TABLE statement has just deleted .TMD file.
>       
>       Regression test isn't added because for such one need insert 
>       synchonization point (DEBUG_SYNC) to function my_redel() defined in
>       mysys/my_redel.c. DEBUG_SYNC is defined in sql/debug_sync.h.
>       So, in order to add this one we need add additional dependency,
>       i.e. mysys/my_redel.c will depend on sql.debug_sync.h, that is wrong.
You can easily make tests using DBUG_EXECUTE_IF(), but I have no strong
opinion if we really need it.

>      @ sql/sql_db.cc
>         mysql_rm_known_files() modified: ignore possible errors
>         when trying delete all table's files. Such aggressigve 
>         algorithm permits skip already deleted (in another thread)
>         files.
Hmm... it doesn't sound up to date.

>      @ storage/myisam/mi_check.c
>         mi_repair() was modified: set param->retry_repair= 0
>         in order to don't call following failover procedure
>         in ha_myisam::repair(); set an error in diagnostic area in
>         case of error in access to .TMD file.
> 
>     modified:
>       sql/sql_db.cc
>       storage/myisam/mi_check.c
> === modified file 'sql/sql_db.cc'
> --- a/sql/sql_db.cc	2010-10-19 10:27:09 +0000
> +++ b/sql/sql_db.cc	2010-12-03 14:07:56 +0000
> @@ -948,9 +948,6 @@ bool mysql_rm_db(THD *thd,char *db,bool 
>      remove_db_from_cache(db);
>      pthread_mutex_unlock(&LOCK_open);
>  
> -    Drop_table_error_handler err_handler(thd->get_internal_handler());
> -    thd->push_internal_handler(&err_handler);
> -
>      error= -1;
>      /*
>        We temporarily disable the binary log while dropping the objects
> @@ -983,8 +980,11 @@ bool mysql_rm_db(THD *thd,char *db,bool 
>        error = 0;
>        reenable_binlog(thd);
>      }
> -    thd->pop_internal_handler();
>    }
> +  DBUG_ASSERT(deleted >= 0 ||
> +              (deleted < 0 && thd->main_da.is_set()) ||
> +              thd->killed);
> +
It makes me think that:
- diagnostics area state is irrelevant when deleted >= 0;
- diagnostics area state is irrelevant (though it must be non-empty)
  when deleted < 0;
- diagnostics area state as well as deleted state is irrelevant
  when thd->killed.

It confuses me, but I can leave with it. So I can only hope that all
points above are acceptable.

>    if (!silent && deleted>=0)
>    {
>      const char *query;
> @@ -1213,16 +1213,36 @@ static long mysql_rm_known_files(THD *th
>      else
>      {
>        strxmov(filePath, org_path, "/", file->name, NullS);
> -      if (my_delete_with_symlink(filePath,MYF(MY_WME)))
> +      /*
> +        We ignore ENOENT error in order to skip files that was deleted
> +        by concurrently running statement like REAPIR TABLE ...
> +      */
> +      if (my_delete_with_symlink(filePath, MYF(0)) &&
> +          my_errno != ENOENT)
>        {
> -	goto err;
> +        my_error(EE_DELETE, MYF(0), filePath, my_errno);
> +        goto err;
>        }
>      }
>    }
> -  if (thd->killed ||
> -      (tot_list && mysql_rm_table_part2(thd, tot_list, 1, 0, 1, 1)))
> +
> +  if (thd->killed)
>      goto err;
>  
> +  if (tot_list)
> +  {
> +    int res= 0;
> +    {
Why extra level of braces and indentation?

> +      Drop_table_error_handler err_handler(thd->get_internal_handler());
> +
> +      thd->push_internal_handler(&err_handler);
> +      res= mysql_rm_table_part2(thd, tot_list, 1, 0, 1, 1);
> +      thd->pop_internal_handler();
> +    }
> +    if (res)
> +      goto err;
> +  }
> +
>    /* Remove RAID directories */
>    {
>      List_iterator<String> it(raid_dirs);
> 
> === modified file 'storage/myisam/mi_check.c'
> --- a/storage/myisam/mi_check.c	2010-03-25 12:08:21 +0000
> +++ b/storage/myisam/mi_check.c	2010-12-03 14:07:56 +0000
> @@ -1741,6 +1741,8 @@ err:
>  			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
>  	  mi_open_datafile(info,share,name,-1))
>  	got_error=1;
> +
> +      param->retry_repair= 0;
>      }
>    }
>    if (got_error)
> 


> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=svoj@stripped

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com