Hi, Jan! A question and a suggestion, see below On Jul 27, jan wrote:
revision-id: 4c26b71c77d58fbeb3192e2142ef66efe5f122dd (mariadb-10.0.31-43-g4c26b71c77d) parent(s): a8c1817846eb9f3fd8738e80db87cff642b9a78b author: Jan Lindström committer: Jan Lindström timestamp: 2017-07-27 13:17:13 +0300 message:
MDEV-8960: Can't refer the same column twice in one ALTER TABLE
Problem was that if column was created in alter table when it was refered again it was not tried to find from list of current columns.
mysql_prepare_alter_table: There is two cases (1) If alter table adds a new column and then later alter changes the field definition, there was no check from list of new columns, instead an incorrect error was given. (2) If alter table adds a new column and then later alter changes the default, there was no check from list of new columns, instead an incorrect error was given.
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 2eff8fd5e2f..3ba3ec6847e 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -7503,9 +7503,25 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, { if (def->change && ! def->field) { - my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, - table->s->table_name.str); - goto err; + /* + Check if there is modify for newly added field. + */ + Create_field *find; + find_it.rewind(); + while((find=find_it++)) + { + if (!my_strcasecmp(system_charset_info,find->field_name, def->field_name)) + break; + } + + if (find && !find->field)
Can it be here that find->field != NULL? I don't see how. But if yes, the ER_BAD_FIELD_ERROR looks rather weird in that case. If not - simple if (find) would be less confusing.
+ find_it.remove(); + else + { + my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, + table->s->table_name.str); + goto err; + } } /* Check that the DATE/DATETIME not null field we are going to add is @@ -7571,6 +7587,29 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, find_it.after(def); // Put column after this } } + /* + Check if there is alter for newly added field. + */ + alter_it.rewind(); + Alter_column *alter; + while ((alter=alter_it++)) + { + if (!my_strcasecmp(system_charset_info,def->field_name, alter->name)) + break; + } + if (alter) + { + if (def->sql_type == MYSQL_TYPE_BLOB) + { + my_error(ER_BLOB_CANT_HAVE_DEFAULT, MYF(0), def->change); + goto err; + } + if ((def->def=alter->def)) // Use new default + def->flags&= ~NO_DEFAULT_VALUE_FLAG; + else + def->flags|= NO_DEFAULT_VALUE_FLAG; + alter_it.remove(); + }
Please, move this into a function. Don't just copy 20 lines around. For example if (apply_alter_column(alter_it, def, field->field_name)) goto err;
} if (alter_info->alter_list.elements) {
Regards, Sergei Chief Architect MariaDB and security@mariadb.org