Skip to content

fix(plan): move parent FK update actions to MULTI_UPDATE - #26442

Open
ck89119 wants to merge 56 commits into
matrixorigin:mainfrom
ck89119:issue-26339-main
Open

fix(plan): move parent FK update actions to MULTI_UPDATE#26442
ck89119 wants to merge 56 commits into
matrixorigin:mainfrom
ck89119:issue-26339-main

Conversation

@ck89119

@ck89119 ck89119 commented Jul 30, 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 #26339

Depends on #26407.

What this PR does / why we need it:

The modern UPDATE planner did not handle referential actions when an UPDATE
changed a referenced parent key. Those statements still depended on the legacy
recursive UPDATE planner.

This change:

  • discovers foreign keys from the explicit and implicit final UPDATE row image;
  • keeps RESTRICT, NO ACTION, and SET DEFAULT compatibility checks on the modern
    path;
  • builds supported CASCADE and SET NULL child writes as explicit MULTI_UPDATE
    steps;
  • rejects affected parent-key UPDATEs in optimistic transactions, including
    plans prepared before the execution transaction is known;
  • uses a zero-cost path for exact primary/unique references and rejects only
    ambiguous mappings when a legacy non-unique referenced prefix matches more
    than one changed parent row;
  • maintains the child base table and only indexes whose key parts change;
  • applies one deterministic base-table-before-hidden-table physical lock order
    across UPDATE, INSERT, REPLACE, and recursive cascades;
  • excludes referential-action side effects from the parent statement's affected
    row count;
  • explicitly rejects unsupported generated/ON UPDATE child row closures,
    recursive, duplicate-target, self-mutating, partitioned,
    primary-key-mutating, and irregular-index graphs instead of silently falling
    back to the legacy planner.

Validation:

  • mo-cgo-test -count=1 -timeout=10m ./pkg/sql/plan ./pkg/sql/compile
  • mo-cgo-test -race -count=1 -timeout=10m ./pkg/sql/plan ./pkg/sql/compile
  • focused lock-order race stress with -count=100
  • go list, go build, and go vet with readonly module mode
  • make build
  • git diff --check
  • mo-tester BVT: 155/155 passed

@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.

[P1] Route cascaded child writes through the complete UPDATE row-closure before MULTI_UPDATE

When ON UPDATE CASCADE changes an FK base column, appendUpdateParentMutation recomputes generated columns and emits new unique-index keys, but it only rejects a directly changed FK column when that column is the child PK. It never runs the normal DEDUP check when a generated PK/UK changes.

A concrete regression is:

create table p(id int primary key);
create table c(
  id int primary key,
  pid int,
  u int generated always as (pid % 10) stored,
  unique key uk_u(u),
  foreign key(pid) references p(id) on update cascade
);
insert into p values (1), (12);
insert into c(id, pid) values (10, 1), (20, 12);
update p set id = 2 where id = 1;

The child row changes u from 1 to 2, so this must report a duplicate entry for uk_u and roll back. The new action path only joins the old unique key used for deletion, never probes the new u=2 key, and then writes the unique-index table directly through MULTI_UPDATE.

This is structural rather than an isolated generated-column case: the same hand-built row image copies ON UPDATE columns and a composite cluster-by hidden key unchanged, and the cascaded parent expression is relabeled with the child type without the normal assignment cast. Please reuse the regular UPDATE final-row, cast, PK/UK dedup, cluster-key, and index closure. If that cannot be shared yet, add one centralized capability gate over the full derived-column dependency closure instead of patching each reproduction independently.

Please add regressions for generated-unique collision with rollback, parent/child width mismatch, composite cluster-by recomputation, and ON UPDATE feeding a generated column.

@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.

[P1] Lock affected parent keys before every child FK check/action

The current plan materializes the parent source and fans it out before the parent old/new referenced keys are locked. appendUpdateParentForeignKeyChecks creates sourceStep, each child CASCADE/SET NULL is appended as an independent step, and the parent LOCK_OP is only added later in bindUpdate. appendStep does not create execution ordering, and compile submits the resulting scopes concurrently.

