Hi Dmitry,
On 6/14/11 10:24 AM, Dmitry Lenev wrote:
> #At file:///home/dlenev/src/bzr/mysql-5.1-12652385/ based on
> revid:marko.makela@stripped
>
> 3648 Dmitry Lenev 2011-06-14
> Tentative fix for bug #12652385 - "61493: REORDERING COLUMNS
> TO POSITION FIRST CAN CAUSE DATA TO BE CORRUPTED".
>
> ALTER TABLE MODIFY/CHANGE ... FIRST did nothing except renaming
> columns if new version of the table had exactly the same
> structure as the old one (i.e. as result of such statement names
Comma after "statement".
> of columns changed their order as specified but data in columns
> didn't). The same thing happened for ALTER TABLE DROP COLUMN, ADD
Slash instead of comma.
> COLUMN statements which were supposed to produce new version of
> table with exactly the same structure as the old version of table.
> I.e. in the latter case the result was the same as if old column
> was renamed instead of being dropped and new column with default
> as value being created.
>
> Both these problems were caused by the fact that ALTER TABLE
> implementation incorrectly interpreted both these situations as
> simple renaming of columns and assumed that in-place ALTER TABLE
> algorithm could have been used for them.
>
> This patch fixes this problem by ensuring that in cases when some
> column is moved to the first position or some column is dropped
> the default ALTER TABLE algorithm involving table copying is
> always used. This is achieved by detecting such situations in
> mysql_prepare_alter_table() and setting Alter_info::change_level
> to ALTER_TABLE_DATA_CHANGED for them.
Looks good, OK to push. Some minor comments below.
> QQ: Question for reviewers, maybe it it is better to adjust
> compare_tables() to properly detect and handle such
> situation? But check is going to be more complicated in
> this case...
I think it might be worth because currently mysql_compare_tables will
improperly flag the field, which might pose problems if the function
starts to be to be used elsewhere. At least, it might be worth adding a
@remark to the function about the fact that it does not properly detect
reordering of fields.
> @ mysql-test/r/alter_table.result
> Added test for bug #12652385 - "61493: REORDERING COLUMNS TO
> POSITION FIRST CAN CAUSE DATA TO BE CORRUPTED".
> @ mysql-test/t/alter_table.test
> Added test for bug #12652385 - "61493: REORDERING COLUMNS TO
> POSITION FIRST CAN CAUSE DATA TO BE CORRUPTED".
> @ sql/sql_table.cc
> Changed mysql_prepare_alter_table() to detect situations in
> which we some column moved to the first position or some column
> is dropped and ensure that such ALTER TABLE statements won't
> be carried out using in-place algorithm. The latter could have
> happened before this patch if new version of table had the same
> structure as the old one (except the column names).
>
> modified:
> mysql-test/r/alter_table.result
> mysql-test/t/alter_table.test
> sql/sql_table.cc
> === modified file 'mysql-test/r/alter_table.result'
> --- a/mysql-test/r/alter_table.result 2009-12-18 12:00:30 +0000
> +++ b/mysql-test/r/alter_table.result 2011-06-14 13:24:07 +0000
> @@ -1345,4 +1345,33 @@ DROP TABLE t1;
> CREATE TABLE t1 (a TEXT, id INT, b INT);
> ALTER TABLE t1 DROP COLUMN a, ADD COLUMN c TEXT FIRST;
> DROP TABLE t1;
> +#
> +# Test for bug #12652385 - "61493: REORDERING COLUMNS TO POSITION
> +# FIRST CAN CAUSE DATA TO BE CORRUPTED".
> +#
> +drop table if exists t1;
> +# Use MyISAM engine as the fact that InnoDB doesn't support
> +# in-place ALTER TABLE in cases when columns are being renamed
> +# hides some bugs.
> +create table t1 (i int, j int) engine=myisam;
> +insert into t1 value (1, 2);
> +# First, test for original problem described in the bug report.
> +select * from t1;
> +i j
> +1 2
> +# Change of column order by the below ALTER TABLE statement should
> +# affect both column names and column contents.
> +alter table t1 modify column j int first;
> +select * from t1;
> +j i
> +2 1
> +# Now test for similar problem with the same root.
> +# The below ALTER TABLE should change not only the name but
> +# also the value for the last column of the table.
> +alter table t1 drop column i, add column k int default 0;
> +select * from t1;
> +j k
> +2 0
> +# Clean-up.
> +drop table t1;
> End of 5.1 tests
>
> === modified file 'mysql-test/t/alter_table.test'
> --- a/mysql-test/t/alter_table.test 2009-12-18 12:00:30 +0000
> +++ b/mysql-test/t/alter_table.test 2011-06-14 13:24:07 +0000
> @@ -1073,4 +1073,31 @@ ALTER TABLE t1 DROP COLUMN a, ADD COLUMN
> DROP TABLE t1;
>
>
> +--echo #
> +--echo # Test for bug #12652385 - "61493: REORDERING COLUMNS TO POSITION
> +--echo # FIRST CAN CAUSE DATA TO BE CORRUPTED".
> +--echo #
> +--disable_warnings
> +drop table if exists t1;
> +--enable_warnings
> +--echo # Use MyISAM engine as the fact that InnoDB doesn't support
> +--echo # in-place ALTER TABLE in cases when columns are being renamed
> +--echo # hides some bugs.
> +create table t1 (i int, j int) engine=myisam;
> +insert into t1 value (1, 2);
> +--echo # First, test for original problem described in the bug report.
> +select * from t1;
> +--echo # Change of column order by the below ALTER TABLE statement should
> +--echo # affect both column names and column contents.
> +alter table t1 modify column j int first;
> +select * from t1;
> +--echo # Now test for similar problem with the same root.
> +--echo # The below ALTER TABLE should change not only the name but
> +--echo # also the value for the last column of the table.
> +alter table t1 drop column i, add column k int default 0;
> +select * from t1;
> +--echo # Clean-up.
> +drop table t1;
> +
> +
> --echo End of 5.1 tests
>
> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc 2011-05-16 20:04:01 +0000
> +++ b/sql/sql_table.cc 2011-06-14 13:24:07 +0000
> @@ -6164,6 +6164,12 @@ mysql_prepare_alter_table(THD *thd, TABL
> create_info->auto_increment_value=0;
> create_info->used_fields|=HA_CREATE_USED_AUTO;
> }
> + /*
> + ALTER TABLE DROP COLUMN always changes table data even in cases
> + when new version of the table has the same structure as the old
> + one.
> + */
> + alter_info->change_level= ALTER_TABLE_DATA_CHANGED;
> break;
> }
> }
> @@ -6247,7 +6253,10 @@ mysql_prepare_alter_table(THD *thd, TABL
> if (!def->after)
> new_create_list.push_back(def);
> else if (def->after == first_keyword)
> + {
> new_create_list.push_front(def);
> + alter_info->change_level= ALTER_TABLE_DATA_CHANGED;
Consider adding a comment about the reordering of the fields and
in-place alter.
Regards,
Davi
