Bug#33873: Fast ALTER TABLE doesn't work with multibyte character sets

The problem was that when comparing tables for a possible
fast alter table, the comparison was being performed using
the parsed information and not the final definition.
      
The solution is to use the possible final table layout to
compare if a fast alter is possible or not.

mysql-test/include/mix1.inc:
  Disable test case for Bug 21704 as it hasn't been fixed.
mysql-test/r/alter_table.result:
  Add test case result for Bug#33873
mysql-test/r/innodb_mysql.result:
  Update test case result
mysql-test/t/alter_table.test:
  Add test case for Bug#33873
sql/sql_table.cc:
  Use updated (final) information to compare fields.

mysql-test/include/mix1.inc

@@ -1428,29 +1428,31 @@ DROP TABLE t1;
 # Bug#21704: Renaming column does not update FK definition.
 #
 
---disable_warnings
-DROP TABLE IF EXISTS t1;
-DROP TABLE IF EXISTS t2;
---enable_warnings
-
-CREATE TABLE t1(id INT PRIMARY KEY)
-  ENGINE=innodb;
-
-CREATE TABLE t2(
-  t1_id INT PRIMARY KEY,
-  CONSTRAINT fk1 FOREIGN KEY (t1_id) REFERENCES t1(id))
-  ENGINE=innodb;
-
---echo
-
---disable_result_log
---error ER_ERROR_ON_RENAME
-ALTER TABLE t1 CHANGE id id2 INT;
---enable_result_log
-
---echo
-
-DROP TABLE t2;
-DROP TABLE t1;
+#
+# --disable_warnings
+# DROP TABLE IF EXISTS t1;
+# DROP TABLE IF EXISTS t2;
+# --enable_warnings
+#
+# CREATE TABLE t1(id INT PRIMARY KEY)
+#   ENGINE=innodb;
+#
+# CREATE TABLE t2(
+#   t1_id INT PRIMARY KEY,
+#   CONSTRAINT fk1 FOREIGN KEY (t1_id) REFERENCES t1(id))
+#   ENGINE=innodb;
+#
+# --echo
+#
+# --disable_result_log
+# --error ER_ERROR_ON_RENAME
+# ALTER TABLE t1 CHANGE id id2 INT;
+# --enable_result_log
+#
+# --echo
+#
+# DROP TABLE t2;
+# DROP TABLE t1;
+#
 
 --echo End of 5.1 tests

mysql-test/r/alter_table.result

@@ -1184,3 +1184,42 @@ check table t1;
 Table	Op	Msg_type	Msg_text
 test.t1	check	status	OK
 drop table t1;
+DROP TABLE IF EXISTS t1;
+CREATE TABLE t1 (id int, c int) character set latin1;
+INSERT INTO t1 VALUES (1,1);
+ALTER TABLE t1 CHANGE c d int;
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE d c int;
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 MODIFY c VARCHAR(10);
+affected rows: 1
+info: Records: 1  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE c d varchar(10);
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE d c varchar(10);
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+DROP TABLE t1;
+DROP TABLE IF EXISTS t1;
+CREATE TABLE t1 (id int, c int) character set utf8;
+INSERT INTO t1 VALUES (1,1);
+ALTER TABLE t1 CHANGE c d int;
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE d c int;
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 MODIFY c VARCHAR(10);
+affected rows: 1
+info: Records: 1  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE c d varchar(10);
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+ALTER TABLE t1 CHANGE d c varchar(10);
+affected rows: 0
+info: Records: 0  Duplicates: 0  Warnings: 0
+DROP TABLE t1;
+End of 5.1 tests

mysql-test/r/innodb_mysql.result

@@ -1640,19 +1640,6 @@ vid	tid	idx	name	type
 3	1	2	c1	NULL
 3	1	1	pk	NULL
 DROP TABLE t1;
-DROP TABLE IF EXISTS t1;
-DROP TABLE IF EXISTS t2;
-CREATE TABLE t1(id INT PRIMARY KEY)
-ENGINE=innodb;
-CREATE TABLE t2(
-t1_id INT PRIMARY KEY,
-CONSTRAINT fk1 FOREIGN KEY (t1_id) REFERENCES t1(id))
-ENGINE=innodb;
-
-ALTER TABLE t1 CHANGE id id2 INT;
-
-DROP TABLE t2;
-DROP TABLE t1;
 End of 5.1 tests
 drop table if exists t1, t2, t3;
 create table t1(a int);

