Re: bzr commit into mysql-6.0 branch (satya.bn:2900) Bug#33696

[Ingo Strüwing]

Hi Satya,

This is ok to push from me. I suggest Martin Skold as second reviewer.
He re-implemented online ALTER TABLE, so has firm knowledge in this area.

I have some comments on your comments. Would be great if you adhere to
them. Another review by me is not required in this case.

Satya B, 05.11.2008 13:04:
> #At file:///home/satya/WORK/mysql-6.0-bugteam-33696/
> 
>  2900 Satya B	2008-11-05
>       Fix for Bug#33696 - CSV storage engine allows nullable columns via ALTER TABLE
> statements
>       
>       Problem:
>       After the fast online-alter table changes, CSV engine allows nullable columns
> via alter table and also the existing non-null columns  can be modified to null columns.
>       All columns in CSV Engine should be NON-NULL always.
>       
>       How it was solved:
>       Found that we evaluate the changes that will be made to altered table. During
> this evaluation, we check for the behaviour of the new fields in temporary altered table
> with the fields in the original table and maintain as table_changes. This is passed to
> storage engines along with the new information. Storage engines can add further checks and
> return the compatibility state.CSV Engine has this information but ignores it and returns
> always true. Changed the behaviour to return compatible or not based on the information
> passed from the server.

Great comments! Exactly what I was looking for.

In the future you can omit the labels "Problem:" and "How it was
solved:". The text is absolutely sufficient.

But please keep the lines short. Please don't rely on the mail readers
to wrap the lines. I'd say ~72 characters are good so that it fits on a
line even after quoting in a reply.

>       
>       In sql_table.cc: mysql_alter_table(...), create_altered_table(...) creates the
> altered table and table->file->check_if_supported_alter(..) checks if a table can be
> altered on-line

This is low-level information. I use to add this as file comments, not
as revision comment. But this may be a matter of taste.

>       
>       Note:
>       The csv_alter_table exists in the repository and was disabled due to this bug.

Please be more clear: "The test case csv_alter_table exists ...". Or
better yet in past tense: "The test case csv_alter_table did already
exist ...".

Thank you very much.
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