From 4eb52abf2c9fa6aefcb8a6b6656cfcd32f04c2e7 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sat, 23 May 2026 08:02:17 +0000 Subject: [PATCH] fix(pgsql,test): PG multi-value SET dispatch + test-fixture cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the cluster-2 ParserSQL 1.0.3 bump (this same PR), addressing review feedback from coderabbit + gemini. ## 1. PgSQL_Session: accept multi-value SET vectors (coderabbit, Major) The previous commit `5a48004af` taught the adapter to walk every RHS sibling so PG multi-value lists (e.g. `SET search_path TO "$user", public`) come through as a `std::vector` of every value. But `lib/PgSQL_Session.cpp:4516` still had `size() > 2` → `unable_to_parse_set_statement()`, which would have caused 3+ value PG SETs to lock the hostgroup instead of being processed. Worse, the downstream value-consumer at the same site used only `it->second.front()`, silently dropping every value past the first for valid 2-value SETs too. Fix: * Drop the upper bound from the cardinality check (keep `< 1` so empty value lists still fail loudly). * Join every value in the vector into `value1` with the comma-separated form PG itself uses for these parameters (`a, b, c`). Single-value SETs hit the loop with `size()==1` and take just `it->second.front()`, preserving the previous behaviour. The validator / hash-compare / tracker / param_status code below the join is unchanged — it now receives the full joined value instead of the truncated first element. ## 2. setparser_parsersql_test: drop INTERVAL test that locked in incomplete behaviour (subagent correctness review, Major) `parsersql_pgsql_time_zone` previously asserted that `SET TIME ZONE INTERVAL '7' HOUR` produces `timezone = "INTERVAL"`, which is the *current* observable output of ParserSQL — the expression parser does not yet consume the `'7' HOUR` modifier chain. Encoding that as expected output would flip the test red when ParserSQL is later improved to capture the full interval expression, conflating an improvement with a regression. Removed the case; left a comment in the fixture explaining the gap so a future contributor adds the case back together with the ExpressionParser fix. ## 3. Diag-output cleanup (gemini, cosmetic) * `%lu` → `%zu` for `size_t` (portability — `size_t` is not always `unsigned long` on 32-bit / Windows). * Extract the per-vector " | "-separated diag-string builder into a single `join_values_for_diag()` helper used by both `TestParse` (now also dumps full vector on mismatch, where it previously printed just `vals[0]`) and `TestParsePgsql`. ## Verification * `./setparser_parsersql_test`: 260/270 ok; the 10 remaining not-ok are unchanged pre-existing failures (`sql_mode 23-25`, `Set1_v2 0-1`), nothing new from this commit. * `pgsql_time_zone` group: 6 assertions (3 cases × 2) — confirms the INTERVAL case is removed cleanly. * `pgsql_search_path` group: 16 assertions still all pass — the multi-value join in PgSQL_Session does not affect the parser- side test (which goes through the adapter, not the session). --- lib/PgSQL_Session.cpp | 13 +++++- test/tap/tests/setparser_parsersql_test.cpp | 45 ++++++++++++--------- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/lib/PgSQL_Session.cpp b/lib/PgSQL_Session.cpp index c63ac7c43..1d64da390 100644 --- a/lib/PgSQL_Session.cpp +++ b/lib/PgSQL_Session.cpp @@ -4513,7 +4513,7 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___handle_ for (auto it = std::begin(set); it != std::end(set); ++it) { std::string var = it->first; proxy_debug(PROXY_DEBUG_MYSQL_COM, 5, "Processing SET variable %s\n", var.c_str()); - if (it->second.size() < 1 || it->second.size() > 2) { + if (it->second.size() < 1) { // error not enough arguments string query_str = string((char*)CurrentQuery.QueryPointer, CurrentQuery.QueryLength); string digest_str = string(CurrentQuery.get_digest_text()); @@ -4532,7 +4532,18 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___handle_ return false; } + // PostgreSQL allows multi-value lists for some variables, notably + // search_path and datestyle (e.g. `SET search_path TO "$user", public`). + // ParserSQL v1.0.3 captures every value as a separate sibling; here + // we collapse them into the comma-separated form PG itself uses for + // these parameters before handing to the per-variable validator and + // tracker. Single-value SETs hit this loop with size()==1 and take + // just it->second.front() (unchanged behaviour). std::string value1 = it->second.front(); + for (size_t vi = 1; vi < it->second.size(); ++vi) { + value1 += ", "; + value1 += it->second[vi]; + } if (std::find(pgsql_critical_variables.begin(), pgsql_critical_variables.end(), var) != pgsql_critical_variables.end() || pgsql_other_variables.find(var) != pgsql_other_variables.end()) { diff --git a/test/tap/tests/setparser_parsersql_test.cpp b/test/tap/tests/setparser_parsersql_test.cpp index 23eeb4c62..548f08c37 100644 --- a/test/tap/tests/setparser_parsersql_test.cpp +++ b/test/tap/tests/setparser_parsersql_test.cpp @@ -85,14 +85,18 @@ static Test parsersql_pgsql_search_path[] = { // Pre-ParserSQL-1.0.3 these were parsed as `time = ZONE` and the rest of the // statement was dropped. The v1.0.3 fix recognizes "TIME ZONE" as the PG // alias for `SET TimeZone = ...` and walks the trailing expression. -// NOTE: `INTERVAL '7' HOUR` is captured as `INTERVAL` only — the expression -// parser does not yet consume the full interval modifier chain. The test -// reflects the *current* observable behaviour. +// +// NOTE: `SET TIME ZONE INTERVAL '7' HOUR` is intentionally *not* covered +// here. ParserSQL's expression parser does not yet consume the full +// INTERVAL ... modifier chain — it currently captures just the +// `INTERVAL` token. Asserting that as the expected output would lock in +// incomplete-but-current behaviour and would flip the test red when the +// parser is later fixed to capture the full interval expression. Add a +// case here once ParserSQL grows full INTERVAL modifier support. static Test parsersql_pgsql_time_zone[] = { { "SET TIME ZONE 'UTC'", { Expected("timezone", {"UTC"}) } }, { "SET TIME ZONE DEFAULT", { Expected("timezone", {"DEFAULT"}) } }, { "SET TIME ZONE '+05:30'", { Expected("timezone", {"+05:30"}) } }, - { "SET TIME ZONE INTERVAL '7' HOUR", { Expected("timezone", {"INTERVAL"}) } }, }; static std::string normalize_value(const std::string& s) { @@ -128,6 +132,17 @@ static bool maps_match( return true; } +// Join a vector of values into a single " | "-separated string for diag output. +// Used by TestParse / TestParsePgsql when reporting expected-vs-actual mismatches. +static std::string join_values_for_diag(const std::vector& vals) { + std::string joined; + for (size_t j = 0; j < vals.size(); ++j) { + if (j) joined += " | "; + joined += vals[j]; + } + return joined; +} + void TestParse(const Test* tests, int ntests, const std::string& title) { for (int i = 0; i < ntests; i++) { std::map> data; @@ -138,7 +153,7 @@ void TestParse(const Test* tests, int ntests, const std::string& title) { std::map> result = parsersql_parse_set_mysql(tests[i].query); bool size_ok = (result.size() == data.size()); - ok(size_ok, "[%s %d] Sizes match: %lu, %lu", title.c_str(), i, result.size(), data.size()); + ok(size_ok, "[%s %d] Sizes match: %zu, %zu", title.c_str(), i, result.size(), data.size()); if (!size_ok) { diag(" FAIL: sizes differ for query: %s", tests[i].query); } @@ -148,10 +163,10 @@ void TestParse(const Test* tests, int ntests, const std::string& title) { if (!elem_ok) { diag(" FAIL: elements differ for query: %s", tests[i].query); for (auto& kv : result) { - diag(" result[%s] = %s", kv.first.c_str(), normalize_value(kv.second.empty() ? "" : kv.second[0]).c_str()); + diag(" result[%s] = [%s]", kv.first.c_str(), join_values_for_diag(kv.second).c_str()); } for (auto& kv : data) { - diag(" expected[%s] = %s", kv.first.c_str(), normalize_value(kv.second.empty() ? "" : kv.second[0]).c_str()); + diag(" expected[%s] = [%s]", kv.first.c_str(), join_values_for_diag(kv.second).c_str()); } } } @@ -169,7 +184,7 @@ void TestParsePgsql(const Test* tests, int ntests, const std::string& title) { std::map> result = parsersql_parse_set_pgsql(tests[i].query); bool size_ok = (result.size() == data.size()); - ok(size_ok, "[%s %d] Sizes match: %lu, %lu", title.c_str(), i, result.size(), data.size()); + ok(size_ok, "[%s %d] Sizes match: %zu, %zu", title.c_str(), i, result.size(), data.size()); if (!size_ok) { diag(" FAIL: sizes differ for query: %s", tests[i].query); } @@ -179,20 +194,10 @@ void TestParsePgsql(const Test* tests, int ntests, const std::string& title) { if (!elem_ok) { diag(" FAIL: elements differ for query: %s", tests[i].query); for (auto& kv : result) { - std::string joined; - for (size_t j = 0; j < kv.second.size(); j++) { - if (j) joined += " | "; - joined += kv.second[j]; - } - diag(" result[%s] = [%s]", kv.first.c_str(), joined.c_str()); + diag(" result[%s] = [%s]", kv.first.c_str(), join_values_for_diag(kv.second).c_str()); } for (auto& kv : data) { - std::string joined; - for (size_t j = 0; j < kv.second.size(); j++) { - if (j) joined += " | "; - joined += kv.second[j]; - } - diag(" expected[%s] = [%s]", kv.first.c_str(), joined.c_str()); + diag(" expected[%s] = [%s]", kv.first.c_str(), join_values_for_diag(kv.second).c_str()); } } }