Skip to content

vtgate: add VEXPLAIN MYSQLPLAN statement - #20817

Open
ejortegau wants to merge 38 commits into
vitessio:mainfrom
ejortegau:ejortegau/vexplain-mysqlplan
Open

vtgate: add VEXPLAIN MYSQLPLAN statement#20817
ejortegau wants to merge 38 commits into
vitessio:mainfrom
ejortegau:ejortegau/vexplain-mysqlplan

Conversation

@ejortegau

Copy link
Copy Markdown
Contributor

Description

Adds a new VEXPLAIN MYSQLPLAN <select> statement that runs MySQL's EXPLAIN FORMAT=JSON against the shards a SELECT would target, without executing the query itself.

It builds the real VTGate plan, resolves each Route's target shards from its vindex at resolution time (the same computation the query would do at execution — pure vindex + topo lookup, no data read), and issues EXPLAIN FORMAT=JSON against every resolved shard. The per-shard MySQL plan is attached to the VTGate plan tree keyed by shard, so per-shard plan and cost differences are visible.

How it differs from the existing modes:

  • VEXPLAIN PLAN shows the plan tree but no MySQL EXPLAIN and no resolved shards.
  • VEXPLAIN ALL attaches MySQL EXPLAIN but executes the query to discover the shard-level queries, and reports only one shard per primitive.
  • VEXPLAIN MYSQLPLAN never runs the wrapped query, and reports every resolved shard separately.

Only SELECT statements whose target shards can be resolved from a vindex without reading cluster data are supported. DML (INSERT/UPDATE/DELETE), and any query whose shard set depends on data — cross-shard joins, subqueries, and lookup vindexes — are rejected at plan time with an error pointing the user to VEXPLAIN ALL.

Example (scatter over a 4-shard keyspace), abbreviated:

{
  "OperatorType": "Route",
  "Variant": "Scatter",
  "Query": "select id from `user`",
  "mysql_explain_json": {
    "-40":   { "query_block": { ... } },
    "40-80": { "query_block": { ... } },
    "80-c0": { "query_block": { ... } },
    "c0-":   { "query_block": { ... } }
  }
}

Related Issue(s)

Fixes #20816

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

Tests: parser round-trip case, planbuilder/engine unit tests, executor tests (per-shard EXPLAIN fan-out keyed by shard; DML/join/lookup fail-fast), and an end-to-end test in go/test/endtoend/vtgate/queries/vexplain verified against real MySQL 8.4.10. A changelog entry was added to changelog/25.0/25.0.0/summary.md.

Deployment Notes

New user-visible SQL statement. The new grammar keyword is MYSQLPLAN (not MYSQL) specifically to avoid turning the common mysql identifier into a reserved word that would be backtick-escaped in query normalization. As with any new syntax, an older vtgate cannot parse it during a rolling upgrade; the end-to-end test is gated with SkipIfBinaryIsBelowVersion(t, 25, "vtgate") for the upgrade/downgrade CI.

AI Disclosure

This PR was written primarily by Claude Code, with direction and review from the author.

VEXPLAIN MYSQLPLAN <select> runs MySQL's EXPLAIN FORMAT=JSON against the
shards a SELECT would target, without executing the query itself. It
resolves each Route's target shards from its vindex at resolution time
and issues EXPLAIN against every resolved shard, attaching the per-shard
MySQL plan to the VTGate plan tree keyed by shard so per-shard plan and
cost differences are visible.

Unlike VEXPLAIN ALL, which executes the query to discover the shard-level
queries before explaining them, MYSQLPLAN never runs the wrapped query.

Only SELECT statements whose target shards can be resolved from a vindex
without reading cluster data are supported. DML (INSERT/UPDATE/DELETE),
and any query whose shard set depends on data - cross-shard joins,
subqueries, and lookup vindexes - are rejected at plan time with an error
pointing the user to VEXPLAIN ALL.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings August 10, 2026 09:43

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added this to the v25.0.0 milestone Aug 10, 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 Aug 10, 2026
@vitess-bot

vitess-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6e5aace89

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread go/vt/vtgate/planbuilder/vexplain.go Outdated
Comment thread go/vt/vtgate/engine/vexplain.go Outdated
Comment thread go/vt/vtgate/planbuilder/vexplain.go Outdated
Comment thread go/vt/vtgate/engine/vexplain.go Outdated
Comment thread go/vt/vtgate/engine/vexplain.go Outdated
Comment thread go/vt/vtgate/engine/vexplain.go Outdated
ejortegau and others added 6 commits August 10, 2026 14:45
The support check for VEXPLAIN MYSQLPLAN was a denylist that rejected a
fixed set of primitive types and allowed everything else. Data-dependent
plans not on the list (e.g. recursive CTEs, whose Term-side Routes are
parameterized by rows produced at runtime) slipped through to a cryptic
"query arguments missing" error instead of the intended message pointing
the user at VEXPLAIN ALL.

Invert the check to an allowlist: permit a Route (with a resolvable
vindex) and the shard-independent container primitives that forward bind
variables to their inputs unchanged, keep the SELECT-only message for
DML, and default everything else to the "cannot resolve target shards"
rejection. This is fail-closed as new primitive types are added.

Add a recursive-CTE rejection case and a multi-Route (UNION) happy-path
test asserting each Route node carries its own per-shard EXPLAIN output.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
When a Route resolves to no shard but is marked for no-routes special
handling (e.g. an aggregate SELECT whose predicate maps to no shard),
normal execution falls back to an arbitrary shard so the query still
reaches a tablet. VEXPLAIN MYSQLPLAN now mirrors that fallback so it
produces EXPLAIN output for such a Route instead of attaching none.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
For queries eligible for deferred plan optimization, VEXPLAIN MYSQLPLAN
explains the general (baseline) plan rather than the value-specific
optimized branch, so it reports the full shard footprint the query can
target. Document this so the behavior is not mistaken for a bug.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
A bind-var scatter limit (e.g. `limit :limit, :offset`) plans as Limit ->
Route where the Route query uses `limit :__upper_limit`. VEXPLAIN MYSQLPLAN
recursed straight into the child Route with the original bind vars, so
`:__upper_limit` was never computed and EXPLAIN was sent with it unbound,
failing against a real tablet.

