[MDEV-28933] CREATE OR REPLACE fails to recreate same constraint name Created: 2022-06-23  Updated: 2023-05-18  Resolved: 2023-01-25

Status: Closed
Project: MariaDB Server
Component/s: Data Definition - Create Table
Affects Version/s: 10.10
Fix Version/s: N/A

Type: Bug Priority: Critical
Reporter: Lena Startseva Assignee: Aleksey Midenkov
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Blocks
blocks MDEV-25292 Atomic CREATE OR REPLACE TABLE Stalled
is blocked by MDEV-28980 InnoDB: Failing assertion: len <= MAX... Closed
Problem/Incident
is caused by MDEV-25292 Atomic CREATE OR REPLACE TABLE Stalled
Relates
relates to MDEV-29409 ASAN failure on long fk_id when renam... Closed
relates to MDEV-29258 Failing assertion for name length on ... Closed
relates to MDEV-30416 Can't redefine constraint in a single... Confirmed

 Description   

CREATE OR REPLACE failed with "Duplicate key on write or update" when it replace an empty table with the non-anonymous constraint name

From comment
Test fails due to a duplicate key on SYS_FOREIGN.NAME on the non-anonymous constraint name:

Test:

--source include/have_innodb.inc
 
CREATE TABLE t(a INT PRIMARY KEY) ENGINE=InnoDB;
CREATE OR REPLACE TABLE u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d (a) REFERENCES t (a)) ENGINE=InnoDB;
CREATE OR REPLACE TABLE u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d (a) REFERENCES t (a)) ENGINE=InnoDB;
DROP TABLE u, t;

Actual result:

At line 5: query 'CREATE OR REPLACE TABLE u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d (a) REFERENCES t (a)) ENGINE=InnoDB' failed: ER_CANT_CREATE_TABLE (1005): Can't create table `test`.`u` (errno: 121 "Duplicate key on write or update")

Expected result:
No errors



 Comments   
Comment by Marko Mäkelä [ 2022-06-28 ]

I posted some comments. I think that we must ensure that

  • the SYS_FOREIGN.ID and SYS_FOREIGN_COLS.ID do not become too long even when using innodb_page_size=4k and pathological names like 64-char database and table names that would expand 5× in my_charset_filename
  • the generated identifiers cannot be generated by the user without using the #mysql50# prefix
  • all changed code is covered by tests
Comment by Aleksey Midenkov [ 2022-06-28 ]

the generated identifiers cannot be generated by the user without using the #mysql50# prefix

Why is that? Temporary tables are generated with #tmp- prefix without #mysql50# .

Comment by Marko Mäkelä [ 2022-06-28 ]

Normally, # in a user-specified database or table name that contains will be encoded as @0023 in my_charset_filename. After executing the following:

CREATE TABLE `t#` (a INT);
CREATE TABLE `#mysql50#T#` (a INT);

the subdirectory of the current database in the data directory should contain both t@0023.frm and T#.frm.

We do not want the mangled constraint names to conflict with names that users can specify. The SYS_FOREIGN.ID is constructed from the database (schema) name and a user-specified constraint name, hence a user-specified constraint name must be unique within the database (schema):

--source include/have_innodb.inc
CREATE TABLE parent(a INT PRIMARY KEY) ENGINE=InnoDB;
CREATE TABLE child1(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY (a) REFERENCES parent(a)) ENGINE=InnoDB;
--error ER_CANT_CREATE_TABLE
CREATE TABLE child2(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY (a) REFERENCES parent(a)) ENGINE=InnoDB;
CREATE TABLE child2(a INT PRIMARY KEY, CONSTRAINT d FOREIGN KEY (a) REFERENCES parent(a)) ENGINE=InnoDB;
DROP TABLE child2, child1, parent;

Please add a test case that demonstrates what would happen on a naming conflict.

Comment by Aleksey Midenkov [ 2022-06-29 ]

marko According to your comment I can guess you didn't read the test case well. Also I can guess you didn't read the commit message.

Comment by Aleksey Midenkov [ 2022-06-29 ]

The check with pathological names is affected by MDEV-28980.

Comment by Marko Mäkelä [ 2022-07-25 ]

I have fixed MDEV-28980.

