Skip to content

planner: substitute merged DML IN/NOT IN subqueries via ListArg placeholder#19973

Merged
arthurschreiber merged 2 commits into
mainfrom
arthur/fix-dml-in-merge-listarg
May 5, 2026
Merged

planner: substitute merged DML IN/NOT IN subqueries via ListArg placeholder#19973
arthurschreiber merged 2 commits into
mainfrom
arthur/fix-dml-in-merge-listarg

Conversation

@arthurschreiber
Copy link
Copy Markdown
Member

@arthurschreiber arthurschreiber commented Apr 28, 2026

Description

When an UPDATE SET expression contains an IN/NOT IN subquery whose routing is compatible with the outer route, the planner merges the subquery into the outer route — but the merge-time AST rewrite only substituted back *sqlparser.Argument placeholders, never sqlparser.ListArg. DML SET expressions placeholder IN/NOT IN subqueries with ListArg (a distinct AST type), so the placeholder leaked into the emitted SQL with no UncorrelatedSubquery primitive above to back it. Sending such a plan to vttablet would fail with missing bind var __sqN.

The fix adds a case sqlparser.ListArg arm to the type switch in rewriteMergedSubqueryExpr (go/vt/vtgate/planbuilder/operators/subquery_planning.go) mirroring the existing *Argument branch — the same cursor.Replace(sq.originalSubquery) path then substitutes the original inline subquery, matching the shape that DELETE … WHERE col = (1 in (select …)) already produces.

Two regression cases are added to dml_cases.json covering UPDATE+IN and UPDATE+NOT IN with merge-compatible routing. The existing UPDATE: same subquery as scalar SET and IN SET case targets user_extra inside a user update, so the routes don't merge and the bug couldn't surface there.

Adjacent rewrite paths (Ordering.settleOrderingExpressions, projection pe.Original) were intentionally left alone — isDML=true is only set in update.go for SET expressions, so a ListArg placeholder cannot reach those paths under any current call graph.

Related Issue(s)

Fixes #19965

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

None — purely planner-level correctness fix. No previously-working query shape changes plan; the affected shape (DML SET with merge-compatible IN/NOT IN subquery) was emitting a broken plan that would have failed at vttablet, so no production deployments are exercising it today.

AI Disclosure

Most of this was written by Claude Code — I just provided direction.

…holder

When an UPDATE SET expression contains an IN/NOT IN subquery whose
routing is compatible with the outer route, the planner merges the
subquery into the outer route. The merge-time AST rewrite walked
expressions looking for *sqlparser.Argument placeholders to substitute
back with the original *Subquery, but the DML SET path mints
sqlparser.ListArg placeholders for IN/NOT IN (a separate AST type).
The placeholder leaked into the emitted SQL with no UncorrelatedSubquery
primitive to back it, so vttablet would have failed with
"missing bind var __sqN".

Add a sqlparser.ListArg arm to the type switch in
rewriteMergedSubqueryExpr so the same substitution path applies.

Fixes #19965

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Copilot AI review requested due to automatic review settings April 28, 2026 09:51
@github-actions github-actions Bot added this to the v25.0.0 milestone Apr 28, 2026
@vitess-bot vitess-bot Bot added NeedsWebsiteDocsUpdate What it says NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Apr 28, 2026
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot Bot commented Apr 28, 2026

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@arthurschreiber arthurschreiber removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Apr 28, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a vtgate planner correctness bug where merged IN/NOT IN subqueries inside UPDATE ... SET could leave behind a ListArg placeholder (e.g. ::__sqN) in the emitted SQL, causing execution-time failures due to missing bind vars.

Changes:

  • Extend rewriteMergedSubqueryExpr to also recognize and rewrite sqlparser.ListArg placeholders when substituting merged subqueries back inline.
  • Add regression plan-test cases covering UPDATE ... SET with merge-compatible IN and NOT IN subqueries.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
go/vt/vtgate/planbuilder/operators/subquery_planning.go Add sqlparser.ListArg handling so merged DML IN/NOT IN placeholders are rewritten back to the original inline subquery.
go/vt/vtgate/planbuilder/testdata/dml_cases.json Add regression cases ensuring UPDATE ... SET merged IN/NOT IN subqueries no longer emit ::__sqN.

@arthurschreiber arthurschreiber marked this pull request as ready for review April 28, 2026 09:55
@arthurschreiber arthurschreiber added Backport to: release-23.0 Needs to be backport to release-23.0 Backport to: release-24.0 Needs to be backport to release-24.0 labels Apr 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.47%. Comparing base (70c7a72) to head (7e6fa5e).
⚠️ Report is 222 commits behind head on main.

Files with missing lines Patch % Lines
.../vtgate/planbuilder/operators/subquery_planning.go 0.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (70c7a72) and HEAD (7e6fa5e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (70c7a72) HEAD (7e6fa5e)
1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #19973       +/-   ##
===========================================
- Coverage   69.67%    3.47%   -66.20%     
===========================================
  Files        1614       60     -1554     
  Lines      216793     9758   -207035     
===========================================
- Hits       151044      339   -150705     
+ Misses      65749     9419    -56330     
Flag Coverage Δ
partial 3.47% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arthurschreiber arthurschreiber marked this pull request as draft May 5, 2026 13:53
The two new dml_cases.json entries hit MySQL errno 1093 ("can't specify
target table for update in FROM clause") because the merged subquery
references the same table being UPDATEd. Wrapping the inner reference
in a derived table avoids the restriction without changing the merge
path the test is meant to cover (verified: reverting the planner fix
still produces the leaked ::__sq1 placeholder).

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
@arthurschreiber arthurschreiber marked this pull request as ready for review May 5, 2026 15:22
Copilot AI review requested due to automatic review settings May 5, 2026 15:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Member

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️

@arthurschreiber arthurschreiber enabled auto-merge (squash) May 5, 2026 15:44
@arthurschreiber arthurschreiber merged commit 5ea4edd into main May 5, 2026
127 of 129 checks passed
@arthurschreiber arthurschreiber deleted the arthur/fix-dml-in-merge-listarg branch May 5, 2026 17:13
arthurschreiber pushed a commit that referenced this pull request May 5, 2026
…a ListArg placeholder (#19973) (#20030)

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
timvaillancourt pushed a commit to timvaillancourt/vitess that referenced this pull request May 12, 2026
…holder (vitessio#19973)

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug Report: planner: merged IN/NOT IN subquery in UPDATE SET leaks ::__sqN ListArg into emitted SQL

4 participants