mysql-test/t/alter_table.test

@@ -914,3 +914,37 @@ unlock tables;
 select * from t1;
 check table t1;
 drop table t1;
+
+#
+# Bug#33873: Fast ALTER TABLE doesn't work with multibyte character sets
+#
+
+--disable_warnings
+DROP TABLE IF EXISTS t1;
+--enable_warnings
+CREATE TABLE t1 (id int, c int) character set latin1;
+INSERT INTO t1 VALUES (1,1);
+--enable_info
+ALTER TABLE t1 CHANGE c d int;
+ALTER TABLE t1 CHANGE d c int;
+ALTER TABLE t1 MODIFY c VARCHAR(10);
+ALTER TABLE t1 CHANGE c d varchar(10);
+ALTER TABLE t1 CHANGE d c varchar(10);
+--disable_info
+DROP TABLE t1;
+
+--disable_warnings
+DROP TABLE IF EXISTS t1;
+--enable_warnings
+CREATE TABLE t1 (id int, c int) character set utf8;
+INSERT INTO t1 VALUES (1,1);
+--enable_info
+ALTER TABLE t1 CHANGE c d int;
+ALTER TABLE t1 CHANGE d c int;
+ALTER TABLE t1 MODIFY c VARCHAR(10);
+ALTER TABLE t1 CHANGE c d varchar(10);
+ALTER TABLE t1 CHANGE d c varchar(10);
+--disable_info
+DROP TABLE t1;
+
+--echo End of 5.1 tests

sql/sql_table.cc

