Alfranio, salut.
Thanks for prompt and sound questions.
> Hi Andrei,
>
> great work.
>
> I have only three comments:
>
> 0 - In the future, we need to refactory/encapsulate all the math regarding
> bits. For instance, this quite is quite strange:
>
> if (likely(!bitmap_init(&m_cols_ai,
> m_width <= sizeof(m_bitbuf_ai)*8 ? m_bitbuf_ai : NULL,
> m_width,
> false)))
>
> Doing so, we can minimize bugs as the BUG#42977.
>
I agree. A possible reason for the bug might be in subtleties of
operations inside of the list of passing arguments. The quoted snippet
reads really hard.
>
> 1 - Why don't you dynamically create the table instead of
> having it in the test? This would reduce the size of the test
> and increase its readability.
I tried actually. However making ALTER table to add some 300 rows
takes about 10 seconds whereas the static declaration is close to
zero. So I decided to spare either the machine and our common
developers' human time :-)
>
> 2 - Why did you need to modify the following code?
>
Previously, with number of columns more than 256 the macro wrote
beyond the stack allocated buf[256].
>>
>>
>> === modified file 'sql/rpl_utility.h'
>> --- a/sql/rpl_utility.h 2008-08-20 14:06:31 +0000
>> +++ b/sql/rpl_utility.h 2009-03-23 16:30:05 +0000
>> @@ -294,10 +294,11 @@ namespace {
>> }
>> #endif
>>
>> +// NB. number of printed bit values is limited to sizeof(buf)
>> #define DBUG_PRINT_BITSET(N,FRM,BS) \
>> do { \
>> char buf[256]; \
>> - for (uint i = 0 ; i < (BS)->n_bits ; ++i) \
>> + for (uint i = 0 ; i < min(sizeof(buf), (BS)->n_bits) ; ++i) \
>> buf[i] = bitmap_is_set((BS), i) ? '1' : '0'; \
>>
> If there is a reason, I think the set below is wrong.
>> buf[(BS)->n_bits] = '\0'; \
>>
> it should be buf[min(sizeof(buf), (BS)->n_bits)] = '\0';
I was not thinking about magics of assignment in the loop.
The change merely narrows looping down to sizeof(buf) == 256
times using min().
Yes, we won't see the exceeding part of the bitmap but
I think we should not allocate/free in this pretty much debug purpose
routine: 256+ columns is far from a common case.
>> DBUG_PRINT((N), ((FRM), buf)); \
>>
cheers,
Andrei
