Repository navigation
Fix query_processor_parser=1 parity bugs (first comment, SET TRANSACTION, KILL, IN lists) - #6441
Merged
Merged
Conversation
…6419) With mysql-query_processor_parser=1 / pgsql-query_processor_parser=1, parsersql_digest_init() cleared qp->first_comment and refilled it only on the tokenizer-fallback branch. ParserSQL's lexer skips plain block comments, so any statement it parsed into a complete AST (the common SELECT/INSERT/UPDATE/DELETE case) reached the query processor with a NULL first_comment. Query annotations such as hostgroup=N, query_timeout=N, max_lag_ms=N or min_gtid=... were silently ignored, for text queries and for prepared statements (which cache the value at prepare time), while statements outside the grammar (e.g. SHOW) still honoured them because they fall back to the tokenizer. On the full-AST branch, extract the first comment with the legacy tokenizer (mysql_/pgsql_query_digest_and_first_comment), i.e. exactly the rules used with query_processor_parser=0, and discard the token digest it produces. The ParserSQL digest text and hash are unchanged. The legacy tokenizer records only '/' '*' block comments, so the extraction runs only when the query contains that two-byte sequence; other queries pay a single memchr() scan. qp->buf serves as the scratch buffer (as in legacy mode) before it receives the ParserSQL digest, so short queries allocate nothing extra. The option setup shared with the fallback is factored into parsersql_tokenizer_options(). The query_processor_parser=0 path is untouched.
…#6419) parsersql_first_comment_unit-t (unit-tests-g1) compares, for MySQL and PgSQL, the first comment produced by parsersql_digest_init_*() with the one produced by the legacy tokenizer used with query_processor_parser=0: leading/trailing/compact annotations on fully parsed SELECT, INSERT, UPDATE and DELETE, optimizer hints, multiple comments, comment-like string literals, '--' comments, '/' without '*', over-long comments, queries longer than the inline digest buffer, MySQL min_gtid, the tokenizer-fallback path (SHOW, '/*!'), PgSQL nested comments and the PgSQL wire terminator. It also checks that the annotation changes neither the ParserSQL digest text/hash nor the statement type. Without the fix 47 of its 103 assertions fail. reg_test_6419_parsersql_first_comment-t (legacy-g1, mysql84-g1) and pgsql-reg_test_6419_parsersql_first_comment-t (legacy-g1) route a 'hostgroup=N' annotation to a dedicated hostgroup with parser 0 and 1 and first_comment_parsing 1 and 2, for text queries and prepared statements (MySQL), and for simple, unnamed extended and named prepared statements (PgSQL), checked through the hostgroup's Queries counter. Statement texts are unique per mode because prepared statement metadata, including the first comment, is cached per query text. Without the fix every parser=1 annotated check fails.
With mysql-query_processor_parser=1, SET GLOBAL TRANSACTION ... and SET LOCAL TRANSACTION ... were answered with a synthetic OK and never reached the backend: the global (or session) isolation level / access mode did not change, and a user lacking SUPER/SYSTEM_VARIABLES_ADMIN got OK instead of ERR 1227. Root cause: parsersql_digest_init() used the ParserSQL DIGEST emitter output as digest_text for every fully parsed statement. The emitter (ParserSQL 1.1.0 emit_set_transaction) drops the scope keyword of SET ... TRANSACTION (GLOBAL, LOCAL, SESSION, PERSIST, PERSIST_ONLY, PgSQL SESSION CHARACTERISTICS AS), renders PERSIST/PERSIST_ONLY as the isolation level, and copies raw whitespace inside multi-word levels and access modes (so 'READ ONLY' becomes 'ISOLATION LEVEL READ ONLY'). 'SET GLOBAL TRANSACTION ISOLATION LEVEL SERIALIZABLE' therefore got the digest 'SET TRANSACTION ISOLATION LEVEL SERIALIZABLE', which matches MySQL_Session's session-level SET TRANSACTION regex (match_regexes[2]). That branch runs MySQL_Set_Stmt_Parser::parse2() on the raw query, which does not recognise the GLOBAL/LOCAL scope and returns an empty map, and the branch then replies OK without applying or forwarding anything. With query_processor_parser=0 the digest keeps the scope, no regex matches, and the statement is forwarded with the hostgroup locked. The same digest also merged GLOBAL/LOCAL/SESSION/unscoped statements into one digest hash, defeating match_digest rules such as '^SET GLOBAL'. Fix: when the ParserSQL AST is a SET statement carrying a NODE_SET_TRANSACTION clause, build the digest with the legacy tokenizer (parsersql_digest_fallback), exactly as for statements outside the grammar. SET ... TRANSACTION then has the same digest text and hash as with query_processor_parser=0 in both dialects, so the session classifies, tracks and forwards it exactly as in legacy mode. The statement type remains SET. Other SET statements keep the ParserSQL digest. The extra cost on the parser=1 path is one node-type comparison per query (plus a walk of the SET statement's children for SET); the query_processor_parser=0 path is untouched.
…r=1 (#6420) parsersql_set_transaction_digest_unit-t (unit-tests-g1) checks that parsersql_digest_init_mysql/_pgsql produce exactly the legacy tokenizer digest text and hash for SET ... TRANSACTION: unscoped, SESSION, GLOBAL, LOCAL, PERSIST and PERSIST_ONLY, ISOLATION LEVEL and READ ONLY/WRITE, lowercase input, extra whitespace/newlines/tabs inside the statement, a leading annotation comment, and PgSQL SET SESSION CHARACTERISTICS AS TRANSACTION. It also asserts that the scope is kept, that scoped and unscoped forms get different hashes, that PERSIST is not rendered as an isolation level, that the first comment and the SET statement type are preserved, and that SET statements without a TRANSACTION clause keep the ParserSQL digest. Without the fix 36 of its 52 assertions fail. reg_test_6420_parsersql_set_transaction-t (legacy-g1, mysql84-g1) runs the same checks with query_processor_parser=0 and =1 through ProxySQL: SET GLOBAL TRANSACTION ISOLATION LEVEL changes the backend's global isolation level (read back and restored inside one transaction, so all statements hit the same backend connection), the query digest keeps the GLOBAL scope, an unprivileged user (sbtest1) gets ERR 1227, SET LOCAL TRANSACTION is applied whenever it is acknowledged and has the same outcome in both modes, and SET SESSION TRANSACTION still works. Without the fix the parser=1 checks fail: the global level stays unchanged after OK, sbtest1 gets OK instead of ERR 1227, SET LOCAL leaves the session at REPEATABLE-READ, and no 'SET GLOBAL TRANSACTION' digest is recorded.
#6421) With mysql-query_processor_parser=1, MySQL_Query_Processor:: query_parser_command_type() returned the ParserSQL classification and never ran the legacy tokenizer classifier. ParserSQL has no statement type for KILL (nor for FLUSH, RELEASE SAVEPOINT, OPTIMIZE, ANALYZE, PURGE, CHANGE MASTER, RESET MASTER/REPLICA, ...), so KILL [CONNECTION|QUERY] <id> was classified as MYSQL_COM_QUERY_UNKNOWN. MySQL_Session:: handle_command_query_kill() intercepts only MYSQL_COM_QUERY_KILL, so the statement was forwarded to a backend: the client got ERR 1094 "Unknown thread id" for the ProxySQL session id returned by CONNECTION_ID(), or an unrelated backend thread with the same id was killed, while the targeted ProxySQL session kept running. The statements were also counted under UNKNOWN in stats_mysql_commands_counters. Factor the legacy token switch into mysql_query_command_type_legacy() and call it whenever the result is still UNKNOWN: always with parser=0 (same behaviour and cost as before), and with parser=1 only for statements ParserSQL does not classify. Statements ParserSQL classifies keep their ParserSQL mapping. The parser=1 path also returned before freeing qp->query_prefix, which is set instead of a digest when mysql-query_digests=0 and mysql-commands_stats=1, leaking it for every query. query_prefix is now freed on every path, as with parser=0.
parsersql_command_type_unit-t (unit-tests-g1) calls MySQL_Query_Processor::query_parser_command_type() on ParserSQL digests (parser=1) and on legacy digests (parser=0). KILL, KILL QUERY and KILL CONNECTION (mixed case, extra whitespace, leading comment, trailing ';') must classify as MYSQL_COM_QUERY_KILL and identically in both modes, as must other statements ParserSQL reports as UNKNOWN (FLUSH, RELEASE SAVEPOINT, OPTIMIZE, PURGE, CHANGE MASTER, RESET MASTER/REPLICA). Statements ParserSQL classifies (SELECT, INSERT, UPDATE, DELETE, SET, BEGIN, COMMIT, USE) must keep the ParserSQL mapping. With query_digests=0 the query-prefix path must classify KILL and free query_prefix. Without the fix 37 of its 76 assertions fail. reg_test_6421_parsersql_kill-t (legacy-g1, mysql84-g1) runs the same scenario through ProxySQL with query_processor_parser=0 and =1: KILL QUERY <id> interrupts a running SELECT SLEEP(30) on the target session, which stays usable; KILL <id> and KILL CONNECTION <id> close their target sessions; the three statements are counted under KILL in stats_mysql_commands_counters. Without the fix all parser=1 checks fail: the backend answers ERR 1094 "Unknown thread id", the SLEEP runs for 30s, the sessions stay open and the KILL counter does not move.
… expressions (#6428) With mysql-query_processor_parser=1 / pgsql-query_processor_parser=1 the digest text of a fully parsed statement comes from the ParserSQL emitter in EmitMode::DIGEST. That emitter renders every IN / NOT IN list that is not a single subquery as "IN (?)", and keeps only the first row of a VALUES clause, whatever the omitted elements are. The legacy tokenizer only replaces literals. ProxySQL string-matches the digest text for routing and multiplexing: MySQL_Session routes a query containing LAST_INSERT_ID / @@IDENTITY to the hostgroup of the last write (#1421), and MySQL_Connection disables multiplexing on GET_LOCK( (and on user variables). So "SELECT ... WHERE id IN (LAST_INSERT_ID())" was routed by query rules (e.g. to a reader, returning no or wrong rows), and "1 IN (GET_LOCK(...))" or "VALUES (1),(GET_LOCK(...))" did not pin the connection holding the lock. parsersql_digest_init() now walks the AST before emitting, and uses the legacy tokenizer (as for SET ... TRANSACTION, #6420) when a collapsed IN list or an additional VALUES row holds anything other than literals and placeholders (optionally signed, parenthesized or in row constructors). Such statements get exactly the digest text and hash of parser=0; literal lists keep the ParserSQL digest. The walk visits each AST node at most once, allocates nothing, and only runs with query_processor_parser=1.
#6428) parsersql_in_values_digest_unit-t (unit-tests-g1): for MySQL and PgSQL, statements whose IN / NOT IN list or additional VALUES rows contain LAST_INSERT_ID(), @@IDENTITY, GET_LOCK(), user variables, lastval(), pg_try_advisory_lock(), ... get the legacy digest text and hash, keep the substring that the session/connection checks look for, keep the ParserSQL statement type and the first comment. Literal-only lists (including a 5001-row INSERT), single subqueries and first VALUES rows keep the ParserSQL digest. reg_test_6428_parsersql_in_values-t (legacy-g1, mysql84-g1): with a "^SELECT" rule to a dedicated hostgroup, for parser=0 and parser=1, checks that "SELECT ... WHERE id IN (LAST_INSERT_ID())" / "IN (@@IDENTITY)" return the inserted row from the last written hostgroup, and that GET_LOCK() in an IN list or in a second VALUES row sets get_lock on the backend connection.
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 This change strengthens coverage for rejecting a partial SET parse and indicates no production behavior change in this increment. No merge-blocking risk is evident; normal checks remain appropriate. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.61% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 131 functions across 18 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main ParserSQL parity fixes covered by the changes, including first comments, SET TRANSACTION, KILL, and IN-list handling. Linked Issues check The PR meets the coding requirements for [ #6419] and [#6421]. For [#6419], the complete ParserSQL AST path calls the legacy first-comment extractor for MySQL and PostgreSQL. The code skips extraction …Out of Scope Changes check The changes remain within the stated ParserSQL parity scope. The SET TRANSACTION, partial SET parsing, and IN/VALUES digest changes prevent routing, interception, or multiplexing differences from pars…
- 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 hops through SQL at night,
First comments now remain in sight.
When AST digests leave things behind,
The legacy tokenizer helps them find.
SET statements parse their full way through,
Unknown commands get legacy review.
The rabbit thumps: the tests are new!
Comment @coderabbitai help to get the list of available commands.
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/tap/tests/reg_test_6421_parsersql_kill-t.cpp">
<violation number="1" location="test/tap/tests/reg_test_6421_parsersql_kill-t.cpp:134">
P1: The successful path emits 13 assertions per mode, but this constant makes TAP expect only 12 (25 total instead of 27), so the test fails with extra tests. Set it to 13; the plan and early-exit skip counts will then match.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| return false; | ||
| } | ||
|
|
||
| static constexpr int CHECKS_PER_MODE = 12; |
There was a problem hiding this comment.
P1: The successful path emits 13 assertions per mode, but this constant makes TAP expect only 12 (25 total instead of 27), so the test fails with extra tests. Set it to 13; the plan and early-exit skip counts will then match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/tap/tests/reg_test_6421_parsersql_kill-t.cpp, line 134:
<comment>The successful path emits 13 assertions per mode, but this constant makes TAP expect only 12 (25 total instead of 27), so the test fails with extra tests. Set it to 13; the plan and early-exit skip counts will then match.</comment>
<file context>
@@ -0,0 +1,226 @@
+ return false;
+}
+
+static constexpr int CHECKS_PER_MODE = 12;
+
+static void run_mode(const CommandLine& cl, MYSQL* admin, int parser) {
</file context>
Suggested change
| static constexpr int CHECKS_PER_MODE = 12; | |
| static constexpr int CHECKS_PER_MODE = 13; |
Contributor
Author
There was a problem hiding this comment.
Not valid. The success path of run_mode() emits exactly 12 assertions: configured, CONNECTION_ID, KILL QUERY succeeds, SLEEP interrupted, session survives KILL QUERY, KILL succeeds, closed by KILL, KILL CONNECTION succeeds, closed by KILL CONNECTION, killer unaffected, three statements counted, regular queries work. The ok(false, "client connections opened") call only runs on the connection-failure path, where it is followed by skip(CHECKS_PER_MODE - 2), which also comes to 12. The configuration-failure path is 1 + skip(CHECKS_PER_MODE - 1) = 12. Re-ran on legacy-g1 at 7fb4b2c: plan 1..25, 25/25 ok, so CHECKS_PER_MODE stays 12.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6441 +/- ##
==========================================
+ Coverage 64.51% 65.48% +0.97%
==========================================
Files 720 725 +5
Lines 206900 208837 +1937
Branches 52781 53399 +618
==========================================
+ Hits 133478 136766 +3288
+ Misses 48605 46604 -2001
- Partials 24817 25467 +650
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 admin interface runs every admin session on one shared SQLite handle, so the fixtures' TEMPORARY tables do outlive a client disconnect and the startup drops are needed to recover from an interrupted run. Qualify both the startup and cleanup drops with the temp schema so they can never fall through to a persistent main-schema table of the same name.
Replace the v1.1.0 source archive with the unmodified v1.1.1 release and point the tracked parsersql symlink at parsersql-1.1.1. The archive is generated with git archive --format=tar.gz --prefix=ParserSQL-1.1.1/ v1.1.1 from upstream commit c7adfcb1d578f3121f876bebf2ee5b4978cbf7af, the same method that reproduces the v1.1.0 archive byte for byte (SHA-256 2d14704f...a29c66). v1.1.1 (ParserSQL PR #71) fixes SET ... TRANSACTION: the scope (GLOBAL, SESSION, LOCAL, PERSIST, PERSIST_ONLY) is kept, each characteristic is a canonical NODE_TRANSACTION_OPTION, whitespace and comments are normalized, PG SESSION CHARACTERISTICS AS is flagged, and SET statements mixing TRANSACTION with other items are rejected. PG SET ROLE, SET SESSION AUTHORIZATION, SET CONSTRAINTS and multi-value search_path digests are now emitted in full. Record the release link, upstream commit and archive SHA-256 (3c4f4916...72fe38) in the dependency audit history. The deps Makefile picks the archive up by glob, so no build rule changes.
…6420) ParserSQL 1.1.0 dropped the scope of SET GLOBAL|LOCAL|PERSIST|PERSIST_ONLY TRANSACTION in its DIGEST output, so parsersql_is_set_transaction() forced the legacy tokenizer for every SET ... TRANSACTION. Otherwise the scope-less digest matched MySQL_Session's session-level SET TRANSACTION regex (match_regexes[2]) and the statement was acknowledged with an OK that nothing applied. ParserSQL 1.1.1 keeps the scope as a canonical identifier, parses every characteristic, normalizes whitespace, comments and keyword case, and keeps PgSQL SESSION CHARACTERISTICS AS distinct from SESSION. Its digest differs from the legacy one only in keyword case and in the space after a list comma, which match_regexes[2] (case-insensitive) and the SET handling in MySQL_Session do not depend on. GLOBAL, PERSIST, PERSIST_ONLY and PgSQL SESSION CHARACTERISTICS forms do not match the regex, and are forwarded exactly as with query_processor_parser=0. Remove the workaround so SET ... TRANSACTION uses the ParserSQL digest like every other fully parsed statement.
MySQL_Session matches the digest of a SET statement against match_regexes[2] (SET [SESSION] TRANSACTION ...) and then tracks what MySQL_Set_Stmt_Parser::parse2() extracts from the query. Each reply is an OK generated by ProxySQL, so whatever parse2() misses is acknowledged but never applied. The same failure mode as #6420, with both query_processor_parser=0 and =1: - The digest collapses whitespace but parse2() runs on the query and accepted only spaces. "SET SESSION\nTRANSACTION READ ONLY" and "... ISOLATION LEVEL READ\nCOMMITTED" returned an empty map, so the statement was acknowledged with no effect. Multiple spaces inside a level or access mode produced a key or value that is not tracked. - The regex matches a prefix and parse2() stops at the first characteristic it cannot consume, so "SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED, READ ONLY" applied only the isolation level. parse2() now accepts any whitespace, collapses it in the captured level and access mode, and returns nothing if any input is left after the last characteristic. MySQL_Session forwards the statement through unable_to_parse_set_statement(), as for any other SET it cannot parse, when parse2() returns nothing, instead of replying OK.
In MySQL, LOCAL is a synonym of SESSION, also for SET TRANSACTION. match_regexes[2] accepted only an empty scope or SESSION, so SET LOCAL TRANSACTION was not tracked: it was forwarded and locked the session to its hostgroup. With ParserSQL 1.1.0 and query_processor_parser=1 its scope-less digest matched the regex, but parse2() did not recognise LOCAL and the statement was acknowledged with no effect (#6420). Add LOCAL to the regex and to parse2(), which reports it with the "session" key, so MySQL_Session updates the session isolation level or access mode exactly as for SESSION. Move the regex to MYSQL_SET_TRANSACTION_DIGEST_REGEX in MySQL_Set_Stmt_Parser.h, next to the parse2() grammar it must agree with, so unit tests can use the same pattern. GLOBAL, PERSIST and PERSIST_ONLY still do not match and are forwarded. The regex is case-insensitive and both digest paths keep the scope keyword (the ParserSQL digest is uppercase, the legacy digest follows mysql-query_digests_lowercase), so every spelling of LOCAL matches. PgSQL is unchanged: PgSQL_Thread defines the same match_regexes[2] but PgSQL_Session never uses it. PgSQL SET [SESSION|LOCAL] TRANSACTION are not tracked and are forwarded.
…6420) Adapt the #6420 tests to the ParserSQL 1.1.1 digest, which ProxySQL now uses for SET ... TRANSACTION instead of the legacy digest. parsersql_set_transaction_digest_unit-t: - Every scope form (none, SESSION, LOCAL, GLOBAL, PERSIST, PERSIST_ONLY, PgSQL SESSION CHARACTERISTICS AS) has the expected ParserSQL digest in both dialects, including mixed case, newlines, tabs and repeated spaces. It equals the legacy digest except for keyword case and the space after a list comma. - MYSQL_SET_TRANSACTION_DIGEST_REGEX (match_regexes[2]) gives the same result on the ParserSQL and legacy digests: it matches the unscoped, SESSION and LOCAL forms, and never GLOBAL, PERSIST, PERSIST_ONLY or SESSION CHARACTERISTICS, so those are still not treated as session changes. - Scoped digests keep distinct hashes, also in PgSQL. - parse2() reports LOCAL as "session", accepts any whitespace, and returns nothing for two characteristics, trailing junk or GLOBAL / PERSIST. - PgSQL SET ROLE, SET SESSION AUTHORIZATION, SET CONSTRAINTS and multi-value search_path keep their full digest (1.1.0 emitted "SET " or only the first value). reg_test_6420_parsersql_set_transaction-t, for query_processor_parser=0 and =1: - SET LOCAL TRANSACTION ISOLATION LEVEL is tracked (client isolation_level in PROXYSQL INTERNAL SESSION) without locking the hostgroup, and is applied to the session. - SET LOCAL\n TRANSACTION READ ONLY is applied. - SET SESSION TRANSACTION with two characteristics applies both.
ParserSQL returns status OK with full_input == false when it parses only a prefix of a SET statement: "SET a=1 junk", "SET NAMES utf8 junk", "SET a=1, =2", "SET a=1; SELECT 1". parsersql_parse_set_mysql() walked that AST for any OK or PARTIAL result unless session_scope_only was set, so it returned the assignments of the prefix. For example "SET sql_mode=(SELCT ...)" returned sql_mode="(SELCT ...)". parsersql_parse_set_pgsql() did not check full_input. It relied only on ast_covers_full_input(). Require status OK, full_input and an AST in both entry points, as parsersql_digest_init() does. Any other result is unparsed (empty map), so the session forwards the statement as for any other non-OK result. The session tracking path for set_parser_algorithm=3 / query_processor_parser=1 (MySQL session_scope_only, PgSQL ast_covers_full_input()) already rejected these inputs, so tracking does not change. This makes the rule hold for every caller. Trailing ';' and whitespace are still a complete parse. A multi-statement such as "SET a=1; SELECT 1" is unparsed. This matches algorithms 1/2: MySQL_Session and PgSQL_Session lock the hostgroup for a multi-statement SET before any SET parser runs, and the parser must not return the prefix.
parsersql_unit-t: SET statements that ParserSQL parses only in part
("SET wait_timeout=100 junk", "SET NAMES utf8 junk", "SET
wait_timeout=100, =2", "SET ...; SELECT 1", "SET ...;;" and PgSQL
search_path / client_encoding equivalents) return an empty map from both
MySQL entry points and the PgSQL one. A trailing ';' and whitespace still
parse.
setparser_parsersql_test: "SET sql_mode=(SELCT CONCAT(...))" is a syntax
error that ParserSQL parses only up to "(SELCT". It is now unparsed, as
with algorithms 1/2 (setparser_test_common.h), instead of returning the
raw parenthesized text.
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/Query_Processor_ParserSQL.cpp">
<violation number="1" location="lib/Query_Processor_ParserSQL.cpp:1355">
P2: `result.full_input` is checked before `ast_covers_full_input()`, but that helper exists precisely because ParserSQL returns OK while consuming only a prefix, and it deliberately tolerates a single trailing comma (`SET search_path TO "$user" ,`), which its docstring says existing tests rely on. An unconsumed trailing comma means full_input == false, so this gate now rejects that previously-tracked form. `ast_covers_full_input()` already rejects the prefix-only cases this gate targets (`SET a=1 junk`, `SET a=1; SELECT 1`); drop the full_input term, or verify it is not false for the tolerated trailing-comma tail.</violation>
</file>
<file name="lib/MySQL_Set_Stmt_Parser.cpp">
<violation number="1" location="lib/MySQL_Set_Stmt_Parser.cpp:506">
P3: The new residue check bails out on valid MySQL SET TRANSACTION statements that carry a second comma-separated characteristic, e.g. `SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED, READ WRITE` (valid per the `SET TRANSACTION` grammar, which allows `transaction_characteristic [, transaction_characteristic] ...`). The loop consumes only the first characteristic, the leftover `, READ WRITE` clears the map, and the new MySQL_Session empty-map branch forwards the statement via `unable_to_parse_set_statement()`. Net change for every parser mode (including legacy, which the PR claims is otherwise unchanged): statements that previously were ACKed and tracked (partially) now emit a warning-level "Unable to parse unknown SET query ... Please report a bug" log and set `lock_hostgroup`, pinning the connection. Since MySQL allows at most the isolation level and one access mode, parse2() could consume the optional `, READ WRITE|READ ONLY` second characteristic and keep session tracking instead of forwarding. At minimum, the warning noise and hostgroup pinning for valid statements deserve a callout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // An OK result with full_input == false also covers only a prefix of | ||
| // the input (`SET a = 1 junk`, `SET a = 1; SELECT 1`). ast_covers_full_input() | ||
| // additionally rejects trailing bytes the AST does not reach. | ||
| if (result.status == ParseResult::OK && result.full_input) { |
There was a problem hiding this comment.
P2: result.full_input is checked before ast_covers_full_input(), but that helper exists precisely because ParserSQL returns OK while consuming only a prefix, and it deliberately tolerates a single trailing comma (SET search_path TO "$user" ,), which its docstring says existing tests rely on. An unconsumed trailing comma means full_input == false, so this gate now rejects that previously-tracked form. ast_covers_full_input() already rejects the prefix-only cases this gate targets (SET a=1 junk, SET a=1; SELECT 1); drop the full_input term, or verify it is not false for the tolerated trailing-comma tail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At lib/Query_Processor_ParserSQL.cpp, line 1355:
<comment>`result.full_input` is checked before `ast_covers_full_input()`, but that helper exists precisely because ParserSQL returns OK while consuming only a prefix, and it deliberately tolerates a single trailing comma (`SET search_path TO "$user" ,`), which its docstring says existing tests rely on. An unconsumed trailing comma means full_input == false, so this gate now rejects that previously-tracked form. `ast_covers_full_input()` already rejects the prefix-only cases this gate targets (`SET a=1 junk`, `SET a=1; SELECT 1`); drop the full_input term, or verify it is not false for the tolerated trailing-comma tail.</comment>
<file context>
@@ -1370,12 +1349,10 @@ std::map<std::string, std::vector<std::string>> parsersql_parse_set_pgsql(
+ // An OK result with full_input == false also covers only a prefix of
+ // the input (`SET a = 1 junk`, `SET a = 1; SELECT 1`). ast_covers_full_input()
+ // additionally rejects trailing bytes the AST does not reach.
+ if (result.status == ParseResult::OK && result.full_input) {
if (result.ast && result.ast->type == NodeType::NODE_SET_STMT &&
ast_covers_full_input(result.ast, query.c_str(), (int)query_length)) {
</file context>
Suggested change
| if (result.status == ParseResult::OK && result.full_input) { | |
| if (result.status == ParseResult::OK) { |
Contributor
Author
There was a problem hiding this comment.
Not an issue. The PgSQL SET parser consumes the trailing comma, so full_input is true for SET search_path TO "$user" ,. The tolerated forms are already covered and pass with this gate: [partial_ast_gate 3] (SET search_path TO "$user" , gives a non-empty map) and TestPgsqlProtocolTerminator ("$user" ,, "$user" ,; and "$user" ,; \t\n, with and without the protocol NUL, give {"\"$user\""}) in setparser_parsersql_test. full_input stays so that this adapter uses the same rule as the digest adapter and parsersql_parse_set_mysql().
| // Anything left (e.g. a second characteristic after a comma) was not parsed: | ||
| // report nothing rather than a subset of the statement. | ||
| while (!input.empty() && isspace(static_cast<unsigned char>(input[0]))) input.remove_prefix(1); | ||
| if (!input.empty()) result.clear(); |
There was a problem hiding this comment.
P3: The new residue check bails out on valid MySQL SET TRANSACTION statements that carry a second comma-separated characteristic, e.g. SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED, READ WRITE (valid per the SET TRANSACTION grammar, which allows transaction_characteristic [, transaction_characteristic] ...). The loop consumes only the first characteristic, the leftover , READ WRITE clears the map, and the new MySQL_Session empty-map branch forwards the statement via unable_to_parse_set_statement(). Net change for every parser mode (including legacy, which the PR claims is otherwise unchanged): statements that previously were ACKed and tracked (partially) now emit a warning-level "Unable to parse unknown SET query ... Please report a bug" log and set lock_hostgroup, pinning the connection. Since MySQL allows at most the isolation level and one access mode, parse2() could consume the optional , READ WRITE|READ ONLY second characteristic and keep session tracking instead of forwarding. At minimum, the warning noise and hostgroup pinning for valid statements deserve a callout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At lib/MySQL_Set_Stmt_Parser.cpp, line 506:
<comment>The new residue check bails out on valid MySQL SET TRANSACTION statements that carry a second comma-separated characteristic, e.g. `SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED, READ WRITE` (valid per the `SET TRANSACTION` grammar, which allows `transaction_characteristic [, transaction_characteristic] ...`). The loop consumes only the first characteristic, the leftover `, READ WRITE` clears the map, and the new MySQL_Session empty-map branch forwards the statement via `unable_to_parse_set_statement()`. Net change for every parser mode (including legacy, which the PR claims is otherwise unchanged): statements that previously were ACKed and tracked (partially) now emit a warning-level "Unable to parse unknown SET query ... Please report a bug" log and set `lock_hostgroup`, pinning the connection. Since MySQL allows at most the isolation level and one access mode, parse2() could consume the optional `, READ WRITE|READ ONLY` second characteristic and keep session tracking instead of forwarding. At minimum, the warning noise and hostgroup pinning for valid statements deserve a callout.</comment>
<file context>
@@ -480,8 +497,13 @@ std::map<std::string,std::vector<std::string>> MySQL_Set_Stmt_Parser::parse2() {
+ // Anything left (e.g. a second characteristic after a comma) was not parsed:
+ // report nothing rather than a subset of the statement.
+ while (!input.empty() && isspace(static_cast<unsigned char>(input[0]))) input.remove_prefix(1);
+ if (!input.empty()) result.clear();
delete opt2;
</file context>
Contributor
Author
There was a problem hiding this comment.
Known and deliberate. Before this change, ... READ COMMITTED, READ WRITE returned OK with only the isolation level applied, so the client was told the access mode was set when it was not. The statement is now forwarded with the hostgroup locked, so the backend applies both characteristics. reg_test_6420_parsersql_set_transaction-t checks that both are applied. Pinning the connection is the safe direction, and the PR description calls this out. Tracking the second characteristic in parse2() is a follow-up and will not be done in this PR.
TestParse accepts any non-empty ParserSQL result when the expectation is empty (an empty expectation documents a regex-parser limitation), so the "SET sql_mode=(SELCT ...)" case in parsersql_syntax_errors did not check that the partial parse is rejected. Move it to parsersql_parse_fail_strict, which requires an empty map.
|
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.
Summary
Fixes the
query_processor_parser=1(ParserSQL mode) bugs found in the v3.0.12 pre-release review (parent #6439). The guiding rule: with*-query_processor_parser=1, routing, interception and multiplexing decisions must matchquery_processor_parser=0; when ParserSQL's output would change them, fall back to the legacy tokenizer/classifier. Theparser=0path is unchanged in behavior and cost, except for the MySQLSET ... TRANSACTIONfixes under "Also in this PR", which apply to both modes.ParserSQL mode is off by default in every tier, but
doc/mysql-user-variable-tracking.mdpoints users at it.first_commentwas filled only on the tokenizer fallback path; fully parsed queries silently lost annotations (hostgroup=,query_timeout=,max_lag_ms=,min_gtid=, …), including for prepared statementsmemchrwhen the query has no/*)SET GLOBAL/LOCAL/SESSION TRANSACTIONdropped the scope, so the session matched it as plainSET TRANSACTION,parse2()returned nothing, and the client got OK without the statement being applied or forwardedSET ... TRANSACTIONuses the ParserSQL digest again (same asparser=0except keyword case and the space after a list comma). GLOBAL/PERSIST/PERSIST_ONLY and PgSQLSESSION CHARACTERISTICSstill don't match the session regex and are forwardedMYSQL_COM_QUERY_UNKNOWN→ KILL forwarded to a backend (could kill an unrelated thread)query_prefixleak in parser mode (query_digests=0+commands_stats=1)IN (?)and keeps only the first VALUES row, hidingLAST_INSERT_ID(),@@IDENTITY,GET_LOCK(), user variables,pg_try_advisory_lock()… from the session/connection checksAlso in this PR
git archive --format=tar.gz --prefix=ParserSQL-1.1.1/ v1.1.1(same method reproduces the 1.1.0 archive), SHA-256 recorded indeps/parsersql/README.md.SET LOCAL TRANSACTION(MySQL): LOCAL is a synonym of SESSION. It is added tomatch_regexes[2](nowMYSQL_SET_TRANSACTION_DIGEST_REGEX) and toparse2(), so it is tracked likeSET SESSION TRANSACTIONinstead of being forwarded with the hostgroup locked. PgSQL is unchanged:PgSQL_Sessionnever uses itsmatch_regexes[2].parse2()partial parses: the digest collapses whitespace butparse2()accepted only spaces, and it stopped at the first characteristic.SET SESSION\nTRANSACTION READ ONLYwas acknowledged with no effect, and... READ COMMITTED, READ ONLYapplied only the isolation level.parse2()now accepts any whitespace and returns nothing if input is left over. The session then forwards the statement (unable_to_parse_set_statement()) instead of replying OK. This applies to both parser modes.set_parser_algorithm=3partial parses:parsersql_parse_set_mysql()/parsersql_parse_set_pgsql()now requirestatus == OK && full_input, like the digest adapter. A prefix-only parse (SET a=1 junk,SET NAMES utf8 junk,SET a=1; SELECT 1) is unparsed. Trailing;and whitespace still parse. The session tracking path already rejected these inputs, so this makes the rule hold for every caller. A multi-statement SET locks the hostgroup before any parser runs, as with algorithms 1/2.PgSQL parser-mode classification of
pg_cancel_backend/pg_terminate_backend(#6422) is fixed in the separate PostgreSQL PR.Note for release notes
With
parser=1, statements whose IN lists / VALUES rows contain non-literals now get theparser=0digest text and hash (e.g. PgSQLIN ($1::int,$2::int)changes fromIN (?)toIN ($1,$2)). Query rules matching onparser=1digests of such statements, and digest stats continuity, are affected.ParserSQL 1.1.1 also changes
parser=1digests:SET [GLOBAL|SESSION|LOCAL|PERSIST|PERSIST_ONLY] TRANSACTION ...keeps its scope and is uppercase/whitespace-normalized (1.1.0:SET TRANSACTION ...for every scope), PgSQLSET ROLE/SET SESSION AUTHORIZATION/SET CONSTRAINTSare emitted in full (1.1.0:SET), and multi-valuesearch_pathkeeps all values.In both parser modes,
SET LOCAL TRANSACTIONis now tracked likeSET SESSION TRANSACTION(no hostgroup lock), and aSET [SESSION] TRANSACTIONthatparse2()cannot parse in full (several characteristics) is forwarded with the hostgroup locked instead of being acknowledged with only the first one applied. Tabs or newlines inside the statement no longer produce an OK with no effect.Not in this PR (tracked in the issues)
emit_set_transactionfix is in 1.1.1, vendored here).doc/mysql-user-variable-tracking.mdrecommendingmysql-set_parser_algorithm=3.Refs #6420, #6428. Closes #6419, closes #6421. Part of #6439.
Tests
parsersql_first_comment_unit-t(103),parsersql_set_transaction_digest_unit-t(140: every scope form in both dialects, regex outcome on ParserSQL vs legacy digest,parse2()LOCAL/whitespace/partial, PgSQL ROLE/AUTHORIZATION/CONSTRAINTS/search_path digests),parsersql_command_type_unit-t(76),parsersql_in_values_digest_unit-t(74).parser=0andparser=1end to end:reg_test_6419_parsersql_first_comment-t,pgsql-reg_test_6419_parsersql_first_comment-t,reg_test_6420_parsersql_set_transaction-t,reg_test_6421_parsersql_kill-t,reg_test_6428_parsersql_in_values-t(legacy-g1, mysql84-g1).reg_test_6420_parsersql_set_transaction-talso checks thatSET LOCAL TRANSACTIONis tracked without a hostgroup lock and applied, and thatSET SESSION TRANSACTION ... READ COMMITTED, READ ONLYapplies both characteristics.parsersql_unit-t(248) covers partial SET parses withset_parser_algorithm=3.parsersql_*_unit-t,query_processor_unit-t,pgsql_query_processor_unit-t,digest_stats_unit-t,setparser_parsersql_test(517),parsersql_digest_test(63),setparser_test2/3all pass;reg_test_6420_parsersql_set_transaction-tandreg_test_6397-set_local-tpass in mysql84-g1 and legacy-g1 (20/20 and 261/261 each). The fail-without-fix check was not repeated for the 1.1.1 follow-up assertions.Summary by cubic
Fixes parity bugs in ParserSQL mode (
*/query_processor_parser=1), where routing, interception, and multiplexing decisions diverged from legacy mode. Whenever ParserSQL's output would change a decision, the code now falls back to the legacy tokenizer/classifier; theparser=0path is unchanged in behavior and cost.hostgroup=,query_timeout=,min_gtid=, …), which they previously lost (ParserSQL mode (query_processor_parser=1) drops first-comment annotations on fully parsed queries #6419).SET ... TRANSACTION, soSET GLOBAL/LOCAL TRANSACTIONno longer matches the session-level handler that replied OK without applying the statement (Parser mode: SET GLOBAL/LOCAL TRANSACTION is acknowledged with OK but never forwarded to the backend #6420).SET LOCAL TRANSACTIONis now tracked likeSET SESSION TRANSACTION, and aSET SESSION TRANSACTIONthatparse2()cannot parse in full is forwarded with the hostgroup locked instead of acknowledged with only part applied.query_prefixleak.LAST_INSERT_ID(),@@IDENTITY, andGET_LOCK()for routing and multiplexing (query_processor_parser=1: IN/NOT IN lists and multi-row VALUES collapse to?, hiding LAST_INSERT_ID/@@IDENTITY/GET_LOCK #6428).SET a=1 junk) is treated as unparsed, so the session forwards it instead of tracking the prefix.Note. With
parser=1, the digest text and hash of those IN/VALUES statements now matchparser=0, so query rules matching onparser=1digests and digest-stats continuity are affected; ParserSQL 1.1.1 also normalizesSET ... TRANSACTIONdigests (scope kept, uppercase, whitespace-normalized). The new tests' admin fixtures qualify temp-table drops withtemp.so they recover cleanly from interrupted runs.Closes #6419 and #6421; refs #6420 and #6428.
Written for commit 297b3cd. Summary will update on new commits.
Summary by CodeRabbit
KILLstatements.INlists and multi-rowVALUESclauses, and leading SQL comments.SETstatements are no longer treated as successfully parsed; they are forwarded for handling.