Hi Satya,
Satya B, 03.04.2009 10:46:
...
> 2725 Satya B 2009-04-03
> Fix for Bug #43973 - backup_myisam.test fails on 6.0-bugteam
>
> The test started failing following the push for BUG#41541.
> The record positions are always increased by the word size and
> it started failing when the push for BUG#41541 increased it by
> only the number of bytes actually available.
>
> Fixed by allocating seven extra bytes to the Memory segment #2
> so that we don't read unallocated memory.
I do not understand, why this fails. I think that our patch for
BUG#41541 has the effect that it reads only available bytes. That is, we
don't read unallocated memory. The reason for increasing the memory
segment is not valid IMHO. If I am in error, please explain in detail,
why the patch for BUG#41541 doe not prevent reading from unallocated memory.
I do not understand, why increasing the record position needs to be done
by the word size, even though the buffer is at its end already.
I do not understand, why the change of fill_buffer() (in the patch for
BUG#41541) has any influence on updating the record position.
Please give a more detailed reasoning for the change.
>
> Note: The bug was reported on 6.0 as the test failed but it
> affects 5.0-bugteam, 5.1-bugteam branches also.
> No testcase added the bug is about test failure.
> modified:
> myisam/mi_packrec.c
>
> per-file messages:
> myisam/mi_packrec.c
> Fixed _mi_read_pack_info() method to allocate seven extra bytes in
> Memory segment #2
> === modified file 'myisam/mi_packrec.c'
> --- a/myisam/mi_packrec.c 2009-03-25 09:15:53 +0000
> +++ b/myisam/mi_packrec.c 2009-04-03 08:46:48 +0000
> @@ -208,9 +208,13 @@ my_bool _mi_read_pack_info(MI_INFO *info
> This segment will be reallocated after construction of the tables.
> */
> length=(uint) (elements*2+trees*(1 << myisam_quick_table_bits));
> + /*
> + Allocate 7 extra bytes to avoid valgrind warnings when reading
> + compressed myisam table record into buffer
> + */
This comment does not really tell, why the 7 bytes are required. The
problem in this bug (#43973) was not a valgrind warning.
If you look at the history of this file, you will see that I eliminated
the 7 bytes before because I did not understand, what there were for. If
you don't specify their need precisely, they will be eliminated one day
again.
Overmore, the 7 does not correspond to the "word size", you mentioned in
the revision comment. Word size for MyISAM can be 2, 4, or 8. I would
expect that adding 1, 3, or 7 respectively would be required. Please
explain in the code comment, why you use 7 unconditionally.
> if (!(share->decode_tables=(uint16*)
> my_malloc((length + OFFSET_TABLE_SIZE) * sizeof(uint16) +
> - (uint) (share->pack.header_length - sizeof(header)),
> + (uint) (share->pack.header_length - sizeof(header) + 7),
> MYF(MY_WME | MY_ZEROFILL))))
> goto err1;
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
