On Thursday 11 October 2007, Ingo Strüwing wrote:
>Hi Alexey,
>
>thank you for fixing this.
>Please accept my apologies that I pointed you the wrong way when you
>started with it.
>
Without your comments as a baseline, I would spend much more time debugging a
non-existing compiler bug, because indeed the problem looked like a
platform-specific one.
>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.
>
OK, fixed in the new patch.
>> 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.
>"
>
OK, fixed in the new patch.
>...
>
>> - 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.
>
merge_buffers() assumes that for each key there is at least one corresponding
BUFFPEK:
maxcount=keys/((uint) (Tb-Fb) +1);
...
for (buffpek= Fb ; buffpek <= Tb ; buffpek++)
{
count+= buffpek->count;
buffpek->base= strpos;
buffpek->max_keys=maxcount;
strpos+= (uint) (error=(int) info->read_to_buffer(from_file,buffpek,
sort_length));
if (error == -1)
goto err; /* purecov: inspected */
queue_insert(&queue,(char*) buffpek);
}
And later in read_to_buffer()/read_to_buffer_varlen() we have:
if ((count=(uint) min((ha_rows) buffpek->max_keys,buffpek->count)))
{
<buffpek key initialization>
}
So if the maximum number of keys for a buffpek is 0, it is left uninitialized.
In thr_find_all_keys() we ensure that:
1. We have suffictient buffer space to place both keys _and_ BUFFPEKs.
2. Number of keys >= number of BUFFPEKs.
In thr_write_keys() the buffer is used only to store keys, memory for BUFFPEKs
is already allocated. This implies that the maximum number of keys that the
buffer can hold is greater than the value calculated in thr_find_all_keys(),
and condition 2 is still true.
However, with incorrectly calculated buffer length condition 2 could become
false when myisam_sort_buffer_length is small enough, since we only took 3/4
of it into account.
Best regards,
--
Alexey Kopytov, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? www.mysql.com/certification