This leaves a transaction-level closure hole that single-session BVT cannot cover:

  1. T1 reads/materializes parent.id = 1 and its child action/check observes the current child snapshot.
  2. T2 inserts a child referencing key 1. The INSERT path takes a shared parent-key lock, but T1 has not taken its exclusive lock yet, so T2 can commit.
  3. T1 later locks and changes the parent key from 1 to 2.
  4. The newly committed child was not in T1's action/check snapshot and remains at 1, producing an orphan. RESTRICT/NO ACTION has the same check-before-lock race.

Stable sorting of mutations or lock targets cannot close this because the steps themselves have no ordering edge.

Please acquire exclusive locks for the affected parent old/new referenced keys before any child scan/check/action, materialize the locked parent row image once, and make the parent mutation plus every child consumer depend on that sink. Add a deterministic two-session regression (preferably with a barrier/failpoint) proving that an INSERT referencing the old key blocks/fails or is included in the cascade, and can never leave an orphan.

Additional findings to address while touching this path:

  • bindAndOptimizeUpdateQuery unconditionally calls getUpdateTableInfo after ResolveUpdateTables, even for tables with no FK. Guard it with actual FK sensitivity or retain the already-resolved metadata.
  • The early HasForeignKeyAction mark disables prepare/compile caching for unrelated child-column updates such as UPDATE child SET note=?, then rebuilds the plan on every EXECUTE. Scope sensitivity to columns that can actually affect an FK action/check.
  • validateModernUpdateParentMutation discards the valid-index slice from getValidIndexes, while appendUpdateParentMutation loops raw regular indexes without checking TableExist. Use the filtered list so transitional TableExist=false metadata does not resolve a nonexistent hidden table.
  • Parent RESTRICT currently uses the two-argument assert and returns internal error; use the existing typed ErrFKRowIsReferenced contract. Also classify row-closure rejections as an FK/row-closure route reason instead of generic rejected/binder_error so the metric and logs retain the cause.

The two previous requested-change issues are addressed on this head: FK-sensitive prepare toggles are now guarded, and unsafe derived-row cascades are explicitly rejected. The blocker above is new and concerns transaction ordering rather than row-image closure.

@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.

[P1] Optimistic parent-key UPDATE can commit an orphan

The new parent-side closure relies on LOCK_OP, but pkg/sql/colexec/lockop/lock_op.go:139-141 makes that operator a pass-through for optimistic transactions, and child FK inserts only acquire the shared parent-key lock in pessimistic mode.

I reproduced this on this exact head with an embedded cluster in optimistic SI mode:

  1. T1 begins and updates parent key 1 to 2.
  2. T2 inserts a child referencing 1 and commits.
  3. T1 commits.
  4. Both writes succeed and the orphan count is 1.

This affects RESTRICT/NO ACTION as well as CASCADE/SET NULL. Please either add a real optimistic conflict/commit-validation closure or, as the minimal safe change for this PR, reject affected parent FK operations in optimistic mode as REPLACE already does. Add a deterministic two-session regression.

[P1] Compute FK cache sensitivity from the final UPDATE row image

pkg/sql/plan/bind_update.go:1214-1262 only checks columns explicitly present in the SET map. Implicit ON UPDATE assignments and generated-column dependencies are added later, so a plan can depend on foreign_key_checks while HasForeignKeyAction remains false.

The black-box reproduction used a child FK timestamp column with ON UPDATE current_timestamp: PREPARE a note-only UPDATE with foreign_key_checks=0, enable checks, then EXECUTE. EXECUTE returned success and left one orphan.

Please derive one final changed-column dependency closure before consulting foreign_key_checks and reuse it for HasForeignKeyAction, FK checks/actions, and index selection. The regression should cover at least the 0-to-1 prepare toggle for implicit ON UPDATE; this should not be special-cased to timestamps.

[P1] Reject mutating actions on a non-unique referenced prefix

MatrixOne permits a legacy FK to reference a leading, non-unique prefix of a composite parent PK. appendUpdateParentKeyLocks falls back to a table lock when no exact PK/unique index exists, but validateModernUpdateParentMutation never verifies that the parent-to-child join is functional.