@@ -5145,51 +5145,51 @@ compare_tables(TABLE *table,
   Field **f_ptr, *field;
   uint changes= 0, tmp;
   uint key_count;
-  List_iterator_fast<Create_field> new_field_it(alter_info->create_list);
-  Create_field *new_field;
+  List_iterator_fast<Create_field> new_field_it, tmp_new_field_it;
+  Create_field *new_field, *tmp_new_field;
   KEY_PART_INFO *key_part;
   KEY_PART_INFO *end;
+  THD *thd= table->in_use;
   /*
     Remember if the new definition has new VARCHAR column;
     create_info->varchar will be reset in mysql_prepare_create_table.
   */
   bool varchar= create_info->varchar;
+  /*
+    Create a copy of alter_info.
+    To compare the new and old table definitions, we need to "prepare"
+    the new definition - transform it from parser output to a format
+    that describes the final table layout (all column defaults are
+    initialized, duplicate columns are removed). This is done by
+    mysql_prepare_create_table.  Unfortunately,
+    mysql_prepare_create_table performs its transformations
+    "in-place", that is, modifies the argument.  Since we would
+    like to keep compare_tables() idempotent (not altering any
+    of the arguments) we create a copy of alter_info here and
+    pass it to mysql_prepare_create_table, then use the result
+    to evaluate possibility of fast ALTER TABLE, and then
+    destroy the copy.
+  */
+  Alter_info tmp_alter_info(*alter_info, thd->mem_root);
+  uint db_options= 0; /* not used */
+
   DBUG_ENTER("compare_tables");
 
-  {
-    THD *thd= table->in_use;
-    /*
-      Create a copy of alter_info.
-      To compare the new and old table definitions, we need to "prepare"
-      the new definition - transform it from parser output to a format
-      that describes the final table layout (all column defaults are
-      initialized, duplicate columns are removed). This is done by
-      mysql_prepare_create_table.  Unfortunately,
-      mysql_prepare_create_table performs its transformations
-      "in-place", that is, modifies the argument.  Since we would
-      like to keep compare_tables() idempotent (not altering any
-      of the arguments) we create a copy of alter_info here and
-      pass it to mysql_prepare_create_table, then use the result
-      to evaluate possibility of fast ALTER TABLE, and then
-      destroy the copy.
-    */
-    Alter_info tmp_alter_info(*alter_info, thd->mem_root);
-    uint db_options= 0; /* not used */
-    /* Create the prepared information. */
-    if (mysql_prepare_create_table(thd, create_info,
-                                   &tmp_alter_info,
-                                   (table->s->tmp_table != NO_TMP_TABLE),
-                                   &db_options,
-                                   table->file, key_info_buffer,
-                                   &key_count, 0))
-      DBUG_RETURN(1);
-    /* Allocate result buffers. */
-    if (! (*index_drop_buffer=
-           (uint*) thd->alloc(sizeof(uint) * table->s->keys)) ||
-        ! (*index_add_buffer=
-           (uint*) thd->alloc(sizeof(uint) * tmp_alter_info.key_list.elements)))
-      DBUG_RETURN(1);
-  }
+  /* Create the prepared information. */
+  if (mysql_prepare_create_table(thd, create_info,
+                                  &tmp_alter_info,
+                                  (table->s->tmp_table != NO_TMP_TABLE),
+                                  &db_options,
+                                  table->file, key_info_buffer,
+                                  &key_count, 0))
+    DBUG_RETURN(1);
+  /* Allocate result buffers. */
+  if (! (*index_drop_buffer=
+          (uint*) thd->alloc(sizeof(uint) * table->s->keys)) ||
+      ! (*index_add_buffer=
+          (uint*) thd->alloc(sizeof(uint) * tmp_alter_info.key_list.elements)))
+    DBUG_RETURN(1);
+
   /*
     Some very basic checks. If number of fields changes, or the
     handler, we need to run full ALTER TABLE. In the future
@@ -5232,19 +5232,29 @@ compare_tables(TABLE *table,
     DBUG_RETURN(0);
   }
 
+  /*
+    Use transformed info to evaluate possibility of fast ALTER TABLE
+    but use the preserved field to persist modifications.
+  */
+  new_field_it.init(alter_info->create_list);
+  tmp_new_field_it.init(tmp_alter_info.create_list);
+
   /*
     Go through fields and check if the original ones are compatible
     with new table.
   */
-  for (f_ptr= table->field, new_field= new_field_it++;
-       (field= *f_ptr); f_ptr++, new_field= new_field_it++)
+  for (f_ptr= table->field, new_field= new_field_it++,
+       tmp_new_field= tmp_new_field_it++;
+       (field= *f_ptr);
+       f_ptr++, new_field= new_field_it++,
+       tmp_new_field= tmp_new_field_it++)
   {
     /* Make sure we have at least the default charset in use. */
     if (!new_field->charset)
       new_field->charset= create_info->default_table_charset;
 
     /* Check that NULL behavior is same for old and new fields */
-    if ((new_field->flags & NOT_NULL_FLAG) !=
+    if ((tmp_new_field->flags & NOT_NULL_FLAG) !=
 	(uint) (field->flags & NOT_NULL_FLAG))
     {
       *need_copy_table= ALTER_TABLE_DATA_CHANGED;
@@ -5253,8 +5263,8 @@ compare_tables(TABLE *table,
 
     /* Don't pack rows in old tables if the user has requested this. */
     if (create_info->row_type == ROW_TYPE_DYNAMIC ||
-	(new_field->flags & BLOB_FLAG) ||
-	new_field->sql_type == MYSQL_TYPE_VARCHAR &&
+	(tmp_new_field->flags & BLOB_FLAG) ||
+	tmp_new_field->sql_type == MYSQL_TYPE_VARCHAR &&
 	create_info->row_type != ROW_TYPE_FIXED)
       create_info->table_options|= HA_OPTION_PACK_RECORD;
 
@@ -5262,11 +5272,11 @@ compare_tables(TABLE *table,
     field->flags&= ~FIELD_IS_RENAMED;
     if (my_strcasecmp(system_charset_info,
 		      field->field_name,
-		      new_field->field_name))
+		      tmp_new_field->field_name))
       field->flags|= FIELD_IS_RENAMED;      
 
     /* Evaluate changes bitmap and send to check_if_incompatible_data() */
-    if (!(tmp= field->is_equal(new_field)))
+    if (!(tmp= field->is_equal(tmp_new_field)))
     {
       *need_copy_table= ALTER_TABLE_DATA_CHANGED;
       DBUG_RETURN(0);