Comment by Aleksey Midenkov [ 2022-08-02 ]

Pathological test added to bb-10.10-MDEV-25292

Comment by Marko Mäkelä [ 2022-08-02 ]

The added test indicates that at least SYS_FOREIGN.ID is being truncated:

select * from information_schema.innodb_sys_foreign;
+ID     FOR_NAME        REF_NAME        N_COLS  TYPE
+@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@274e@27	…

Those 193 ASCII characters (NAME_LEN+1) correspond to only 38½ characters. In MDEV-28980 I wrote:

The SYS_FOREIGN.ID will contain the database name in the filename-safe encoding, a separator /, and the constraint name. This easily exceeds the MAX_TABLE_NAME_LEN (5*64=320). The actual maximum length of SYS_FOREIGN.ID should be 5*64+1+3*64=513 bytes, but operating systems may restrict the path component length further. In Linux, a path component may be up to 255 bytes. So, the maximum database name length would be 51 characters when each of them is encoded in 5 bytes.

Comment by Marko Mäkelä [ 2022-08-02 ]

In case the data is truncated by the information_schema.innodb_sys_foreign implementation, it would be good to use multiple long database or table names that differ in the last character, to demonstrate the absence of duplicates. ASCII characters that expand to 5 characters in my_charset_filename would make it easier to read the test.

Comment by Aleksey Midenkov [ 2022-08-02 ]

This is truncated in original 10.10 in CREATE TABLE. How this relates to CREATE OR REPLACE and MDEV-25292?

Comment by Marko Mäkelä [ 2022-08-03 ]

midenok, the reason why I am interested in anomalies around the maximum length is that I understood that your suggested fix could create a longer SYS_FOREIGN.ID during the execution of CREATE OR REPLACE TABLE.

I think that there is a simpler solution: When renaming the SYS_FOREIGN.ID of the table that is being replaced, replace all '/' with the byte 0xff. That byte is already being used as the first byte of SYS_INDEXES.NAME related to an uncommitted ADD INDEX operation, because it is an invalid UTF-8 sequence.

If we do this, then in the example test case, there would be no duplicate SYS_FOREIGN.ID value of 'test/c' or 'test/d', because the being-replaced table would use the unique value 'test\xffc' or 'test\xffd'. The same separator can also be used for any auto-generated constraint name 'databasename\xfftablename_ibfk_1'.

Comment by Marko Mäkelä [ 2022-08-03 ]

If constraint names are not allowed to start with '/', the intermediate constraintname in the 'databasename/constraintname' string could be prepended with a number of '/' to distinguish it from a real user-specified name. Otherwise, we can use a scheme like `databasename/\xffconstraintname' or `databasename\xffconstraintname'.

Comment by Marko Mäkelä [ 2022-08-04 ]

The following test passes before MDEV-25292, but fails due to a duplicate key error on SYS_FOREIGN.ID thereafter:

--source include/have_innodb.inc
 
CREATE TABLE t(a INT PRIMARY KEY) ENGINE=InnoDB;
 
CREATE DATABASE `#mysql50##tmp-test`;
CREATE DATABASE `#mysql50##bak-test`;
CREATE TABLE `#mysql50##tmp-test`.u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d(a) REFERENCES test.t(a)) ENGINE=InnoDB;
CREATE TABLE `#mysql50##bak-test`.u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d(a) REFERENCES test.t(a)) ENGINE=InnoDB;
 
CREATE OR REPLACE TABLE u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d (a) REFERENCES t (a)) ENGINE=InnoDB;
CREATE OR REPLACE TABLE u(a INT PRIMARY KEY, CONSTRAINT c FOREIGN KEY d (a) REFERENCES t (a)) ENGINE=InnoDB;
 
DROP DATABASE `#mysql50##tmp-test`;
DROP DATABASE `#mysql50##bak-test`;
DROP TABLE u, t;

With the submitted fix of the original reported problem, already the first CREATE OR REPLACE TABLE would fail. Without the attempted fix, the second CREATE OR REPLACE TABLE would fail.

I also checked that constraint names are allowed to start with /. This means that a fix attempt that involves writing databasename//constraintname would not prevent duplicate key errors with user-specified constraints.

