[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: |
|
||||||||||||||||||||||||||||||||||||
| Description |
CREATE OR REPLACE failed with "Duplicate key on write or update" when it replace an empty table with the non-anonymous constraint nameFrom comment Test:
Actual result:
Expected result: |
| Comments |
| Comment by Marko Mäkelä [ 2022-06-28 ] | ||||||||||||||||||||||||
|
I posted some comments. I think that we must ensure that
| ||||||||||||||||||||||||
| Comment by Aleksey Midenkov [ 2022-06-28 ] | ||||||||||||||||||||||||
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:
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):
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 | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2022-07-25 ] | ||||||||||||||||||||||||
|
I have fixed | ||||||||||||||||||||||||
| 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:
Those 193 ASCII characters (NAME_LEN+1) correspond to only 38½ characters. In
| ||||||||||||||||||||||||
| 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:
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):
| ||||||||||||||||||||||||
| 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:
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: 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 | ||||||||||||||||||||||||
| 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! |