From 1182451af1081cb4c08459669255bd9bc66544c4 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Sat, 26 Aug 2023 13:32:47 +1000 Subject: [PATCH] MDEV-32018 Allow the setting of Auto_increment on FK referenced columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In MDEV-31086, SET FOREIGN_KEY_CHECKS=0 cannot bypass checks that make column types of foreign keys incompatible. An unfortunate consequence is that adding an AUTO_INCREMENT is considered incompatible in Field_{num,decimal}::is_equal and for the purpose of FK checks this isn't relevant. innodb.foreign_key - pragmaticly left wait_until_count_sessions.inc at end of test to match the second line of test. Reporter: horrockss@github - https://github.com/MariaDB/mariadb-docker/issues/528 Co-Author: Marko Mäkelä Reviewer: Nikita Malyavin For the future reader this was attempted: Removing AUTO_INCREMENT checks from Field_{num,decimal}::is_equals failed in the following locations (noted for future fixing): * MyISAM and Aria (not InnoDB) don't adjust AUTO_INCREMENT next number correctly, hence added a test to main.auto_increment to catch the next person that attempts this fix. * InnoDB must perform an ALGORITHM=COPY to populate NULL values of an original table (MDEV-19190 mtr test period.copy), this requires ALTER_STORED_COLUMN_TYPE to be set in fill_alter_inplace_info which doesn't get hit because field->is_equal is true. * InnoDB must not perform the change inplace (below patch) * innodb.innodb-alter-timestamp main.partition_innodb test would also need futher investigation. InnoDB ha_innobase::check_if_supported_inplace_alter to support the removal of Field_{num,decimal}::is_equal AUTO_INCREMENT checks would need the following change diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index a5ccb1957f3..9d778e2d39a 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -2455,10 +2455,15 @@ ha_innobase::check_if_supported_inplace_alter( /* An AUTO_INCREMENT attribute can only be added to an existing column by ALGORITHM=COPY, but we can remove the attribute. */ - ut_ad((MTYP_TYPENR((*af)->unireg_check) - != Field::NEXT_NUMBER) - || (MTYP_TYPENR(f->unireg_check) - == Field::NEXT_NUMBER)); + if ((MTYP_TYPENR((*af)->unireg_check) + == Field::NEXT_NUMBER) + && (MTYP_TYPENR(f->unireg_check) + != Field::NEXT_NUMBER)) + { + ha_alter_info->unsupported_reason = my_get_err_msg( + ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_AUTOINC); + DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED); + } With this change the main.auto_increment test for bug #14573, under innodb, will pass without the 2 --error ER_DUP_ENTRY entries. The function header comment was updated to reflect the MDEV-31086 changes. --- mysql-test/suite/innodb/r/foreign_key.result | 15 +++++++++++++ mysql-test/suite/innodb/t/foreign_key.test | 22 +++++++++++++++++++- sql/sql_table.cc | 16 +++++++++++--- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/mysql-test/suite/innodb/r/foreign_key.result b/mysql-test/suite/innodb/r/foreign_key.result index 68a54344519..f87aeb56b77 100644 --- a/mysql-test/suite/innodb/r/foreign_key.result +++ b/mysql-test/suite/innodb/r/foreign_key.result @@ -892,4 +892,19 @@ DROP INDEX fx ON t1; INSERT INTO t1 VALUES (2,11,11); DROP TABLE t1; SET FOREIGN_KEY_CHECKS=DEFAULT; +# +# MDEV-32018 Allow the setting of Auto_increment on FK referenced columns +# +CREATE TABLE t1 ( +id int unsigned NOT NULL PRIMARY KEY +); +CREATE TABLE t2 ( +id int unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY, +t1_id int unsigned DEFAULT NULL, +CONSTRAINT FK_t1_id FOREIGN KEY (t1_id) REFERENCES t1 (id) +); +ALTER TABLE t1 MODIFY id INT unsigned AUTO_INCREMENT; +DROP TABLE t1,t2; +# # End of 10.4 tests +# diff --git a/mysql-test/suite/innodb/t/foreign_key.test b/mysql-test/suite/innodb/t/foreign_key.test index c4b2b8331b0..032bab2408d 100644 --- a/mysql-test/suite/innodb/t/foreign_key.test +++ b/mysql-test/suite/innodb/t/foreign_key.test @@ -935,6 +935,26 @@ INSERT INTO t1 VALUES (2,11,11); DROP TABLE t1; SET FOREIGN_KEY_CHECKS=DEFAULT; --- echo # End of 10.4 tests +--echo # +--echo # MDEV-32018 Allow the setting of Auto_increment on FK referenced columns +--echo # + +CREATE TABLE t1 ( + id int unsigned NOT NULL PRIMARY KEY +); + +CREATE TABLE t2 ( + id int unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY, + t1_id int unsigned DEFAULT NULL, + CONSTRAINT FK_t1_id FOREIGN KEY (t1_id) REFERENCES t1 (id) +); + +ALTER TABLE t1 MODIFY id INT unsigned AUTO_INCREMENT; + +DROP TABLE t1,t2; + +--echo # +--echo # End of 10.4 tests +--echo # --source include/wait_until_count_sessions.inc diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 987c66e8aeb..6ffdea3b8cd 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9135,8 +9135,7 @@ enum fk_column_change_type @retval FK_COLUMN_NO_CHANGE No significant changes are to be done on foreign key columns. @retval FK_COLUMN_DATA_CHANGE ALTER TABLE might result in value - change in foreign key column (and - foreign_key_checks is on). + change in foreign key column. @retval FK_COLUMN_RENAMED Foreign key column is renamed. @retval FK_COLUMN_DROPPED Foreign key column is dropped. */ @@ -9172,7 +9171,18 @@ fk_check_column_changes(THD *thd, Alter_info *alter_info, return FK_COLUMN_RENAMED; } - if ((old_field->is_equal(*new_field) == IS_EQUAL_NO) || + /* + Field_{num|decimal}::is_equal evaluates to IS_EQUAL_NO where + the new_field adds an AUTO_INCREMENT flag on a column due to a + limitation in MyISAM/ARIA. For the purposes of FK determination + it doesn't matter if AUTO_INCREMENT is there or not. + */ + const uint flags= new_field->flags; + new_field->flags&= ~AUTO_INCREMENT_FLAG; + const bool equal_result= old_field->is_equal(*new_field); + new_field->flags= flags; + + if ((equal_result == IS_EQUAL_NO) || ((new_field->flags & NOT_NULL_FLAG) && !(old_field->flags & NOT_NULL_FLAG))) {