Repository navigation
Fix SET LOCAL session-variable tracking and hostgroup routing (#6397) - #6403
Closed
renecannao wants to merge 6 commits into
Closed
renecannao wants to merge 6 commits into
renecannao wants to merge 6 commits into
Conversation
MySQL treats LOCAL as a synonym for SESSION when assigning system variables. ProxySQL's tracked-SET dispatch regex did not accept the LOCAL keyword, so SET LOCAL innodb_lock_wait_timeout=5 bypassed normal session tracking and entered the conservative hostgroup-lock fallback. Subsequent queries routed to another hostgroup could fail with error 9006 even though the equivalent SET SESSION statement worked. Accept LOCAL at the tracked-variable dispatch gate, and add it to the shared scope-prefix regex used by legacy SET parser algorithms 1 and 2. ParserSQL and its algorithm-3 adapter already parse these assignments; fixing the earlier gate lets that existing support reach session tracking. Keep the tracked-variable allowlist, value validation, and fallback logic unchanged. PostgreSQL SET LOCAL has different semantics and is unaffected. Validation: full PROXYSQL31=1 debug build; all three SET parser suites (236, 234, and 504 assertions); MySQL 8.4 routing regression covering all three algorithms (124 assertions). The same routing test reproduced the LOCAL failures on the baseline while all SESSION controls passed. Refs: #6397
) Extend the shared SET parser fixtures with LOCAL assignments, lowercase syntax, a quoted variable name, repeated LOCAL prefixes, and mixed SESSION/LOCAL assignments. These fixtures exercise both legacy parsers and the ParserSQL adapter using the same expected variable/value maps. Add a live regression that exercises algorithms 1, 2, and 3 with SESSION controls and four LOCAL variants. Check that assignments leave the frontend unlocked, that writer-to-reader-to-writer queries return the assigned value, and that backend query counters prove each intended route was actually used. The multiple-assignment case also checks sql_safe_updates=1 on the reader so an ignored second assignment cannot pass on its default. Use dedicated temporary hostgroups and restore the isolated runner's configuration through a fixture. Explicitly remove admin snapshot tables, which survive client disconnects, and avoid retrying configuration restore after snapshot cleanup has started. Register the test in legacy-g1 and mysql84-g1. Validation: the baseline failed the first LOCAL case in both legacy parser suites and failed 63 LOCAL-related live assertions while all 24 SESSION control assertions passed. With the fix, parser suites pass 236/234/504 assertions and the MySQL 8.4 regression passes all 124 assertions. The live test also passes on a second run against the same instance, checking fixture cleanup. No benchmark code or results are included. Refs: #6397
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change makes SET LOCAL follow SET SESSION handling and adds a digest fallback, with tests covering both. No concrete merge-blocking risk was found in the supplied review. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The PR also changes ParserSQL digest fallback behavior and digest-constrained query-rule matching. It adds production code and tests for unsupported DDL, incomplete input, AST limits, protocol framing… | Split the digest fallback and digest-rule changes into a separate issue or pull request, or remove them from this pull request. Keep the SET LOCAL tracking, scope-validation, and related regression-test changes for issue #6397. |
| Docstring Coverage | |
Docstring coverage is 4.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 11 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Linked Issues check Issue #6397requires MySQLSET LOCALassignments to supported tracked variables to behave likeSET SESSIONassignments and avoid hostgroup locking. The production changes addLOCALto tracked-va…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: fixing SET LOCAL session-variable tracking and its hostgroup routing behavior. It matches the main objective of the pull request.
Full details: Out of Scope Changes check
-
Explanation
The PR also changes ParserSQL digest fallback behavior and digest-constrained query-rule matching. It adds production code and tests for unsupported DDL, incomplete input, AST limits, protocol framing, and missing digest metadata. These changes have no demonstrated connection to issue
#6397, which concerns MySQLSET LOCALtracking and hostgroup locking.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 4.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 11 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each SET with care,
LOCAL hops through parsing there.
Digests find a token trail,
Even when the grammar’s frail.
Tests keep watch on routes and rules,
Then the bunny bounds to carrots’ pools.
Comment @coderabbitai help to get the list of available commands.
|
|
![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: 22f06125bb
ℹ️ 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".
|
|
||
| std::stringstream ss; | ||
| ss << "^SET (|SESSION |@@|@@session.|@@local.)`?(" << mysql_variables.variables_regexp << "SESSION_TRACK_GTIDS|TX_ISOLATION|TX_READ_ONLY|TRANSACTION_ISOLATION|TRANSACTION_READ_ONLY)`?( *)(:|)=( *)"; | ||
| ss << "^SET (|SESSION |LOCAL |@@|@@session.|@@local.)`?(" << mysql_variables.variables_regexp << "SESSION_TRACK_GTIDS|TX_ISOLATION|TX_READ_ONLY|TRANSACTION_ISOLATION|TRANSACTION_READ_ONLY)`?( *)(:|)=( *)"; |
There was a problem hiding this comment.
When parser algorithm 3 or mysql-query_processor_parser=1 is enabled, this new dispatch path also intercepts valid mixed-scope statements such as SET LOCAL innodb_lock_wait_timeout=5, GLOBAL time_zone='+00:00'. parsersql_parse_set_mysql() normalizes away GLOBAL/PERSIST prefixes, so the session handler treats every recognized assignment as session-scoped and synthesizes an OK response; the requested global or persisted change is never sent to MySQL and time_zone is incorrectly changed in ProxySQL's session state instead. MySQL permits changing scope between assignments, so this path should reject/fall back when any assignment has a non-session scope rather than consuming the whole statement.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6403 +/- ##
==========================================
+ Coverage 59.12% 64.01% +4.88%
==========================================
Files 713 716 +3
Lines 205034 206988 +1954
Branches 52565 52862 +297
==========================================
+ Hits 121229 132497 +11268
+ Misses 54456 48534 -5922
+ Partials 29349 25957 -3392
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:
|
The SET LOCAL dispatch added for #6397 allows mixed-scope statements to reach the ParserSQL adapter. Its normalization strips GLOBAL/PERSIST qualifiers, so SET LOCAL innodb_lock_wait_timeout=5, GLOBAL time_zone=... could be acknowledged locally and applied as session state without ever sending the global assignment to MySQL. Add an opt-in session-only extraction mode and use it from the session handler. Validate targets before stripping scopes or coalescing duplicate variable names. Reject the entire extraction for global, persistent, or user-variable targets, including conflicting LOCAL @@global forms, and forward the original statement through the existing fallback before any tracked variable is changed. Keep the general adapter's default behavior for callers that need scope-normalized extraction. Use ParserSQL's SET parser with a directly accessible tokenizer in this mode. The vendored 1.0.11 general entry point can report OK/full input while omitting malformed assignments; checking its result alone lets a missing GLOBAL RHS bypass scope validation. Inspect tokenizer errors after the terminal token and preserve the existing subquery callback/value handling. Add 26 adapter regressions for LOCAL/SESSION targets, global and persistent scopes, repeated names, user variables, malformed first/middle/final assignments, multiple statements, and conflicting keyword/@@ scopes. Positive controls retain quoted values, global RHS reads, function calls, parentheses, subqueries, character sets, and trailing comments. Validation: PROXYSQL31=1 debug build passed; parsersql_unit-t passed all 230 assertions. Existing SET parser suites passed 236, 234, and 504 assertions. Regression cases reproduced malformed/global scope bypasses before the fix; the expanded isolated MySQL 8.4 test passed 261 assertions. Addresses the mixed-scope review finding on PR #6403. No benchmarks added.
Address the seven SonarCloud findings on the SET LOCAL regression fixture: make fixture ownership noncopyable/nonmovable, make destructor cleanup noexcept with caught diagnostics, use explicit mysql_close function addresses, and split setup, routing checks, and query counters out of main. The counter helper also removes the implicit reference-capturing lambda. Extend the live regression from three to four parser configurations: legacy algorithms 1 and 2, ParserSQL algorithm 3 with the legacy query processor, and algorithm 3 with the ParserSQL query processor. Preserve the writer/reader/writer checks for SESSION and LOCAL assignments. Add eight mixed or malformed scope cases for GLOBAL, @@global, PERSIST, PERSIST_ONLY, missing values, and LOCAL @@global. Each must produce a backend error, leave ProxySQL's tracked session values unchanged, and increase backend query counters. Use invalid values or session-only persistence targets so the test remains valid with privileged test users. Count all hostgroups when checking fallback forwarding because legacy fallback can route to the default hostgroup. Validation: the refined five-case regression failed 30 assertions in the two ParserSQL modes before the production fix, while legacy modes passed. The final eight-case test passed all 261 assertions through the isolated mysql84-g1 runner with the rebuilt PROXYSQL31=1 debug binary, including fixture restoration. Existing SET parser suites also passed. No benchmark files are included.
ParserSQL grammar errors previously left digest text null and the digest hash zero. Partial ASTs could also omit an unparsed suffix. The shared rule matcher then skipped digest predicates when their input was absent, allowing an unsupported ALTER statement to match a SELECT-only rule. Use AST normalization only for a successful, fully consumed parse. For errors, partial parses, missing ASTs, or unavailable emitter output, run ProxySQL's existing dialect tokenizer on the original SQL. This fallback is independent of ParserSQL's bounded arena, including when a large valid SELECT exhausts AST storage. Preserve tokenizer options, configured query and hash lengths, first-comment metadata, and normal digest ownership. Keep successful complete-AST normalization and SpookyHash unchanged. Strip PostgreSQL's terminal wire NUL before normalization so protocol framing cannot change a fallback digest. Keep empty token input without a digest and preserve available statement classification after fallback. Require actual digest metadata before evaluating digest-constrained rules, including negated digest regexes. A missing digest must never make such a rule unconditional; rules without digest predicates are unaffected. Validation: PROXYSQL31 debug build; all 104 unit test programs passed; 110 new fallback/rule assertions passed; live MySQL regression passed all 5 assertions with both tokenizer and ParserSQL modes. Tests are committed separately to follow the repository's commit convention.
Add 110 unit assertions against the real MySQL/PostgreSQL digest adapters and shared rule matcher. Check unsupported DDL, malformed statements, unconsumed suffixes, literals/comments, classification, empty input, inline/heap ownership, parser reuse, and PostgreSQL wire terminators. Exercise a 400 KB valid SELECT that exceeds the AST arena, plus tokenizer length/hash limits, lowercase normalization, and first-comment retention. Verify both regex engines reject unrelated digest regex/hash rules and that null or empty metadata cannot satisfy digest predicates, including negated regexes. Unconstrained rules must continue to match normally. Add a live five-assertion regression with SELECT-only and ALTER-only error rules. Compare legacy tokenizer and ParserSQL modes without sending DDL to a backend. Independently save and restore runtime and memory rules, fast routing, and MySQL variables, including cleanup after early returns. Register the unit test and live test in their normal CI groups. Validation: before the fix, 59 assertions failed in the initial 92-case regression, and the large-input extension reproduced a crash in an arena-dependent fallback. The final 110 assertions pass, the full unit group passes 104/104 programs, the live regression passes 5/5 assertions, and groups.json format lint and git diff --check pass.
renecannao
added a commit
that referenced
this pull request
Oct 10, 2026
…#6405) Merge fix/set-local-6397 into deps/parsersql-1.1.0 so the ParserSQL upgrade, SET LOCAL tracking and digest fallback can be reviewed and released through one PR. Preserve the original production and test commits from both branches. Carry over LOCAL dispatch and legacy-parser support, atomic session-scope validation, malformed/mixed-scope forwarding, and the live cross-hostgroup regression. Retain ParserSQL 1.1.0, PostgreSQL protocol-terminator fixes, updated user-variable rejection expectations, and digest-buffer ownership checks. The identical digest fallback changes shared by both branches are retained once in the merged tree. Resolve the adapter include conflict by keeping the SET parser and subquery callback includes alongside the tokenizer. Resolve the TAP plan conflict as 232 assertions: the 206 upgrade assertions plus 26 scope-validation cases. No assertions or benchmark artifacts are removed or added by consolidation. Validation against the combined tree: - PROXYSQL31 debug build and test builds pass. - Full isolated unit group: 104/104 test programs pass. - Parser adapter: 232 assertions; digest fallback: 110 assertions pass. - Standalone SET parser suites: 236, 234 and 517 assertions pass. - Isolated MySQL 8.4: 261 SET LOCAL and 5 digest-rule assertions pass. - groups.json format lint and git diff --check pass. PR #6405 supersedes #6403; this merge does not update the v3.0 base branch.
Contributor
Author
|
Consolidated into #6405 via merge commit 1b41977. All commits and regression coverage from this PR are included there. The combined branch passes 104/104 unit programs, the standalone SET suites, and the live SET LOCAL and digest fallback regressions. Closing this PR as superseded so review and CI continue in #6405; no changes have been merged into v3.0. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SET LOCAL innodb_lock_wait_timeout=5bypasses ProxySQL's tracked-variable dispatch and locks the frontend to a hostgroup. A later query routed elsewhere can fail with error 9006. This change treats MySQLLOCALlikeSESSIONfor supported tracked-variable assignments by extending the dispatch and legacy SET-parser scope regexes.The ParserSQL session-tracking path also validates assignment scopes before normalization. Mixed LOCAL/GLOBAL, persistent, and user-variable targets fall back to forwarding the complete original statement before changing tracked state. This prevents false local success when scope stripping would otherwise turn a global assignment into a session assignment. The validation checks ParserSQL tokenizer errors directly because the vendored parser can omit a malformed assignment while reporting a successful parse. Conflicting
LOCAL @@globaltargets are rejected as well.Regression coverage includes lowercase syntax, quoted names, multiple LOCAL assignments, writer/reader/writer routing, and mixed-scope fallback. The live test checks backend errors and counters as well as unchanged tracked state. It runs algorithms 1 and 2 and algorithm 3 with both query-processor modes. Fixture cleanup is exception-safe, ownership is explicit, and setup/test helpers address the SonarCloud findings.
The ParserSQL digest adapter now also falls back to ProxySQL's existing dialect tokenizer when grammar parsing fails, stops early, has no AST, or cannot emit a digest. This protects the currently vendored ParserSQL 1.0.11 independently of the 1.1.0 upgrade in #6405. Successful complete-AST normalization stays unchanged; fallback formatting/options follow the legacy tokenizer. The fallback is independent of AST arena limits, retains first-comment metadata, and excludes the PostgreSQL wire terminator. Digest-constrained query rules now reject missing digest metadata instead of treating the predicate as satisfied.
Validation:
PROXYSQL31=1 make -j8 debugbuild passed.parsersql_unit-t: 230 assertions passed, including 26 new scope and value-handling cases.reg_test_6397-set_local-tthrough the isolatedmysql84-g1runner: 261 assertions passed across four parser configurations, including fixture restoration.legacy-g1; MariaDB was not run locally.PostgreSQL SET scope semantics are unchanged; the digest fallback and missing-metadata rule protection cover both protocols. Commits have detailed descriptions. No benchmarks are included. Remote CI and SonarCloud must validate the updated head.
Fixes #6397.
Summary by cubic
Fixes
SET LOCALsession-variable tracking so statements likeSET LOCAL innodb_lock_wait_timeout=5no longer lock the frontend to a hostgroup, preventing error 9006 when a later query routes elsewhere. Treats MySQLLOCALas aSESSIONsynonym at the tracked-variable dispatch gate and in the shared scope regex used by SET parser algorithms 1 and 2, and validates assignment scopes before tracking so mixed GLOBAL/PERSIST/user-variable statements are forwarded to the backend instead of being accepted as local state.Adds a digest fallback that uses ProxySQL's tokenizer when ParserSQL cannot build a complete AST, so unsupported DDL, grammar errors, and statements exceeding AST arena limits still produce digests, and requires actual digest metadata before digest-constrained query rules match.
Written for commit 0469e97. Summary will update on new commits.
Summary by CodeRabbit
SET LOCALsession-variable assignments are now recognized and tracked for routing decisions, including quoted names, varied spacing, and multiple assignments.SET LOCALstatements across supported parser modes.SETstatements no longer update tracked session-variable values.