Skip to content

fix(replace): apply parent-side foreign key actions during REPLACE#25089

Open
ck89119 wants to merge 10 commits into
matrixorigin:mainfrom
ck89119:issue-24951-main
Open

fix(replace): apply parent-side foreign key actions during REPLACE#25089
ck89119 wants to merge 10 commits into
matrixorigin:mainfrom
ck89119:issue-24951-main

Conversation

@ck89119

@ck89119 ck89119 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24951

What this PR does / why we need it:

Subtask 3.2 of #24918. REPLACE replaces a conflicting row as delete-then-insert, so MySQL applies each referencing child table's ON DELETE action against the removed parent row before the new row is inserted. The operator-based REPLACE path did not do this: replacing a referenced parent row succeeded and left child rows untouched.

This generates parent-side FK background SQLs for the non-self-referencing child tables (RefChildTbls) of the replaced parent. They are gated by foreign_key_checks and run before the main REPLACE in the same transaction, so they roll back together if the REPLACE later fails:

  • RESTRICT / NO_ACTION — a select count(*) = 0 ... pre-check that fails with ErrFKRowIsReferenced (MySQL 1451) when a child still references a replaced parent value (prefix REPLACE_PARENT_CHK:).
  • CASCADEdelete from child where fk in (pk values).
  • SET NULLupdate child set fk = null where fk in (pk values) (both prefixed REPLACE_PARENT_ACTION:).

Limitations mirror the existing self-referencing pre-check path: only literal VALUES and single-column FKs that reference the parent's single-column PRIMARY KEY are handled; other shapes (prepared params, expressions, composite/non-PK references, REPLACE ... SELECT) are skipped.

Behavior (matches MySQL)

  • RESTRICT: replacing a referenced parent row fails and leaves the parent/child unchanged; replacing an unreferenced parent row succeeds.
  • CASCADE: the referenced parent row is replaced and its child rows are deleted.
  • SET NULL: the referenced parent row is replaced and the child FK columns are set to NULL.

Tests

  • Plan unit tests with mock RefChildTbls fixtures (RESTRICT / CASCADE / SET NULL / explicit columns / non-literal skip); coverage of the new generator is 77.3%.
  • BVT coverage in test/distributed/cases/dml/replace/replace.test for all three actions.

PR review point - Highlight it if your changes are based on aspect below:

  • performance regression: Consumes more CPU/Memory
  • Behavior changed/configuration changed

REPLACE replaces a conflicting row as delete-then-insert, so MySQL applies
the referencing child tables' ON DELETE action against the removed parent
row before the new row is inserted. The operator-based REPLACE path did not
do this, so replacing a referenced parent row succeeded and left child rows
untouched.

Generate parent-side FK background SQLs for non-self-referencing child
tables (RefChildTbls) of the replaced parent, gated by foreign_key_checks
and run before the main REPLACE in the same transaction:

  - RESTRICT / NO_ACTION -> a count(*)=0 pre-check that fails with
    ErrFKRowIsReferenced (MySQL 1451) when a child still references a
    replaced parent value (prefix REPLACE_PARENT_CHK:).
  - CASCADE  -> delete from child where fk in (pk values).
  - SET NULL -> update child set fk = null where fk in (pk values)
    (both prefixed REPLACE_PARENT_ACTION:).

Limitations mirror the existing self-referencing pre-check path: only
literal VALUES, single-column FKs that reference the parent's single-column
PRIMARY KEY are handled; other shapes are skipped.

Adds plan unit tests (mock RefChildTbls fixtures) and BVT coverage for
RESTRICT, CASCADE and SET NULL.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

- Do not skip the whole REPLACE when a multi-row VALUES list mixes literal
  and non-literal PKs; generate the parent-side action for the literal rows
  so CASCADE/SET NULL no longer leave orphan child rows for them.
- Gate the self-referencing FK SQL generation under foreign_key_checks too,
  so all REPLACE FK checks/actions are consistently disabled when the session
  variable is off.
- Strengthen tests: assert SQL content for the explicit-columns case, add a
  reverse assertion for SET NULL, and cover NO ACTION, multi-row, and mixed
  literal/non-literal rows. Adds a multi-row CASCADE BVT case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ck89119 ck89119 marked this pull request as ready for review July 12, 2026 04:02
@ck89119 ck89119 requested a review from XuPeng-SH as a code owner July 12, 2026 04:02
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

# Conflicts:
#	test/distributed/cases/dml/replace/replace.result
#	test/distributed/cases/dml/replace/replace.test

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs changes.

The new parent-side FK handling is still not semantics-complete for a common valid REPLACE shape: a referenced parent table that also has a secondary UNIQUE key.

In genParentSideReplaceFKSqls (pkg/sql/plan/build_util.go), the generator bails out as soon as parent.Indexes contains any unique index, and it also requires the incoming VALUES list to carry the parent PK literal. That means it only knows how to drive child RESTRICT/CASCADE/SET NULL from the incoming parent PK values. But REPLACE deletes rows selected by any conflicting unique key, not just by the incoming PK.

A concrete case is already in the new BVT coverage:

create table fk_review_p(id int primary key, u int unique, v int);
create table fk_review_c(id int primary key, pid int,
foreign key(pid) references fk_review_p(id) on delete cascade);
insert into fk_review_p values (1, 10, 100);
insert into fk_review_c values (1, 1);
replace into fk_review_p values (2, 10, 200);

MySQL semantics here are: delete old parent row id=1 because of the UNIQUE(u) conflict, apply the ON DELETE CASCADE to child rows that reference id=1, then insert the new parent row id=2. This patch instead returns 'not supported: REPLACE on a referenced table with secondary unique keys'.

The same root cause also rejects the common auto-increment case where the REPLACE column list omits the parent PK, because pkPos cannot be found even though the conflicting old parent row is still well-defined by another unique key.

Suggestion: derive the actual set of old parent PKs that REPLACE will delete from the full conflict set (PK and secondary unique conflicts), then drive parent-side RESTRICT/CASCADE/SET NULL from that old-PK set. As long as the implementation only works from the incoming literal PK values, it will keep rejecting valid MySQL REPLACE statements on referenced parents.

}
parts := make([]string, len(idx.Parts))
for i, part := range idx.Parts {
parts[i] = catalog.ResolveAlias(part)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1/blocker] Preserve unique-prefix semantics when deriving the old parent rows that REPLACE will delete. This reduces every unique index to raw column names and drops idx.IndexAlgoParams, including prefix lengths. For UNIQUE KEY u(body(4)), an existing abcdxxxx and incoming abcdyyyy conflict in the actual unique index, but the generated predicate below compares parent.body = 'abcdyyyy', so it does not find the old row. The main REPLACE can then delete that referenced parent while RESTRICT incorrectly permits it or CASCADE/SET NULL skips its child action. Prefix indexes are supported and already covered by prefix_index_dml.sql and the ODKU planner path. Parse catalog.IndexPrefixLengthsFromParamsWithError(idx.IndexAlgoParams) and generate each conflict predicate from the same indexed prefix expression, then add a referenced-parent BVT regression.

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head addresses the earlier secondary-unique, auto-increment, composite-FK, and non-PK-reference gaps, but it is still not merge-ready because conflict discovery does not preserve unique prefix-index semantics. The inline example shows how REPLACE can delete an old referenced parent while the generated exact-column predicate misses it, causing RESTRICT/CASCADE/SET NULL to be skipped. Please build the old-row predicate from the same prefix key semantics used by the unique index and add a BVT regression. Prepared parameters and REPLACE SELECT remain explicit limitations; the PR description should also be updated because it still says composite/non-PK references are skipped even though this head supports them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants