Re: bk commit into 4.1 tree (holyfoot:1.2688) BUG#31305

[Alexey Botchkov]

Hello, Ingo.

> Please change MAX_HEADER_LEGNTH to MI_MAX_DYN_BLOCK_HEADER in the
> comment. Please place a dot after the last sentence.
....
> Please place a dot after the last sentence.
....
> Please remove at least one of the empty lines.

these were fixed.

> Why do you use ABS() here and in other REPEAT() functions?
Just copied it from the original bugcase.
Removed to not confuse possible reviewers.

> If you like to shorten the result file a bit ...
--disable_query_log  added

>> +UPDATE t1 SET b=repeat('a', abs(800));
>> +CHECK TABLE t1 EXTENDED;
>So you try an even bigger one. What does this statement show, what the
>previous one didn't show?

Previous statement was INSERT, and we needed bigger record
size for UPDATE to make table full as it uses the old record's
space as well.

>Wouldn't it be great to show that a smaller record fits after a bigger
>one was rejected?
Added.

> Is this the default in our tests? Won't it be better to save and restore
> the old value?
No need to store in fact - just 'default' value used.

As i didn't modify the code, i assume 'ok to push' from you,
though you welcome to take a look at the last patch,

Regards
HF



----- Original Message ----- 
From: "Ingo Strüwing" <ingo@stripped>
To: <holyfoot@stripped>
Cc: <commits@stripped>
Sent: Sunday, November 11, 2007 7:22 PM
Subject: Re: bk commit into 4.1 tree (holyfoot:1.2688) BUG#31305


