refactor(plan): remove legacy UPDATE planner - #27482
Conversation
# Conflicts: # pkg/sql/colexec/multi_update/insert.go # pkg/sql/colexec/multi_update/types.go # pkg/sql/plan/bind_update.go
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
[blocker] Parent-key CASCADE can drop a child row when its PK changes and it has a nullable single-column secondary index.
appendUpdateParentMutation now maintains every regular index when primaryKeyChanged, but the old-index scan is still INNER for every non-unique index. A one-part index stores the raw key, and check_null_and_insert_table omits NULL keys, so a child row whose indexed value is NULL has no hidden-index row. The INNER join therefore removes the base child row before the CASCADE write; the parent key can be updated while the child FK/PK is left unchanged.
Minimal case:
create table p (id int primary key);
create table c (
parent_id int primary key,
payload int,
key idx_payload (payload),
foreign key (parent_id) references p(id) on update cascade
);
insert into p values (1);
insert into c values (1, null);
update p set id = 2 where id = 1;I also added a temporary plan assertion on this exact head: the secondary-index lookup join must be LEFT, but it is emitted as INNER. Please keep the child base row independent of hidden-index presence (for example, mirror the root UPDATE path and use LEFT when the child PK changes), and add a runtime regression with payload IS NULL.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ac7cfae. The prior nullable-secondary-index blocker is fixed and mutation-sensitive: removing the primary-key-change LEFT-join condition makes the focused regression fail. Focused count=10, focused race, full owning-package tests, build, vet, diff check, and current required CI all pass. No remaining code correctness blocker found. Approving the implementation; the issue's soak/optimistic evidence remains an independent merge-policy gate for the merge owner.
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #26343
What this PR does / why we need it:
This change performs the final source-level cutover from the legacy UPDATE
planner to
MULTI_UPDATEand explicit route dispositions while preserving theexisting SQL compatibility contract.
ErrUnsupportedDML -> buildTableUpdatefallback, thelegacy route type, and the now-unreachable
build_update.goimplementation;and explain consumers intact;
graphs and irregular-index algorithms into stable pre-mutation rejections;
without treating them as writable DML targets;
ON UPDATE CASCADE/SET NULLby folding explicitroots and action rows into one physical
MULTI_UPDATEwriter;updates, including partitioned targets,
CLIENT_FOUND_ROWS, and changed-rowmodes;
atomically maintains their regular hidden indexes;
checkInsertPkDupplanner field so SCA remains clean;physical index checks;
batches as data rows;
ErrUnsupportedDMLcannot trigger asecond planner attempt;
Validation completed across this change series, with final affected-row
retesting on
mo/main6ff9be9646a6ac43e05ba3063de27b2248bbac62at PR head
89c73965f55bde0c41044d1e124da402966f61b0:make generate-pbtwice with identical generated hashes;mo-cgo-test -count=1 -timeout=600s ./pkg/sql/plan ./pkg/sql/colexec/multi_update ./pkg/sql/colexec/dispatch;TestIssue27334UpdateAffectedRowsHonorsClientFoundRows;TestUpdateIgnoreRepeatedAliasesAdvanceGreedily;insert_table91.2%; partition context clones andphysicalInsertAffectedRows100%;make build;make static-check(go vet/molint, license checks, and golangci-lint withzero final issues);
fk_issue_26831.sql: 36/36 passed;foreign_key_multilayer.sql: 160/160 passed;update_modern_fk.sql: 183/183 passed;update_multi_target_modern.sql: 309/309 passed;git diff --check.The external regression/soak observation window required by #26343 was not
available in the local environment. This PR remains Draft pending that evidence
and repository CI; no zero-legacy/zero-unknown production observation is claimed
by the local validation above.