List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:July 26 2010 12:12pm
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377
View as plain text  
Hi Davi,

see comments inline.

On Fri, Jul 23, 2010 at 12:04:25PM +0000, Davi Arnaut wrote:
> # At a local mysql-trunk-bugfixing repository of davi
> 
>  3133 Davi Arnaut	2010-07-23
>       Bug#45377: ARCHIVE tables aren't discoverable after OPTIMIZE
>       
>       The problem was that the optimize method of the ARCHIVE storage
>       engine was not preserving the FRM embedded in the ARZ file when
>       rewriting the ARZ file for optimization. The ARCHIVE engine stores
>       the FRM in the ARZ file so it can be transferred from machine to
>       machine without also copying the FRM -- the engine restores the
>       embedded FRM during discovery.
>       
>       The solution is to copy over the FRM when rewriting the ARZ file.
>       In addition, some initial error checking is performed to ensure
>       garbage is not copied over.
...skip...

> === modified file 'storage/archive/ha_archive.cc'
> --- a/storage/archive/ha_archive.cc	2010-07-08 21:20:08 +0000
> +++ b/storage/archive/ha_archive.cc	2010-07-23 12:04:22 +0000
> @@ -1345,10 +1345,11 @@ int ha_archive::repair(THD* thd, HA_CHEC
>  */
>  int ha_archive::optimize(THD* thd, HA_CHECK_OPT* check_opt)
>  {
> -  DBUG_ENTER("ha_archive::optimize");
>    int rc= 0;
> +  char *frm_ptr= NULL;
Any reason to initialize frm_ptr?

>    azio_stream writer;
>    char writer_filename[FN_REFLEN];
> +  DBUG_ENTER("ha_archive::optimize");
>  
>    init_archive_reader();
>  
> @@ -1366,6 +1367,26 @@ int ha_archive::optimize(THD* thd, HA_CH
>    if (!(azopen(&writer, writer_filename, O_CREAT|O_RDWR|O_BINARY)))
>      DBUG_RETURN(HA_ERR_CRASHED_ON_USAGE); 
>  
> +  /* Transfer the embedded FRM so that the file can be discoverable. */
> +  if (!(frm_ptr= (char *)my_malloc(archive.frm_length, MYF(0))))
Coding style: a space after cast.

> +  {
> +    rc= HA_ERR_OUT_OF_MEM;
> +    goto error;
> +  }
> +
> +  my_errno= 0;
I'm against this way of use of my_errno - it hides mistakes. Either
ensure that azread_frm() and azwrite_frm() set my_errno properly or
make them return error code.

> +
> +  /*
> +    Read file offset is repositioned when writing a new header.
> +    Write file offset is set to the end of the file.
> +  */
> +  if (azread_frm(&archive, frm_ptr) ||
> +      azwrite_frm(&writer, frm_ptr, archive.frm_length))
> +  {
> +    rc= my_errno ? my_errno : HA_ERR_INTERNAL_ERROR;
> +    goto error;
> +  }
> +
Ehm. I wouldn't insist, but... I doubt it is a good idea to copy
frm blob from old archive data file. There should be a couple of
cases when dot-frm may get out of sync with frm blob. It is
probably a good moment to re-read up-to-date definition.

Also there is a code that does it in ::create(), I'd prefer to
see it reused it here.

>    /* 
>      An extended rebuild is a lot more effort. We open up each row and re-record it.
> 
>      Any dead rows are removed (aka rows that may have been partially recorded). 
> @@ -1442,11 +1463,13 @@ int ha_archive::optimize(THD* thd, HA_CH
>    // make the file we just wrote be our data file
>    rc= my_rename(writer_filename, share->data_file_name, MYF(0));
>  
> +  my_free(frm_ptr);
>  
>    DBUG_RETURN(rc);
>  error:
>    DBUG_PRINT("ha_archive", ("Failed to recover, error was %d", rc));
>    azclose(&writer);
> +  my_free(frm_ptr);
You don't need to keep frm_ptr allocated until this point, do you?

>  
>    DBUG_RETURN(rc); 
>  }
> 

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377Davi Arnaut23 Jul
Re: bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377Sergey Vojtovich26 Jul
  • Re: bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377Davi Arnaut26 Jul
    • Re: bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377Sergey Vojtovich27 Jul