Re: bk commit into 5.1 tree (istruewing:1.2641) BUG#33222

[Ingo Strüwing]

Hi Sergey,

thank you for the very thorough review.

Sergey Vojtovich, 10.01.2008 14:53:
...
> On Tue, Dec 18, 2007 at 03:19:07PM +0100, Ingo Struewing wrote:
...
>>   ALTERing a table with long char columns warned about corruptions
>>   and left the altered table empty.
> I'd disagree with this statement and I believe it is incomplete:
> - as we perform ALTER via REPAIR code, REPAIR is affected as well;
> - CHECK TABLE is also affected, it wrongly reports that the table is
>   crashed;
> - it doesn't state that only tables that use dynamic table format are
>   affected;
> - it doesn't state that CHAR field value (not counting trailing spaces)
>   must be longer than 127;
> - it doesn't state that only ALTER TABLE that rewrites data file is
>   affected;
> - it wrongly states that the altered table is left empty, whereas only long
>   CHAR fields are thrown away.
> 
> I believe that my comment sounds better (but still not perfect):
> For dynamic record format tables with CHAR field, that contain value longer
> than 127 bytes CHECK TABLE may report that table has wrongly packed record,
> whereas records are fine. REPAIR TABLE (and sibling operations like ADD/DROP
> COLUMN) may throw away such records.

Hm. I must admit that I didn't check all these cases. I wonder if it is
possible to specify all conditions for the bug to show up without
forgetting a single one.

After all we need a changelog entry. This should be easily
understandable by users IMHO. It should also remind the bug reporter of
the problem he reported. And it should give an impression of the impact
of the bug.

In this case, I think, we should mention ALTER TABLE as the bug reporter
used it to show the problem. We should also mention data loss.

However mentioning the complete set of conditions and say that in these
cases single records could be lost, while technically correct, might
hide the potential impact from the user.

...
> Frankly speaking I expected to see here my test case, which is much smaller
> and thus easier to understand. But ok, I'm almost fine with this test with
> one exception: could you also add CHECK TABLE t1 and REPAIR TABLE t1?

I agree that I should have reduced the amount of columns and data. I
also agree that it would be ok to show the complete set of table admin
operations with a long char value. But I still think it is important to
show the data loss (ALTER TABLE).

> 
>> -	  else if (*to++ != (char) new_length)
>> +	  else if (*to++ != (uchar) new_length)
> I believe we do not need cast here at all: both variables are unsigned.

Yes, but 'to' is 'uchar' and 'new_length' is 'uint'. Won't we risk
complaints by some compilers that data might be truncated as the source
variable can carry a bigger range of values?

> 
> Also there are pretty a lot of tab characters in the code that you updated,
> please fix it.

Ok.

What do you think of taking the bug back? You made the analysis,
proposed the fix, and left it to me before your vacation so that it
could be pushed earlier. But noone reviewed it in between. Now you could
complete it and take the honor and increase your bug-fix count. I would
approve the patch then.

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