MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:October 11 2007 9:14am
Subject:Re: bk commit into 4.1 tree (kaa:1.2685) BUG#31174
View as plain text  
Hi Alexey,

thank you for fixing this.
Please accept my apologies that I pointed you the wrong way when you
started with it.

I am ok with the patch in general, but I have remarks regarding the
changeset comments and a question to the code. Please see below.

Alexey Kopytov, 10.10.2007 21:01:
> ChangeSet@stripped, 2007-10-10 23:01:21+04:00, kaa@polly.(none) +3 -0
>   Fix for bug #31174: "Repair" command on MyISAM crashes with small
> myisam_sort_buffer_size.

Please try to keep the lines <= 80 characters or better even <= 72
characters for more convenient email quoting. I use to keep the 'citool'
window small enough to see where I have to insert line breaks.

>   An incorrect length of the sort buffer was used when calculating the maximum number
> of keys. This resulted in the number of keys < number of BUFFPEK structures, which in
> turn led to use of uninitialized BUFFPEK structures.

This comment is good to have here, but IMHO we should also have a more
high-level description of 1. what was the problem and 2. how was it fixed.

On the technical level you have 1., and 2. is obvious. But could you
please additionally write something like this:
A buffer length was incorrectly calculated. On AIX the size of some
objects was so that the wrong calculation lead to use of uninitialized
data. With a different buffer size this did also affect other systems.

Fixed by correcting the buffer length calculation.

> -        while (length >= MIN_SORT_MEMORY && !mergebuf)
> +        while (length >= MIN_SORT_MEMORY)
>          {
> -          mergebuf=my_malloc(length, MYF(0));
> +          if ((mergebuf= my_malloc(length, MYF(0))))
> +              break;
>            length=length*3/4;
>          }

'length' is only used to calculate the number of keys that the merge
buffer can hold. The miscalculation made less keys merged in one buffer.

Can you please help me to understand why this could make use of
uninitialized BUFFPEKs? I would have expected that less keys would also
use less BUFFPEKs.

Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140
bk commit into 4.1 tree (kaa:1.2685) BUG#31174Alexey Kopytov10 Oct
  • Re: bk commit into 4.1 tree (kaa:1.2685) BUG#31174Ingo Strüwing11 Oct
    • Re: bk commit into 4.1 tree (kaa:1.2685) BUG#31174Alexey Kopytov11 Oct