Bug#102586: multi-table DELETE with ON DELETE CASCADE breaks row-based replication - #719
Bug#102586: multi-table DELETE with ON DELETE CASCADE breaks row-based replication#719matanbaruch wants to merge 7 commits into
Conversation
A multi-table DELETE that names both a foreign key parent table and a child table with a cascading delete rule breaks row-based replication. The replica applier stops with ER_KEY_NOT_FOUND. The parent row is deleted while the join is still scanning, so the cascade removes the child rows and logs row events for them. The statement logs row events for the child rows it deletes itself as well. On the replica the parent delete is applied first, its own cascade removes the child rows, and the logged child events then cannot find them. Exclude a delete target from immediate deletion when deleting from it cascades to another table in the same query, which defers the delete until the join has finished. The check is added to both the classic optimizer (GetImmediateDeleteTables) and the hypergraph optimizer (IsImmediateDeleteCandidate). Only ON DELETE CASCADE is considered. ON DELETE SET NULL updates the child rows rather than deleting them, so they stay findable for the logged events and replicate correctly. Deferring those deletes as well would change which rows the statement removes. This is the approach Zsolt Parragi contributed on Bug#80821 in 2019, adapted to the current code: get_cascade_foreign_key_table_list() no longer exists, so the cascade dependency is resolved from TABLE_SHARE::foreign_key_parent instead.
|
I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it |
|
The three red checks here are CI-side and all of them reproduce without this patch. Writing up what I found, in case it is useful: 1. Build, MTR and Format Check never see the code. All three workflows trigger on Checkout fails in about 5 seconds and every later step then fails with 2. Format Check fails on trunk itself. It runs 3. MTR cannot finish inside the budget. Since CI here cannot run the code, I ran your workflows unmodified on a fork-internal PR, where checkout is allowed:
Local Debug build of 26.7.0, same commit, before and after:
Both optimizer paths decide immediate deletion in different places and trunk fails in both, which is why the patch touches both. If you want the format gate green on this PR I can add a separate commit that reformats those three files, but it is 2306 unrelated lines and I would rather not unless you ask for it. |
|
@matanbaruch You are totally right regarding about the github actions failures. I have described the issue in more details in the issue #715. I have a patch for it that will be deployed early next week, it took a bit of time to deploy it as I also tried to stabilize the MTRs. Sorry about the noise ! |
Run untrusted pull request builds with restricted permissions against validated revisions, and publish statuses and labels only from trusted workflows that revalidate the repository, workflow run, PR head, and ordering. Replace the custom review client with the pinned OpenAI Codex Action, bound its input to a validated PR diff, pin third-party actions, and add dependency maintenance for GitHub Actions. Retry a failed or empty Codex review once after a delay with a configurable fallback model while preserving the same read-only isolation boundary and structured output contract. Publish structured Codex findings as one commit-bound GitHub review. Validate each file and right-side line range against the current diff, keep unanchored findings in the summary, prevent duplicate reviews, and revalidate both reviewed revisions before posting. Warm trusted Boost and ccache entries, align the MTR compiler cache with the GCC build, shard MTR suites across runners, run tests in parallel with bounded retries, and retain diagnostics. Safely reset head-scoped CI state, standardize labels, and remove the obsolete OCA checkbox. Require both the OCA Verified label and a current trusted approval before adding Integrate. Revalidate both conditions around label publication and remove Integrate if either condition no longer holds. Temporarily disable parallel-run failures tracked by Bug#39882117 and restore the required restart and expected output for the buffer-pool-load MTR. Change-Id: I7393e75cab3afa172a99237337c26e3974f955fa
|
@matanbaruch The fix for issue #715 have been merged in trunk. Can you please rebase your branch to trigger the pipelines ? |
… change Format Check runs clang-format-18 over whole changed files. These three are not clean under 18 on trunk, so the gate fails for any PR touching them. Cosmetic only: a label space in sql_base.cc, one DBUG_LOG argument wrap in sql_delete.cc, and two string literal joins in join_optimizer.cc.
|
@RidhaOracle MTR (replication) failed only on |
|
@RidhaOracle can you re-run the MTR (replication) shard and point a reviewer at this? Everything else is green and the only failure is the temp table race, not this patch. |
kahatlen
left a comment
There was a problem hiding this comment.
Thanks for the patch.
I think you have identified the right places in the code to fix this bug. But I think the fix is incomplete and needs to be expanded a bit before it can be merged.
The missing pieces are:
- The bug affects other referential actions than
CASCADE, such asSET NULL, which should also be covered. - The bug affects multi-table UPDATE statements too.
- The fix only check directly referenced tables. It should check for indirectly referenced tables too.
More details in inline comments.
| # | ||
| ############################################################################### | ||
| --source include/have_binlog_format_row.inc | ||
| --source include/rpl/init_source_replica.inc |
There was a problem hiding this comment.
I think it would be good if the test case reproduced the bug without setting up a replication environment. The bug is in optimizer code, not replication code, so using replication to show the bug is probably an unnecessary complication.
Here's one way to show the bug without relying on replication:
CREATE TABLE t1(id INT PRIMARY KEY, i INT);
INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 1), (4, 2), (5, 1), (6, 2), (7, 1);
CREATE TABLE t2(
id INT PRIMARY KEY,
t1_id INT REFERENCES t1(id) ON DELETE CASCADE
);
INSERT INTO t2 VALUES
(1, 1), (2, 1), (3, 1), (4, 1), (5, NULL), (6, 6), (7, 7), (8, 1), (9, 2),
(10, 3), (11, 4), (12, 5), (13, 6), (14, 7), (15, 1), (16, 2), (17, 3),
(18, 4), (19, 5), (20, 6), (21, 7), (22, NULL), (23, 1), (24, 2), (25, 3);
ANALYZE TABLE t1, t2;
DELETE t1 FROM t1, t2 WHERE t1.i = t2.id;
SELECT * FROM t1 ORDER BY id;
SELECT * FROM t2 ORDER BY id;
Without your patch, t1 contains 6 rows after the DELETE statement is executed, and t2 contains 18 rows.
With your patch applied, t1 is empty after the DELETE statement is executed, and t2 contains 2 rows. This is the correct result.
| for (const TABLE_SHARE_FOREIGN_KEY_PARENT_INFO *fk_p = | ||
| share->foreign_key_parent; | ||
| fk_p < share->foreign_key_parent + share->foreign_key_parents; ++fk_p) { | ||
| if (fk_p->delete_rule != dd::Foreign_key::RULE_CASCADE) continue; |
There was a problem hiding this comment.
I think we need to do this check for all actions except RESTRICT and NO ACTION. I'm seeing wrong results with SET NULL even after your patch. Try this:
CREATE TABLE t1(id INT PRIMARY KEY, i INT);
INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 1), (4, 2), (5, 1), (6, 2), (7, 1);
CREATE TABLE t2(
id INT PRIMARY KEY,
t1_id INT REFERENCES t1(id) ON DELETE SET NULL
);
INSERT INTO t2 VALUES
(1, 1), (2, 1), (3, 1), (4, 1), (5, NULL), (6, 6), (7, 7), (8, 1), (9, 2),
(10, 3), (11, 4), (12, 5), (13, 6), (14, 7), (15, 1), (16, 2), (17, 3),
(18, 4), (19, 5), (20, 6), (21, 7), (22, NULL), (23, 1), (24, 2), (25, 3);
ANALYZE TABLE t1, t2;
DELETE t1 FROM t1, t2 WHERE t1.i = t2.id AND t1_id IS NOT NULL;
SELECT * FROM t1 ORDER BY id;
SELECT * FROM t2 ORDER BY id;
The expected result is that t1 is empty and all rows in t2 have NULL in the t1_id column. The actual result is that there are six rows left in t1 and t2 has many rows with non-NULL values in the t1_id column.
| assert(table->table != nullptr); | ||
|
|
||
| const TABLE_SHARE *share = table->table->s; | ||
| for (const TABLE_SHARE_FOREIGN_KEY_PARENT_INFO *fk_p = |
There was a problem hiding this comment.
This only checks if deletes from table cascades directly into one of the query tables. But it should also check if they cascade indirectly into one of the query tables too. That is, if deletes from t1 cascade into t2 and deletes from t2 again cascade into t3, it would not be safe to delete immediately from t1 if the DELETE statement queries t3. But this check will only reject immediate deletes if t2 is in the query.
Example:
CREATE TABLE t1(id INT PRIMARY KEY, i INT);
INSERT INTO t1 VALUES (1, 1), (2, 2);
CREATE TABLE t2(
id INT PRIMARY KEY,
t1_id INT REFERENCES t1(id) ON DELETE CASCADE
);
INSERT INTO t2 VALUES (1, 1), (2, 1);
CREATE TABLE t3(
id INT PRIMARY KEY,
t2_id INT REFERENCES t2(id) ON DELETE CASCADE
);
INSERT INTO t3 VALUES (1, 1), (2, 2);
ANALYZE TABLE t1, t2, t3;
DELETE t1 FROM t1 STRAIGHT_JOIN t3 ON t1.i = t3.id;
SELECT * FROM t1 ORDER BY id;
SELECT * FROM t2 ORDER BY id;
SELECT * FROM t3 ORDER BY id;
Expected result: All three tables are empty. Actual result: t1 contains one row, t2 and t3 are empty.
| SELECT * FROM t2 ORDER BY id; | ||
| id parent_id | ||
| 2 NULL | ||
| 3 2 |
There was a problem hiding this comment.
I believe this result is incorrect. It should have contained only the (3, 2) row. The (2, NULL) row was originally (2, 1) and should have qualified both in the ON clause and in the WHERE clause and get deleted. delete_cascades_to_queried_table() should also check SET NULL actions to fix this.
| Test whether deleting a row from the subject table of a multi-table DELETE | ||
| can cascade to another table which the same statement reads. |
There was a problem hiding this comment.
The problem isn't limited to multi-table DELETE. Multi-table UPDATE seems to have the same issue. For example:
CREATE TABLE t1(
id INT PRIMARY KEY,
u INT UNIQUE,
i INT
);
INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 1);
CREATE TABLE t2(
id INT PRIMARY KEY,
t1_u INT REFERENCES t1(u) ON UPDATE CASCADE
);
INSERT INTO t2 VALUES (1, 1), (2, 2);
ANALYZE TABLE t1, t2;
UPDATE t1 STRAIGHT_JOIN t2 ON t1.i = t2.t1_u
SET t1.u = t1.u + 10;
SELECT * FROM t1 ORDER BY id;
SELECT * FROM t2 ORDER BY id;
When updating the first row in t1, the update cascades to t2 and modifies it in a way so that the second row of t1 no longer has a match in t2. The statement therefore updates only the first row of t1, whereas it should have updated both rows.
…e UPDATE Review follow-up for Bug#102586 / Bug#80821: - Consider every referential action except RESTRICT and NO ACTION. ON DELETE SET NULL gives wrong results the same way CASCADE does, since the action rewrites child rows the join has not read yet. - Follow referential actions transitively. A delete cascading from t1 into t2 can trigger t2's own actions into t3, so t3 being in the query makes immediate deletes from t1 unsafe even when t2 is not in the query. Whether a table's children are affected through their delete rule or their update rule depends on whether the action deletes or updates that table's rows. The walk finds intermediate tables among the open tables, which prelocking guarantees to include every table reachable through referential actions. - Apply the same check to multi-table UPDATE, in safe_update_on_fly() for the traditional optimizer and IsImmediateUpdateCandidate() for the hypergraph optimizer. - Replace the replication test with a main-suite test showing wrong results on a single server, using the reviewer's reproductions, plus an indirect-cascade case and two multi-table UPDATE cases.
|
Pushed. All four points addressed in 6a37612:
Verified locally on a Debug build: the new test passes with both optimizer modes, and foreign_key (44 tests), delete (25), update (24), innodb.innodb, innodb.innodb_misc1 and foreign_key_cascade all pass. |
|
@kahatlen @RidhaOracle can one of you approve the workflow runs for 6a37612? Build and MTR are waiting on approval since the last push. |
| find_open_table_share(all_tables, fk_p->referencing_table_db.str, | ||
| fk_p->referencing_table_name.str); | ||
| if (child_share == nullptr) { | ||
| assert(false); |
There was a problem hiding this comment.
This assertion is reachable with native InnoDB FK handling. For example, you can run your test case as mtr --mysqld=--innodb_native_foreign_keys=TRUE main.foreign_key_multi_table_dml and it will hit this assertion.
| assert(false); | ||
| return true; | ||
| } | ||
| if (std::find(visited.begin(), visited.end(), child_share) == |
There was a problem hiding this comment.
visited currently keys only on the table, but the traversal state also includes whether its rows are deleted or updated. The same table can be reached along both kinds of paths in a diamond-shaped FK graph, and its ON DELETE and ON UPDATE rules can lead to different descendants. Whichever state arrives second is skipped here, which can miss a queried table and incorrectly allow immediate modification.
Here is a test case that shows the problem:
CREATE TABLE root(id INT PRIMARY KEY, i INT);
INSERT INTO root VALUES (1, 2), (2, 1);
CREATE TABLE a_update(
id INT PRIMARY KEY,
root_id INT UNIQUE,
FOREIGN KEY (root_id) REFERENCES root(id) ON DELETE SET NULL
);
INSERT INTO a_update VALUES (1, 1), (2, 2);
CREATE TABLE z_delete(
id INT PRIMARY KEY,
root_id INT,
FOREIGN KEY (root_id) REFERENCES root(id) ON DELETE CASCADE
);
INSERT INTO z_delete VALUES (1, 1);
CREATE TABLE common_child(
id INT PRIMARY KEY,
a_ref INT UNIQUE,
z_ref INT,
FOREIGN KEY (a_ref) REFERENCES a_update(root_id) ON UPDATE CASCADE,
FOREIGN KEY (z_ref) REFERENCES z_delete(id) ON DELETE CASCADE
);
INSERT INTO common_child VALUES (1, 1, NULL), (2, NULL, 1), (3, 2, NULL);
CREATE TABLE query_child(
id INT PRIMARY KEY,
common_ref INT,
FOREIGN KEY (common_ref) REFERENCES common_child(a_ref) ON UPDATE CASCADE
);
INSERT INTO query_child VALUES
(1, 1), (2, 2), (3, NULL), (4, NULL), (5, NULL), (6, NULL), (7, NULL),
(8, NULL), (9, NULL), (10, NULL), (11, NULL), (12, NULL), (13, NULL),
(14, NULL), (15, NULL), (16, NULL), (17, NULL), (18, NULL), (19, NULL),
(20, NULL), (21, NULL), (22, NULL), (23, NULL), (24, NULL), (25, NULL);
ANALYZE TABLE root, a_update, z_delete, common_child, query_child;
DELETE root FROM root JOIN query_child ON root.i = query_child.common_ref;
SELECT * FROM root ORDER BY id;I verified this at the current HEAD with both the default optimizer and the hypergraph optimizer (mtr --hypergraph). Both choose an immediate nested-loop delete consisting of a two-row scan of root and an indexed lookup into query_child; both incorrectly leave (2, 1) in root. The correct result is an empty root table.
| # Every row in t1 matches a row in t2 that satisfies the predicate at the | ||
| # start of the statement, so all rows in t1 should be updated, and the SET | ||
| # NULL action should clear t1_id in every t2 row. | ||
| UPDATE t1, t2 SET t1.id = t1.id + 100 |
There was a problem hiding this comment.
Could these two UPDATE cases be changed to update a referenced secondary UNIQUE key instead of the clustered primary key? Updating t1.id already makes the traditional optimizer reject on-the-fly update because the clustered primary key participates in the read index, so the current tests pass without exercising the new FK-action checks.
I tested the complete committed test file without the fix under both the default optimizer and the hypergraph optimizer (mtr --hypergraph). All three DELETE cases fail as intended in both modes. Both UPDATE cases produce the expected result in both modes, so they do not reproduce the UPDATE bug.
The following table shape reproduces the UPDATE bug with both optimizers:
CREATE TABLE t1(
id INT PRIMARY KEY,
u INT UNIQUE,
i INT
);
INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 1);
CREATE TABLE t2(
id INT PRIMARY KEY,
t1_u INT,
FOREIGN KEY (t1_u) REFERENCES t1(u) ON UPDATE CASCADE
);
INSERT INTO t2 VALUES
(1, 1), (2, 2), (3, NULL), (4, NULL), (5, NULL), (6, NULL), (7, NULL),
(8, NULL), (9, NULL), (10, NULL), (11, NULL), (12, NULL), (13, NULL),
(14, NULL), (15, NULL), (16, NULL), (17, NULL), (18, NULL), (19, NULL),
(20, NULL), (21, NULL), (22, NULL), (23, NULL), (24, NULL), (25, NULL);
ANALYZE TABLE t1, t2;
UPDATE t1 JOIN t2 ON t1.i = t2.t1_u
SET t1.u = t1.u + 10;
SELECT * FROM t1 ORDER BY id;
SELECT * FROM t2 WHERE t1_u IS NOT NULL ORDER BY id;Without the fix, both optimizers choose an immediate nested-loop update: scan the two rows of t1, then use the FK index for lookups into the 25-row t2. Both incorrectly update only the first t1 row. With the fix, both choose the same indexed nested loop but buffer the update, and both t1 rows are updated. I also verified the same behavior after changing the FK action to ON UPDATE SET NULL.
Could the two UPDATE sections use this shape, with CASCADE in one and SET NULL in the other? This also makes the intended plan naturally cheaper than a hash join for the hypergraph optimizer.
|
I focused this pass on the optimizer-visible behavior and concrete reproductions. Since the new dependency traversal is in sql_base.cc and owned by the Runtime team, I think this should also receive a Runtime-team review. |
Review follow-up for Bug#102586 / Bug#80821: - Do not assert when a child table is not among the open tables. With engine-managed referential actions (innodb_native_foreign_keys) prelocking does not add child tables, so the walk cannot continue; buffer the modification in that case, since the engine-internal action poses the same hazard. - Track visited tables per (table, deleted-vs-updated) state. The two states can lead to different descendants, so a table reached along both a delete path and an update path in a diamond-shaped foreign key graph must be walked once per state. Keying on the table alone could skip the state that reaches a queried table and incorrectly allow immediate modification. - Make the multi-table UPDATE test cases update a referenced secondary unique key instead of the clustered primary key. Updating the primary key already made the traditional optimizer buffer the update, so the old shape did not exercise the new check.
|
All three points addressed and pushed in b1f9b04.
One separate observation from recording the diamond case: the default (non-native) foreign key handling leaves a dangling reference in that schema. A plain single-table @kahatlen @RidhaOracle the push will need workflow approval again for build and MTR to run on b1f9b04. |
|
@kahatlen @RidhaOracle can you re-run the storage and services shards for b1f9b04? The failures are runner flakes, not the patch: MY-011959 buffer pool warnings in innodb.log_file_name_1 and innodb_undo.truncate_recover_02 (neither test touches foreign keys or multi-table DML, both pass locally on this commit), and on services the MTR step passed and only the unit test step failed on the NDB testSecureSocket-t abort plus router sharing timing flakes. |
…ble-delete-fk-cascade # Conflicts: # mysql-test/collections/disabled.def
|
@kahatlen @RidhaOracle MTR (core) on da35310 fails in information_schema.i_s_schema_definition_debug with "no known I_S version with the given checksum", and it fails identically on unmodified trunk e174239 (our merge base), so trunk is missing a checksum row for I_S version 261000 rather than anything in this patch. All other shards and builds are green. |
What does this change do?
A multi-table DELETE that names both a foreign key parent table and its
ON DELETE CASCADEchild breaks row-based replication: the replica applier stops with ER_KEY_NOT_FOUND. This defers the delete until the join has finished when a cascade dependency exists between tables inside the query, on both the classic and the hypergraph optimizer path.BUG#102586, BUG#80821
Why is it needed?
Both bugs are Verified and still unfixed. #80821 was reported in 2016, #102586 in 2021, and neither has moved since October 2023. #102586 is S1.
What happens:
A single-table DELETE that relies on the cascade is fine. Only the multi-table form is affected.
We hit this in production on 8.4.8 LTS, which is listed on neither bug report. Three replicas broke inside the same five minute window and stayed broken for three weeks, because Amazon RDS kept restarting the applier and the lag looked normal. A local harness reproduces it identically on 8.0.35, so this is not a regression in a recent version, it has just never been fixed.
The design is Zsolt Parragi's, contributed on #80821 in 2019 and acknowledged by Oracle at the time. That patch no longer applies:
get_cascade_foreign_key_table_list()was removed. This version resolves the cascade dependency fromTABLE_SHARE::foreign_key_parentinstead, and covers the hypergraph optimizer, which did not exist in 2019.How was it tested?
mysql-test/scripts/ci/mtr.shpasses locallyrpl, 1100 tests,--parallel=8 --forceNew test:
rpl.rpl_multi_table_delete_fk_cascade. Debug build, 26.7.0, Linux, before and after the same commit:HA_ERR_KEY_NOT_FOUNDontest.t2Both optimizer paths were checked separately because they decide immediate deletion in different places, and trunk fails in both.
Also passing on this branch:
main.foreign_key_cascade,innodb.innodb,innodb.innodb_misc1(the existing tests that combine cascading foreign keys with multi-table DELETE), and the 25 tests matching--do-test=delete.The test keeps an
ON DELETE SET NULLcase as negative coverage. That one replicates correctly on trunk, so it is deliberately left in immediate mode: deferring it would change which rows the statement removes.Two tests failed in the full
rplrun and both pass in isolation on the same binary, so I read them as flaky under--parallel=8rather than caused by this change:rpl_crash_on_pfs_worker_table_against_replica_stopandrpl_parallel_alter_db_table. Neither involves DELETE or foreign keys. If they are known-stable in your CI, say so and I will dig further.Contributor checklist
scripts/ci/format.sh)AI assistance
Claude Code wrote the patch and the MTR test. The fix design is not new, it is Parragi's 2019 approach from #80821 rebased onto current trunk. Everything claimed above was verified by running it, not by inspection: the before and after matrix comes from real MTR runs on a local Debug build, in both optimizer modes. The production root cause behind it was diagnosed separately against a Docker reproduction of our schema.
Areas touched
optimizer, replication