Mirror Limit.TryExecute: compute count+offset into a copied bind var map and
bind __upper_limit before recursing into the input.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
A SELECT with an explicit shard/keyrange target plans as a bypass Send, not a
Route. That was rejected by the fail-closed allowlist even though the target
shards are trivially resolvable from the destination.

Accept a read Send in the allowlist (DML/DDL sends still reject) and, in the
explain walk, resolve its target destination and run EXPLAIN FORMAT=JSON per
shard, keyed by shard like Route. Factor the shared per-shard EXPLAIN loop into
explainOnShards.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 10, 2026 16:54

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42c6562135

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread go/vt/sqlparser/constants.go

@mattlord mattlord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ejortegau, this looks good. Beyond the existing VTAdmin conversion thread, I think there are three substantive things left:

  1. I think that MYSQLPLAN should preserve the current reserved MySQL session, or explicitly reject session-local objects. In go/vt/vtgate/engine/vexplain.go:326, every EXPLAIN uses ExecuteStandalone, which creates an autocommit session without the existing shard-session/reserved-connection IDs. I reproduced a temporary-table session taking a second reservation for MYSQLPLAN. That new MySQL connection cannot see the temporary table, so the SELECT works while its MYSQLPLAN fails instead of showing the plan the SELECT would use. I think we should use the existing reserved shard session when one exists, or return a deliberate unsupported error, with an end-to-end temporary-table regression test. No?

  2. I think that we should reject sequence Next routes explicitly. The Route allowlist in go/vt/vtgate/planbuilder/vexplain.go:100 only checks the vindex, so I reproduced vexplain mysqlplan select next 2 values from user_seq sending explain format = json select next 2 values from user_seq. Tabletserver recognizes the bare SELECT as PlanNextval, but the wrapped EXPLAIN is passed through to MySQL, where that Vitess-specific syntax is invalid. This should use a dedicated error that does not suggest VEXPLAIN ALL, since ALL executes the sequence request and consumes values. I think we should also test that no tablet query is sent.

  3. I think that the shard work should use bounded concurrent fanout and be included in ShardQueries accounting. go/vt/vtgate/engine/vexplain.go:323-334 executes every shard serially; eight shards with 50 ms simulated latency took about 410 ms, so latency grows with every shard and every Route. The same ExecuteStandalone path bypasses the normal ShardQueries increment: I observed eight EXPLAIN calls logged as zero shard queries. Since all-shard scatter is the primary use case, I think we should add concurrent collection with accurate accounting and focused tests for both.

One tiny cleanup: TestVExplainMySQLPlanKeysByShard and TestVExplainMySQLPlanRequiresExecution are the only newly added functions without short purpose comments.

Otherwise it LGTM! ❤️

ejortegau and others added 4 commits August 11, 2026 16:54
VTGateProxy.VExplain dropped MySQLVExplainType into the default switch
branch and returned (nil, nil), silently discarding the JSON plan.
MYSQLPLAN uses the same single-column result shape as ALL, so convert it
via convertVExplainResultToString.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Each EXPLAIN in MYSQLPLAN runs through ExecuteStandalone, which uses a
fresh autocommit session with no shard/reserved-connection state. When
the session holds a reserved connection (e.g. one that created a
temporary table), that separate connection cannot see the session-local
state, so the captured plan would not match the one the real query would
use - the SELECT works while its MYSQLPLAN fails or reports a misleading
plan.

Fail closed in that case with a dedicated UNIMPLEMENTED error. VEXPLAIN
ALL shares the same standalone-EXPLAIN path, so the message does not
point users at it.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
A sequence next-value query plans as an unsharded Route with
Opcode == engine.Next and a nil vindex, so it passed the MYSQLPLAN
allowlist and MySQL was asked to EXPLAIN the Vitess-specific
'select next ... values' syntax it cannot parse. Reject it at plan
time with a dedicated error that does not point at VEXPLAIN ALL,
since ALL would execute the query and consume sequence values.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…ries

VEXPLAIN MYSQLPLAN previously ran its per-shard EXPLAIN FORMAT=JSON
queries serially and never counted them in ShardQueries, so a wide
scatter reported zero shard queries even though it fanned out to every
shard.

Run the per-shard EXPLAINs concurrently through an errgroup bounded by
maxParallelMySQLExplains (8) so a wide scatter or a multi-Route plan
cannot fan out an unbounded number of shard queries at once. Shard
resolution stays serial - it is cheap and a pushed-down Limit must
compute __upper_limit before its child Route is visited.

Add RecordShardsQueried to the engine.VCursor interface and call it once
per run with the number of shards queried. ExecuteStandalone (unlike the
normal shard path) does not increment ShardQueries, so MYSQLPLAN now
reports N shard queries for an N-shard scatter, consistent with how
VEXPLAIN ALL accounts for the same query.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
ejortegau and others added 2 commits August 18, 2026 10:31
runMySQLExplainTasks only counted shards whose EXPLAIN returned a nonempty
plan, so ShardQueries and accumulated plan statistics under-reported shards
whose EXPLAIN errored or returned no rows. This diverged from
VCursorImpl.ExecuteMultiShard, which counts the targeted shards up front
regardless of per-shard outcome.

Record one shard query per task before running them, matching the normal
shard path. This reverses the earlier "count only successfully-explained
shards" behavior; the empty-result and error accounting tests now expect the
full attempted count.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
A SELECT of an advisory lock function (get_lock, release_lock,
release_all_locks, is_free_lock, is_used_lock) plans as an engine.Lock
primitive, which is neither in the MYSQLPLAN allowlist nor the DML list, so
it fell through to the default rejection that points the user at VEXPLAIN
ALL. Following that advice runs the wrapped SELECT, which executes the lock
function and acquires or releases advisory locks as a side effect.

Detect the lock function on the AST before planning, like the sequence case,
and reject it without pointing at VEXPLAIN ALL. The read-only is_free_lock
and is_used_lock are rejected uniformly: they are equally unexplainable
through MYSQLPLAN.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 18, 2026 08:49

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ejortegau

Copy link
Copy Markdown
Contributor Author

I agree with the two unresolved comments about not recommending VEXPLAIN ALL for lock functions and counting every attempted shard query. The older bind-variable thread appears addressed by the per-input copies and seems safe to resolve.

These should be addressed now.

