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

[Ingo Strüwing]

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