sql_mode: reject unsupported modes at every layer, neutralize them on every connection - #20883
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: TabletManager
Component: Documentation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20883 +/- ##
===========================================
+ Coverage 69.67% 77.03% +7.36%
===========================================
Files 1614 553 -1061
Lines 216793 84317 -132476
===========================================
- Hits 151044 64952 -86092
+ Misses 65749 19365 -46384
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:
|
… every connection The Vitess parser does not support or honor the sql_modes that change how SQL text is interpreted: ANSI_QUOTES, NO_BACKSLASH_ESCAPES, PIPES_AS_CONCAT, REAL_AS_FLOAT, IGNORE_SPACE, HIGH_NOT_PRECEDENCE, and the ANSI combination. This change makes sure none of them can take effect anywhere in a Vitess cluster. The new go/mysql/sqlmode package implements MySQL 8.x's sql_mode value semantics, verified against a live MySQL 8.0.46: name lists, numeric bitmasks, combination-mode expansion, canonical formatting, and MySQL's own error codes (ER_WRONG_VALUE_FOR_VAR 1231, ER_UNSUPPORTED_SQL_MODE 3899, a new vterrors state). vtgate validates every SET sql_mode assignment with it: constants — including constant expressions like CONCAT over literals — at planning time regardless of --enable-system-settings, and evaluated values at execution time before the change detection, closing the previous bypasses (no-op assignments matching the backend's mode, numeric bitmasks, the ANSI combination). IGNORE_SPACE and HIGH_NOT_PRECEDENCE join the rejected set. vttablet mirrors the validation with identical errors for clients that bypass vtgate — older vtgates in mixed-version clusters and direct query-service clients: connection settings (settings pool and true reservations), SET_VAR hints at plan build, SET statements (constants at plan time; non-constants via verify-after-execute: the applied @@sql_mode is read back, validated, and the previous mode restored on violation), and ApplySchema --session-variable values on both the vtctld and tablet sides. The server's own configuration cannot re-introduce these modes either: MySQL lexes each statement under the pre-statement session sql_mode — a SET_VAR hint cannot influence the parsing of its own statement — so every connection Vitess creates strips the lexer modes from the session sql_mode inherited from the global value, at the dbconfigs connector choke point all components dial through. The statement is a nested-REPLACE chain (MySQL offers no numeric arithmetic on the sql_mode system variable); runtime modes are preserved, the settings-pool reset restores the neutralized value instead of `default`, and backup init SQL runs through a new ExecuteSuperQueryListTainted that discards its connection so operator statements cannot leak session state into pooled connections. The guarantee is scoped to MySQL; MariaDB (a deprecated migration source) is documented and the verify-after-execute read-back only judges values that parse as MySQL modes. Legacy engine tests that characterized the old textual comparison with made-up mode names now use real modes; fakesqldb answers the connection-setup statement automatically, counting it for GetQueryCalledNum while keeping it out of QueryLog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…ording it The neutralization statement every Vitess-created connection runs is connection state, not query execution; recording it made explain output depend on connection-creation timing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
9159cff to
059abbf
Compare
maxenglander
left a comment
maxenglander
left a comment
There was a problem hiding this comment.
Disclaimer: findings and writing generated by AI.
Residual risk: Reserve preQueries still ignore non-constant sql_mode expressions (verify flag discarded in ValidateSettingsSQLMode), which matches the “settings are constants from VTGate” assumption but leaves a small hole for direct queryservice clients.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| applied, err := qre.readSQLMode(conn) |
There was a problem hiding this comment.
In execSet, when VerifySQLMode is set, the SET is applied, then readSQLMode runs. If that readback fails, the function returns the error immediately and never restores prev or closes the connection. Session sql_mode is not undone by transaction rollback, so a reserved/transactional connection can keep an unsupported lexer mode after a failed verify. That breaks the invariant this PR adds (and that the restore-failure path already enforces by closing the connection). After a successful SET under VerifySQLMode, any failure before a successful validation verdict should restore prev or close the connection.
There was a problem hiding this comment.
That's a good catch! I'll take a look.
Review feedback on the sql_mode verify-after-execute path, two gaps in its failure handling: A multi-assignment SET whose non-constant sql_mode value failed the post-execution verification had only its sql_mode assignment restored — the statement had already executed on MySQL, so its other assignments stayed applied on the connection while the client saw an error. MySQL applies none of a SET's assignments when the statement fails, and the executor has no snapshot of the other variables to restore, so the connection is now closed instead of being returned with some assignments applied. Single-assignment SETs keep the exact restore. A failed read-back of the applied value returned the error without any restore, leaving the connection in whatever mode the statement set. The read-back failure now takes the same undo path: restore for a single-assignment SET, close for a multi-assignment one, close if the restore fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Review feedback, two classes of gaps. The settings paths accepted values they could not judge. Connection settings — the settings pool and reservation pre-queries — are applied with no verification afterwards, yet a non-constant sql_mode value (e.g. CONCAT over literals) passed through both unjudged, and the reservation path additionally skipped settings that did not parse or were not SET statements, which the settings pool already rejects. All of it is now rejected upfront: settings must parse as SET statements carrying constant sql_mode values. vtgates only ever render constants into settings, so this concerns direct query-service clients. Several verification states failed open and now fail closed: - execSet accepted an applied sql_mode that did not decode as MySQL 8.x modes. A constant with an unknown mode name is rejected at plan time, and an expression producing one must not fare better just because it cannot be judged — the statement is now undone and the error returned. This drops the tolerance for foreign mode vocabularies: the tablet's own database is MySQL. - sqlModeChangedValue treated an unexpected verification result shape as "no change", reporting success for an assignment that was never validated; it now returns an error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The targeted SET path is validated by the base PR (#20883) already, so the clause read as if this PR were what fixed it. 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>
Promptless documentation updates
|
On the reserved-connection path the SET sent to the shards carried the original expression, so a non-constant sql_mode was evaluated twice: once by the judgment query, whose result the session stores, and again by the SET itself. A nondeterministic expression could therefore apply a value the session never judged. The SET now carries the judged literal, on both the targeted and the untargeted path. 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>
MySQL applies none of a SET's assignments when one of them fails. A multi-assignment SET whose sql_mode can only be judged after execution had already applied its other assignments by the time the read-back rejected the mode; with autocommit among them, a transaction could be committed by a statement that then fails. Closing the connection afterwards did not undo that. Such statements are now rejected at plan time, which leaves the verify-after-execute path to single-assignment SETs, where restoring the previous mode undoes everything the statement applied, as MySQL leaves it. 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>
With --enable-system-settings=false a SET is checked and ignored rather than applied. Constant sql_mode values were already rejected at plan time on that path, but a non-constant one was never judged, so an unsupported mode passed silently. The check query now evaluates the value the same way the reserved-connection path does, and an unsupported mode is an error there too. 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>
The dba-grants wait, GetMysqlPort and the clone donor check dialed MySQL directly, bypassing the connector that neutralizes the session's sql_mode on every Vitess-created connection. They now connect through it. The socket-wait probe and the external-MySQL ping are left as they are: neither sends a statement, and the socket wait runs with root credentials during initialization. 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>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| s := buf.String() | ||
| vcursor.Session().SetSysVar(svs.Name, s) | ||
| storedValue = buf.String() | ||
| vcursor.Session().SetSysVar(svs.Name, storedValue) |
There was a problem hiding this comment.
🟡 Failed settings persist in sessions
When execSetStatement fails on existing shard sessions, the value was already stored. Later queries replay the rejected setting despite the SET error.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not changing this here. The ordering is the same as on main for every reserved-connection variable; this PR changed it only on the targeted path, where a single shard makes store-after-success unambiguous. On the untargeted path the SET is sent to every existing shard session, and after a partial failure the stored value is what makes the next query replay the setting on the shard that failed; storing only after success would leave a shard that accepted the SET carrying the mode with the session unaware, and nothing to reconcile them. For sql_mode the value is judged before it is stored, so a permanently rejected value cannot get in; what remains are transient failures, which the replay converges. The general problem — a failed SET poisoning the session rather than failing the statement — is tracked in #20893, whose proposed eager verification stores values only once a shard has accepted the whole settings bundle. The reservation flag is set before the SET because the scatter layer decides whether to reserve from it at execution time, so it staying set after a failure is the same as on main and on the targeted path.
| func ValidateSettingsSQLMode(settings []string, parser *sqlparser.Parser) error { | ||
| for _, setting := range settings { | ||
| stmt, err := parser.Parse(setting) | ||
| if err != nil { | ||
| return vterrors.Wrapf(err, "failed to parse connection setting: %s", setting) | ||
| } | ||
| set, ok := stmt.(*sqlparser.Set) | ||
| if !ok { | ||
| return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "connection setting is not a SET statement: %s", setting) | ||
| } | ||
| if err := validateConstantSetExprsSQLMode(set.Exprs); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🔍 Reservation pre-query contract narrows
ValidateSettingsSQLMode now rejects every non-SET pre-query, including SQL unrelated to sql_mode. Review compatibility for direct query-service clients.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Keeping the requirement; the reasoning is on the Codex thread at sql_mode.go:57. pre_queries carries vtgate's session settings, which are always SET statements, and the settings-pool path has required that since it was written. Arbitrary statements from a direct query-service client were never a supported interface.
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-layer connection and session-state changes require final human validation of compatibility and operational impact.
Review details
- Files reviewed: 42/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
The Codex P1 on |
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>
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
-
Non-blocking: I think we should validate the result row count before indexing it. In
go/vt/vtgate/engine/set.go:397,sqlModeChangedValuereadsqr.Rows[0]while checking the result shape. An unexpected successful response with zero rows would panic VTGate rather than fail closed with anINTERNALerror. The scalar query should always return one row from MySQL, but since this helper already defends against malformed results, I think we should require exactly one row and add zero/multiple-row test cases. -
Non-blocking: I think we should update the introductory comment in
go/vt/vttablet/tabletserver/planbuilder/sql_mode.go:28. It says non-constant expressions pass through and are left to MySQL, but the current implementation rejects them for connection settings and multi-assignmentSETs, while a sole dynamic assignment is post-verified and restored if necessary. The stale description could mislead future changes to this safety boundary.
sqlModeChangedValue indexed the first row before checking that there was one, so a successful result with no rows would panic instead of failing closed. The reserved connection path also short-circuited on an empty result as "no change" before the judgment ran, which would let an unjudged value through. The judgment now runs first for sql_mode and requires one row of two columns, and the empty and multi-row shapes are covered by tests. 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>
The package comment still said non-constant values pass through to MySQL. Connection settings reject them, multi-assignment SETs reject them upfront, and a sole assignment is verified after it runs and restored on failure. 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>
|
Thanks for the review, Matt! Both follow-ups are in:
|
![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: 0a1bac9cce
ℹ️ 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".
mysql.Connect abandons the dial and the handshake when the context ends, but the sql_mode neutralization that follows ran with no bound at all, so a backend that stalls after the handshake would hang every Vitess connection attempt. Closing the connection when the context ends now fails the pending exchange, and ConnectTimeoutMs covers the setup as well as the dial. 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>
| func TestConnectorConnectSetupBoundedByContext(t *testing.T) { | ||
| t.Run("context deadline", func(t *testing.T) { | ||
| _, params := newStallingServer(t) | ||
| ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) |
There was a problem hiding this comment.
🔍 Sub-second deadlines risk flaky tests
These 200 ms deadlines include TCP setup and a MySQL handshake. Resource-starved runners can time out before reaching the stalled query.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70770ab80e
ℹ️ 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 migration statements executed under this variable are Vitess-formatted | ||
| // SQL, so modes that change how SQL text is interpreted are rejected the same | ||
| // way a vtgate session rejects them (see sqlmode.Validate). | ||
| if _, err := sqlmode.Validate(sqltypes.NewVarChar(variable.Value)); err != nil { |
There was a problem hiding this comment.
When users copy the vtctldclient ApplySchema --ddl-strategy help example in go/cmd/vtctldclient/command/schema.go:442, they are told to use vitess --session-variable sql_mode=ANSI, but this new validation rejects that exact value with setting the ANSI sql_mode is unsupported. Update the advertised example to a supported mode so the documented command remains usable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No. This will be fixed anyway once #20884 lands.
Description
The Vitess parser doesn't support (or doesn't honor) the sql_modes that change how SQL text is interpreted:
ANSI_QUOTES,NO_BACKSLASH_ESCAPES,PIPES_AS_CONCAT,REAL_AS_FLOAT,IGNORE_SPACE,HIGH_NOT_PRECEDENCE, and theANSIcombination. Today they can still sneak in at several layers. This PR makes sure none of them can take effect anywhere in a cluster:go/mysql/sqlmodepackage implements MySQL 8.x's sql_mode value semantics, verified against a live MySQL 8.0.46 — name lists, numeric bitmasks, combination-mode expansion, and MySQL's own error codes (1231 / 3899).SET sql_modewith it, closing the existing bypasses: no-op assignments matching the backend's mode, numeric bitmasks, and theANSIcombination all now error. Constants (includingCONCATover literals) fail at plan time, evaluated expressions at execution time.IGNORE_SPACEandHIGH_NOT_PRECEDENCEjoin the rejected set. The shard-targetedSETpath (USE ks:-80, thenSET), which previously skipped validation entirely and stored the raw expression before executing, is judged the same way and stores only values the shard accepted.SETstatements (non-constants via verify-after-execute with restore-on-violation), and ApplySchema--session-variablevalues.SET_VARoptimizer hints are deliberately not judged: a hint applies to its statement's execution only and can't change how that statement's own text is lexed, which is vttablet's only stake in sql_mode, so the hint is forwarded verbatim and MySQL warns about and ignores invalid values exactly as it does for direct clients.init_connectapplied survives. MySQL lexes each statement under the pre-statement session mode — aSET_VARhint can't influence the parsing of its own statement (verified live) — so Vitess-generated SQL should always be parsed under the same lexer rules it was serialized with, regardless of how the backend happens to be configured. Runtime modes (strict, zero-date, ...) are preserved. The settings-pool reset still sources from the global, matching whatSET sql_mode = DEFAULTdoes onmain. The choke-point placement follows a full audit of every connection-creation path — per-site placement kept leaving gaps (vreplication/vdiff clients, schema preflight/apply, vstreamer helpers).Rejecting these modes is not the end state: with this in place, supporting a lexer mode for incoming queries later becomes a pure parser change plus removing it from the rejected set — outbound query serialization never has to account for them.
Related Issue(s)
Part of #20984.
Follow-ups raised in review, tracked separately:
SETshould apply none of its assignments (pre-existing onmain; this PR narrows it to non-constant values)SET_VARhint (pre-existing; needs a parser for MySQL's hint value grammar)A follow-up PR builds on this to make sql_mode a vtgate-session-owned setting (default via a
--sql-modeflag, sent with every query).Checklist
Deployment Notes
SET sql_modewith an unsupported mode now get an error — even as a no-op matching the backend's configured mode. Such sessions were already unreliable, since vtgate parses queries without honoring these modes.ExecuteFetchAsDba-style admin RPC connections (an explicit in-batchSETstill wins) and to external MySQL servers used as vreplication/PITR sources. The guarantee is scoped to MySQL; MariaDB (deprecated, migration-source only) is documented.AI Disclosure
This PR was written primarily by Claude Code — I provided direction and review.
🤖 Generated with Claude Code