MDEV-40776 Atomic CREATE OR REPLACE silently breaks the foreign key

[Thirunarayanan]

Problem:

Atomic CREATE OR REPLACE renames the original table to a #sql-create- backup copy, and because such a name is not treated as a temporary name, InnoDB renamed every constraint of that table, including SYS_FOREIGN.REF_NAME of the constraints that belong to other tables. Those constraints ended up referencing the backup copy, which is dropped at the end of the statement, so the child tables were left with a dangling reference that survived a restart.

Solution:

row_rename_table_for_mysql(): Fix this by splitting the two halves of the constraint renaming. Identifiers that the renamed table owns (SYS_FOREIGN.ID, FOR_NAME, SYS_FOREIGN_COLS.ID, and REF_NAME of a self-referencing constraint) keep following the table, so that they cannot conflict with the constraints of the table that is created under the original name, while REF_NAME of the constraints of other tables is left pointing to the original name, so that the newly created table inherits them and is rejected if it is not compatible with them.

dict_table_rename_in_cache(): Does the same in the data dictionary cache by detaching the referencing constraints from the backup copy instead of renaming them, so that dict_load_foreigns() attaches them to the new table.

As the newly created table can now be a parent table, the tables that CREATE OR REPLACE drops in order to restore the previous state must be dropped without foreign key checks, both in drop_open_table() and in the DDL log recovery (execute_drop_table()).

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dr-m]

There are some pre-existing failures in the base branch:

mysqltest: At line 879: query 'SELECT 1' failed: ER_TABLE_NOT_LOCKED (1100): Table 'mysqltest_tmp_v' was not locked with LOCK TABLES

(embedded server)

and a MSVC build failure thanks to /WX:

    99>C:\buildbot\workers\prod\amd64-windows\build\sql\sql_acl.cc(13064,28): warning C4805: '|=': unsafe mix of type 'int' and type 'bool' in operation [C:\buildbot\workers\prod\amd64-windows\build\sql\sql.vcxproj]
             result |= write_bin_log(thd, FALSE, thd->query(), thd->query_length());

[dr-m]

I don’t think that it would be acceptable to allow a supposedly atomic CREATE OR REPLACE TABLE potentially discard FOREIGN KEY constraints.

I think that we will need sufficient MDL_EXCLUSIVE coverage on all referencing and referenced tables, not only during this DDL operation (at a higher level, not in this low-level function), to ensure that no DML or DDL can run concurrently. I would escalate the locking on the DDL rather than DML paths, because DDL operations are rare and therefore changing only the DDL code path should incur a smaller risk of a performance regression.

In #5085 there already is a proposed fix of the FOREIGN KEY locking bug MDEV-37365, which to my understanding was changing the DML code path rather than the DDL path.