@ejortegau
ejortegau requested a review from mattlord August 18, 2026 08:49

@mattlord mattlord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is looking really good! I did another fresh deep review given that we've come so far and are nearing the end and getting close to merging.

The first item feels like it should block merging; item 3 I think could go either way (commenting or fixing). The rest are worth doing IMO but smaller and non-blocking.

1. A plain SET sql_mode breaks MYSQLPLAN and pins the sessionengine/vexplain.go:267 Setting any SET_VAR-eligible sysvar (as JDBC/ORM clients do at connect) leaves the session unpinned, but planning the VEXPLAIN trips the deferred CheckForReservedConnection because VExplainStmt isn't in the SupportOptimizerHint list. So the guard fires with the "temporary table" message, and the session stays pinned to a reserved connection afterward — on every retry, since VEXPLAIN plans aren't cached. It's a pure false positive too: the SET_VAR hint rides on the inner SELECT, so the standalone EXPLAIN would honor the sysvars anyway. Suggest teaching the guard (or CheckForReservedConnection) to distinguish real temp-table pinning, and adding a sysvar-only-session test.

2. SQL_CALC_FOUND_ROWS + GROUP BY sneaks a derived table past the guard
planbuilder/vexplain.go:206
The nested-query walk runs before planning, but buildSQLCalcFoundRowsPlan re-parses the query and wraps it in select count(*) from (select …) as t when GROUP BY/HAVING is present — and SQLCalcFoundRows is allowlisted, so that derived-table query ships to every shard inside EXPLAIN. That's exactly the materialization case the "never runs the wrapped query" promise is about (the plan fixture "sql_calc_found_rows with group by and having" shows the shard-query shape). Simplest fix: reject SQL_CALC_FOUND_ROWS up front, or re-check the planned Route queries.

3. The EXPLAIN fan-out widens a pre-existing table-ACL gapengine/vexplain.go:386
vttablet gives EXPLAIN statements empty plan.Permissions, so per-shard EXPLAINs are never ACL-checked. That hole exists today (plain EXPLAIN through vtgate reaches one arbitrary shard), but MYSQLPLAN extends it to every resolved shard of every keyspace in the plan — and unlike VEXPLAIN ALL, nothing ACL-checked ever runs. A user with no READER role gets full per-shard plan metadata (index names, row estimates, filtered %). The deep fix is tabletserver-side (derive permissions for the wrapped statement in BuildPermissions), but it's worth deciding explicitly and adding a test either way.

4. Views get a misleading, config-dependent rejectionplanbuilder/vexplain.go:119
With view tracking on, the normalizer turns a view reference into a DerivedTable before the guard walk, so a single-shard select * from my_view where id = 5 fails with "cross-shard join, subquery, or lookup vindex" — none of which the query contains. With tracking off, the same query works. Worth a view-specific message (or support), and a changelog mention that views/derived tables/CTEs are unsupported.

5. The VCursor interface change is an unannounced break — and may not be needed
engine/primitive.go:83
RecordShardsQueried breaks every out-of-tree VCursor implementation at compile time, with no Breaking Changes entry (the same file documents BackupHandle.Wait() for exactly this shape). Its own doc comment says it exists because ExecuteStandalone doesn't count shard queries — a one-line increment inside VCursorImpl.ExecuteStandalone (mirroring ExecuteKeyspaceID) would fix that for VEXPLAIN ALL and sequence generation too, and the interface method disappears.

6. The fan-out re-implements scatter, slowly, with a latent raceengine/vexplain.go:382
One ExecuteStandalone per shard means one session deep-clone per shard, all behind a hardcoded 8-way errgroup — a 256-shard scatter becomes 32 serialized waves, while the query being explained is a single parallel fan-out. It also makes ExecuteStandalone concurrent for the first time, and that method writes SafeSession.LastInsertId unsynchronized (vcursor_impl.go:943) — safe only because EXPLAIN never sets InsertID, and the concurrency test uses the fake vcursor so -race never sees it. Grouping tasks per Route into one multi-shard call would remove the cap, the clones, the mutex, and item 5's workaround in one go.

