MDEV-22775 [HY000][1553] Changing name of primary key column with foreign key constraint fails.

abarkov · abarkov · commit 58780b5afbd0 · 2021-04-07T12:09:53.000+04:00
Problem:

The problem happened because of a conceptual flaw in the server code:

a. The table level CHARSET/COLLATE clause affected all data types,
  including numeric and temporal ones:

   CREATE TABLE t1 (a INT) CHARACTER SET utf8 [COLLATE utf8_general_ci];

  In the above example, the Column_definition_attributes
  (and then the FRM record) for the column "a" erroneously inherited
  "utf8" as its character set.

b. The "ALTER TABLE t1 CONVERT TO CHARACTER SET csname" statement
   also erroneously affected Column_definition_attributes::charset
   for numeric and temporal data types and wrote "csname" as their
   character set into FRM files.

So now we have arbitrary non-relevant charset ID values for numeric
and temporal data types in all FRM files in the world :)

The code in the server and the other engines did not seem to be affected
by this flaw. Only InnoDB inplace ALTER was affected.

Solution:

Fixing the code in the way that only character string data types
(CHAR,VARCHAR,TEXT,ENUM,SET):
- inherit the table level CHARSET/COLLATE clause
- get the charset value according to "CONVERT TO CHARACTER SET csname".

Numeric and temporal data types now always get &amp;my_charset_numeric
in Column_definition_attributes::charset and always write its ID into FRM files:
- no matter what the table level CHARSET/COLLATE clause is, and
- no matter what "CONVERT TO CHARACTER SET" says.

Details:

1. Adding helper classes to pass small parts of HA_CREATE_INFO
   into Type_handler methods:

   - Column_derived_attributes - to pass table level CHARSET/COLLATE,
     so columns that do not have explicit CHARSET/COLLATE clauses
     can derive them from the table level, e.g.

       CREATE TABLE t1 (a VARCHAR(1), b CHAR(1)) CHARACTER SET utf8;

   - Column_bulk_alter_attributes - to pass bulk attribute changes
     generated by the ALTER related code. These bulk changes affect
     multiple columns at the same time:

       ALTER TABLE ... CONVERT TO CHARACTER SET csname;

   Note, passing the whole HA_CREATE_INFO directly to Type_handler
   would not be good: HA_CREATE_INFO is huge and would need not desired
   dependencies in sql_type.h and sql_type.cc. The Type_handler API should
   use smallest possible data types!

2. Type_handler::Column_definition_prepare_stage1() is now responsible
   to set Column_definition::charset properly, according to the data type,
   for example:

   - For string data types, Column_definition_attributes::charset is set from
     the table level CHARSET/COLLATE clause (if not specified explicitly in
     the column definition).

   - For numeric and temporal fields, Column_definition_attributes::charset is
     set to &amp;my_charset_numeric, no matter what the table level
     CHARSET/COLLATE says.

   - For GEOMETRY, Column_definition_attributes::charset is set to
     &amp;my_charset_bin, no matter what the table level CHARSET/COLLATE says.

   Previously this code (setting `charset`) was outside of of
   Column_definition_prepare_stage1(), namely in
   mysql_prepare_create_table(), and was erroneously called for
   all data types.

3. Adding Type_handler::Column_definition_bulk_alter(), to handle
   "ALTER TABLE .. CONVERT TO". Previously this code was inside
   get_sql_field_charset() and was erroneously called for all data types.

4. Removing the Schema_specification_st parameter from
   Type_handler::Column_definition_redefine_stage1().
   Column_definition_attributes::charset is now fully properly initialized by
   Column_definition_prepare_stage1(). So we don't need access to the
   table level CHARSET/COLLATE clause in Column_definition_redefine_stage1()
   any more.

5. Other changes:
   - Removing global function get_sql_field_charset()

   - Moving the part of the former get_sql_field_charset(), which was
     responsible to inherit the table level CHARSET/COLLATE clause to
     new methods:
      -- Column_definition_attributes::explicit_or_derived_charset() and
      -- Column_definition::prepare_charset_for_string().
     This code is only needed for string data types.
     Previously it was erroneously called for all data types.

   - Moving another part, which was responsible to apply the
     "CONVERT TO" clause, to
     Type_handler_general_purpose_string::Column_definition_bulk_alter().

   - Replacing the call for get_sql_field_charset() in sql_partition.cc
     to sql_field-&gt;explicit_or_derived_charset() - it is perfectly enough.
     The old code was redundant: get_sql_field_charset() was called from
     sql_partition.cc only when there were no a "CONVERT TO CHARACTER SET"
     clause involved, so its purpose was only to inherit the table
     level CHARSET/COLLATE clause.

   - Moving the code handling the BINCMP_FLAG flag from
     mysql_prepare_create_table() to
     Column_definition::prepare_charset_for_string():
     This code is responsible to resolve the BINARY comparison style
     into the corresponding _bin collation, to do the following transparent
     rewrite:
        CREATE TABLE t1 (a VARCHAR(10) BINARY) CHARSET utf8;  -&gt;
        CREATE TABLE t1 (a VARCHAR(10) CHARACTER SET utf8 COLLATE utf8_bin);
     This code is only needed for string data types.
     Previously it was erroneously called for all data types.

6. Renaming Table_scope_and_contents_source_pod_st::table_charset
   to alter_table_convert_to_charset, because the only purpose it's used for
   is handlering "ALTER .. CONVERT". The new name is much more self-descriptive.
diff --git a/mysql-test/suite/innodb/r/instant_alter_charset.result b/mysql-test/suite/innodb/r/instant_alter_charset.result
@@ -2032,3 +2032,14 @@ ALTER TABLE t1 MODIFY a VARCHAR(2)
 CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;
 INSERT INTO t1 VALUES ('a');
 DROP TABLE t1;
+#
+# MDEV-22775 [HY000][1553] Changing name of primary key column with foreign key constraint fails.
+#
+create table t1 (id int primary key) engine=innodb default charset=utf8;
+create table t2 (input_id int primary key, id int not null,
+key a (id),
+constraint a foreign key (id) references t1  (id)
+)engine=innodb default charset=utf8;
+alter table t1 change id id2 int;
+drop table t2;
+drop table t1;
diff --git a/mysql-test/suite/innodb/t/instant_alter_charset.test b/mysql-test/suite/innodb/t/instant_alter_charset.test
@@ -843,3 +843,17 @@ ALTER TABLE t1 MODIFY a VARCHAR(2)
 CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;
 INSERT INTO t1 VALUES ('a');
 DROP TABLE t1;
+
+
+--echo #
+--echo # MDEV-22775 [HY000][1553] Changing name of primary key column with foreign key constraint fails.
+--echo #
+
+create table t1 (id int primary key) engine=innodb default charset=utf8;
+create table t2 (input_id int primary key, id int not null,
+    key a (id),
+    constraint a foreign key (id) references t1  (id)
+)engine=innodb default charset=utf8;
+alter table t1 change id id2 int;
+drop table t2;
+drop table t1;
diff --git a/sql/field.cc b/sql/field.cc
@@ -11076,16 +11076,26 @@ Column_definition::Column_definition(THD *thd, Field *old_field,
     CREATE TABLE t1 (a INT) AS SELECT a FROM t2;
   See Type_handler::Column_definition_redefine_stage1()
   for data type specific code.
+
+  @param this         - The field definition corresponding to the expression
+                        in the "AS SELECT.." part.
+
+  @param dup_field    - The field definition from the "CREATE TABLE (...)" part.
+                        It has already underwent prepare_stage1(), so
+                        must be fully initialized:
+                        -- dup_field->charset is set and BINARY
+                           sorting style is applied, see find_bin_collation().
+
+  @param file         - The table handler
 */
 void
 Column_definition::redefine_stage1_common(const Column_definition *dup_field,
-                                          const handler *file,
-                                          const Schema_specification_st *schema)
+                                          const handler *file)
 {
   set_handler(dup_field->type_handler());
   default_value= dup_field->default_value;
-  charset=       dup_field->charset ? dup_field->charset :
-                                      schema->default_table_charset;
+  DBUG_ASSERT(dup_field->charset); // Set by prepare_stage1()
+  charset=      dup_field->charset;
   length=       dup_field->char_length;
   pack_length=  dup_field->pack_length;
   key_length=   dup_field->key_length;
diff --git a/sql/field.h b/sql/field.h
@@ -4631,6 +4631,11 @@ class Column_definition_attributes
   void frm_pack_charset(uchar *buff) const;
   void frm_unpack_basic(const uchar *buff);
   bool frm_unpack_charset(TABLE_SHARE *share, const uchar *buff);
+  CHARSET_INFO *explicit_or_derived_charset(const Column_derived_attributes
+                                                  *derived_attr) const
+  {
+    return charset ? charset : derived_attr->charset();
+  }
 };
 
 
