[MDEV-21175] Remove dict_table_t::n_foreign_key_checks_running from InnoDB Created: 2019-11-29 Updated: 2023-12-22 Resolved: 2021-06-09 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Data Manipulation - Insert |
| Affects Version/s: | 10.2, 10.3, 10.4, 10.5 |
| Fix Version/s: | 10.6.2 |
| Type: | Bug | Priority: | Major |
| Reporter: | Thirunarayanan Balathandayuthapani | Assignee: | Marko Mäkelä |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
InnoDB should clean n_foreign_key_checks usage and assume that SQL is already holding The following test case could repeat the issue:
In above test case, ALTER TABLE t2 FORCE waits for MDL to open the table. Another test case:
In this case, alter waits at commit_inplace_alter_table(). That too for innodb table lock.
First case is acquiring MDL on child table (un-necessary locking) |
| Comments |
| Comment by Marko Mäkelä [ 2019-11-29 ] | ||||||||||||||||||||||||||||||||||
|
Related to this, I believe that an UPDATE or DELETE (or REPLACE or INSERT…ON DUPLICATE KEY UPDATE) on a parent table may require accessing a child table, to reject or to cascade the operation. Furthermore, I believe that any modification of the child table may require accessing the parent table. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2019-11-29 ] | ||||||||||||||||||||||||||||||||||
|
I think that we should aim for replacing InnoDB table locks with MDL. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-02-01 ] | ||||||||||||||||||||||||||||||||||
|
As part of this, some code from row_ins_check_foreign_constraints() should be removed. This should greatly speed up large INSERT operations or ALTER TABLE on tables that are connected by FOREIGN KEY:
Slightly related note: InnoDB used to break its own transactional table locks until a follow-up fix to | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-04-23 ] | ||||||||||||||||||||||||||||||||||
|
It looks like fixing the prelocking code would allow us to remove dict_operation_lock (which in 10.5 was renamed to dict_sys.latch) altogether. The S-latch is currently only used in two cases in 10.5:
Fixing the ROLLBACK might be a little tricky, because for normal non-recovered transactions we should already be covered by MDL. ( | ||||||||||||||||||||||||||||||||||
| Comment by Sergei Golubchik [ 2020-05-06 ] | ||||||||||||||||||||||||||||||||||
|
marko, I cannot fix prelocking blindly. Can you please add some assert to InnoDB that would fire unless prelocking works correctly? In other words, when assert is always true, you can remove dict_table_t::n_foreign_key_checks_running. After adding the assert, please reassign back to me. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-07-24 ] | ||||||||||||||||||||||||||||||||||
|
serg, I think that I will remove the InnoDB work-arounds and add an assertion that should catch the problems in row_drop_table_for_mysql(), so that you can fix the upper layer. This will likely also require fixing For TRUNCATE TABLE we might have to retain row_mysql_drop_list, but we will see about that (if we ever want to address MDEV-16306). | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-07-24 ] | ||||||||||||||||||||||||||||||||||
|
serg, I pushed bb-10.5-MDEV-21175 as a starting point. I disabled all the failing tests with specific reasons:
Once you have addressed these, I think that this will require a round of RQG testing. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-08-14 ] | ||||||||||||||||||||||||||||||||||
|
In my prototype, I forgot to remove some more code, but strangely no tests were failing due to that:
If the tables are sufficiently protected by MDL, there is no need to hold the data dictionary latch to prevent concurrent DDL operations. At the start of this function, my fix already removed an assertion that we must be holding the latch. There is no public test (not even in MySQL) that would exercise the DEBUG_SYNC point. | ||||||||||||||||||||||||||||||||||
| Comment by Sergei Golubchik [ 2020-10-03 ] | ||||||||||||||||||||||||||||||||||
|
marko, I don't understand. Tests that you've disabled and that crash in your prototype branch are tests like
How is that related to the problem at hand, to MDL or foreign key referenced tables? And the test from the bug description, that shows that an fk-referenced table does not have a proper MDL, this does does not crash in your prototype branch. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2020-10-05 ] | ||||||||||||||||||||||||||||||||||
|
Related to what I wrote earlier, serg, my earlier comment in this ticket tried to explain that also
Unfortunately, thiru did not include enough details to reproduce the originally reported problem. At least, we would have to know the source code revision. Based on the filing date, I would guess that it is somewhere around 5a00792c69065140a34315e3c5aec00bdbef8447. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2021-05-29 ] | ||||||||||||||||||||||||||||||||||
|
I think that the field can be safely removed in the third part of the | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2021-06-09 ] | ||||||||||||||||||||||||||||||||||
|
This was fixed in |