7. INTO OUTFILE / FOR UPDATE slip through as raw MySQL errors
planbuilder/vexplain.go:115
Both parse as explainable selects and pass every guard, so shards receive
explain format = json select … into outfile …, MySQL rejects it, and the whole statement fails with an unattributed shard error (cancelling the healthy shards' EXPLAINs). VEXPLAIN ALL's parse-before-send gate wasn't carried over — either that gate or an AST check would give the clean rejection the docs promise.

8. The MergeSort allowlist entry can't do its jobplanbuilder/vexplain.go:198
MergeSort embeds noInputs and keeps its children in Primitives, invisible to Inputs() — so if it ever appeared in a plan, its whole subtree would skip validation and get no EXPLAINs, silently. Today planbuilder never emits it, so the entry is dead code; suggest removing it (fail-closed will catch it if that ever changes).

9. The e2e suite never fans out to more than one shardqueries/vexplain/vexplain_test.go:167
Both positive e2e cases are EqualUnique routes hitting one of the four shards, so the core feature — the concurrent multi-shard fan-out and shard-keyed map — is only ever tested against sandboxconn. One scatter query asserting all four shard names appear as keys would close it. (Tiny nit: the MySQL-version probe reads Keyspaces[0], which the new setup order makes the unsharded keyspace — harmless, but worth pinning to the sharded one.)

10. One JSON key, two shapesengine/vexplain.go:416
mysql_explain_json holds a single object under ALL and a shard→object map under MYSQLPLAN. Nothing decodes it generically today, so this is cheap to fix now (e.g.
mysql_explain_json_by_shard) and breaking to fix later.

Some smaller things that I also think are probably worth doing:

  • The three separate AST rejection walks could be one Walk with a type switch (and early abort).
  • The eight-type DML primitive case could be a single statement-type check before planning; the hand-maintained list will drift.
  • mysqlExplainTask sits mid-file instead of the top type () block.
  • An empty EXPLAIN result silently drops that shard from the map — a warning would beat silence.
  • Inside an open transaction the EXPLAIN reflects pre-transaction state (same as ALL) — worth one changelog sentence or a session warning.

ejortegau and others added 16 commits August 19, 2026 15:30
CheckForReservedConnection treated a VExplainStmt as an unknown statement
and fell through to NeedsReservedConn, so planning VEXPLAIN MYSQLPLAN in a
session that had only set a SET_VAR-eligible sysvar (e.g. sql_mode) pinned
the session and tripped the MYSQLPLAN reserved-connection guard with a
misleading temporary-table error. Unwrap the VExplainStmt and decide against
its inner statement, so a VEXPLAIN of a SELECT is treated like the SELECT and
is not spuriously pinned. The SET_VAR hint rides on the inner SELECT, so the
standalone EXPLAIN still honours the sysvar.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…Y/HAVING

The nested-query guard walks the original AST, where SELECT SQL_CALC_FOUND_ROWS
has no derived table. But when a LIMIT and GROUP BY/HAVING are present, the
planner rewrites the row-count half into `select count(*) from (select ...) as t`
(buildSQLCalcFoundRowsPlan), wrapping the original SELECT in a derived table.
Because SQLCalcFoundRows is on the MYSQLPLAN primitive allowlist, that query
would ship to every shard inside EXPLAIN FORMAT=JSON, which can materialize the
derived table during optimization and run a stored function once per shard -
violating MYSQLPLAN's promise never to run the wrapped query.

Add an AST guard that rejects exactly this shape before planning, with a
dedicated message pointing at VEXPLAIN ALL. The condition matches the planner's
rewrite trigger precisely: without a LIMIT the directive is ignored, and without
GROUP BY/HAVING the count query reuses the original SELECT with a single
count(*), so neither introduces a derived table.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…AINs

EXPLAIN statements carry no table permissions (BuildPermissions routes them to
the no-op case), so vttablet never runs a table-ACL check on the explained
tables. This hole predates VEXPLAIN MYSQLPLAN - a plain EXPLAIN through vtgate
already reaches one arbitrary shard unchecked - but MYSQLPLAN widens it to every
resolved shard of every keyspace in the plan, and unlike VEXPLAIN ALL nothing
ACL-checked ever runs. Query denylist rules still apply.

Closing the hole correctly (deriving the wrapped statement's permissions) is a
compatibility-sensitive behavior change to a shared tabletserver path and
belongs in its own PR. For now, document the limitation in the 25.0 summary and
add a characterization test pinning that EXPLAIN yields no permissions, using
the exact per-shard query shape MYSQLPLAN issues, so a future deep fix must
update it consciously.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…t message

The nested-query guard rejected derived tables and views with the generic
"cannot resolve the target shards (cross-shard join, subquery, or lookup
vindex)" message, which names none of the actual reasons an otherwise-routable
derived table or view is refused. Split the *sqlparser.DerivedTable case out
into its own message that names derived tables and views and explains the
materialization hazard; subqueries and CTEs keep the generic message.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…ability

RecordShardsQueried was added directly to the exported engine.VCursor
interface, which breaks any out-of-tree VCursor implementation at compile
time. Move it to a separate single-method ShardsQueriedRecorder interface and
type-assert to it at the VEXPLAIN MYSQLPLAN call site, so a VCursor that does
not track shard-query stats simply skips the accounting instead of failing to
compile. VCursorImpl satisfies the optional interface implicitly, so accounting
is unchanged.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
VEXPLAIN MYSQLPLAN previously fanned its per-shard EXPLAIN FORMAT=JSON
queries out with its own errgroup, bounded by maxParallelMySQLExplains,
issuing each shard's EXPLAIN through ExecuteStandalone. That duplicated
the scatter machinery, capped concurrency differently from a real
scatter, and relied on an optional ShardsQueriedRecorder to account for
the shard queries because ExecuteStandalone does not increment
ShardQueries on its own.

Extract the shared fan-out from ScatterConn.ExecuteMultiShard into a
private executeMultiShard helper that runs one query per shard and hands
each shard's result to a collect callback (serialized under the existing
mutex). ExecuteMultiShard keeps merging via AppendResult; a new
ExecuteMultiShardPerShard keeps the per-shard results aligned by index so
callers can attribute each to its shard. Expose it through the Executor,
the iExecute interface and VCursorImpl, where it runs in a fresh
autocommit session and never requests FetchLastInsertId - so, unlike
ExecuteMultiShard, it never writes SafeSession.LastInsertId and cannot
race on it.

MYSQLPLAN now type-asserts the VCursor to a new optional
MultiShardPerShardExecutor interface and issues one
ExecuteMultiShardPerShard call per Route/Send, reusing the normal
scatter's retry, transaction and accounting behavior: every targeted
shard is counted in ShardQueries and a failure against any shard fails
the whole command. This removes maxParallelMySQLExplains, the bespoke
errgroup fan-out and the now-dead ShardsQueriedRecorder /
RecordShardsQueried plumbing.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
engine.MergeSort is only ever constructed at stream-execute time (in
route.mergeSort), never emitted by the planbuilder into a compiled plan
tree, so the *engine.MergeSort case in checkVExplainMySQLSupported could
never match. Remove it; the fail-closed default reject already covers the
impossible case correctly. No behavior change.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
TestVExplainMySQLPlan gates its query_block assertion on the MySQL flavor of
clusterInstance.Keyspaces[0], but since the unsharded keyspace was added it
starts first, so Keyspaces[0] is the unsharded keyspace, not the sharded one
the MYSQLPLAN query runs against. Positional indexing into Keyspaces is fragile
because the slice order follows keyspace startup order in TestMain.

Add a keyspaceByName helper and look the sharded keyspace up by name for the
version probe. Route the reserved-connection test's unsharded-keyspace usage
through the same helper so keyspace lookups are consistent.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Every existing MYSQLPLAN e2e assertion used a single-shard point query, so the
per-shard EXPLAIN fan-out that the multi-shard executor performs was only
covered by unit tests. Add a scatter SELECT (no WHERE) to TestVExplainMySQLPlan
and assert the result carries per-shard EXPLAIN output keyed by at least two
distinct shard names, proving the fan-out end-to-end against real MySQL.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
VEXPLAIN MYSQLPLAN attaches a per-shard map (shard name -> MySQL EXPLAIN
FORMAT=JSON output) to each Route/Send node, whereas VEXPLAIN ALL attaches a
single EXPLAIN blob. Both used the same "mysql_explain_json" key, which
misrepresents the MYSQLPLAN value as a single plan rather than a by-shard map.