I think that it is safest to use the byte 0xff to distinguish temporarily renamed constraints from user-specified ones. The InnoDB internal table SYS_FOREIGN uses the single-byte-charset-collation latin1_swedish_ci for the key (even though at least starting with MySQL 4.1, the stored data is in system_charset_info, that is UTF-8). A nice property of 0xff (U+00FF) is that it has only one case variant in latin1. U+00FF is ÿ but there is no Ÿ in latin1; its place U+00DF is occupied by ß. So, the 0xff should not cause bogus duplicates with any other string.

I also verified that the invalid UTF-8 sequence 0xff is correctly being rejected in the following (the ÿ is encoded as 0xff, that is, ISO 8859-1):

--source include/have_innodb.inc
 
CREATE TABLE t(a INT PRIMARY KEY) ENGINE=InnoDB;
 
SET NAMES utf8;
CREATE TABLE u(a INT PRIMARY KEY, CONSTRAINT `ÿ` FOREIGN KEY(a) REFERENCES t(a)) ENGINE=InnoDB;
DROP TABLE u, t;

10.10

mysqltest: At line 6: query 'CREATE TABLE u(a INT PRIMARY KEY, CONSTRAINT `�` FOREIGN KEY(a) REFERENCES t(a)) ENGINE=InnoDB' failed: ER_INVALID_CHARACTER_STRING (1300): Invalid utf8mb3 character string: '\xFF'

Comment by Aleksey Midenkov [ 2022-08-08 ]

Constraint cannot be prepended nor interpended by `/`. This is the magic symbol which is parsed by rename constraint. Used scheme with 0xFF symbol.

Comment by Marko Mäkelä [ 2022-08-17 ]

Yes, I wrote exactly that in my previous comment:

I also checked that constraint names are allowed to start with /. This means that a fix attempt that involves writing databasename//constraintname would not prevent duplicate key errors with user-specified constraints.

We can replace the / with 0xff, or write a 0xff byte before or after the /.

Comment by Marko Mäkelä [ 2022-08-23 ]

The latest update appears to be split into two parts:
MDEV-28933 CREATE OR REPLACE fails to recreate same constraint name and MDEV-28933 Review.
The first commit message is misleadingly using what looks like an octal constant \255 which would seem to refer to U+00AD SOFT HYPHEN. The correct octal constant would be \377.

I think that it suffices to increase the maximum length of the name by 1 octet, not 2 as in the submitted change. Example: databasename\377/constraintname, databasename/\377constraintname. Using the same databasename prefix will make the changes more local, hopefully reducing some random access overhead for accessing the SYS_FOREIGN records.

Please do not remove the debug assertion in trx_undo_page_report_rename() that aims to catch a buffer overflow, in when writing a TRX_UNDO_RENAME_TABLE record. To avoid hitting that assertion (which you filed MDEV-29258 for), you can use a shorter table name in the test, or provide a fix of MDEV-29258. (Edit: Even with innodb_file_per_table=0, the undo log record will be created, only some FILE_ records in the redo log will be skipped.)

Comment by Marko Mäkelä [ 2022-08-26 ]

Please squash the fix into a single commit and make sure that the commit message accurately describes and provides justification for all changes.

It is unclear to me why the size of fk_id[] would have to be reduced by 2 bytes instead of being extended by 2 bytes. It is also unclear why dict_remove_db_name() would have to be changed.

One of the commit messages mentions table_name, even though SYS_FOREIGN.ID (which the commit message fails to mention) stores constraint names and not table names. An added comment to row_rename_table_for_mysql() in the same commit appears to disagree with the commit message.

Because innodb_drop_database implements some "garbage collection" of any orphaned FOREIGN KEY constraints on DROP DATABASE, it is simplest that we will retain the databasename/ prefix for all SYS_FOREIGN.ID. Some \377 bytes can be added to the start or the end of the constraint name, e.g., databasename/\377constraintname, databasename/constraintname\377, or databasename/\377\377constraintname.

Comment by Aleksey Midenkov [ 2022-08-29 ]

Please review bb-10.10-MDEV-25292

Comment by Marko Mäkelä [ 2022-08-31 ]

bb-10.11-midenok-MDEV-25292 looks OK to both me and the CI systems. Thank you!

Generated at Thu Feb 08 10:04:34 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.