MDEV-9045 Inconsistent handling of "ALGORITHM=INPLACE" with PERSISTENT generated columns

vuvova · vuvova · commit 3c6b771753d8 · 2016-02-15T13:02:21.000+01:00
Only set Alter_inplace_info::ALTER_COLUMN_VCOL flag if
a vcol might be affected by the ALTER TABLE statement
diff --git a/mysql-test/suite/vcol/r/alter_inplace-9045.result b/mysql-test/suite/vcol/r/alter_inplace-9045.result
@@ -0,0 +1,44 @@
+create table t1(id int auto_increment primary key, handle int, data bigint not null default 0) engine = innodb;
+insert into t1(handle) values(12),(54),(NULL);
+select *, md5(handle) from t1;
+id	handle	data	md5(handle)
+1	12	0	c20ad4d76fe97759aa27a0c99bff6710
+2	54	0	a684eceee76fc522773286a895bc8436
+3	NULL	0	NULL
+alter table t1 add index handle(handle), algorithm=inplace;
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, algorithm=inplace;
+ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try ALGORITHM=COPY.
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, add unique index hash(hash), algorithm=inplace;
+ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try ALGORITHM=COPY.
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, add unique index hash(hash), algorithm=copy;
+select * from t1;
+id	handle	data	hash
+1	12	0	c20ad4d76fe97759aa27a0c99bff6710
+2	54	0	a684eceee76fc522773286a895bc8436
+3	NULL	0	NULL
+alter table t1 modify column hash varchar(32) as (md5(handle+1)) persistent, algorithm=inplace;
+ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try ALGORITHM=COPY.
+alter table t1 modify column hash varchar(32) as (md5(handle+1)) persistent, algorithm=copy;
+select * from t1;
+id	handle	data	hash
+1	12	0	c51ce410c124a10e0db5e4b97fc2af39
+2	54	0	b53b3a3d6ab90ce0268229151c9bde11
+3	NULL	0	NULL
+alter table t1 modify column handle int not null, algorithm=inplace;
+ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try ALGORITHM=COPY.
+alter table t1 modify column handle int not null, algorithm=copy;
+Warnings:
+Warning	1265	Data truncated for column 'handle' at row 3
+select * from t1;
+id	handle	data	hash
+1	12	0	c51ce410c124a10e0db5e4b97fc2af39
+2	54	0	b53b3a3d6ab90ce0268229151c9bde11
+3	0	0	c4ca4238a0b923820dcc509a6f75849b
+alter table t1 drop index handle, algorithm=inplace;
+create index data on t1(data) algorithm=inplace;
+alter table t1 drop column data, algorithm=inplace;
+alter table t1 add column sha varchar(32) as (sha1(handle)) persistent, algorithm=inplace;
+ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try ALGORITHM=COPY.
+alter table t1 add column sha varchar(32), algorithm=inplace;
+alter table t1 drop column hash, algorithm=inplace;
+drop table t1;
diff --git a/mysql-test/suite/vcol/t/alter_inplace-9045.test b/mysql-test/suite/vcol/t/alter_inplace-9045.test
@@ -0,0 +1,31 @@
+#
+# MDEV-9045 Inconsistent handling of "ALGORITHM=INPLACE" with PERSISTENT generated columns
+#
+--source include/have_innodb.inc
+
+create table t1(id int auto_increment primary key, handle int, data bigint not null default 0) engine = innodb;
+insert into t1(handle) values(12),(54),(NULL);
+select *, md5(handle) from t1;
+alter table t1 add index handle(handle), algorithm=inplace;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, algorithm=inplace;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, add unique index hash(hash), algorithm=inplace;
+alter table t1 add column hash varchar(32) as (md5(handle)) persistent, add unique index hash(hash), algorithm=copy;
+select * from t1;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t1 modify column hash varchar(32) as (md5(handle+1)) persistent, algorithm=inplace;
+alter table t1 modify column hash varchar(32) as (md5(handle+1)) persistent, algorithm=copy;
+select * from t1;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t1 modify column handle int not null, algorithm=inplace;
+alter table t1 modify column handle int not null, algorithm=copy;
+select * from t1;
+alter table t1 drop index handle, algorithm=inplace;
+create index data on t1(data) algorithm=inplace;
+alter table t1 drop column data, algorithm=inplace;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t1 add column sha varchar(32) as (sha1(handle)) persistent, algorithm=inplace;
+alter table t1 add column sha varchar(32), algorithm=inplace;
+alter table t1 drop column hash, algorithm=inplace;
+drop table t1;
diff --git a/sql/field.h b/sql/field.h
@@ -283,6 +283,13 @@ class Virtual_column_info: public Sql_alloc
   {
     in_partitioning_expr= TRUE;
   }