@@ -4765,6 +4770,15 @@ class Column_definition: public Sql_alloc,
   void create_length_to_internal_length_bit();
   void create_length_to_internal_length_newdecimal();
 
+  /*
+    Prepare the "charset" member for string data types,
+    such as CHAR, VARCHAR, TEXT, ENUM, SET:
+    - derive the charset if not specified explicitly
+    - find a _bin collation if the BINARY comparison style was specified, e.g.:
+       CREATE TABLE t1 (a VARCHAR(10) BINARY) CHARSET utf8;
+  */
+  bool prepare_charset_for_string(const Column_derived_attributes *dattr);
+
   /**
     Prepare a SET/ENUM field.
     Create "interval" from "interval_list" if needed, and adjust "length".
@@ -4800,23 +4814,33 @@ class Column_definition: public Sql_alloc,
   bool sp_prepare_create_field(THD *thd, MEM_ROOT *mem_root);
 
   bool prepare_stage1(THD *thd, MEM_ROOT *mem_root,
-                      handler *file, ulonglong table_flags);
+                      handler *file, ulonglong table_flags,
+                      const Column_derived_attributes *derived_attr);
+  void prepare_stage1_simple(CHARSET_INFO *cs)
+  {
+    charset= cs;
+    create_length_to_internal_length_simple();
+  }
   bool prepare_stage1_typelib(THD *thd, MEM_ROOT *mem_root,
                               handler *file, ulonglong table_flags);
   bool prepare_stage1_string(THD *thd, MEM_ROOT *mem_root,
                              handler *file, ulonglong table_flags);
   bool prepare_stage1_bit(THD *thd, MEM_ROOT *mem_root,
                           handler *file, ulonglong table_flags);
 
+  bool bulk_alter(const Column_derived_attributes *derived_attr,
+                  const Column_bulk_alter_attributes *bulk_attr)
+  {
+    return type_handler()->Column_definition_bulk_alter(this,
+                                                        derived_attr,
+                                                        bulk_attr);
+  }
   void redefine_stage1_common(const Column_definition *dup_field,
-                              const handler *file,
-                              const Schema_specification_st *schema);
-  bool redefine_stage1(const Column_definition *dup_field, const handler *file,
-                       const Schema_specification_st *schema)
+                              const handler *file);
+  bool redefine_stage1(const Column_definition *dup_field, const handler *file)
   {
     const Type_handler *handler= dup_field->type_handler();
-    return handler->Column_definition_redefine_stage1(this, dup_field,
-                                                      file, schema);
+    return handler->Column_definition_redefine_stage1(this, dup_field, file);
   }
   bool prepare_stage2(handler *handler, ulonglong table_flags);
   bool prepare_stage2_blob(handler *handler,
diff --git a/sql/handler.h b/sql/handler.h
@@ -2092,7 +2092,7 @@ struct Vers_parse_info: public Table_period_info
 
 struct Table_scope_and_contents_source_pod_st // For trivial members
 {
-  CHARSET_INFO *table_charset;
+  CHARSET_INFO *alter_table_convert_to_charset;
   LEX_CUSTRING tabledef_version;
   LEX_CSTRING connect_string;
   LEX_CSTRING comment;
@@ -2237,7 +2237,7 @@ struct HA_CREATE_INFO: public Table_scope_and_contents_source_st,
     DBUG_ASSERT(cs);
     if (check_conflicting_charset_declarations(cs))
       return true;
-    table_charset= default_table_charset= cs;
+    alter_table_convert_to_charset= default_table_charset= cs;
     used_fields|= (HA_CREATE_USED_CHARSET | HA_CREATE_USED_DEFAULT_CHARSET);  
     return false;
   }
diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
@@ -2388,6 +2388,8 @@ static int add_column_list_values(String *str, partition_info *part_info,
         */
         if (create_info)
         {
+          const Column_derived_attributes
+            derived_attr(create_info->default_table_charset);
           Create_field *sql_field;
 
           if (!(sql_field= get_sql_field(field_name,
@@ -2402,7 +2404,7 @@ static int add_column_list_values(String *str, partition_info *part_info,
                                &need_cs_check))
             return 1;
           if (need_cs_check)
-            field_cs= get_sql_field_charset(sql_field, create_info);
+            field_cs= sql_field->explicit_or_derived_charset(&derived_attr);
           else
             field_cs= NULL;
         }
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
@@ -2926,6 +2926,15 @@ bool check_duplicates_in_interval(const char *set_or_name,
 }
 
 
+bool Column_definition::
+       prepare_charset_for_string(const Column_derived_attributes *dattr)
+{
+  if (!charset)
+    charset= dattr->charset();
+  return (flags & BINCMP_FLAG) && !(charset= find_bin_collation(charset));
+}
+
+
 bool Column_definition::prepare_stage2_blob(handler *file,
                                             ulonglong table_flags,
                                             uint field_flags)
@@ -3007,38 +3016,6 @@ bool Column_definition::prepare_stage2(handler *file,
 }
 
 
-/*
-  Get character set from field object generated by parser using
-  default values when not set.
-
-  SYNOPSIS
-    get_sql_field_charset()
-    sql_field                 The sql_field object
-    create_info               Info generated by parser
-
-  RETURN VALUES
-    cs                        Character set
-*/
-
-CHARSET_INFO* get_sql_field_charset(Column_definition *sql_field,
-                                    HA_CREATE_INFO *create_info)
-{
-  CHARSET_INFO *cs= sql_field->charset;
-
-  if (!cs)
-    cs= create_info->default_table_charset;
-  /*
-    table_charset is set only in ALTER TABLE t1 CONVERT TO CHARACTER SET csname
-    if we want change character set for all varchar/char columns.
-    But the table charset must not affect the BLOB fields, so don't
-    allow to change my_charset_bin to somethig else.
-  */
-  if (create_info->table_charset && cs != &my_charset_bin)
-    cs= create_info->table_charset;
-  return cs;
-}
-
-
 /**
    Modifies the first column definition whose SQL type is TIMESTAMP
    by adding the features DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP.
@@ -3211,11 +3188,14 @@ bool Column_definition::prepare_stage1_bit(THD *thd,
 bool Column_definition::prepare_stage1(THD *thd,
                                        MEM_ROOT *mem_root,
                                        handler *file,
-                                       ulonglong table_flags)
+                                       ulonglong table_flags,
+                                       const Column_derived_attributes
+                                             *derived_attr)
 {
   return type_handler()->Column_definition_prepare_stage1(thd, mem_root,
                                                           this, file,
-                                                          table_flags);
+                                                          table_flags,
+                                                          derived_attr);
 }
 
 
@@ -3428,6 +3408,9 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
   int select_field_count= C_CREATE_SELECT(create_table_mode);
   bool tmp_table= create_table_mode == C_ALTER_TABLE;
   bool is_hash_field_needed= false;
+  const Column_derived_attributes dattr(create_info->default_table_charset);
+  const Column_bulk_alter_attributes
+    battr(create_info->alter_table_convert_to_charset);
   DBUG_ENTER("mysql_prepare_create_table");
 
   DBUG_EXECUTE_IF("test_pseudo_invisible",{
@@ -3484,26 +3467,27 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
 
   for (field_no=0; (sql_field=it++) ; field_no++)
   {
+    /* Virtual fields are always NULL */
+    if (sql_field->vcol_info)
+      sql_field->flags&= ~NOT_NULL_FLAG;
+
     /*
       Initialize length from its original value (number of characters),
       which was set in the parser. This is necessary if we're
       executing a prepared statement for the second time.
     */
     sql_field->length= sql_field->char_length;
-    /* Set field charset. */
-    sql_field->charset= get_sql_field_charset(sql_field, create_info);
-    if ((sql_field->flags & BINCMP_FLAG) &&
-        !(sql_field->charset= find_bin_collation(sql_field->charset)))
-      DBUG_RETURN(true);
 
-    /* Virtual fields are always NULL */
-    if (sql_field->vcol_info)
-      sql_field->flags&= ~NOT_NULL_FLAG;
+    if (sql_field->bulk_alter(&dattr, &battr))
+      DBUG_RETURN(true);
 
     if (sql_field->prepare_stage1(thd, thd->mem_root,
-                                  file, file->ha_table_flags()))
+                                  file, file->ha_table_flags(),
+                                  &dattr))
       DBUG_RETURN(true);
 
+    DBUG_ASSERT(sql_field->charset);
+
     if (sql_field->real_field_type() == MYSQL_TYPE_BIT &&
         file->ha_table_flags() & HA_CAN_BIT_FIELD)
       total_uneven_bit_length+= sql_field->length & 7;
@@ -3554,7 +3538,7 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
           if (!(sql_field->flags & NOT_NULL_FLAG))
             null_fields--;
 
-          if (sql_field->redefine_stage1(dup_field, file, create_info))
+          if (sql_field->redefine_stage1(dup_field, file))
             DBUG_RETURN(true);
 
 	  it2.remove();			// Remove first (create) definition
@@ -4569,7 +4553,9 @@ bool Column_definition::prepare_blob_field(THD *thd)
 
 bool Column_definition::sp_prepare_create_field(THD *thd, MEM_ROOT *mem_root)
 {
-  return prepare_stage1(thd, mem_root, NULL, HA_CAN_GEOMETRY) ||
+  DBUG_ASSERT(charset);
+  const Column_derived_attributes dattr(&my_charset_bin);
+  return prepare_stage1(thd, mem_root, NULL, HA_CAN_GEOMETRY, &dattr) ||
          prepare_stage2(NULL, HA_CAN_GEOMETRY);
 }
 
@@ -11331,8 +11317,8 @@ bool Sql_cmd_create_table_like::execute(THD *thd)
   {
     create_info.used_fields&= ~HA_CREATE_USED_CHARSET;
     create_info.used_fields|= HA_CREATE_USED_DEFAULT_CHARSET;
-    create_info.default_table_charset= create_info.table_charset;
-    create_info.table_charset= 0;
+    create_info.default_table_charset= create_info.alter_table_convert_to_charset;
+    create_info.alter_table_convert_to_charset= 0;
   }
 
   /*
diff --git a/sql/sql_table.h b/sql/sql_table.h
@@ -252,8 +252,6 @@ bool quick_rm_table(THD *thd, handlerton *base, const LEX_CSTRING *db,
                     const char *table_path=0);
 void close_cached_table(THD *thd, TABLE *table);
 void sp_prepare_create_field(THD *thd, Column_definition *sql_field);
-CHARSET_INFO* get_sql_field_charset(Column_definition *sql_field,
-                                    HA_CREATE_INFO *create_info);
 bool mysql_write_frm(ALTER_PARTITION_PARAM_TYPE *lpt, uint flags);
 int write_bin_log(THD *thd, bool clear_error,
                   char const *query, ulong query_length,
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
diff --git a/sql/sql_type.h b/sql/sql_type.h
diff --git a/sql/table.cc b/sql/table.cc
diff --git a/storage/federatedx/ha_federatedx.cc b/storage/federatedx/ha_federatedx.cc
