List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:June 16 2011 7:56pm
Subject:Re: bzr commit into mysql-5.1 branch (Dmitry.Lenev:3648) Bug#12652385
View as plain text  
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
Thread
bzr commit into mysql-5.1 branch (Dmitry.Lenev:3648) Bug#12652385Dmitry Lenev14 Jun
  • Re: bzr commit into mysql-5.1 branch (Dmitry.Lenev:3648) Bug#12652385Davi Arnaut17 Jun