feat: add Galera wsrep variables and MariaDB SET STATEMENT ... FOR support#5708
Merged
Conversation
… variable tracking Add support for two additional Galera wsrep session-level variables that can be set by clients through ProxySQL: - wsrep_trx_fragment_size (numeric): controls transaction fragmentation size - wsrep_trx_fragment_unit (string): controls fragmentation unit (BYTES/ROWS/STATEMENTS) These follow the same tracking pattern as the existing wsrep_sync_wait variable. ProxySQL intercepts SET statements for these variables, tracks them per-session, and synchronizes them to backend connections when needed. Changes: - Add SQL_WSREP_TRX_FRAGMENT_SIZE and SQL_WSREP_TRX_FRAGMENT_UNIT to mysql_variable_name enum in proxysql_structs.h - Add metadata entries in mysql_tracked_variables[] array - Add to classification sets (mysql_variables_numeric and mysql_variables_strings) in MySQL_Session.cpp - Update test infrastructure: filtered_set_statements test, set_testing-t.csv, generate_set_session_csv.cpp, and possible_unknown_variables in set_testing headers Ref: sub-issue 1 of #5686
ProxySQL now recognizes MariaDB's SET STATEMENT ... FOR syntax and forwards it to the backend without locking the hostgroup. Previously, this syntax was unrecognized and triggered unable_to_parse_set_statement(), which locked the session to a single hostgroup. This caused error 9006 on subsequent queries targeting different hostgroups. The implementation is a minimal passthrough: ProxySQL detects the "SET STATEMENT <vars> FOR <query>" pattern, forwards the query as-is to the backend, and does not lock the hostgroup. Variable tracking is not needed since SET STATEMENT variables are temporary (scoped to the single statement execution). Changes: - Add SET STATEMENT ... FOR detection in MySQL_Session.cpp before the SET parser, returning false without lock_hostgroup - Add new TAP test (test_set_statement_for-t.cpp) that verifies: - Basic SET STATEMENT ... FOR succeeds - Multi-variable SET STATEMENT ... FOR succeeds - No hostgroup lock persists after execution - Case-insensitive matching works - Register test in groups.json (legacy-g2, mysql84-g2, etc.) Ref: sub-issue 2 of #5686
Rewrite test_set_statement_for-t.cpp with: - Clear diag() output explaining what each test does and why - Explicit description of the bug being tested (hostgroup lock) - Verify the query returns the correct result, not just succeeds - 5 tests: basic, result correctness, multi-variable, no-lock, case-insensitive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two wsrep session variables to ProxySQL's tracked variables and allowlists, updates session parsing to bypass hostgroup locking for MariaDB Changeswsrep variable tracking + SET STATEMENT passthrough
Sequence DiagramsequenceDiagram
participant Client as Client
participant Proxy as ProxySQL
participant DB as MariaDB
Client->>Proxy: SET STATEMENT max_statement_time=60 FOR SELECT 1
Proxy->>Proxy: Detect "SET STATEMENT ... FOR"
Proxy->>Proxy: Skip hostgroup lock & variable parsing
Proxy->>DB: Forward query unchanged
DB-->>Proxy: Result
Proxy-->>Client: Result
Client->>Proxy: SELECT 1
Proxy->>Proxy: Normal processing (no lock)
Proxy->>DB: Forward to appropriate hostgroup
DB-->>Proxy: Result
Proxy-->>Client: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for MariaDB's SET STATEMENT ... FOR syntax, ensuring these queries are passed to the backend without hostgroup locking. Additionally, it adds tracking and comprehensive testing for the wsrep_trx_fragment_size and wsrep_trx_fragment_unit variables. The review feedback suggests removing redundant C-style casts on string literals for better C++ idiomaticity and correcting a typo in a date tag within the new test file.
| } | ||
| // detect MariaDB SET STATEMENT ... FOR syntax | ||
| // pass through to backend without hostgroup locking | ||
| if (strncasecmp(nq.c_str(), (char *)"SET STATEMENT ", 14) == 0 && strcasestr(nq.c_str(), (char *)" FOR ")) { |
There was a problem hiding this comment.
The C-style casts (char *) on string literals are unnecessary and not idiomatic C++. String literals are already const char*, which is the type expected by strncasecmp and strcasestr. Removing these casts will make the code cleaner and more correct according to modern C++ standards.
if (strncasecmp(nq.c_str(), "SET STATEMENT ", 14) == 0 && strcasestr(nq.c_str(), " FOR ")) {| * | ||
| * All tests are skipped on non-MariaDB backends. | ||
| * | ||
| * @date 2026-05-01 |
There was a problem hiding this comment.
The date in the @date tag is set to the future (2026). This appears to be a typo and should be corrected to the current date of creation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/MySQL_Session.cpp`:
- Around line 7463-7467: The current check in MySQL_Session.cpp uses exact-space
string checks on nq (the normalized query) which misses variants with
tabs/newlines or multiple spaces; update the detection logic in the block that
checks nq (the strncasecmp(... "SET STATEMENT ", 14) and strcasestr(... " FOR
")) to match whitespace more flexibly—either normalize consecutive whitespace in
nq to single spaces before testing or use a regex that matches "SET" \s+
"STATEMENT" and a later \s+ "FOR" (case-insensitive); ensure the function that
returns false for these matches (the SET STATEMENT ... FOR passthrough branch)
still executes when any whitespace variant is present.
In `@test/tap/tests/test_set_statement_for-t.cpp`:
- Around line 131-143: The current test uses mysql_query(proxysql, "SELECT 1 AS
no_lock_test") which can succeed even if the session is still pinned, so replace
the simple SELECT with a query that is guaranteed to route to a different
hostgroup (or assert the session's routing directly) to prove the hostgroup lock
was cleared; update the test around unable_to_parse_set_statement() to run a
cross-hostgroup query (one that requires the alternate hostgroup) or call the
session routing-check helper in the test harness instead of SELECT 1, and keep
using mysql_query(proxysql, ...) and the existing ok()/diag() checks to validate
success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47cdaf8e-f7f5-4f34-bd98-400b06267c73
⛔ Files ignored due to path filters (1)
test/tap/tests/set_testing-t.csvis excluded by!**/*.csv
📒 Files selected for processing (8)
include/proxysql_structs.hlib/MySQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/generate_set_session_csv.cpptest/tap/tests/set_testing-240.htest/tap/tests/set_testing.htest/tap/tests/test_filtered_set_statements-t.cpptest/tap/tests/test_set_statement_for-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/generate_set_session_csv.cpptest/tap/tests/set_testing.htest/tap/tests/set_testing-240.htest/tap/tests/test_filtered_set_statements-t.cpptest/tap/tests/test_set_statement_for-t.cppinclude/proxysql_structs.hlib/MySQL_Session.cpp
test/tap/groups/groups.json
📄 CodeRabbit inference engine (CLAUDE.md)
Register new test files in
test/tap/groups/groups.jsonfor test discovery via pattern rules intest/tap/tests/Makefile
Files:
test/tap/groups/groups.json
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/test_filtered_set_statements-t.cpptest/tap/tests/test_set_statement_for-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#ifndef __CLASS_*_Hinclude guards in header files
Files:
include/proxysql_structs.h
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in
lib/directory
Files:
lib/MySQL_Session.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/generate_set_session_csv.cpptest/tap/tests/set_testing.htest/tap/tests/set_testing-240.htest/tap/groups/groups.jsontest/tap/tests/test_filtered_set_statements-t.cpptest/tap/tests/test_set_statement_for-t.cpplib/MySQL_Session.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/generate_set_session_csv.cpptest/tap/tests/test_filtered_set_statements-t.cpptest/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness
Applied to files:
test/tap/tests/set_testing.htest/tap/tests/set_testing-240.htest/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/groups/groups.json : Register new test files in `test/tap/groups/groups.json` for test discovery via pattern rules in `test/tap/tests/Makefile`
Applied to files:
test/tap/groups/groups.json
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/test_filtered_set_statements-t.cpplib/MySQL_Session.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/*-t.cpp : Test files should follow naming pattern `test_*.cpp` or `*-t.cpp` in `test/tap/tests/`
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
🔇 Additional comments (7)
test/tap/tests/set_testing.h (1)
24-25: Looks good.These new wsrep variables are wired into the unknown-variable fallback path as expected, so the harness stays consistent with the backend tracking changes.
test/tap/groups/groups.json (1)
401-401: Looks good.The new TAP test is registered in the discovery matrix, so it will be picked up by the standard test harness.
test/tap/tests/generate_set_session_csv.cpp (1)
306-309: Looks good.The generator now covers both wsrep session variables with the expected numeric/string shapes, which keeps the generated SET corpus aligned with the new metadata.
test/tap/tests/set_testing-240.h (1)
24-25: Looks good.This keeps the 240 harness in sync with the main set-testing helper, so the new wsrep variables will be handled consistently in comparisons.
test/tap/tests/test_filtered_set_statements-t.cpp (1)
44-45: Looks good.The filtered SET-statement coverage now includes both wsrep variables, and the test plan still scales correctly with the expanded matrix.
lib/MySQL_Session.cpp (1)
133-135: wsrep tracked-variable classification looks correct.This correctly wires
wsrep_trx_fragment_sizeinto numeric handling andwsrep_trx_fragment_unitinto string handling for existing session-variable tracking/sync paths.Also applies to: 145-146
include/proxysql_structs.h (1)
253-254: Enum/table additions stay in sync.The new wsrep entries are inserted in matching order in both the enum and
mysql_tracked_variables[], so the lookup table remains consistent with the index mapping.Also applies to: 1855-1856
Comment on lines
+131
to
+143
| // Test 3: No hostgroup lock persists after SET STATEMENT | ||
| diag("Test 3: Execute a plain SELECT after SET STATEMENT"); | ||
| diag(" Prior to the fix, unable_to_parse_set_statement() would lock the"); | ||
| diag(" hostgroup, causing subsequent queries to fail with error 9006."); | ||
| { | ||
| int rc = mysql_query(proxysql, "SELECT 1 AS no_lock_test"); | ||
| ok(rc == 0, "Plain SELECT after SET STATEMENT should succeed (no hostgroup lock)"); | ||
| if (rc != 0) { | ||
| diag(" ERROR: %s (this indicates the hostgroup is still locked)", mysql_error(proxysql)); | ||
| } else { | ||
| MYSQL_RES* res = mysql_store_result(proxysql); | ||
| mysql_free_result(res); | ||
| } |
There was a problem hiding this comment.
Strengthen the post-SET STATEMENT lock check.
A plain SELECT 1 on the same session can still succeed even if the session remains pinned to the same hostgroup, so this does not actually prove the lock was cleared. Use a query that must route to a different hostgroup, or assert the routing state directly, to make the regression check meaningful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_set_statement_for-t.cpp` around lines 131 - 143, The
current test uses mysql_query(proxysql, "SELECT 1 AS no_lock_test") which can
succeed even if the session is still pinned, so replace the simple SELECT with a
query that is guaranteed to route to a different hostgroup (or assert the
session's routing directly) to prove the hostgroup lock was cleared; update the
test around unable_to_parse_set_statement() to run a cross-hostgroup query (one
that requires the alternate hostgroup) or call the session routing-check helper
in the test harness instead of SELECT 1, and keep using mysql_query(proxysql,
...) and the existing ok()/diag() checks to validate success.
Address review feedback: detect_mariadb() now returns three states (1=MariaDB, 0=not MariaDB, -1=error) so that connection failures cause the test to fail rather than silently skip.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/test_set_statement_for-t.cpp`:
- Around line 106-120: The test currently calls mysql_query(proxysql, "SET
STATEMENT...") and only emits the second ok() inside the rc==0 branch, causing
TAP plan(5) to mismatch when rc!=0; modify the failure branch (where rc != 0) to
call skip(1, "SET STATEMENT failed: %s", mysql_error(proxysql)) so the test
still emits the expected number of TAP results, keep the diagnostic diag("
ERROR: %s", mysql_error(proxysql)) and do not remove the existing ok() calls in
the success path (the block using mysql_store_result, mysql_fetch_row,
mysql_num_rows and the ok comparing row[0] to "1").
- Around line 89-101: The test calls plan(5) before establishing the ProxySQL
connection so if mysql_real_connect(proxysql, ...) fails the TAP output has a
plan but no ok() results; move the connection setup (the MYSQL* proxysql =
mysql_init(NULL); and the mysql_real_connect(...) block) to occur before
plan(5), or alternatively replace the early return -1 with a bail_out(...) call;
update the code around plan(), proxysql, and mysql_real_connect to ensure plan()
is only emitted after a successful connection (or use bail_out on failure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86d475ca-913a-46b7-9fa6-e85d83cf7a02
📒 Files selected for processing (1)
test/tap/tests/test_set_statement_for-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/test_set_statement_for-t.cpp
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/test_set_statement_for-t.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Use feature flags `PROXYSQL31=1` for v3.1.x Innovative tier and `PROXYSQLGENAI=1` for v4.0.x AI/MCP tier; PROXYSQLGENAI implies both PROXYSQL31 and PROXYSQLFFTO/PROXYSQLTSDB
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
📚 Learning: 2026-02-13T09:29:39.713Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.
Applied to files:
test/tap/tests/test_set_statement_for-t.cpp
🔇 Additional comments (1)
test/tap/tests/test_set_statement_for-t.cpp (1)
141-154: Weak lock-check assertion (carries over from previous review).
SELECT 1 AS no_lock_testsucceeds regardless of which hostgroup the session is pinned to, so it cannot distinguish "lock was cleared" from "lock remains on the same hostgroup." A cross-hostgroup query or a direct inspection of session routing state would make this regression check meaningful.
Comment on lines
+89
to
+101
| plan(5); | ||
|
|
||
| diag("=== Test: MariaDB SET STATEMENT ... FOR passthrough ==="); | ||
| diag("Issue #5686: ProxySQL should forward SET STATEMENT ... FOR"); | ||
| diag("to the backend without locking the hostgroup."); | ||
| diag(""); | ||
|
|
||
| // Connect to ProxySQL | ||
| MYSQL* proxysql = mysql_init(NULL); | ||
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
plan(5) is emitted before the ProxySQL connection is established.
If mysql_real_connect fails at line 98, plan(5) has already been output but zero ok() calls follow. The TAP harness will report 0 tests run against a plan of 5. Either establish the connection before calling plan(), or call bail_out() instead of return -1 on connection failure.
🐛 Proposed fix (establish connection before plan)
+ // Connect to ProxySQL
+ MYSQL* proxysql = mysql_init(NULL);
+ if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
+ fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql));
+ return -1;
+ }
+
plan(5);
diag("=== Test: MariaDB SET STATEMENT ... FOR passthrough ===");
...
-
- // Connect to ProxySQL
- MYSQL* proxysql = mysql_init(NULL);
- if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
- fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql));
- return -1;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| plan(5); | |
| diag("=== Test: MariaDB SET STATEMENT ... FOR passthrough ==="); | |
| diag("Issue #5686: ProxySQL should forward SET STATEMENT ... FOR"); | |
| diag("to the backend without locking the hostgroup."); | |
| diag(""); | |
| // Connect to ProxySQL | |
| MYSQL* proxysql = mysql_init(NULL); | |
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| return -1; | |
| } | |
| // Connect to ProxySQL | |
| MYSQL* proxysql = mysql_init(NULL); | |
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| return -1; | |
| } | |
| plan(5); | |
| diag("=== Test: MariaDB SET STATEMENT ... FOR passthrough ==="); | |
| diag("Issue `#5686`: ProxySQL should forward SET STATEMENT ... FOR"); | |
| diag("to the backend without locking the hostgroup."); | |
| diag(""); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_set_statement_for-t.cpp` around lines 89 - 101, The test
calls plan(5) before establishing the ProxySQL connection so if
mysql_real_connect(proxysql, ...) fails the TAP output has a plan but no ok()
results; move the connection setup (the MYSQL* proxysql = mysql_init(NULL); and
the mysql_real_connect(...) block) to occur before plan(5), or alternatively
replace the early return -1 with a bail_out(...) call; update the code around
plan(), proxysql, and mysql_real_connect to ensure plan() is only emitted after
a successful connection (or use bail_out on failure).
Comment on lines
+106
to
+120
| { | ||
| int rc = mysql_query(proxysql, "SET STATEMENT max_statement_time=60 FOR SELECT 1 AS val"); | ||
| ok(rc == 0, "SET STATEMENT max_statement_time=60 FOR SELECT 1 should succeed"); | ||
| if (rc != 0) { | ||
| diag(" ERROR: %s", mysql_error(proxysql)); | ||
| } else { | ||
| MYSQL_RES* res = mysql_store_result(proxysql); | ||
| MYSQL_ROW row = mysql_fetch_row(res); | ||
| int rows = mysql_num_rows(res); | ||
| diag(" Result: rows=%d, val=%s", rows, row ? row[0] : "NULL"); | ||
| ok(rows == 1 && row && strcmp(row[0], "1") == 0, | ||
| "SET STATEMENT ... FOR should return the result from the inner SELECT"); | ||
| mysql_free_result(res); | ||
| } | ||
| } |
There was a problem hiding this comment.
TAP count mismatch when Test 1 fails: plan(5) but only 4 ok() calls are emitted.
The second assertion (line 116) is inside the else branch that only runs when rc == 0. If the SET STATEMENT query fails, only 4 ok() calls are made against plan(5), which the TAP harness will report as a bad test count — potentially masking the real failure.
Use skip(1, ...) in the failure path to keep the emitted count at exactly 5.
🐛 Proposed fix
int rc = mysql_query(proxysql, "SET STATEMENT max_statement_time=60 FOR SELECT 1 AS val");
ok(rc == 0, "SET STATEMENT max_statement_time=60 FOR SELECT 1 should succeed");
if (rc != 0) {
diag(" ERROR: %s", mysql_error(proxysql));
+ skip(1, "Skipping result check because query failed");
} else {
MYSQL_RES* res = mysql_store_result(proxysql);
MYSQL_ROW row = mysql_fetch_row(res);
int rows = mysql_num_rows(res);
diag(" Result: rows=%d, val=%s", rows, row ? row[0] : "NULL");
ok(rows == 1 && row && strcmp(row[0], "1") == 0,
"SET STATEMENT ... FOR should return the result from the inner SELECT");
mysql_free_result(res);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_set_statement_for-t.cpp` around lines 106 - 120, The test
currently calls mysql_query(proxysql, "SET STATEMENT...") and only emits the
second ok() inside the rc==0 branch, causing TAP plan(5) to mismatch when rc!=0;
modify the failure branch (where rc != 0) to call skip(1, "SET STATEMENT failed:
%s", mysql_error(proxysql)) so the test still emits the expected number of TAP
results, keep the diagnostic diag(" ERROR: %s", mysql_error(proxysql)) and do
not remove the existing ok() calls in the success path (the block using
mysql_store_result, mysql_fetch_row, mysql_num_rows and the ok comparing row[0]
to "1").
Resolve groups.json conflict: keep test_set_statement_for-t from PR and set_parser_algorithm=3-g1 additions from v3.0.
…d set_parser_algorithm_3
…ossible_unknown_variables The set_testing tests generate multi-variable SET statements that mix Galera/Group-Replication-only variables (wsrep_trx_fragment_size, wsrep_trx_fragment_unit, group_replication_consistency) with standard MySQL variables. When sent to a non-Galera/non-GR backend, the entire SET fails because MySQL rejects the unknown variable. Previously only wsrep_sync_wait had an exception: if MySQL doesnt have it but ProxySQL correctly tracked the value, the test passes. Extend this exception to all variables in possible_unknown_variables for set_testing-t, set_testing-multi-t, and set_testing-240-t.
renecannao
force-pushed
the
issue-5686-galera-vars
branch
from
May 5, 2026 02:39
5bd04b1 to
e212998
Compare
- Guard with is_cluster in set_testing-t and set_testing-multi-t (same pattern as wsrep_sync_wait) - Set mix=false in generate_set_session_csv so they only appear in single-variable SETs (avoids corrupting connection state when backend rejects them) - Restore CSV entries that were incorrectly removed
renecannao
force-pushed
the
issue-5686-galera-vars
branch
from
May 5, 2026 03:09
e212998 to
51b279c
Compare
…on backend When ProxySQL tries to SET a tracked variable on a backend that has the variable but rejects the value (error 1231: Variable can't be set to the value), it now marks the variable as absent on that connection and continues instead of destroying the connection. This matches existing handling for errors 1193 (unknown variable), 1064 (syntax error), and 1651 (query cache disabled). Fixes set_testing failures on MariaDB 10.11 with wsrep_provider=none, where wsrep_trx_fragment_size exists but all SET attempts fail with error 1231.
- CI base image can be pulled from GHCR instead of built locally - Build commands should use debug mode: make debug + build_tap_test_debug
set_testing-240-t was missing the is_cluster detection and wsrep variable guards that set_testing-t and set_testing-multi-t already had. This caused failures on MariaDB 10.11 backends with wsrep_provider=none where wsrep_trx_fragment_size exists but can't be set.
…n backend ProxySQL now gracefully handles error 1210 (Incorrect arguments to SET) in addition to the previously handled 1231 (Variable can't be set to the value). Both errors occur when a tracked variable is SET on a backend that has the variable but rejects the value (e.g. wsrep_trx_fragment_size on MariaDB 10.11 with wsrep_provider=none). Instead of destroying the backend connection, ProxySQL marks the variable as absent on that connection and continues. Also fix test comparison logic: when the backend returns UNKNOWNVAR for a variable (backend doesn't have it) but ProxySQL's tracked value matches the expected CSV value, count it as a pass. This prevents false failures when test connections are routed to heterogeneous backends.
|
This was referenced May 12, 2026
renecannao
added a commit
that referenced
this pull request
May 23, 2026
fix(session): SET STATEMENT ... FOR detection tolerates any whitespace (PR #5708 follow-up)
4 tasks
tabacco
pushed a commit
to tabacco/proxysql
that referenced
this pull request
May 26, 2026
PR sysown#5708 added detection for MariaDB's `SET STATEMENT … FOR …` syntax to keep ProxySQL from invoking `unable_to_parse_set_statement()` and locking the session to a single hostgroup. The detection at `lib/MySQL_Session.cpp` was: if (strncasecmp(nq.c_str(), "SET STATEMENT ", 14) == 0 && strcasestr(nq.c_str(), " FOR ")) { return false; } `strcasestr(nq, " FOR ")` is a literal-substring search requiring a space BOTH before and after the FOR keyword. A customer report (see PR sysown#5708 thread) reproduced the same error 9006 as before the fix by typing the statement across multiple lines at the mysql client prompt — the character after FOR is then a newline, not a space, so the detection misses, the code falls through to the regular SET parser, the parser doesn't know SET STATEMENT, and the hostgroup is locked to the user's default. The next non-default-hostgroup query fails with 9006. Verified with a standalone strcasestr probe: q1 (customer multi-line): strcasestr(" FOR ") matches ? NO q2 (existing TAP single-line): YES The fix uses `dig` (the digest text) for the detection instead of `nq` (the raw query). `dig` is whitespace-normalised by the digest builder, so " FOR " always has a space on both sides regardless of how the client formatted the query. This is consistent with the prefix check 35 lines above, which already uses `dig` for keyword detection. == Test coverage == `test/tap/tests/test_set_statement_for-t.cpp` previously had `plan(5)` and self-skipped on non-MariaDB backends via `skip_all()`. It was also registered ONLY in MySQL-family TAP groups (`legacy-g2`, `mysql84-g2`, etc.). The net effect was that the test self-skipped on every CI run since it was added — never actually executing an assertion in CI. The customer's bug regressed entirely unnoticed. This commit: 1. Removes the runtime MariaDB-detection helper and the `skip_all` call. A misregistered test should fail loudly, not silently skip; correct registration is enforced through the groups.json layer instead. 2. Moves the groups.json registration from the MySQL-only groups to `mariadb10-galera-g2`, which actually provides a MariaDB backend. 3. Adds four new test cases (assertions 6–13) covering the customer whitespace shapes that strcasestr (" FOR ") missed: newline, tab, CRLF, and multiple spaces between FOR and the inner statement. Each case verifies (a) the SET STATEMENT succeeds, and (b) a follow-up plain SELECT succeeds (i.e. no hostgroup lock persists — the customer's actual failure mode). Verified locally on TAP group mariadb10-galera-g2 against MariaDB 10.11.9: 13/13 ok, RC=0. A separate framework-cleanup follow-up should remove `skip_all()` from `test/tap/tap/tap.{h,cpp}` entirely and convert the other four callers (`test_mysqlx_e2e_*`, `test_mysqlx_route_drop_inflight-t`, `test_mysqlx_sigterm_inflight-t`) to either fail loudly or move to appropriately-gated TAP groups.
tabacco
pushed a commit
to tabacco/proxysql
that referenced
this pull request
May 26, 2026
…ions Stage 2 follow-up to the PR sysown#5708 reproduction debug. The framework helper test/tap/tap/tap.{h,cpp}::skip_all() prints "1..0 # skip ..." and exit(0), which the harness reports as "ignored" — zero assertions emitted, zero failures recorded, no notice that a test never ran. While investigating the customer regression on PR sysown#5708 we discovered that test_set_statement_for-t had been silently skipping in CI since the day it was added: it self-skipped on non-MariaDB backends, but was registered only in MySQL-family TAP groups, so every run skipped. PR sysown#5794 fixes that one test. The principled fix is to remove the mechanism that lets a test silently disappear. This commit converts the remaining four skip_all() callers and deletes the framework function: - test_mysqlx_e2e_routing-t BAIL_OUT if MYSQLX_E2E_PROXYSQL_PORT is not set. The group env file test/tap/groups/mysqlx-e2e/env.sh sets it; missing at runtime means the harness did not source it. - test_mysqlx_e2e_handshake-t BAIL_OUT if MYSQLX_E2E_HOST is not set (same env file, same rationale). - test_mysqlx_route_drop_inflight-t BAIL_OUT if the X-Protocol listener is not reachable. The mysqlx-soak-g1 harness is supposed to start ProxySQL with the mysqlx plugin and provision the route; a missing listener is a setup bug. - test_mysqlx_sigterm_inflight-t Deleted. The binary was a stub whose body was a TODO; the real sigterm scenario is implemented in test/scripts/mysqlx/behavioral_validation.py --scenario sigterm. With skip_all gone the stub had no remaining purpose. Also dropped from groups.json and the Makefile. After this commit, the test/tap codebase has zero call sites of skip_all(). The framework declaration and definition are deleted from tap.h and tap.cpp; the doc references at the bottom of tap.cpp are updated; a comment explains the rationale and points at BAIL_OUT() as the replacement. Verified locally: full TAP debug build succeeds, no undefined-reference errors, lint_groups_json + check_groups --source both green. Depends on PR sysown#5794 (which removed the fifth caller in test_set_statement_for-t.cpp). This PR is based on that branch; merging in order keeps each merge a clean compile.
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
1 participant
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
Closes #5686
This PR addresses the two feature requests from issue #5686 as separate, independent commits:
Add
wsrep_trx_fragment_sizeandwsrep_trx_fragment_unitas tracked session variables — ProxySQL now interceptsSETstatements for these two Galera variables, tracks them per-session, and synchronizes them to backend connections, following the same pattern as the existingwsrep_sync_waitsupport.Support MariaDB
SET STATEMENT ... FORpassthrough — ProxySQL now recognizes MariaDB'sSET STATEMENT var=val [, ...] FOR statementsyntax and forwards it to the backend without locking the hostgroup. Previously, this syntax was unrecognized, triggeringunable_to_parse_set_statement()which locked the session to a single hostgroup, causing error 9006 on subsequent queries targeting different hostgroups.Changes
Commit 1:
a5714d56f— wsrep variable trackingCore changes (follows the
wsrep_sync_waitpattern exactly):include/proxysql_structs.hSQL_WSREP_TRX_FRAGMENT_SIZEandSQL_WSREP_TRX_FRAGMENT_UNITtomysql_variable_nameenum; add two entries tomysql_tracked_variables[]metadata arraylib/MySQL_Session.cppwsrep_trx_fragment_sizetomysql_variables_numericset; addwsrep_trx_fragment_unittomysql_variables_stringssetNo other code changes needed — the existing variable tracking infrastructure handles regex matching, SET parsing, hash tracking, backend synchronization, pool return, and
var_absenthandling automatically based on the enum/metadata registration.wsrep_trx_fragment_size: numeric variable (quote=false,is_number=true, default"0")wsrep_trx_fragment_unit: string variable (quote=true,is_number=false, default""). No client-side validation of allowed values (BYTES/ROWS/STATEMENTS) — the backend handles validation, consistent with howwsrep_osu_methodhandlesTOI/RSU.Test changes:
test/tap/tests/test_filtered_set_statements-t.cppfiltered_set_queriestest/tap/tests/set_testing-t.csvtest/tap/tests/generate_set_session_csv.cpptest/tap/tests/set_testing.hpossible_unknown_variablestest/tap/tests/set_testing-240.hpossible_unknown_variablesCommit 2:
0c832e5ce+c4666ae8f— SET STATEMENT ... FOR passthroughCore change — 5-line detection in
lib/MySQL_Session.cppinserted before the SET parser (after query cleanup, beforematch_regexescheck):This returns
false(forward to backend) with*lock_hostgroupunchanged (defaultfalse), so the session remains free to route to any hostgroup for subsequent queries.New TAP test —
test/tap/tests/test_set_statement_for-t.cpp:groups.json(legacy-g2, mysql84-g2, mysql90-g2, mysql95-g2)How it works
wsrep variable tracking (automatic infrastructure)
When a client sends
SET wsrep_trx_fragment_size=1000:variables_regexp(auto-generated frommysql_tracked_variables[])client_set_value()OKpacket is returned to the client immediately (no backend traffic)handler_again___verify_multiple_variables()compares client/server hashes and sendsSET wsrep_trx_fragment_size=1000to the backend if they differSET STATEMENT ... FOR (passthrough)
When a client sends
SET STATEMENT max_statement_time=60 FOR SELECT 1:SET STATEMENT ... FORpattern before the SET parserSET STATEMENTvariables are temporary (scoped to the single statement)Test plan
make clean && make— verify compilation (enum ordering assertion inMySQL_Variables.cppvalidates at startup)make build_tap_tests && WORKSPACE=$(pwd) INFRA_ID=dev-$USER TAP_GROUP=mysql84-g2 test/infra/control/run-tests-isolated.bashmake build_tap_tests && WORKSPACE=$(pwd) INFRA_ID=dev-$USER TAP_GROUP=legacy-g2 test/infra/control/run-tests-isolated.bashSET wsrep_trx_fragment_size=1000; SELECT @@wsrep_trx_fragment_size;on a Galera clusterSET STATEMENT max_statement_time=60 FOR SELECT 1;followed bySELECT 1;against a different hostgroup — no error 9006Summary by CodeRabbit
New Features
Tests