+  bool is_equal(Virtual_column_info* vcol)
+  {
+    return field_type == vcol->get_real_type()
+        && stored_in_db == vcol->is_stored()
+        && expr_str.length == vcol->expr_str.length
+        && memcmp(expr_str.str, vcol->expr_str.str, expr_str.length) == 0;
+  }
 };
 
 class Field
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
@@ -6146,6 +6146,7 @@ static bool fill_alter_inplace_info(THD *thd,
     c) flags passed to storage engine contain more detailed information
        about nature of changes than those provided from parser.
   */
+  bool maybe_alter_vcol= false;
   for (f_ptr= table->field; (field= *f_ptr); f_ptr++)
   {
     /* Clear marker for renamed or dropped field
@@ -6169,7 +6170,8 @@ static bool fill_alter_inplace_info(THD *thd,
       /*
         Check if type of column has changed to some incompatible type.
       */
-      switch (field->is_equal(new_field))
+      uint is_equal= field->is_equal(new_field);
+      switch (is_equal)
       {
       case IS_EQUAL_NO:
         /* New column type is incompatible with old one. */
@@ -6222,15 +6224,17 @@ static bool fill_alter_inplace_info(THD *thd,
       }
 
       /*
-        Check if the altered column is computed and either
+        Check if the column is computed and either
         is stored or is used in the partitioning expression.
-        TODO: Mark such a column with an alter flag only if
-        the defining expression has changed.
       */
       if (field->vcol_info && 
           (field->stored_in_db || field->vcol_info->is_in_partitioning_expr()))
       {
-        ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_VCOL;
+        if (is_equal == IS_EQUAL_NO ||
+            !field->vcol_info->is_equal(new_field->vcol_info))
+          ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_VCOL;
+        else
+          maybe_alter_vcol= true;
       }
 
       /* Check if field was renamed */
@@ -6296,6 +6300,21 @@ static bool fill_alter_inplace_info(THD *thd,
     }
   }
 
+  if (maybe_alter_vcol)
+  {
+    /*
+      No virtual column was altered, but perhaps one of the other columns was,
+      and that column was part of the vcol expression?
+      We don't detect this correctly (FIXME), so let's just say that a vcol
+      *might* be affected if any other column was altered.
+    */
+    if (ha_alter_info->handler_flags &
+          ( Alter_inplace_info::ALTER_COLUMN_TYPE
+          | Alter_inplace_info::ALTER_COLUMN_NOT_NULLABLE
+          | Alter_inplace_info::ALTER_COLUMN_OPTION ))
+      ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_VCOL;
+  }
+
   new_field_it.init(alter_info->create_list);
   while ((new_field= new_field_it++))
   {

Original file line number	Diff line number	Diff line change
`@@ -283,6 +283,13 @@ class Virtual_column_info: public Sql_alloc`
`283`	`283`	`{`
`284`	`284`	`in_partitioning_expr= TRUE;`
`285`	`285`	`}`
	`286`	`+ bool is_equal(Virtual_column_info* vcol)`
	`287`	`+ {`
	`288`	`+ return field_type == vcol->get_real_type()`
	`289`	`+ && stored_in_db == vcol->is_stored()`
	`290`	`+ && expr_str.length == vcol->expr_str.length`
	`291`	`+ && memcmp(expr_str.str, vcol->expr_str.str, expr_str.length) == 0;`
	`292`	`+ }`
`286`	`293`	`};`
`287`	`294`
`288`	`295`	`class Field`