Concrete runtime reproduction:

  • parent PK is (a,b), with rows (1,1) and (1,2);
  • one child row references parent(a)=1 with ON UPDATE CASCADE;
  • UPDATE parent SET a=b+1 WHERE a=1 returns success;
  • both parent rows move to a=2/3, while the child remains a=1, leaving an orphan.

For CASCADE/SET NULL, require the referenced columns to exactly match the parent PK or a live exact UNIQUE index; otherwise reject the graph before mutation. Do not leave duplicate physical child targets to MULTI_UPDATE.

[P2] Use one canonical FK physical lock order across UPDATE/INSERT/REPLACE

The parent UPDATE path sorts lock targets by ObjRef.ObjName at bind_update_fk.go:521-536, which puts hidden index tables before a normal base-table name. Child INSERT explicitly orders base table before hidden indexes at bind_insert.go:1151-1170.

A focused plan test produces UPDATE order [hidden, hidden, base, base] and child INSERT order [base, hidden] for the same parent PK plus unique-key namespaces. This creates an avoidable reverse wait order and repeatable deadlock/retry pressure. Please centralize the physical target comparator and reuse it in every FK lock producer.

[P2] Do not rewrite unrelated child indexes during a cascade

appendUpdateParentMutation resolves and writes every valid child index. In the existing emp/dept fixture, cascading only deptno still creates three UpdateCtx entries: the base table plus two ename/job indexes whose keys do not change. This adds hidden-table scans, delete/insert work, locks, and conflict amplification.

Filter index maintenance from the same final changed-column closure. With the current capability gates, only indexes whose parts intersect the replacement set need maintenance; retain the normal PK-change rule for future supported closures.

Validation on head d721e47:

  • existing affected-column routing, lock-before-child-consumer, and explicit-FK prepare tests pass;
  • all three new planner safety assertions expose the missing gates;
  • embedded-cluster tests reproduce all three orphan outcomes;
  • focused plan tests confirm the reversed lock order and 3-versus-1 index UpdateCtx regression;
  • remote serialization, affected-row propagation, resource ownership, and rollback plumbing reviewed cleanly;
  • current main is four commits ahead, but none changes the responsible paths.

The PR description should also be aligned with the current capability gate: generated/ON UPDATE child closures are rejected rather than maintained, and duplicate-target rejection is not complete until the non-unique-prefix case is closed.

@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.

[P1] The non-unique-prefix compatibility closure still rejects safe parent updates.

The new guard partitions changed parent rows only by the old referenced key and executes before the child-table scan. As a result, two changed parents sharing an old legacy prefix always trip the assertion, even when there is no matching child row and therefore no referential action or ambiguous target at all. It also rejects a functional many-to-one mapping where every changed parent maps the old key to the same new key.

I reproduced the first case through the SQL frontend on this exact head:

create table p(a int, b int, primary key(a,b));
create table c(id int primary key, pa int,
  foreign key(pa) references p(a) on update cascade);
insert into p values (1,1),(1,2);
update p set a=b+1 where a=1;

With c empty, the UPDATE still fails with Error 20105: parent foreign key action has an ambiguous non-unique referenced-key mapping. There is no child mapping to disambiguate, so this is a behavior regression caused by placing the guard on the parent stream.

Please detect ambiguity over actual child targets and the old-to-new referenced-key mapping. For CASCADE, equal new tuples are one functional mapping; differing new tuples must fail atomically. SET NULL has one output regardless of the parent new tuple and should deduplicate child row identities rather than classify the mapping as ambiguous. Add black-box regressions for no matching child, same-old-to-same-new, and same-old-to-different-new rollback.

The previous optimistic-mode, implicit final-row cache sensitivity, lock ordering, lock-before-consumer, and affected-index findings are otherwise addressed. Focused planner/compile tests and the two new embedded execution regressions pass on this head; the added counterexample above fails deterministically.

Comment thread pkg/sql/plan/bind_update_fk.go Outdated
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/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants