commit 35e580ffec55293651a6cbd616b3d4b7ed1e3d4e Author: Laurynas Biveinis Date: Thu Jun 1 17:39:32 2017 +0300 Fix bug 77591 / PS-1635 (ALTER TABLE does not allow to change NULL/NOT NULL if foreign key exists) Make fk_check_column_changes tell apart NULL -> NOT NULL column changes from all other changes, and assume that this this change is safe for referenced parent table columns while keeping it unsafe for FK child table columns. At the same time convert this enum to an enum class for better type safety. Add a testcase. diff --git a/mysql-test/r/bug77591.result b/mysql-test/r/bug77591.result new file mode 100644 index 00000000000..506e0c19bb8 --- /dev/null +++ b/mysql-test/r/bug77591.result @@ -0,0 +1,42 @@ +# +# Bug 77591 (ALTER TABLE does not allow to change NULL/NOT NULL if foreign key exists) +# +CREATE TABLE t1(x VARCHAR(36) DEFAULT NULL, UNIQUE(x)) ENGINE=InnoDB; +CREATE TABLE t2(y VARCHAR(36) DEFAULT NULL, +FOREIGN KEY(y) REFERENCES t1(x)) ENGINE=InnoDB; +INSERT INTO t1 VALUES ("foo"), (NULL), ("bar"); +INSERT INTO t2 VALUES ("foo"), (NULL), ("bar"); +SELECT * FROM t1 ORDER BY x ASC; +x +NULL +bar +foo +SELECT * FROM t2 ORDER BY y ASC; +y +NULL +bar +foo +Warnings: +Warning 3135 'NO_ZERO_DATE', 'NO_ZERO_IN_DATE' and 'ERROR_FOR_DIVISION_BY_ZERO' sql modes should be used with strict mode. They will be merged with strict mode in a future release. +ALTER TABLE t1 CHANGE COLUMN x x VARCHAR(36) NOT NULL; +Warnings: +Warning 1265 Data truncated for column 'x' at row 2 +SELECT * FROM t1; +x + +bar +foo +SELECT * FROM t2; +y +NULL +bar +foo +DROP TABLE t2, t1; +CREATE TABLE t1 (x INT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (y INT PRIMARY KEY, z INT DEFAULT NULL, +FOREIGN KEY(z) REFERENCES t1(x)) ENGINE=InnoDB; +INSERT INTO t1 VALUES (0); +INSERT INTO t2 VALUES (0, NULL); +ALTER TABLE t2 CHANGE COLUMN z z INT NOT NULL; +ERROR HY000: Cannot change column 'z': used in a foreign key constraint 't2_ibfk_1' +DROP TABLE t2, t1; diff --git a/mysql-test/t/bug77591.test b/mysql-test/t/bug77591.test new file mode 100644 index 00000000000..cae4f4ea34d --- /dev/null +++ b/mysql-test/t/bug77591.test @@ -0,0 +1,41 @@ +--echo # +--echo # Bug 77591 (ALTER TABLE does not allow to change NULL/NOT NULL if foreign key exists) +--echo # + +CREATE TABLE t1(x VARCHAR(36) DEFAULT NULL, UNIQUE(x)) ENGINE=InnoDB; + +CREATE TABLE t2(y VARCHAR(36) DEFAULT NULL, + FOREIGN KEY(y) REFERENCES t1(x)) ENGINE=InnoDB; + +INSERT INTO t1 VALUES ("foo"), (NULL), ("bar"); +INSERT INTO t2 VALUES ("foo"), (NULL), ("bar"); + +SELECT * FROM t1 ORDER BY x ASC; +SELECT * FROM t2 ORDER BY y ASC; + +--source include/turn_off_strict_mode.inc + +# With the bug present this fails with ER_FK_COLUMN_CANNOT_CHANGE_CHILD +ALTER TABLE t1 CHANGE COLUMN x x VARCHAR(36) NOT NULL; + +SELECT * FROM t1; +SELECT * FROM t2; + +DROP TABLE t2, t1; + +# Check that we still do not allow NULL -> NOT NULL transition for FK columns +CREATE TABLE t1 (x INT PRIMARY KEY) ENGINE=InnoDB; + +CREATE TABLE t2 (y INT PRIMARY KEY, z INT DEFAULT NULL, + FOREIGN KEY(z) REFERENCES t1(x)) ENGINE=InnoDB; + +INSERT INTO t1 VALUES (0); + +INSERT INTO t2 VALUES (0, NULL); + +--error ER_FK_COLUMN_CANNOT_CHANGE +ALTER TABLE t2 CHANGE COLUMN z z INT NOT NULL; + +--source include/restore_strict_mode.inc + +DROP TABLE t2, t1; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index d4561db6d15..7f843e47508 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10894,11 +10894,12 @@ static const Create_field *get_field_by_old_name(Alter_info *alter_info, /** Type of change to foreign key column, */ -enum fk_column_change_type { - FK_COLUMN_NO_CHANGE, - FK_COLUMN_DATA_CHANGE, - FK_COLUMN_RENAMED, - FK_COLUMN_DROPPED +enum class fk_column_change_type { + NO_CHANGE, + DATA_CHANGE, + RENAMED, + DROPPED, + SAFE_FOR_PARENT }; /** @@ -10914,13 +10915,15 @@ enum fk_column_change_type { @note This function takes into account value of @@foreign_key_checks setting. - @retval FK_COLUMN_NO_CHANGE No significant changes are to be done on + @retval NO_CHANGE No significant changes are to be done on foreign key columns. - @retval FK_COLUMN_DATA_CHANGE ALTER TABLE might result in value + @retval DATA_CHANGE ALTER TABLE might result in value change in foreign key column (and foreign_key_checks is on). - @retval FK_COLUMN_RENAMED Foreign key column is renamed. - @retval FK_COLUMN_DROPPED Foreign key column is dropped. + @retval ENAMED Foreign key column is renamed. + @retval DROPPED Foreign key column is dropped. + @retval SAFE_FOR_PARENT The column change is safe if this is a + referenced column. */ static enum fk_column_change_type fk_check_column_changes( @@ -10947,12 +10950,14 @@ static enum fk_column_change_type fk_check_column_changes( like it happens in case of in-place algorithm. */ *bad_column_name = column->str; - return FK_COLUMN_RENAMED; + return fk_column_change_type::RENAMED; } - if ((old_field->is_equal(new_field) == IS_EQUAL_NO) || - ((new_field->flags & NOT_NULL_FLAG) && - !(old_field->flags & NOT_NULL_FLAG))) { + const auto fields_differ = + (old_field->is_equal(new_field) == IS_EQUAL_NO); + + if (fields_differ || ((new_field->flags & NOT_NULL_FLAG) && + !(old_field->flags & NOT_NULL_FLAG))) { if (!(thd->variables.option_bits & OPTION_NO_FOREIGN_KEY_CHECKS)) { /* Column in a FK has changed significantly. Unless @@ -10961,7 +10966,9 @@ static enum fk_column_change_type fk_check_column_changes( and thus referential integrity might be broken, */ *bad_column_name = column->str; - return FK_COLUMN_DATA_CHANGE; + /* NULL to NOT NULL column change is safe for referenced columns */ + return fields_differ ? fk_column_change_type::DATA_CHANGE + : fk_column_change_type::SAFE_FOR_PARENT; } } DBUG_ASSERT(old_field->is_gcol() == new_field->is_gcol() && @@ -10979,11 +10986,11 @@ static enum fk_column_change_type fk_check_column_changes( integrity in this case. */ *bad_column_name = column->str; - return FK_COLUMN_DROPPED; + return fk_column_change_type::DROPPED; } } - return FK_COLUMN_NO_CHANGE; + return fk_column_change_type::NO_CHANGE; } /** @@ -11055,10 +11062,11 @@ static bool fk_check_copy_alter_table(THD *thd, TABLE *table, &bad_column_name); switch (changes) { - case FK_COLUMN_NO_CHANGE: + case fk_column_change_type::NO_CHANGE: + case fk_column_change_type::SAFE_FOR_PARENT: /* No significant changes. We can proceed with ALTER! */ break; - case FK_COLUMN_DATA_CHANGE: { + case fk_column_change_type::DATA_CHANGE: { char buff[NAME_LEN * 2 + 2]; strxnmov(buff, sizeof(buff) - 1, f_key->foreign_db->str, ".", f_key->foreign_table->str, NullS); @@ -11066,13 +11074,13 @@ static bool fk_check_copy_alter_table(THD *thd, TABLE *table, f_key->foreign_id->str, buff); DBUG_RETURN(true); } - case FK_COLUMN_RENAMED: + case fk_column_change_type::RENAMED: my_error(ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, MYF(0), "ALGORITHM=COPY", ER_THD(thd, ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_FK_RENAME), "ALGORITHM=INPLACE"); DBUG_RETURN(true); - case FK_COLUMN_DROPPED: { + case fk_column_change_type::DROPPED: { char buff[NAME_LEN * 2 + 2]; strxnmov(buff, sizeof(buff) - 1, f_key->foreign_db->str, ".", f_key->foreign_table->str, NullS); @@ -11115,20 +11123,21 @@ static bool fk_check_copy_alter_table(THD *thd, TABLE *table, &bad_column_name); switch (changes) { - case FK_COLUMN_NO_CHANGE: + case fk_column_change_type::NO_CHANGE: /* No significant changes. We can proceed with ALTER! */ break; - case FK_COLUMN_DATA_CHANGE: + case fk_column_change_type::SAFE_FOR_PARENT: + case fk_column_change_type::DATA_CHANGE: my_error(ER_FK_COLUMN_CANNOT_CHANGE, MYF(0), bad_column_name, f_key->foreign_id->str); DBUG_RETURN(true); - case FK_COLUMN_RENAMED: + case fk_column_change_type::RENAMED: my_error(ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, MYF(0), "ALGORITHM=COPY", ER_THD(thd, ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_FK_RENAME), "ALGORITHM=INPLACE"); DBUG_RETURN(true); - case FK_COLUMN_DROPPED: + case fk_column_change_type::DROPPED: // Should already have been checked in column_used_by_foreign_key(). DBUG_ASSERT(false); default: