Hi Sergey,
On 7/26/10 9:12 AM, Sergey Vojtovich wrote:
> 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?
No.
>> 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.
OK.
>> + {
>> + 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.
Since my_errno is just a wrapper around errno and that errno is reserved
for for system calls and library functions, this kind of construct is
quite common, sometimes even required, to detect failures.
This is actually a recommend practice, see:
http://tinyurl.com/2d49xca
>> +
>> + /*
>> + 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
No disagreement that it is a bad idea. But we either make it work or we
remove this broken feature. What's it gonna be?
> 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.
This would be a change in behavior.
> Also there is a code that does it in ::create(), I'd prefer to
> see it reused it here.
OK.
>> /*
>> 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?
>
No.
