aherman
/
proxysql
mirror of https://github.com/sysown/proxysql
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(pgsql,test): PG multi-value SET dispatch + test-fixture cleanup

Browse Source 
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<std::string>` 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).
fix/parsersql-1.0.3-pg-set-fixes
Rene Cannao 1 month ago
parent e668a9ddac
commit 4eb52abf2c
2 changed files with 37 additions and 21 deletions
Show Stats Download Patch File Download Diff File

13
lib/PgSQL_Session.cpp
Unescape View File

@ -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()) {

45
test/tap/tests/setparser_parsersql_test.cpp
Unescape View File

@ -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 ... <unit> 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<std::string>& 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<std::string, std::vector<std::string>> data;
@ -138,7 +153,7 @@ void TestParse(const Test* tests, int ntests, const std::string& title) {
std::map<std::string, std::vector<std::string>> 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<std::string, std::vector<std::string>> 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());
}
}
}

Write Preview
Loading…
Cancel
Save