Re: 692ca003f8c: MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE
Hi, Nikita, On Oct 18, Nikita Malyavin wrote:
revision-id: 692ca003f8c (mariadb-10.5.26-169-g692ca003f8c) parent(s): ba988513ec7 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2024-10-13 19:33:14 +0200 message:
MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE
Cannot add a foreign key on a table with a long UNIQUE multi-column index, that contains a foreign key as a prefix.
Check that index algorithms match during the "generated" keys duplicate removal.
diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test index 2f85328bda3..08b49b6c5da 100644 --- a/mysql-test/main/long_unique_bugs.test +++ b/mysql-test/main/long_unique_bugs.test @@ -706,4 +706,19 @@ alter table t1 enable keys; insert into t1 values (2); drop table t1;
+--echo # +--echo # Cannot add a foreign key on a table with a long UNIQUE multi-column +--echo # index, that contains a foreign key as a prefix. +--echo # +create table a (id int(19) primary key) engine = innodb; +create table b (id int primary key, + long_text varchar(1000), + constraint unique_b unique key (id, long_text) + ) engine = innodb default charset utf8mb4; + +alter table b add constraint b_fk_ida foreign key (id) references a (id); +show create table b; +drop table b; +drop table a;
Two problems with this test case. A minor one - it doesn't start with the --echo # MDEV-33658 Cannot add a foreign key on a table with a matching long UNIQUE line. Please add it. A more serious problem - it doesn't fail on a vanilla 10.5. The original set of SQL commands from MDEV-33658 fails with "Foreign key constraint is incorrectly formed", but your test doesn't. You might've oversimplified it. Please, fix the test case to fail on vanilla 10.5 with the above error.
+ --echo # End of 10.5 tests diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 99ba9e3587b..3d01b9d5584 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -203,7 +203,7 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root)
/* Test if a foreign key (= generated key) is a prefix of the given key - (ignoring key name, key type and order of columns) + (ignoring key name, key type and order of columns, but minding the algorithm)
it doesn't ignore the order of columns and "key type" is basically algorithm, so the above can simply be "(ignoring key name)"
NOTES: This is only used to test if an index for a FOREIGN KEY exists @@ -218,6 +218,17 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root)
bool is_foreign_key_prefix(Key *a, Key *b) { + ha_key_alg a_alg= a->key_create_info.algorithm; + ha_key_alg b_alg= b->key_create_info.algorithm; + + bool is_a_btree= a_alg == HA_KEY_ALG_BTREE || a_alg == HA_KEY_ALG_UNDEF; + bool is_b_btree= b_alg == HA_KEY_ALG_BTREE || b_alg == HA_KEY_ALG_UNDEF; + + // Either the algorithms are total match, or they both are btree + // in order to be able to prefix + if (a_alg != b_alg && !(is_a_btree && is_b_btree)) + return false;
I don't see why algorithms can be not b-tree, if they match. What use case did you have in mind?
+ /* Ensure that 'a' is the generated key */ if (a->generated) {
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello, Sergei! Thanks for an instant review. I've updated the branch according to your findings, now the top commit is 331430a6a <https://github.com/MariaDB/server/commit/331430a6a738906ed4c48faa74f5ce2cb51473b1> . See the replies below. On Fri, 18 Oct 2024 at 17:17, Sergei Golubchik <serg@mariadb.org> wrote:
+--echo # +--echo # Cannot add a foreign key on a table with a long UNIQUE multi-column +--echo # index, that contains a foreign key as a prefix. +--echo # +create table a (id int(19) primary key) engine = innodb; +create table b (id int primary key, + long_text varchar(1000), + constraint unique_b unique key (id, long_text) + ) engine = innodb default charset utf8mb4; + +alter table b add constraint b_fk_ida foreign key (id) references a (id); +show create table b; +drop table b; +drop table a;
Two problems with this test case. A minor one - it doesn't start with the
--echo # MDEV-33658 Cannot add a foreign key on a table with a matching long UNIQUE
line. Please add it.
A more serious problem - it doesn't fail on a vanilla 10.5. The original set of SQL commands from MDEV-33658 fails with "Foreign key constraint is incorrectly formed", but your test doesn't. You might've oversimplified it.
Actually it was yet undersimplified: we don't need the int size, and having primary key on the column used in the foreign key has broken the test correctness.
/* Test if a foreign key (= generated key) is a prefix of the given key - (ignoring key name, key type and order of columns) + (ignoring key name, key type and order of columns, but minding the
algorithm)
it doesn't ignore the order of columns and "key type" is basically algorithm, so the above can simply be "(ignoring key name)"
Key type is Key::type, which is: enum Keytype { PRIMARY, UNIQUE, MULTIPLE, FULLTEXT, SPATIAL, FOREIGN_KEY}; enum Keytype type; So I can't agree completely. But the order of columns is indeed checked, depending on never-implemented feature macro. I'd leave it like this:
(ignoring key name and type, but minding the algorithm)
NOTES: This is only used to test if an index for a FOREIGN KEY exists @@ -218,6 +218,17 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root)
bool is_foreign_key_prefix(Key *a, Key *b) { + ha_key_alg a_alg= a->key_create_info.algorithm; + ha_key_alg b_alg= b->key_create_info.algorithm; + + bool is_a_btree= a_alg == HA_KEY_ALG_BTREE || a_alg == HA_KEY_ALG_UNDEF; + bool is_b_btree= b_alg == HA_KEY_ALG_BTREE || b_alg == HA_KEY_ALG_UNDEF; + + // Either the algorithms are total match, or they both are btree + // in order to be able to prefix + if (a_alg != b_alg && !(is_a_btree && is_b_btree)) + return false;
I don't see why algorithms can be not b-tree, if they match. What use case did you have in mind?
What I have in mind is that key->type is set according to what was parsed, and if no type was specified by the user, then I believe the type will be UNDEF, at least at this stage. But by default, I believe, we suppose the algorithm is BTREE. And I suppose that we shouldn't remove a duplicate if one is RTREE, and another one is BTREE. Is the above correct? If yes, I see now, how I can rewrite this to a better reading:
ha_key_alg a_alg= a->key_create_info.algorithm; ha_key_alg b_alg= b->key_create_info.algorithm;
// The real algorithm will be BTREE is none was given by user. a_alg= a_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : a_alg; b_alg= b_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : b_alg;
if (a_alg != b_alg) return false;
-- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik