vtgate: make sql_mode a session-owned setting - #20880
Conversation
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
github-actions
Bot
added
Component: Documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## arthur/sql-mode-lexer-parser-support #20880 +/- ##
=======================================================================
Coverage ? 81.71%
=======================================================================
Files ? 347
Lines ? 57557
Branches ? 0
=======================================================================
Hits ? 47030
Misses ? 10527
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
arthurschreiber
added
NeedsDescriptionUpdate
arthurschreiber
added
the
Type: Enhancement
5a1a157 to
a627f18
Compare
arthurschreiber
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8668d9459b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _, ok := session.SystemVariables[sysvars.SQLMode.Name]; !ok { | ||
| session.SystemVariables[sysvars.SQLMode.Name] = sqltypes.EncodeStringSQL(defaultSQLMode) | ||
| session.setVarCommentBuilt = false |
There was a problem hiding this comment.
During a rolling upgrade, a session from the previous VTGate can have InReservedConn and nonzero shard ReservedIds but no sql_mode; this branch adds the default only to the session map. actionInfo then sees the existing reserved ID and chooses the normal execute path without SetPreQueries, so a first DDL, lock, or other non-hintable statement reuses a connection still running under its old backend mode. Fresh evidence after the explicit-SET convergence fix is that this seeding path still never updates or recreates carried reserved connections.
AGENTS.md reference: AGENTS.md:L240-L240
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. A session carried from an older vtgate with reserved connections and no sql_mode has those connections in the mode that vtgate left them in, the backend's configured one, which is what the deployment note asks --sql-mode to describe; the seed adds that same value to the session. The next SET sql_mode on the session converges them (74dbdeb). Re-creating reserved connections at seed time would drop the locks and temporary tables they hold, which is worse than the window.
| // backend, so it does not count. The execution-time modes leave SHOW output alone. | ||
| func (vc *VCursorImpl) showOutputFollowsSessionSQLMode() bool { | ||
| const showModes = ^sqlparser.SQLModeNoBackslashEscapes | ||
| return vc.ParseSQLMode()&showModes != sqlparser.ParseSQLMode(vc.DefaultSQLMode())&showModes |
There was a problem hiding this comment.
When the configured/default mode differs from a backend's global mode, this comparison returns false for every fresh session because its mode equals DefaultSQLMode(). A backend-routed SHOW CREATE TABLE therefore bypasses connection settings and formats its result under the backend mode—for example, a backend with ANSI_QUOTES still emits double-quoted identifiers even when VTGate's configured default omits it. Fresh evidence in the final SHOW fix is this default-relative predicate; it does not establish that the backend already has that default, so backend SHOWs requiring session-owned semantics still need the settings transport.
AGENTS.md reference: AGENTS.md:L159-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly addressed in 99bb8d5: the predicate is no longer relative to the configured default, so a session in any SHOW-shaping mode reserves, whatever the backends are configured with. The remaining direction (a backend whose global mode carries such a mode while the session does not) keeps the behaviour of every release so far: the SHOW runs on a plain connection and prints the backend's own formatting. Reserving every backend SHOW would pin a connection for each session that runs SHOW CREATE TABLE, and the deployment note already asks operators to describe the backends' mode with --sql-mode; the description now says so under the deployment notes.
There was a problem hiding this comment.
Correction to the note above: a SHOW that reserves does not pin a MySQL connection. vtgate flips the session into the settings transport, and vttablet serves the statement from its settings pool (ReserveExecute tries executeWithSettings first and only truly reserves when the statement demands it, which a SHOW never does). The cost is that the session flag is sticky: every later query of that session goes through the reserve RPC with pre-queries and the settings pool instead of the plain pool with a SET_VAR hint. Making the settings transport per statement for hint-incapable statements would make it cheap to send every backend SHOW with the session's settings; that is the direction being considered.
There was a problem hiding this comment.
The per-statement settings transport is now #21016, against main. Once it lands, the rule here can become "every backend SHOW carries the session's settings", which closes the remaining direction of this thread.
…antics Builds on the sql_mode rejection layer: vtgate now owns the session's sql_mode instead of asking an arbitrary shard and hoping all backends agree. - A new --sql-mode flag (named after MySQL's, validated like a SET statement) defines the mode every session starts with, defaulting to MySQL's factory default. Deployments with --enable-system-settings disabled are exempt and keep their backends' modes. - @@sql_mode and @@global.sql_mode reads and all SET sql_mode handling happen entirely at the vtgate with no shard round trips: expressions evaluate through the evalengine, values are stored in MySQL's canonical form, and a dedicated SysVarSQLMode primitive replaces the interim validation shim on the reserved-connection machinery. - The session's mode is applied to every query: a SET_VAR hint on statements that can carry one, connection settings for statements that cannot (DDL, locks) and setups without SET_VAR support. The SET_VAR comment is rendered once and cached on the session, placed only in the top-level statement where MySQL honors it, and never carries lexer modes; plan cache keys keep discriminating by session variables even when no hint is injected. - Session state carried from older vtgates is grandfathered: validation applies to new SET statements, undecodable stored values are repaired on the next SET, and a retried SET converges open shard sessions on the settings transport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
CI fixes for the always-sent sql_mode: expectations that assert exact query text now account for the SET_VAR(sql_mode = ...) hint or strip optimizer hints before comparing, the twopc redo-statement goldens carry the combined settings statement, and the schemadiff vrepl suite sets --sql-mode so its zero-date table definitions remain valid under the vtgate-enforced session mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
MySQL's DEFAULT restores the backend's global sql_mode, which is per-keyspace and incoherent under scatter. The session-owned sql_mode defines DEFAULT as the configured default the session started with: the value of the --sql-mode flag, or the compiled-in default when the flag is unset. The planner substitutes the DEFAULT keyword with a string literal of that value, so it flows through the same evaluation, validation, and canonical storage as any other assignment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Editorial only, no facts added or removed: the session-default and always-send sections carry one idea per sentence now, with the long clause chains split up — easier reading for operators skimming release notes, non-native English readers included. The sections inherited from the base branch are left as they are: the base branch carries their rewritten versions, and this branch picks those up on its next rebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…g help The session-owned sql_mode forwards every mode but NO_BACKSLASH_ESCAPES to the backends, the rule the parse-relevant mode support established; the always-sent section and the --sql-mode help still described the earlier strip of every parse-relevant mode. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hkhyn4HQQfBbJkmyPe2Gtm Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…e error value A table test verified against MySQL 8.0.46 covers the odder expressions a SET sql_mode can carry: other session variables, user variables, subqueries, RAND(), literals of every kind, and the error cases. Expressions VTGate can compute are evaluated locally; a sub-expression it cannot, such as RAND() or a system variable the session never set, is fetched from a shard with the session's @@sql_mode passed along and the result validated like any other value. The changelog said no shard round trip at all. ER_WRONG_VALUE_FOR_VAR now carries the offending value truncated to 200 characters, as MySQL's does, instead of embedding an arbitrarily long expression result. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hkhyn4HQQfBbJkmyPe2Gtm Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…comment only Review feedback: with every query carrying the session's sql_mode, a statement with two optimizer hint comments failed with "Must have only one query hint" where it used to go through. MySQL honors only the first hint comment of a statement and reads any later one as an ordinary comment, so the session's setting now joins the first and the rest are kept as they are. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
MySQL resolves every expression of a SET before applying any assignment, verified on 8.0.46; the once-per-request @@sql_mode bind variable gives exactly that. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Review feedback: SHOW statements were exempt from reserving a connection, on the grounds that vtgate answers them itself. The ones it sends to a backend — SHOW CREATE TABLE, SHOW TABLES, SHOW COLUMNS, ... — cannot carry a SET_VAR hint, and their output depends on the mode: SHOW CREATE TABLE quotes identifiers by it. Only a SHOW whose plan is local stays exempt; one sent to a backend now runs on a connection with the session's settings applied. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…holds Review feedback: with SET_VAR available the SET stored the value and stopped, since every hint-capable query carries the session's sql_mode itself. A reserved connection the session already holds — after a DDL, a lock, or any other statement that could not carry a hint — kept the mode it was set up with and served the next such statement under it. Those connections are now updated whether or not SET_VAR is in use. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
8668d94 to
74dbdeb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74dbdeb953
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The reserved connections the session already holds — after a DDL, a lock, or | ||
| // any other statement that could not carry a hint — keep the mode they were set | ||
| // up with and serve the next such statement, so they are updated, SET_VAR or not. | ||
| rss := vcursor.Session().ShardSession() |
There was a problem hiding this comment.
When SET_VAR is unavailable and a transaction has created reserved lookup-vindex connections under PreSessions or PostSessions, this call only returns Session.ShardSessions, so those connections retain the previous sql_mode; later PRE/POST vindex operations reuse them without a hint and execute under stale semantics. Fresh evidence after the earlier convergence fix is that VCursorImpl.ShardSession() reads only the normal commit-order list and does not select all shard sessions whose ReservedId is nonzero.
AGENTS.md reference: AGENTS.md:L159-L160
Useful? React with 👍 / 👎.
…able is removed SQLMode looked the session up twice in a row. RemoveSystemVariable now invalidates the cached SET_VAR comment the way SetSystemVariable does, so a later render cannot hand out a hint that still names the removed variable. The vexplain end-to-end check reports through testify like the rest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
… SHOW never does The rule compared the session's parse-relevant modes with the configured default's, so a default that carries ANSI_QUOTES sent SHOW CREATE TABLE on a plain connection and got backticks back. The session's own value decides now: any parse-relevant mode other than NO_BACKSLASH_ESCAPES reserves. A SHOW answered from the topology (SHOW VITESS_METADATA VARIABLES) never reaches a tablet and is treated like one vtgate answers itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…SLASH_ESCAPES The SET that updates the reserved shard sessions a session holds sent the full value; the hint and the settings transports already leave the one mode out that a backend must not lex under. It is stripped here too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
… sql_mode Both happen before the first statement reaches the executor and seeds the session: a multi-statement request is split by the session's parser, which fell back to the default-mode parser when the session carried no sql_mode yet, and the handshake advertised backslash escaping whatever --sql-mode said. The splitter now reads the configured default, and NewConnection sets the NO_BACKSLASH_ESCAPES status flag when the default carries it, unless the deployment leaves the sql_mode to the backends. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
DefaultSQLMode substituted the compiled-in modes when the configured value was empty, so sessions seeded with the empty mode saw SET sql_mode = DEFAULT enable modes they never started with. The executor resolves the flag once; the vcursor now returns that value as it is. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…he sql_mode to them With --enable-system-settings=false the session is never seeded, but the normalizer still answered @@sql_mode, @@global.sql_mode and the sql_mode row of SHOW VARIABLES with vtgate's configured default, which says nothing about the mode such a deployment's queries run under. The rewrite now happens only when the session carries a sql_mode, which every session vtgate manages the sql_mode of does from its first request on; otherwise the read goes to the backend as written. Materialization of a carried expression puts the session in the configured default before evaluating, rather than removing the variable, so @@sql_mode inside the expression keeps resolving to the default the session started with. The plan tests normalize with a seeded session, like a real one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Both scopes of SHOW VARIABLES filled the sql_mode row from the session, while @@global.sql_mode already read the configured default. The global form now carries its own bind variable for the row. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…nner The evaluation translated the expression directly, so a sub-expression vtgate cannot compute (a backend function, an unset system variable) failed the request and dropped the carried value. The expression now runs as the SET sql_mode statement that stored it, through the planner, which fetches such a sub-expression from a shard the way it does for any SET. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a89c4491dd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // a default that carries such a mode is not known to be what the backends run under. | ||
| func (vc *VCursorImpl) showOutputFollowsSessionSQLMode() bool { | ||
| const showModes = ^sqlparser.SQLModeNoBackslashEscapes | ||
| return vc.ParseSQLMode()&showModes != 0 |
There was a problem hiding this comment.
When the session's parse mode is empty but a backend's global mode contains ANSI_QUOTES, this predicate is false, so a backend-routed SHOW CREATE TABLE runs without connection settings and emits double-quoted identifiers despite VTGate owning and reporting the empty mode. Fresh evidence after the claimed SHOW fix is that the final predicate still tests only whether the session has a nonzero parse bit, not whether the backend may have a bit that the session needs cleared; backend SHOWs must therefore apply the transported mode even in this direction.
AGENTS.md reference: AGENTS.md:L159-L160
Useful? React with 👍 / 👎.
| safeSession.SetSystemVariable(sysvars.SQLMode.Name, sqltypes.EncodeStringSQL(e.config.SQLMode)) | ||
| err := e.evaluateSessionSQLMode(ctx, safeSession, value) |
There was a problem hiding this comment.
When a mixed-version session carries an expression-form sql_mode and has EnableSystemSettings=false, this installs the configured default before replaying the expression through the SET planner; buildSetOpSQLMode then takes its disabled-settings check-and-ignore path, returns success without storing the evaluated value, and this function leaves the injected default in the session. Fresh evidence after the opt-out rewrite is this successful ignored-SET path, which makes subsequent parsing, reads, and hints use VTGate's default instead of retaining backend ownership; skip materialization for such sessions or restore/drop the carried value explicitly when the planner ignores it.
AGENTS.md reference: AGENTS.md:L240-L240
Useful? React with 👍 / 👎.
Description
Builds on #20883 (merged) and #20884 (the base branch, in stack #21006), which added MySQL-faithful
sql_modevalidation and support for the parse-relevant modes. This PR completes the picture: vtgate currently asks an arbitrary shard for@@sql_mode(assuming all backends agree, which nothing guarantees) and runs sessions that never setsql_modeunder whatever mode each backend happens to be configured with.This makes
sql_modea setting the vtgate session owns:--sql-modeflag (named after MySQL's, validated the same way as aSETstatement) defines the mode every session starts with, defaulting to MySQL's factory default — the mode Vitess-managedmysqldruns with.@@sql_mode/@@global.sql_modereads and allSET sql_modehandling happen entirely at the vtgate: expressions evaluate through the evalengine with no shard round trips, and the session stores MySQL's canonical form. A dedicatedSysVarSQLModeprimitive replaces the interim validation shim sql_mode: reject unsupported modes at every layer, neutralize them on every connection #20883 put on the reserved-connection machinery.SET_VAR(sql_mode = ...)hint on statements that can carry one, and connection settings (the tablet settings pool) for statements that can't (DDL, locks) or setups withoutSET_VAR. Deployments running--enable-system-settings=falseare exempt and keep their backends' modes.NO_BACKSLASH_ESCAPES(see sql_mode: support the parse-relevant modes at vtgate and vttablet #20884). The session'ssql_modegoverns how the client's SQL is interpreted, queries sent to vttablet are always vtgate-canonical text, and that text is inert under every mode but the backslash-escaping one. Worth noting that aSET_VAR(sql_mode = ...)hint can never change how its own statement is lexed anyway — MySQL lexes a statement under the session mode in effect before it — so the hint only carries the mode's execution-time semantics, which is exactly what it is for.Sessions are seeded with the configured default on request entry; the normalizer resolves
@@sql_mode(and@@global.sql_mode) to bind variables filled from the session, soSET sql_mode = CONCAT(@@sql_mode, ...)and friends evaluate locally with no shard round trips. A dedicatedSysVarSQLModeengine primitive validates, normalizes and stores the canonical value.Along the way: the
SET_VARcomment is now only placed in the top-level statement comment where MySQL honors it (it used to be duplicated into subqueries, which MySQL warns about),explain/vexplainhint the statement they wrap,USE/(V)EXPLAINno longer force reserved connections, hint ordering is deterministic, and the rendered hint is cached on the session. Review follow-up commits address findings from automated review: plan keys keep discriminating by session sysvars when no hint is injected, undecodable carried session values are repaired on the nextSET, and a retriedSET sql_modere-converges open shard sessions on the settings transport.Measured overhead of always sending the mode: ~5-7µs per query end-to-end (~3µs of that is mysqld parsing/applying the hint), which was ~3% on a loopback-only local cluster and should be well under 1% on real networks.
Related Issue(s)
Part of #20984. Stacked on #20988 and #20884 (stack #21006). Follow-ups: #20892 (gate-side evaluation under the session's mode), #20893 (multi-assignment
SETatomicity), #21003 (function-name keyword lexing), #21010 (EXECUTEmargin comments), #21014 (theSET_VARhint on queries the planner derives from the statement), #21015 (aSETcommits to the session before the open shard sessions converge).Checklist
reservedconnend-to-end suites — including mysqldown/servingchange/tabletchange/vttabletdown — pass locally against real clusters; CI pending on this draft)Decision: no opt-in flag for the session-owned default
The repository's staging rule asks for behavior changes to ship opt-in first and flip a release later. This PR flips the default in one release, deliberately, with
--enable-system-settings=falseas the opt-out. The reasoning:SELECT @@sql_modereturned whatever the shard that happened to answer was configured with, and a session that never setsql_moderan each query under the mode of whichever backend served it. Across keyspaces, or across shards with differing globals, one session got different answers. A flag would keep that as the default for another release without protecting anyone from anything.mysqldruns with. Deployments on that default see no change in what their queries run under; what changes is that the vtgate now guarantees it.--sql-modereproduces their global as the session default;--enable-system-settings=falsekeepssql_modebackend-defined wholesale, as before.sql_modein the session proto as a plain system variable, which a v24 vtgate already honors as a session setting, so any session that has touched a v25 vtgate behaves identically on a v24 one. A session that has only seen v24 vtgates behaves as v24 always did. The only divergence window is therefore the pre-existing one.The behavior change is called out in the deployment notes and the changelog; this section records that shipping it without an opt-in period was a decision, not an oversight.
Deployment Notes
sql_modenow read the configured default fromSELECT @@sql_modeinstead of the value of whichever shard happened to answer, and their queries actually run under that mode viaSET_VARhints. Deployments intentionally running a non-default global mode should set--sql-modeaccordingly, or set--enable-system-settings=falseto keepsql_modebackend-defined.SET sql_modewith a value the backend would reject (or vtgate can't support) now errors — even as a no-op matching the backend's current mode.SHOWsent to a backend (SHOW CREATE TABLEand friends) runs on a connection carrying the session's settings whenever the session is in a mode that changes its output (ANSI_QUOTESand the other parse-relevant modes, from aSETor from--sql-mode). A session without such a mode sends it on a plain connection, whose output reflects the backend's own configured mode, as in every release so far.sql_modevalues set on an older vtgate are carried across (sql_mode: support the parse-relevant modes at vtgate and vttablet #20884 canonicalizes them on entry and evaluates a carried expression at the vtgate), so in-flight gRPC sessions don't fail mid-upgrade. OnlyNO_BACKSLASH_ESCAPESis left out of what's sent to the backends.Details in the changelog summary (
changelog/25.0/25.0.0/summary.md).AI Disclosure
This PR was written primarily by Claude Code — I provided direction and review.
🤖 Generated with Claude Code