fix(set): handle PostgreSQL SET TIME ZONE alias and multi-value lists#38
Conversation
Two PG SET parser bugs surfaced when the algorithm=3 (ParserSQL)
SET path was first actually exercised in ProxySQL CI:
## Bug 1: SET TIME ZONE mis-parsed
Per the PostgreSQL docs, `SET TIME ZONE <value>` is an alias for
`SET TimeZone = <value>`. The tokenizer has no dedicated TK_TIME
or TK_ZONE keyword, so the input was lexed as IDENTIFIER("TIME")
followed by IDENTIFIER("ZONE"). `parse_variable_assignment()` saw
"TIME" as the variable target and "ZONE" as the unquoted RHS
identifier value, producing the assignment `time = ZONE`, and any
trailing value (`'UTC'`, `DEFAULT`, an INTERVAL expression, etc.)
was silently dropped on the floor.
Fix: at the top of `parse()`, in PostgreSQL dialect, peek for
IDENTIFIER("TIME") followed by IDENTIFIER("ZONE") (case-insensitive
via StringRef::equals_ci) and synthesise a `timezone = <rhs>`
assignment. The RHS is `DEFAULT` / `LOCAL` if those keywords are
the next token, otherwise the expression parser's result.
## Bug 2: PG SET multi-value list dropped after the first value
Per the PostgreSQL docs:
SET configuration_parameter { TO | = } { value | 'value' | DEFAULT }
"Some configuration parameters take a list of values, such as
search_path and datestyle."
`parse()` was sharing the MySQL-style comma loop, where each comma
introduces a *new* variable assignment. For PG that is wrong —
the comma is value-list continuation for the SAME variable. Given
`SET search_path TO "$user", public`, the loop tried to parse
`public` as a new assignment, found no `=`/`TO` after it, returned
nullptr, and the second value disappeared.
Fix: dialect-split the post-assignment comma loop. MySQL keeps the
existing behaviour (`parse_comma_item()` for each comma).
PostgreSQL appends each subsequent expression as an additional child
of the SAME `NODE_VAR_ASSIGNMENT` node (alongside the first RHS).
Downstream consumers (e.g. ProxySQL's `walk_set_stmt` adapter) walk
all `target->next_sibling`s to collect the full value list.
## Test coverage
`tests/test_set.cpp` gains:
- 9 entries in `pgsql_set_cases` for the new SET shapes (bulk
parse-success coverage)
- 5 structural `TEST_F(PgSQLSetTest, SetTimeZone...)` blocks
verifying the variable name resolves to `timezone`
- 4 structural `TEST_F(PgSQLSetTest, SetSearchPath...)` /
`SetDatestyleMultiValue` blocks verifying the assignment node
has target + N value children (instead of target + 1)
Full test suite: 1223/1223 pass; 0 new failures.
Reported via ProxySQL CI baseline failures in TAP group
set_parser_algorithm_3-g1 (tests pgsql-set_parameter_validation_test-t,
pgsql-set_statement_test-t) — both now produce the expected
parser output.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR enhances the PostgreSQL ChangesPostgreSQL SET Parser Enhancement
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Summary
Two PostgreSQL SET parser bugs surfaced when ProxySQL's
set_parser_algorithm=3(ParserSQL) path was first actually exercised in CI (it had been silently no-op'd by a test-harness gap). Both are PG-dialect-only — MySQL behavior is unchanged.Bug 1:
SET TIME ZONEmis-parsedPer the PG docs,
SET TIME ZONE <value>is an alias forSET TimeZone = <value>. The tokenizer has noTK_TIME/TK_ZONEkeyword, so the input lexed asIDENTIFIER("TIME")followed byIDENTIFIER("ZONE").parse_variable_assignment()tookTIMEas the variable target andZONEas the unquoted RHS identifier value, emitting the assignmenttime = ZONEand silently dropping everything that followed ('UTC',DEFAULT, an INTERVAL expression, …).Fix: At the top of
parse(), in PG dialect, peek forIDENTIFIER("TIME")followed byIDENTIFIER("ZONE")(case-insensitive viaStringRef::equals_ci) and synthesise atimezone = <rhs>assignment. RHS isDEFAULT/LOCALif those keywords are next, otherwise the expression parser's result.Bug 2: PG multi-value list dropped after the first value
Per the PG docs:
parse()shared the MySQL-style comma loop, where each comma introduces a new variable assignment. For PG that is wrong — commas are value-list continuation for the same variable. GivenSET search_path TO "$user", public, the loop tried to parsepublicas a new assignment, found no=/TOafter it, returnednullptr, and the second value disappeared.Fix: Dialect-split the post-assignment comma loop. MySQL keeps the existing behavior (
parse_comma_item()for each comma). PostgreSQL appends each subsequent expression as an additional child of the sameNODE_VAR_ASSIGNMENTnode (alongside the first RHS). Downstream consumers walk alltarget->next_siblings to collect the full value list.Test coverage
tests/test_set.cppgains:pgsql_set_casesfor the new SET shapes (bulk parse-success coverage)TEST_F(PgSQLSetTest, SetTimeZone…)blocks verifying the variable name resolves totimezone(wastime)TEST_F(PgSQLSetTest, SetSearchPath…)/SetDatestyleMultiValueblocks verifying the assignment node has target + N value children (was target + 1)Test results
(37 SKIPPED are integration tests needing live backends — same as baseline.)
Why now
Reported via ProxySQL CI baseline failures in TAP group
set_parser_algorithm_3-g1:pgsql-set_parameter_validation_test-t—SET search_pathproduced empty valuepgsql-set_statement_test-t—SET TIME ZONEnot handledBoth pass after this fix lands and ProxySQL bumps the vendored tarball to v1.0.3.
Test plan
make clean && make test→ 1223/1223 passsetparser_parsersql_testagainst a tarball built from this branch HEADSummary by CodeRabbit
Bug Fixes
SET TIME ZONEstatements to handle various input formats correctly.SETassignments.Tests
SETstatement syntax variations.