fix(replace): apply parent-side foreign key actions during REPLACE#25089
fix(replace): apply parent-side foreign key actions during REPLACE#25089ck89119 wants to merge 27 commits into
Conversation
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 reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
- 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>
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
aunjgr
left a comment
There was a problem hiding this comment.
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.
XuPeng-SH
left a comment
There was a problem hiding this comment.
This latest head looks materially better: the previous unique-prefix blocker appears fixed in both the REPLACE conflict join path and the parent-side FK old-row predicate generation. But I still see one remaining blocker.
High: referenced parent tables still reject valid REPLACE statements when a secondary unique-key column is omitted, even if the omitted value deterministically cannot conflict.
The remaining problem is in genParentSideReplaceFKSqls (pkg/sql/plan/build_util.go, around the unique-key predicate builder). It still treats any missing unique-key part as fatal unless all missing parts are auto-increment PK columns:
- for each unique key, it looks up every key part in the explicit REPLACE column list,
- if a part is omitted and is not auto-increment, it eventually returns
not supported: REPLACE with an omitted conflict key.
That is still too strong. A concrete valid MySQL shape is:
create table p(
id int primary key,
u varchar(20) unique, -- nullable, omitted columns default to NULL
v int
);
create table c(
id int primary key,
pid int,
foreign key(pid) references p(id) on delete cascade
);
insert into p values (1, 'x', 100);
insert into c values (1, 1);
replace into p(id) values (1);MySQL semantics here are straightforward: the old parent row is selected by the PK conflict; the omitted nullable u becomes NULL, so it cannot introduce a secondary-unique conflict; parent-side FK action should therefore run from the old PK set and the REPLACE should succeed.
But the current planner still rejects this shape before execution because the secondary unique key u is omitted from the explicit column list.
I verified this two ways:
- By reading the current
genParentSideReplaceFKSqlslogic inpkg/sql/plan/build_util.go:1267-1296. - With a deterministic scratch planner repro in a throwaway worktree: I deep-copied the existing
replace_fk_cpmock table, added a secondaryunique(v)on its nullablevcolumn, andgenParentSideReplaceFKSqlsreturnednot supported: REPLACE with an omitted conflict keyforREPLACE INTO replace_fk_cp(id) VALUES (1).
So the PR still rejects valid REPLACE-on-referenced-parent statements whenever a secondary unique key is omitted, even if the omitted value is a deterministic non-conflicting default.
Suggested fix: derive the conflict-key set from the fully materialized inserted row after default expansion, not only from the explicitly supplied literals in the AST. That would let omitted nullable/defaulted unique columns participate correctly: NULL would contribute “no conflict”, constant defaults would contribute their actual value, and only truly non-materializable shapes would need to fail closed.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (054bb35). The earlier secondary-unique, nullable/default, temporal-default, and prefix-index comments are substantially addressed, but this is still not merge-ready.
- High: parent-side conflict discovery still uses the unmaterialized AST literal, so it can disagree with the actual REPLACE conflict set.
genParentSideReplaceFKSqls formats supplied NumVal/StrVal nodes directly into the background predicate (build_util.go:1389-1417). The main REPLACE path does not use those raw values: initInsertReplaceStmt applies forceAssignmentCastExpr to the destination column type before unique-key probing (bind_insert.go:2729-2762).
A deterministic example is a referenced parent with UNIQUE(u) where u DECIMAL(5,2):
create table p(id int primary key, u decimal(5,2) unique);
create table c(id int primary key, pid int,
foreign key(pid) references p(id) on delete cascade);
insert into p values (1, 1.23);
insert into c values (1, 1);
replace into p values (2, 1.234);The main plan assignment-casts 1.234 to DECIMAL(5,2) (1.23), so it conflicts with and deletes parent id=1. The generated parent-side action instead searches with p.u = 1.234, which is false (CAST(1.234 AS DECIMAL(5,2)) = 1.234 evaluates false), so the cascade misses the child. Equivalent gaps exist for other width/scale/type normalizations.
Please derive the old-row set from the same fully materialized row image/conflict machinery used by REPLACE, or apply exactly the same assignment conversion before serializing every conflict-key part. Add a BVT regression using a narrowing/rounding type, not only already-canonical literals/defaults.
- High: checks/actions run before the REPLACE lock pipeline, leaving a child-insert TOCTOU window.
Compile.runOnce executes every REPLACE_PARENT_CHK / REPLACE_PARENT_ACTION first (compile.go:527-548). The parent/new-and-old-key LOCK_OP is inside the main REPLACE pipeline (bind_replace.go:1011-1020), which starts only afterward. A plain check query, or a child DELETE/UPDATE whose parent is only read by an EXISTS, does not hold the old parent delete lock across this gap.
For example: T1 runs the cascade action and finds/deletes the current children; before T1 starts the main pipeline, T2 inserts a child referencing the still-existing old parent and commits; T1 then locks/deletes that old parent because of a secondary-unique conflict and inserts a different PK. T2s child is left orphaned. Sharing the transaction is necessary for rollback, but it does not close this inter-statement race.
Please drive FK actions from the locked old-row conflict stream in the main plan, or otherwise acquire and hold the required old-parent locks before checks/actions so concurrent child FK checks serialize correctly. A deterministic synchronization test should cover the action-to-main-pipeline boundary.
- The latest Coverage check is a real PR failure. It reports 132/186 modified lines covered (70.97%, required >75%);
bind_replace.gois 55.26% andbuild_util.gois 73.08%. The Ubuntu UT failure is unrelated (TestDispatchEmptyInputWaitsForRemoteReceiverinpkg/sql/colexec/dispatch), and all selected BVT jobs passed, but the coverage gate still needs to be fixed.
Performance/unhappy-path note: the generated text is amplified as rows × unique keys × key parts and then repeated for each referencing FK, with each statement rescanning the parent conflict predicate. Reusing one materialized, locked old-row set would also avoid that parse/scan amplification while closing both correctness gaps above.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed current head aad5a518. The earlier secondary-unique, prefix-index, omitted-default, assignment-rounding, lock-mode/deep-copy, and coverage findings are materially addressed, but I still do not think this is merge-ready.
- High — the TOCTOU fix is a no-op in supported optimistic/SI deployments.
Both sides of the new serialization contract are LOCK_OPs: the parent SELECT ... FOR UPDATE and the shared parent lock added to child FK validation. However, lockop.(*LockOp).Call explicitly bypasses all locking when !Txn().IsPessimistic() (pkg/sql/colexec/lockop/lock_op.go:135-139). Optimistic mode is still a valid CN configuration and defaults to SI (pkg/cnservice/types.go:209-214, 362-374), while both optimistic BVT jobs on this run were skipped.
A concrete orphan race remains possible in that mode:
- T1 starts
REPLACEon a referenced parent and completes the generated RESTRICT check/CASCADE action. - T2 inserts a child referencing the still-visible old parent and commits. Its new shared lock is also bypassed.
- T1 runs the main REPLACE pipeline, deletes the old parent because of a PK/secondary-UK conflict, and commits.
The transactions write different rows/tables, so ordinary SI write-write validation does not supply the missing read-write conflict. Either provide an equivalent correctness mechanism for optimistic mode or fail closed/document that this statement shape is unsupported there. Please add a deterministic two-transaction regression with an explicit synchronization barrier at the action-to-main-pipeline boundary and bounded timeouts; fixed sleeps or probabilistic retry loops would be flaky and would not prove the invariant.
- High — every child FK insert/replace now locks the full parent scan, causing unbounded lock amplification and unrelated write blocking.
appendModernChildFkMarkOks puts the shared LOCK_OP directly above the unfiltered parent TABLE_SCAN (pkg/sql/plan/bind_insert.go:1060-1084), before the MARK join. Runtime-filter generation explicitly returns for MARK joins (pkg/sql/plan/runtime_filter.go:95-96), so the lock input is not narrowed to the referenced parent keys. Consequently, one child row can acquire shared locks for every row in a large parent table (and multiple FKs repeat the scan/lock footprint), held until transaction end; sufficiently large estimates can also be upgraded by the existing whole-table-lock logic. This turns a point FK validation into O(parent cardinality) lock-service memory/work and blocks updates/deletes of unrelated parent keys.
The PR description currently understates this as possibly locking “more parent rows.” The lock must be driven by the matched parent-key set, not the raw parent scan. Please add a plan/behavior regression proving that inserting a child for parent key 1 does not lock or block a writer of unrelated parent key 2, including a large-parent case.
- Medium — conflict discovery still does not use the exact assignment conversion for CHAR/VARCHAR.
castForColumn emits ordinary SQL CAST for every type (pkg/sql/plan/build_util.go:1383-1385), but the real INSERT/REPLACE row uses forceAssignmentCastExpr, which deliberately selects cast_strict for CHAR/VARCHAR (pkg/sql/plan/build_constraint_util.go:1041-1046). Ordinary CAST truncates an over-width string; assignment cast rejects it.
For example, with parent p(id PK, u VARCHAR(3) UNIQUE), a RESTRICT child referencing p.id, existing (1,'abc'), and REPLACE p VALUES (2,'abcd'), the generated precheck truncates 'abcd' to 'abc', finds the old parent, and reports “row is referenced.” The actual REPLACE row should instead fail with data-too-long before that conflict exists. The decimal test only covers rounding and does not close this conversion-equivalence gap. Please derive conflict keys from the same materialized row image/conversion path, or otherwise make the conversion exactly equivalent, and cover an over-width CHAR/VARCHAR case end to end.
The targeted planner tests passed 10 consecutive runs, and the lockop/compile test selections also passed. All current required checks are green, but the optimistic jobs were skipped and the existing tests assert generated SQL/plan shape rather than the two concurrency invariants above.
XuPeng-SH
left a comment
There was a problem hiding this comment.
The latest head materially fixes the optimistic-mode false safety and the full-parent-scan lock amplification, but one High concurrency blocker remains for foreign keys that reference a non-primary UNIQUE key.
The child and parent paths lock different physical namespaces:
- appendModernChildFkMarkOks resolves a matching non-PK UNIQUE index to its hidden index table and takes a shared lock on that index table key (pkg/sql/plan/bind_insert.go:1135-1199).
- REPLACE_PARENT_LOCK is a SELECT FOR UPDATE on the base parent table (pkg/sql/plan/build_util.go:1473-1476). collectLockTargets creates its target from the original base TABLE_SCAN and locks that table ID and primary-key column (pkg/sql/plan/query_builder.go:6325-6348); this target is created before index access-path rewriting.
Concrete failure:
- p(id PK, u UNIQUE) is (1,10), and c.parent_u references p.u.
- T1 runs REPLACE p VALUES (1,20). Its pre-phase locks base row id=1, sees or removes the current children, but has not run the main REPLACE yet.
- T2 inserts a child referencing u=10. It shared-locks the hidden UNIQUE-index key u=10, which does not conflict with T1s base-table PK lock, validates the still-visible old parent, and commits.
- T1 runs the main REPLACE, removes u=10, and commits. The new child is orphaned. The same gap lets a RESTRICT pre-check be bypassed.
The existing tests confirm each lock independently but never prove that the parent and child operations contend on the same lock resource; TestChildInsertLocksReferencedUniqueIndexKey actually confirms the child target is the index table.
Please align the lock namespaces before the parent action/check boundary: for example, lock every referenced parent key being removed in the same base-PK/index-key namespace used by child validation, or use another design that supplies an equivalent happens-before relation. Add a deterministic two-transaction regression with a barrier between the parent action/check and the main REPLACE, specifically for an FK referencing a non-PK UNIQUE key. This is part of the #24951 correctness closure.
# Conflicts: # pkg/sql/plan/utils_test.go
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The new FK locking and parent-side conflict detection have correctness gaps that can violate referential integrity.
P1 - Lock prerequisite steps are not ordered before the main DML (pkg/sql/plan/bind_insert.go:1201)
Each LOCK_OP is appended as an independent query step, while the main path later consumes another scan from the same source step. Query steps are compiled and run concurrently, and the sink dispatches batches to all receivers; step-number ordering is not a happens-before dependency. The main MARK scan and child write can therefore run before the shared lock is acquired. A concurrent parent REPLACE can delete the referenced parent after the stale MARK result and before the delayed lock, bypassing RESTRICT/CASCADE and allowing an orphan child. An actual barrier is required.
P1 - Child FK lock keys are not converted to the parent key type (pkg/sql/plan/bind_insert.go:1108)
The lock expression keeps the child column type, but the lock target type comes from the parent or hidden index table. FK DDL validation only compares Type.Id, so differing decimal scales are accepted. For example, parent DECIMAL(5,2) and child DECIMAL(5,3) can compare equal in the FK MARK join, while the decimal lock fetcher encodes their raw values differently; 1.23 and 1.230 produce different lock keys. The child shared lock then does not conflict with the parent REPLACE lock. Cast/rescale each FK component to the referenced physical key type before locking.
P1 - Generated columns shift the parent-side REPLACE value mapping (pkg/sql/plan/build_util.go:1226)
The main REPLACE input excludes both hidden and generated columns (getInsertColsFromStmt), but this implicit position map skips only hidden columns. With columns (id, g GENERATED, u UNIQUE), REPLACE ... VALUES (2, 10) is interpreted here as id=2, g=10, u omitted, while the main plan correctly interprets it as id=2, u=10. The generated parent lock/check/action SQL therefore misses the secondary-key conflict; a CASCADE can leave the old child orphaned and RESTRICT can incorrectly succeed. Explicit g=DEFAULT has the same mismatch because stripGeneratedDefaultCols rewrites the row but leaves stmt.Columns unchanged.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Parent-side REPLACE FK SQL has two correctness regressions and a lock-order deadlock risk.
P1 - Re-evaluate foreign_key_checks for cached REPLACE plans (pkg/sql/plan/build.go:185)
The new parent-side DetectSqls are generated only when foreign_key_checks is enabled at plan-build time, but the resulting query is still cacheable: it does not set HasForeignKeyAction, and compile.go executes DetectSqls without checking the current session variable. SET foreign_key_checks does not invalidate these plan caches. Thus a plan built with checks enabled still runs CASCADE/SET NULL/restrict SQL after checks are disabled, while a plan built with checks disabled omits actions after checks are re-enabled. This affects both prepared statements and ordinary session plan caching.
P1 - Do not treat explicit auto-increment zero as a conflicting parent key (pkg/sql/plan/build_util.go:1409)
The conflict predicate only treats an omitted auto-increment column as non-conflicting; an explicit literal 0 is formatted as the parent key. The actual REPLACE PRE_INSERT path converts explicit zero to NULL and allocates a new auto-increment value unless NO_AUTO_VALUE_ON_ZERO is set. For example, after inserting parent id=0 with that mode enabled, resetting the mode and executing REPLACE INTO parent VALUES (0, ...) will cause the generated parent-side action to delete or update children of id=0, even though the main REPLACE inserts a new id and leaves the id=0 parent untouched.
P2 - Avoid rejecting valid REPLACE statements because of unrelated generated unique indexes (pkg/sql/plan/build_util.go:1354)
uniqueKeys includes every unique index on the parent, and materializeDefault returns NotSupported whenever an omitted key column is generated. Therefore a valid parent such as p(a PRIMARY KEY, g GENERATED ALWAYS AS (a+1) VIRTUAL, UNIQUE(g)) causes every REPLACE that supplies only a to fail, even when the child foreign key references a and the generated index is not otherwise problematic. The normal REPLACE binder already supports generated columns; this new guard unnecessarily broadens the fail-closed case to valid schemas.
P2 - Use one canonical order for child and parent prerequisite locks (pkg/sql/plan/bind_insert.go:1093)
Child INSERT FK prerequisite locks are chained in foreign-key declaration order. The new parent-side lock SQL instead sorts referenced hidden index tables before acquiring FOR UPDATE locks in build_util.go. With two child FKs referencing two unique keys on the same parent, declared in the reverse of that sort order, a child INSERT can hold a shared lock on key B while waiting for A, while a concurrent REPLACE holds A exclusively and waits for B. This creates a reproducible lock cycle on valid schemas; both sides need the same canonical key order and a two-FK concurrency test.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24951
What this PR does / why we need it:
Subtask 3.2 of #24918.
REPLACEremoves every existing row selected by aprimary-key or secondary-unique conflict before inserting the incoming row.
MySQL applies each referencing child table's
ON DELETEaction to those actualold parent rows. The operator-based path previously skipped these parent-side
foreign-key actions.
This change generates parent-side FK background SQL for non-self-referencing
child tables and executes it before the main
REPLACEin the same transaction.The action predicates resolve the complete old-row conflict set from the
primary key and all secondary unique keys, including unique prefix indexes, so
they do not assume that an incoming primary key identifies the deleted row.
while a child references an affected old parent row.
NULL.Every conflict-key value is explicitly converted with the destination column
type before it is used by the shared lock/check/action predicate. This matches
the main
REPLACEassignment conversion, including DECIMAL rounding andfractional temporal scale. ENUM/SET casts retain their declared values.
To close the gap between parent-side actions and the main delete,
REPLACEfirst locks the actual conflicting old parent rows with
SELECT ... FOR UPDATE.Child FK validation holds shared locks on its parent scan, and local and remote
lock operators preserve each
LockTarget.Mode. The locks use the same outertransaction and are held through the main replacement.
The implementation supports secondary-unique conflicts with a different old
primary key, omitted auto-increment primary keys, conflict fan-out across
multiple unique keys, composite foreign keys, foreign keys referencing a
non-primary unique key, and unique prefix indexes. Omitted unique-key columns
use their materialized static defaults: a
NULLpart makes that unique keynon-conflicting, while constant defaults, including DATE, TIME, DATETIME, and
session-timezone-aware TIMESTAMP values, participate in conflict discovery.
Identifiers are quoted with embedded backticks escaped.
Prepared parameters and other non-literal conflict-key expressions, dynamic or
generated defaults that cannot be represented as literals, plus
REPLACE ... SELECT/REPLACE ... TABLE, remain explicitly unsupported forreferenced parent tables. These shapes fail closed instead of skipping FK
actions.
Behavior
parent key, even when the incoming primary key differs.
predicates and the main replacement, including
DECIMAL(5,2)rounding.its shared parent lock conflicts with the replacement's exclusive lock.
key identifies the old row.
UNIQUE(u)with aNULLdefault does not reject a validreplacement selected by another key.
declared fractional scale; TIMESTAMP uses the current session timezone.
UNIQUE(body(4))treats values with the same four-character prefix as thesame conflict in both the main
REPLACEplan and parent-side FK predicates.Tests
unsupported sources, identifier quoting, unique-prefix predicates, omitted
nullable unique keys, numeric/temporal defaults, assignment casts, lock
ordering, shared child-parent locks, and dynamic-default rejection.
replace.testcoverage for RESTRICT, CASCADE, SET NULL,SET DEFAULT, secondary-unique conflicts, auto-increment omission, fan-out,
rollback, composite/non-primary references, unique-prefix replacement,
omitted nullable unique columns, DATE/TIME/DATETIME/TIMESTAMP defaults, and
DECIMAL assignment rounding.
PR review point - Highlight it if your changes are based on aspect below:
scan rows in shared mode; this can lock more rows than the matching FK key.