Rename only the MYSQLPLAN key to "mysql_explain_json_by_shard" so its shape is
self-describing; VEXPLAIN ALL keeps "mysql_explain_json". MYSQLPLAN is new and
unreleased in v25, so there is no compatibility concern with the rename.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
The nested-query, sequence, and advisory-lock rejections each ran their own
sqlparser.Walk over the statement. Combine them into a single checkVExplainMySQLAST
walk with one type switch that aborts on the first offending node, and merge their
rationale into one doc comment. No behavior change for single-violation queries;
the SQL_CALC_FOUND_ROWS check stays separate as it is a top-level type assertion,
not a walk.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
checkVExplainMySQLSupported hand-listed eight DML engine primitives (Insert,
InsertSelect, Upsert, Update, Delete, DMLWithInput, FkCascade, FkVerify) just to
give DML a SELECT-only error message; that list would drift as new DML primitives
are added. Reject DML up front with sqlparser.IsDMLStatement before planning
instead, and drop the engine-primitive case. The fail-closed default still catches
anything unexpected; the DML/DDL bypass-Send guard stays as it keys off stable
flags, not a type list.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Per the file convention of declaring all types in a single top-of-file type ()
block, relocate mysqlExplainTask (and its doc comment) there from mid-file. Pure
move, no behavior change.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
EXPLAIN FORMAT=JSON always returns one row of one column, and a per-shard failure
already aborts the whole command, so an empty result in runMySQLExplainTasks is
anomalous. Previously that shard was dropped from the output map silently; now we
log a warning with the keyspace and shard before omitting it. Adds a test that
captures log.Warn to prove the warning fires and the shard is omitted.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Each per-shard EXPLAIN runs on a separate connection, so a VEXPLAIN MYSQLPLAN
issued inside an open transaction reflects the pre-transaction state of each shard
rather than uncommitted changes - the same limitation as VEXPLAIN ALL. Document it
alongside the other MYSQLPLAN runtime caveats.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
…ide effect

VEXPLAIN MYSQLPLAN wraps the target query in EXPLAIN FORMAT=JSON, which
never executes it, so a SELECT ... INTO OUTFILE/DUMPFILE reaching a shard
writes no file. Add an end-to-end test on the unsharded keyspace that reads
the tablet's secure_file_priv directory, confirms a real SELECT ... INTO
OUTFILE writes there (proving the path works), then asserts that
VEXPLAIN MYSQLPLAN of the OUTFILE and DUMPFILE variants returns a plan and
writes no file.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
@ejortegau

Copy link
Copy Markdown
Contributor Author

1. A plain SET sql_mode breaks MYSQLPLAN and pins the sessionengine/vexplain.go:267 Setting any SET_VAR-eligible sysvar (as JDBC/ORM clients do at connect) leaves the session unpinned, but planning the VEXPLAIN trips the deferred CheckForReservedConnection because VExplainStmt isn't in the SupportOptimizerHint list. So the guard fires with the "temporary table" message, and the session stays pinned to a reserved connection afterward — on every retry, since VEXPLAIN plans aren't cached. It's a pure false positive too: the SET_VAR hint rides on the inner SELECT, so the standalone EXPLAIN would honor the sysvars anyway. Suggest teaching the guard (or CheckForReservedConnection) to distinguish real temp-table pinning, and adding a sysvar-only-session test.

Fixed in bc3d25b33f. CheckForReservedConnection now unwraps the VExplainStmt and decides against its inner statement, so a VEXPLAIN of a SELECT is treated like the SELECT and is no longer spuriously pinned. Added a sysvar-only-session test.

2. SQL_CALC_FOUND_ROWS + GROUP BY sneaks a derived table past the guardplanbuilder/vexplain.go:206 The nested-query walk runs before planning, but buildSQLCalcFoundRowsPlan re-parses the query and wraps it in select count(*) from (select …) as t when GROUP BY/HAVING is present — and SQLCalcFoundRows is allowlisted, so that derived-table query ships to every shard inside EXPLAIN. That's exactly the materialization case the "never runs the wrapped query" promise is about. Simplest fix: reject SQL_CALC_FOUND_ROWS up front, or re-check the planned Route queries.

Fixed in 2b9f32fd8d. SELECT SQL_CALC_FOUND_ROWS with a LIMIT that also carries GROUP BY/HAVING is now rejected on the AST before planning, since that's the exact shape buildSQLCalcFoundRowsPlan rewrites into a derived table.

3. The EXPLAIN fan-out widens a pre-existing table-ACL gapengine/vexplain.go:386 vttablet gives EXPLAIN statements empty plan.Permissions, so per-shard EXPLAINs are never ACL-checked. That hole exists today, but MYSQLPLAN extends it to every resolved shard of every keyspace in the plan — and unlike VEXPLAIN ALL, nothing ACL-checked ever runs. The deep fix is tabletserver-side (derive permissions for the wrapped statement in BuildPermissions), but it's worth deciding explicitly and adding a test either way.

Refrained from the deep fix here, and documented the limitation instead (0355407204).

The deep fix - deriving the wrapped statement's permissions in tabletserver's BuildPermissions - is a compatibility-sensitive behavior change to a shared path that also governs plain EXPLAIN and VEXPLAIN ALL. Changing it in this PR would silently start ACL-rejecting EXPLAINs that succeed today, which is exactly the kind of cross-cutting change VEP-1 asks us to stage on its own rather than smuggle into a feature PR.

4. Views get a misleading, config-dependent rejectionplanbuilder/vexplain.go:119 With view tracking on, the normalizer turns a view reference into a DerivedTable before the guard walk, so a single-shard select * from my_view where id = 5 fails with "cross-shard join, subquery, or lookup vindex" — none of which the query contains. Worth a view-specific message (or support), and a changelog mention that views/derived tables/CTEs are unsupported.

Fixed in e52ccf9b2f. The *sqlparser.DerivedTable case now has its own message naming derived tables and views and explaining the materialization hazard, instead of the generic cross-shard/subquery/lookup message; subqueries and CTEs keep the generic one. The 25.0 summary already states derived tables, views, and CTEs are unsupported.

5. The VCursor interface change is an unannounced break — and may not be neededengine/primitive.go:83 RecordShardsQueried breaks every out-of-tree VCursor implementation at compile time, with no Breaking Changes entry. Its own doc comment says it exists because ExecuteStandalone doesn't count shard queries — a one-line increment inside VCursorImpl.ExecuteStandalone would fix that for VEXPLAIN ALL and sequence generation too, and the interface method disappears.

No longer needed - the method is gone. It was first moved off the exported VCursor interface into an optional single-method interface so it couldn't break out-of-tree implementations (04dbb37c6a), and then removed entirely when the fan-out moved onto the shared scatter path (bdca6b04e5), which counts shard queries natively. This also resolves your "may not be needed" half: there's no bespoke accounting method left at all.

6. The fan-out re-implements scatter, slowly, with a latent raceengine/vexplain.go:382 One ExecuteStandalone per shard means one session deep-clone per shard, all behind a hardcoded 8-way errgroup. It also makes ExecuteStandalone concurrent for the first time, and that method writes SafeSession.LastInsertId unsynchronized. Grouping tasks per Route into one multi-shard call would remove the cap, the clones, the mutex, and item 5's workaround in one go.

Fixed in bdca6b04e5. Extracted the shared fan-out from ScatterConn.ExecuteMultiShard into a private executeMultiShard helper, and added ExecuteMultiShardPerShard that keeps per-shard results aligned by index. MYSQLPLAN now groups each Route's shards into one multi-shard call through that path, with no bespoke errgroup, no hardcoded cap, and no per-shard clones. It runs in a fresh autocommit session and never requests FetchLastInsertId, so unlike ExecuteMultiShard it never writes SafeSession.LastInsertId and can't race on it.

7. INTO OUTFILE / FOR UPDATE slip through as raw MySQL errorsplanbuilder/vexplain.go:115 Both parse as explainable selects and pass every guard, so shards receive explain format = json select … into outfile …, MySQL rejects it, and the whole statement fails with an unattributed shard error. VEXPLAIN ALL's parse-before-send gate wasn't carried over — either that gate or an AST check would give the clean rejection the docs promise.

Refrained from adding a guard, because on investigation the premise does not seem to hold: MySQL does not reject these under EXPLAIN FORMAT=JSON. I verified against MySQL 8.0.46 and 8.4.10 (script and output below) that EXPLAIN FORMAT=JSON of INTO OUTFILE/DUMPFILE and of FOR UPDATE/FOR SHARE/LOCK IN SHARE MODE returns a valid JSON plan with no error, and that the INTO OUTFILE/DUMPFILE case writes no file (a control real SELECT ... INTO OUTFILE on the same path does write, so the path works and EXPLAIN simply doesn't execute). So the opaque per-shard failure the item describes isn't there, and a rejection guard would only remove legitimate functionality (e.g. viewing the plan for a SELECT ... FOR UPDATE).

To pin the OUTFILE side-effect against regressions through the full vtgate→vttablet→mysqld path, I added an e2e test in aa5d20de2b: on the unsharded keyspace it reads the tablet's secure_file_priv dir, confirms a real SELECT ... INTO OUTFILE writes there (so the path works), then asserts VEXPLAIN MYSQLPLAN of the INTO OUTFILE and INTO DUMPFILE variants returns a plan and writes no file.

One limit on what I checked: I tested MySQL 8.0 and 8.4 only, not MariaDB or 5.7 — if either is still in the support matrix it's worth a confirming check before we fully close this.

Verification script and output

The script starts a throwaway MySQL container per version, mounts a host dir as the secure-file-priv target so INTO OUTFILE writes land where we can inspect them, runs a control real SELECT ... INTO OUTFILE (which must write a file, proving the path works), then the EXPLAIN variants (which must write nothing but still return a plan):

#!/usr/bin/env bash
#
# Self-contained verification that `EXPLAIN FORMAT=JSON` never executes the
# wrapped SELECT, for the clauses VEXPLAIN MYSQLPLAN wraps:
#   - INTO OUTFILE / INTO DUMPFILE  (must NOT write a file, but must return a plan)
#   - FOR UPDATE / FOR SHARE / LOCK IN SHARE MODE (must return a plan)
#
# For each MySQL version it:
#   1. starts a throwaway container (docker run --rm --network host), mounting a
#      host dir as the secure_file_priv target so OUTFILE writes land on the host
#      where we can inspect them directly (no docker exec needed);
#   2. waits for the server to accept connections;
#   3. runs the checks;
#   4. stops the container (--rm removes it) and cleans up the host dir.
#
# Requires: docker, and a `mysql` client on the host. No sudo, no password.
# Exits non-zero if any check fails.
set -u

# version -> host port (fresh ports so we don't collide with anything already
# bound on 3307/3308).
CASES=( "8.0:13307" "8.4:13308" )

FAILURES=0

# pass/fail helpers -----------------------------------------------------------
ok()   { echo "  PASS: $1"; }
bad()  { echo "  FAIL: $1"; FAILURES=$((FAILURES + 1)); }

run_case() {
  local ver="$1" port="$2"
  local img="mysql:$ver"
  local host_dir cid

  echo "############################################################"
  echo "# MySQL $ver  (port $port)"
  echo "############################################################"

  # Host dir mounted into the container as the ONLY place OUTFILE may write.
  # Fixed path per version. Clean it up-front (if a prior run left files owned by
  # the container's mysqld uid / re-owned by LXD userns id-mapping, a host-side
  # rm hits "Operation not permitted", so wipe it from inside a throwaway
  # container that shares the same uid mapping), then (re)create it. Nothing is
  # cleaned up at the end — the files are left in place for inspection.
  host_dir="/tmp/vexp-files-$ver"
  if [[ -d "$host_dir" ]]; then
    docker run --rm -v /tmp:/host alpine rm -rf "/host/$(basename "$host_dir")" >/dev/null 2>&1
  fi
  mkdir -p "$host_dir"
  # 0777 so the container's mysqld uid can create files here.
  chmod 777 "$host_dir"

  cid="$(docker run -d --rm --network host \
    -e MYSQL_ALLOW_EMPTY_PASSWORD=1 \
    -v "$host_dir":/exported \
    "$img" --port="$port" --secure-file-priv=/exported 2>&1)"
  if [[ $? -ne 0 || -z "$cid" ]]; then
    bad "could not start container for $img: $cid"
    rm -rf "$host_dir"
    return
  fi

  # Ensure the container is stopped no matter how we leave this function
  # (--rm removes it). Files written under host_dir are intentionally left in
  # place for inspection; they are cleaned up at the start of the next run.
  # shellcheck disable=SC2064
  trap "docker stop '$cid' >/dev/null 2>&1" RETURN

  # Wait (generously) for the server to accept queries — first boot initializes
  # the data dir and can take 10-30s on a cold image.
  local up=0 i
  for ((i = 0; i < 120; i++)); do
    if mysql -h 127.0.0.1 -P "$port" -u root -N -e "SELECT 1;" >/dev/null 2>&1; then
      up=1
      break
    fi
    sleep 1
  done
  if [[ $up -ne 1 ]]; then
    bad "server on port $port never became ready"
    echo "    --- last 20 log lines ---"
    docker logs "$cid" 2>&1 | tail -20 | sed 's/^/    /'
    return
  fi

  local realver
  realver="$(mysql -h 127.0.0.1 -P "$port" -u root -N -e "SELECT VERSION();" 2>/dev/null)"
  echo "  server up: $realver"

  mysql -h 127.0.0.1 -P "$port" -u root <<'SQL' 2>/dev/null
DROP DATABASE IF EXISTS vexptest;
CREATE DATABASE vexptest;
USE vexptest;
CREATE TABLE t (id INT PRIMARY KEY, v INT);
INSERT INTO t VALUES (1,10),(2,20),(3,30);
SQL

  local q
  q() { mysql -h 127.0.0.1 -P "$port" -u root vexptest -N -e "$1" 2>&1; }

  # --- CONTROL: a real SELECT ... INTO OUTFILE MUST write a file. -----------
  # Proves the OUTFILE path is functional, so "no file" from EXPLAIN below
  # genuinely means "not executed" rather than "writes are broken here".
  q "SELECT id FROM t INTO OUTFILE '/exported/control_real.txt';" >/dev/null
  if [[ -f "$host_dir/control_real.txt" ]]; then
    ok "control: real SELECT ... INTO OUTFILE wrote a file"
  else
    bad "control: real SELECT ... INTO OUTFILE did NOT write (OUTFILE path broken; rest is meaningless)"
  fi

  # --- INTO OUTFILE under EXPLAIN: no file, but a plan. ---------------------
  local plan
  plan="$(q "EXPLAIN FORMAT=JSON SELECT id FROM t INTO OUTFILE '/exported/explain_out.txt';")"
  if [[ -f "$host_dir/explain_out.txt" ]]; then
    bad "EXPLAIN ... INTO OUTFILE WROTE a file (query was executed!)"
  else
    ok "EXPLAIN ... INTO OUTFILE wrote no file"
  fi
  if grep -q '"query_block"' <<<"$plan"; then
    ok "EXPLAIN ... INTO OUTFILE returned a JSON plan"
  else
    bad "EXPLAIN ... INTO OUTFILE returned no plan: $plan"
  fi

  # --- INTO DUMPFILE under EXPLAIN: no file, but a plan. --------------------
  plan="$(q "EXPLAIN FORMAT=JSON SELECT id FROM t WHERE id=1 INTO DUMPFILE '/exported/explain_dump.txt';")"
  if [[ -f "$host_dir/explain_dump.txt" ]]; then
    bad "EXPLAIN ... INTO DUMPFILE WROTE a file (query was executed!)"
  else
    ok "EXPLAIN ... INTO DUMPFILE wrote no file"
  fi
  if grep -q '"query_block"' <<<"$plan"; then
    ok "EXPLAIN ... INTO DUMPFILE returned a JSON plan"
  else
    bad "EXPLAIN ... INTO DUMPFILE returned no plan: $plan"
  fi

  # --- Lock clauses under EXPLAIN: must return a plan (EXPLAIN takes no locks).
  local clause
  for clause in "FOR UPDATE" "FOR SHARE" "LOCK IN SHARE MODE"; do
    plan="$(q "EXPLAIN FORMAT=JSON SELECT id FROM t WHERE id=1 $clause;")"
    if grep -q '"query_block"' <<<"$plan"; then
      ok "EXPLAIN ... $clause returned a JSON plan"
    else
      bad "EXPLAIN ... $clause returned no plan: $plan"
    fi
  done

  echo "  final contents of mounted secure_file_priv dir (only control_real.txt expected):"
  ls -la "$host_dir" | sed 's/^/    /'

  # No end-of-run cleanup: the container is stopped by the RETURN trap, but the
  # written files are left in "$host_dir" for inspection and wiped at the start
  # of the next run.
}

for c in "${CASES[@]}"; do
  run_case "${c%%:*}" "${c##*:}"
  echo
done

echo "############################################################"
if [[ $FAILURES -eq 0 ]]; then
  echo "# ALL CHECKS PASSED"
  echo "############################################################"
  exit 0
fi
echo "# $FAILURES CHECK(S) FAILED"
echo "############################################################"
exit 1

Output:

############################################################
# MySQL 8.0  (port 13307)
############################################################
  server up: 8.0.46
  PASS: control: real SELECT ... INTO OUTFILE wrote a file
  PASS: EXPLAIN ... INTO OUTFILE wrote no file
  PASS: EXPLAIN ... INTO OUTFILE returned a JSON plan
  PASS: EXPLAIN ... INTO DUMPFILE wrote no file
  PASS: EXPLAIN ... INTO DUMPFILE returned a JSON plan
  PASS: EXPLAIN ... FOR UPDATE returned a JSON plan
  PASS: EXPLAIN ... FOR SHARE returned a JSON plan
  PASS: EXPLAIN ... LOCK IN SHARE MODE returned a JSON plan
  final contents of mounted secure_file_priv dir (only control_real.txt expected):
    total 128
    drwxrwxrwx  2 lxd  eduardo.ortega   4096 Aug 20 11:33 .
    drwxrwxrwt 54 root root           118784 Aug 20 11:33 ..
    -rw-r-----  1 lxd  sambashare          6 Aug 20 11:33 control_real.txt

############################################################
# MySQL 8.4  (port 13308)
############################################################
  server up: 8.4.10
  PASS: control: real SELECT ... INTO OUTFILE wrote a file
  PASS: EXPLAIN ... INTO OUTFILE wrote no file
  PASS: EXPLAIN ... INTO OUTFILE returned a JSON plan
  PASS: EXPLAIN ... INTO DUMPFILE wrote no file
  PASS: EXPLAIN ... INTO DUMPFILE returned a JSON plan
  PASS: EXPLAIN ... FOR UPDATE returned a JSON plan
  PASS: EXPLAIN ... FOR SHARE returned a JSON plan
  PASS: EXPLAIN ... LOCK IN SHARE MODE returned a JSON plan
  final contents of mounted secure_file_priv dir (only control_real.txt expected):
    total 128
    drwxrwxrwx  2 lxd  eduardo.ortega   4096 Aug 20 11:34 .
    drwxrwxrwt 55 root root           118784 Aug 20 11:33 ..
    -rw-r-----  1 lxd  sambashare          6 Aug 20 11:34 control_real.txt

############################################################
# ALL CHECKS PASSED
############################################################

The only file present in the mounted dir is control_real.txt from the real SELECT; every EXPLAIN variant wrote nothing and returned a plan (query_block), on both 8.0.46 and 8.4.10.

8. The MergeSort allowlist entry can't do its jobplanbuilder/vexplain.go:198 MergeSort embeds noInputs and keeps its children in Primitives, invisible to Inputs() — so if it ever appeared in a plan, its whole subtree would skip validation and get no EXPLAINs, silently. Today planbuilder never emits it, so the entry is dead code; suggest removing it.

Fixed in 6cdbdda7ce. Removed the dead *engine.MergeSort allowlist entry; the fail-closed default will catch it if planbuilder ever starts emitting one.

9. The e2e suite never fans out to more than one shardqueries/vexplain/vexplain_test.go:167 Both positive e2e cases are EqualUnique routes hitting one of the four shards, so the core feature — the concurrent multi-shard fan-out and shard-keyed map — is only ever tested against sandboxconn. One scatter query asserting all four shard names appear as keys would close it. (Tiny nit: the MySQL-version probe reads Keyspaces[0], which the new setup order makes the unsharded keyspace.)

Fixed in 421d6f38c9 (scatter case) and ce2eef5933 (version-probe nit). Added a scatter SELECT e2e case asserting per-shard EXPLAIN output keyed by at least two distinct shard names (plus a query_block check on MySQL 8.0+), and pinned the version probe to the sharded keyspace by name via a keyspaceByName helper instead of Keyspaces[0].

10. One JSON key, two shapesengine/vexplain.go:416 mysql_explain_json holds a single object under ALL and a shard→object map under MYSQLPLAN. Nothing decodes it generically today, so this is cheap to fix now (e.g. mysql_explain_json_by_shard) and breaking to fix later.

Fixed in afb986f149. The MYSQLPLAN shard→object map is now keyed mysql_explain_json_by_shard; VEXPLAIN ALL keeps mysql_explain_json for its single object.

Smaller things:

The three separate AST rejection walks could be one Walk with a type switch (and early abort).

Fixed in 5880984688. Folded into a single checkVExplainMySQLAST walk with one type switch that aborts on the first offending node.

The eight-type DML primitive case could be a single statement-type check before planning; the hand-maintained list will drift.

Fixed in ac3f349320. DML is now rejected up front with sqlparser.IsDMLStatement, and the eight-type engine-primitive case is gone (the fail-closed default still backstops it).

mysqlExplainTask sits mid-file instead of the top type () block.

Fixed in e5cd381fe9.

An empty EXPLAIN result silently drops that shard from the map — a warning would beat silence.

Fixed in 44077e67b8. That shard is still omitted, but we now log a warning with the keyspace and shard first; added a test capturing log.Warn to prove it fires.

Inside an open transaction the EXPLAIN reflects pre-transaction state (same as ALL) — worth one changelog sentence or a session warning.

Fixed in eaee2ccfcc. Added a changelog sentence noting the in-transaction pre-tx-state behavior (matching VEXPLAIN ALL); went with the changelog note over a session warning to stay consistent with ALL, which shares the behavior and emits none.

@ejortegau
ejortegau requested a review from mattlord August 20, 2026 13:28
Copilot AI review requested due to automatic review settings August 20, 2026 13:28

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa5d20de2b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +387 to +388
for _, task := range tasks {
results, errs := executor.ExecuteMultiShardPerShard(ctx, task.primitive, task.rss, task.queries)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Run EXPLAIN tasks for separate routes concurrently

For plans with multiple Route or Send nodes, such as a UNION, this loop waits for each ExecuteMultiShardPerShard call to finish before starting the next task. Only the shards within one route run concurrently, so total latency grows with the number of route nodes and contradicts the new release-note guarantee that the per-shard EXPLAIN queries run concurrently; execute the collected tasks concurrently as well.

Useful? React with 👍 / 👎.

default:
return true, nil
}
return false, nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Abort the AST walk after detecting an unsafe function

When a lock function is followed by another rejected construct, such as select get_lock('x', 1), (select 1), the later Subquery visit overwrites the lock-specific error with the generic error recommending VEXPLAIN ALL; following that recommendation then executes get_lock. Fresh evidence beyond the earlier lock-function fix is that sqlparser.Walk defines false as pruning only the current node's children, not aborting the walk, so return a sentinel error or otherwise preserve the first rejection.

Useful? React with 👍 / 👎.

explainQueries := make([]*querypb.BoundQuery, len(queries))
for i, q := range queries {
explainQueries[i] = &querypb.BoundQuery{
Sql: "explain format = json " + q.Sql,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve table denylist enforcement for generated EXPLAINs

For a table protected by a tablet query denylist, this generated EXPLAIN FORMAT=JSON bypasses the table-conditioned rule: tablet planning treats sqlparser.Explain as PlanSelect without populating Table or AllTables, so Plan.TableNames() supplies no explained table to FilterByPlan. Consequently MYSQLPLAN can reach every resolved shard for a denied table despite the release note stating that query denylist rules still apply; enforce the original route's table denylist before issuing these EXPLAINs or retain the inner table names in tablet planning.

Useful? React with 👍 / 👎.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: VEXPLAIN MYSQLPLAN to inspect MySQL EXPLAIN per shard without executing the query

3 participants