Details
-
Technical task
-
Status: Stalled (View Workflow)
-
Critical
-
Resolution: Unresolved
-
None
-
None
Description
Refactor dict_load_foreigns() for synchronising TABLE_SHARE foreign data with dict_table_t cache.
Remove a number of routines working with SYS_FOREIGNS and SYS_FOREIGN_COLS. innobase_update_foreign_try() is now used solely for ER_FK_INCORRECT_OPTION check.
Prelock parent tables as well as child tables. This is done for the case when parent table doesn't know about its children when they created before parent with foreign_key_checks=0. Opening the parent table initiates fk_resolve_referenced_keys() which updates its referenced_keys. Due to CREATE TABLE doesn't not know about "illegal" children it can not check for foreign consistency. F.ex. this would succeed:
set foreign_key_checks= 0; |
create table child (fk int references parent (id)) engine=innodb; |
set foreign_key_checks= 1; |
|
|
create table parent (id bigint primary key) engine=innodb; |
In the above case dict_load_foreigns() deduces which tables are unknown to opened parent (tables_missing) and reloads their foreign data via recursion. Infinite recursion is not possible via test case: a table cannot be "parent after child" and "child before parent" simultaneously. Though infinite recursion is possible via malicously crafted FRM file, there is no protection from that at InnoDB level but there is protection at SQL level: thd->fk_circular_check.
Later though it would not allow DML on child as well as on parent (see innodb.foreign_key MDEV-10083). So this is pretty acceptable: foreign_key_checks is unnormal setting, checking parent on CREATE TABLE would impose all frms scanning which is not acceptable.
ha_innobase::open() then synchronizes these referenced_keys with its referenced_set cache by calling dict_load_foreigns().
Disable self-references on same column. The Bug 12902967 restricted them on some condition of "same column/index" (see innodb_bug12902967.test), though such self-references were not completely disabled (see other self-ref cases changed in this patch). It is not clear why they worked if they are "self-refs on same column/index".
Attachments
Issue Links
- blocks
-
MDEV-23433
Parent table discovery for children (update parent's frm on REPAIR TABLE)
-
- Stalled
-
- is blocked by
-
MDEV-20865
Store foreign key info in FRM and TABLE_SHARE
-
- In Review
-
- relates to
-
MCOL-4441 Move using namespace std to avoid namespace conflicts
-
- Closed
-
-
-
- Closed
-
-
MDEV-21652
FK migration from old version
-
- Stalled
-
-
MDEV-23433
Parent table discovery for children (update parent's frm on REPAIR TABLE)
-
- Stalled
-
Activity
The commit message as well as the Description must explain the InnoDB changes in detail. It now is unclear why dict_foreign_add_to_cache() was changed in the way it was.
Added an explanation:
The semantics of dict_foreign_add_to_cache() has been changed. Foreigns must be synchronized on every table open and cache must be updated. The old semantics was to keep old foreign while it is in cache.
The function dict_load_table_one() could now probably be merged to dict_load_table().
In ha_innobase::open(), we now invoke dict_load_foreigns(). It looks like we will prevent the eviction of connected tables from dict_sys as long as foreign key constraints exist. So, why do we have to invoke the function on every ha_innobase::open()? And what actually is protecting the dict_table_t in dict_load_foreigns()? It does not look like we are holding dict_sys.mutex or dict_table_t::lock_mutex there.
Actually it is protected by a dict_sys mutex. Added assertion into dict_load_foreigns() and debug test case into innodb.foreign-keys.
Any TODO or FIXME comments should be mentioned in the commit message with a plan how to address them.
I agree on that partially. All FIXMEs must be fixed or planned. Not all TODOs must be planned, only important ones.
Before a review can be conducted, these changes will need to be rebased to the 10.7 branch, once that branch has been properly created so that it includes the changes of the 10.6.3 release.
I think that this needs to be rebased on a branch that includes MDEV-20865.
Can you please file a pull request based on a current main branch that contains only the minimal changes for this? Currently, I see a branch that is 18 commits ahead of main and 776 commits behind it. The first commits are apparently changing the .frm file format. I don’t think we need such changes here. The metadata should still be stored in the InnoDB tables SYS_FOREIGN and SYS_FOREIGN_COLS here. Only the cache location is supposed to be changed.
The commit message as well as the Description must explain the InnoDB changes in detail. It now is unclear why dict_foreign_add_to_cache() was changed in the way it was.
The function dict_load_table_one() could now probably be merged to dict_load_table().
In ha_innobase::open(), we now invoke dict_load_foreigns(). It looks like we will prevent the eviction of connected tables from dict_sys as long as foreign key constraints exist. So, why do we have to invoke the function on every ha_innobase::open()? And what actually is protecting the dict_table_t in dict_load_foreigns()? It does not look like we are holding dict_sys.mutex or dict_table_t::lock_mutex there.
Any TODO or FIXME comments should be mentioned in the commit message with a plan how to address them.