> Hi Alexey,
>
> overall a good patch. It could even be better with some comments in the
> code and perhaps even the test case. See below for some comments. Some
> of them could be added to the code/testcase if you like.
>
> holyfoot@stripped, 10.11.2007 20:11:
> ...
>> ChangeSet@stripped, 2007-11-10 23:11:37+04:00, holyfoot@stripped +4 -0
>>   Bug #31305 myisam tables crash when they are near capacity.
>>
>>   When we insert a record into MYISAM table which is almost 'full',
>>   we first write record data in the free space inside a file, and then
>>   check if we have enough space after the end of the file.
>>   So if we don't have the space, table will left corrupted.
>>   Similar error also happens when we updata MYISAM tables.
>>
>>   Fixed by modifying write_dynamic_record and update_dynamic_record 
>> functions
>>   to check for free space before writing parts of a record
> ...
>> diff -Nrup a/BitKeeper/etc/ignore b/BitKeeper/etc/ignore
> ...
>> +libmysql_r/client_settings.h
>> +libmysqld/ha_blackhole.cc
>
> Unrelated, but acceptable.
>
>> diff -Nrup a/myisam/mi_dynrec.c b/myisam/mi_dynrec.c
> ...
>> +  /*
>> +    Check if we have enough room for the new record.
>> +    First we do simplified check to make usual case faster.
>> +    Then we do more precise check for the space left.
>> +    Though it still is not absolutely precise, as
>> +    we always use MAX_HEADER_LEGNTH while it can be
>> +    less in the most of the cases
>> +  */
>
> Yes, I agree with the approach. We might refuse to insert a bit too
> early, but this is not dramatic, unless the table is very heavily
> fragmented. But when the user optimizes the table, the check starts to
> work precisely again.
>
> Please change MAX_HEADER_LEGNTH to MI_MAX_DYN_BLOCK_HEADER in the
> comment. Please place a dot after the last sentence.
>
>> +
>> +  if (unlikely(info->s->base.max_data_file_length -
>> +               info->state->data_file_length <
>> +               reclength + MI_MAX_DYN_BLOCK_HEADER))
>
> Yes, this is correct.
>
>> +  {
>> +    if (info->s->base.max_data_file_length - 
>> info->state->data_file_length +
>> +        info->state->empty - info->state->del *
> MI_MAX_DYN_BLOCK_HEADER 
>> <
>> +        reclength + MI_MAX_DYN_BLOCK_HEADER)
>
> Ok.
>
> ...
>>    DBUG_ENTER("update_dynamic_record");
>>
>>    flag=block_info.second_read=0;
>> +  /*
>> +     Check if we have enough room for the record.
>> +     First we do simplified check to make usual case faster.
>> +     Then we do more precise check for the space left.
>> +     Though it still is not absolutely precise, as
>> +     we always use MI_MAX_DYN_BLOCK_HEADER while it can be
>> +     less in the most of the cases
>> +  */
>
> Please place a dot after the last sentence.
>
>> +
>> +  if (unlikely(info->s->base.max_data_file_length -
>> +        info->state->data_file_length < reclength))
>
> I understand that you do not subtract MI_MAX_DYN_BLOCK_HEADER here
> because the old record does use a header already.
>
> In the first moment I was confused and thought the requirement for new
> block headers may add to the space required in the "endspace". But then
> I became aware that you use the full new record size for the check (you
> do not subtract the old record size).
>
> Even if the new record is longer and needs to be extended into the
> "endspace", enough bytes can be stored in the old blocks so that there
> is room for a new block header in the "endspace".
>
> However, there is one problem left. If the "endspace" is <
> MI_MIN_BLOCK_LENGTH, we cannot use that space at all.
>
> Since blocks are aligned at 4 bytes, and max_data_file_length should
> also be aligned a 4, "endspace" < 20 means "endspace" <= 16.
>
> If the new record is <= 16 bytes long, it will always fit into a type 1
> block (1/01: Full small record, full block, 3 bytes) or a type 3 block
> (3/03: Full small record, unused space, 4 bytes). (The block size is
> always at least 20 bytes: MI_MIN_BLOCK_LENGTH.)
>
> So I'd say: good. This should be safe.
>
>> +  {
>> +    if ((error=_mi_get_block_info(&block_info,info->dfile,filepos))
>> +        & (BLOCK_DELETED | BLOCK_ERROR | BLOCK_SYNC_ERROR | 
>> BLOCK_FATAL_ERROR))
>> +    {
>> +      DBUG_PRINT("error",("Got wrong block info"));
>> +      if (!(error & BLOCK_FATAL_ERROR))
>> +        my_errno=HA_ERR_WRONG_IN_RECORD;
>> +      goto err;
>> +    }
>> +
>> +    if (block_info.rec_len < reclength)
>
> If I understand the above correctly, you read the block header of the
> first block of the old record to get at the length of the old record.
> Only if the new record is longer, you need to do another check. Good.
>
>> +    {
>> +      if (info->s->base.max_data_file_length - 
>> info->state->data_file_length +
>> +          info->state->empty - info->state->del * 
>> MI_MAX_DYN_BLOCK_HEADER <
>> +          reclength - block_info.rec_len + MI_MAX_DYN_BLOCK_HEADER)
>
> If I understand correctly, you assume that the new record might require
> to create a new block in the "endspace". This is correct.
>
> You subtract the old record length from the new record length and
> compensate by adding MI_MAX_DYN_BLOCK_HEADER. Good.
>
>> +      {
>> +        my_errno=HA_ERR_RECORD_FILE_FULL;
>> +        goto err;
>> +      }
>> +    }
>> +    block_info.second_read=0;
>
> Good. This is important here.
>
> ...
>>  uint tmp=MY_ALIGN(reclength - length + 3 +
>>    test(reclength >= 65520L),MI_DYN_ALIGN_SIZE);
>> +
>> +
>
> If you feel that we should have an empty line here, that's ok. But
> please do never have two empty lines in a row within a function. Please
> remove at least one of the empty lines.
>
> ...
>> diff -Nrup a/mysql-test/t/almost_full.test 
>> b/mysql-test/t/almost_full.test
> ...
>> +let $1= 303;
>> +while ($1)
>> +{
>> +  INSERT INTO t1 SET b=repeat('a',abs(200));
>
> Why do you use ABS() here and in other REPEAT() functions?
>
>> +  dec $1;
>> +}
>
> If you like to shorten the result file a bit, you could also loop over
> INSERT...SELECT. Just a hint, I don't insist.
>
>> +
>> +DELETE FROM t1 WHERE a=1 or a=5;
>
> Here you punch two holes in the table, I guess.
>
>> +
>> +--error 1114
>> +INSERT INTO t1 SET b=repeat('a',abs(600));
>> +CHECK TABLE t1 EXTENDED;
>
> That record doesn't fit anymore.
>
>> +
>> +--error 1114
>> +UPDATE t1 SET b=repeat('a', abs(800));
>> +CHECK TABLE t1 EXTENDED;
>
> So you try an even bigger one. What does this statement show, what the
> previous one didn't show?
>
> Wouldn't it be great to show that a smaller record fits after a bigger
> one was rejected?
>
>> +
>> +drop table t1;
>> +set global myisam_data_pointer_size=4;
>
> Is this the default in our tests? Won't it be better to save and restore
> the old value?
>
> ...
>
> Ok to push from me, under the condition that you fix the places I asked
> for using the word 'please'. :-)
>
> Please do also answer my questions.
>
> I don't demand that you add comments, but I would appreciate it.
>
> I don't need to take another look, unless you change things not
> mentioned in this review.
>
> Regards
> Ingo
> -- 
> Ingo Strüwing, Senior Software Developer
> MySQL GmbH, Dachauer Str. 37, D-80335 München
> Geschäftsführer: Kaj Arnö - HRB München 162140
>