feat: add mysql-zstd_compression_level variable#5637
Conversation
|
Caution Review failedPull request was closed or merged during review 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 a new integer configuration Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin/User
participant Config as Config Registry
participant Thread as Thread Handler
participant Protocol as Protocol Layer
participant Stream as Data Stream
Admin->>Config: SET mysql-zstd_compression_level = N
Config->>Config: Validate N ∈ [1, ZSTD_maxCLevel()]
Admin->>Config: LOAD MYSQL VARIABLES TO RUNTIME
Config->>Thread: Update mysql_thread___zstd_compression_level
Thread->>Thread: Cache per-thread zstd level
Protocol->>Thread: Request zstd level for connection
Thread-->>Protocol: Return cached mysql_thread___zstd_compression_level
Protocol->>Stream: Initialize compressor with level
Stream->>Stream: get_zstd_compression_level() -> clamp to [1, ZSTD_maxCLevel()]
Stream-->>Protocol: Use effective compression level for data
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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 |
![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 a dedicated mysql-zstd_compression_level variable to ProxySQL, allowing zstd compression levels (1–22) to be configured independently from zlib. The changes include the necessary thread-local variable declarations, registration within the MySQL thread system, and updates to the protocol handshake and data stream logic to utilize the new setting. Additionally, comprehensive TAP tests and design documentation have been added. Feedback on the code changes identifies a potential crash in the test suite when handling null connection pointers, redundant clamping logic in the protocol implementation, and a suggestion to avoid the MYSQL_QUERY_T macro when testing for rejected configuration values to prevent premature test failure.
| // Connect to admin | ||
| MYSQL* admin = init_mysql_conn(cl.host, cl.admin_port, cl.admin_username, cl.admin_password); | ||
| if (!admin) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); |
There was a problem hiding this comment.
Calling mysql_error(admin) when admin is NULL results in undefined behavior and will likely cause a crash. Since this block is only entered if admin is NULL, you should use a generic error message or check the connection failure differently.
| (vars1.zstd_compression_level > 0 && vars1.zstd_compression_level <= ZSTD_maxCLevel()) | ||
| ? vars1.zstd_compression_level | ||
| : static_cast<uint8_t>(std::min<int>(ZSTD_maxCLevel(), std::max<int>(1, mysql_thread___protocol_compression_level))); | ||
| : static_cast<uint8_t>(std::min<int>(ZSTD_maxCLevel(), std::max<int>(1, mysql_thread___zstd_compression_level))); |
There was a problem hiding this comment.
The clamping logic using std::min and std::max is redundant here. The variable mysql_thread___zstd_compression_level is already constrained to the range [1, ZSTD_maxCLevel()] by the admin interface registration in VariablesPointers_int (see lib/MySQL_Thread.cpp).
: static_cast<uint8_t>(mysql_thread___zstd_compression_level);| } | ||
|
|
||
| // Test 5: Reject value 0 | ||
| { |
There was a problem hiding this comment.
If MYSQL_QUERY_T is defined to assert success (which is common in this test suite), these tests for rejection will fail or cause the test process to exit prematurely because SET mysql-zstd_compression_level=0 is expected to return an error from the server. To test rejection, you should explicitly check that mysql_query returns a non-zero value instead of using the MYSQL_QUERY_T macro.
![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
🧹 Nitpick comments (1)
.gitignore (1)
219-219: Remove duplicate.worktrees/ignore entry.Line 219 duplicates the existing rule at Line 5, so this can be dropped to keep
.gitignoreclean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 219, Remove the duplicate ".worktrees/" entry from .gitignore by deleting the redundant ".worktrees/" ignore rule (leave the original single occurrence intact) so there is only one .worktrees/ entry in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-16-zstd-compression-level.md`:
- Line 16: The task headings like "### Task 1: Add thread-local variable and
struct member" jump directly from the document H1 and violate markdown heading
increment rules; change these "### Task ..." headings to "## Task ..." (or
insert an intervening H2 section before the task group) so the sequence
increments correctly; apply the same fix to the other task headings mentioned
(the repeated "### Task ..." occurrences) to satisfy MD001/heading-increment.
In `@test/tap/tests/mysql-zstd_compression_level-t.cpp`:
- Around line 73-95: The test currently hardcodes 22/23 and uses MYSQL_QUERY_T
which aborts on SQL errors; instead query ProxySQL at runtime for the true upper
bound (use a SHOW VARIABLES LIKE 'mysql-zstd_compression_level' or set a
known-valid value then probe upward) and replace the hardcoded constants with
that derived max (matching ZSTD_maxCLevel()). For the rejection cases
(previously Tests 5–6 using MYSQL_QUERY_T), call mysql_query() directly and
assert it returns non-zero to prove the SET failed, then LOAD MYSQL VARIABLES TO
RUNTIME and verify get_variable_value_int(admin, "mysql-zstd_compression_level",
true) still equals the known-valid value; update references to MYSQL_QUERY_T,
mysql_query, get_variable_value_int and any probing logic to implement this
behavior.
- Around line 130-144: The test currently calls init_mysql_conn(..., false,
true) which only enables generic MYSQL_OPT_COMPRESS so the client/server may
negotiate zlib instead of zstd; update the test to either force zstd-only
negotiation or assert the negotiated algorithm before passing: after
establishing proxy_cmp, query connection info (or use client capability flags)
to confirm CLIENT_ZSTD_COMPRESSION_ALGORITHM was negotiated (or set the client
to advertise only zstd capabilities), and only then run the SELECT/ok assertion
that mysql-zstd_compression_level=3 is exercised; reference init_mysql_conn,
MYSQL_OPT_COMPRESS, CLIENT_COMPRESS, CLIENT_ZSTD_COMPRESSION_ALGORITHM,
mysql-zstd_compression_level and proxy_cmp when making the change.
---
Nitpick comments:
In @.gitignore:
- Line 219: Remove the duplicate ".worktrees/" entry from .gitignore by deleting
the redundant ".worktrees/" ignore rule (leave the original single occurrence
intact) so there is only one .worktrees/ entry in the file.
🪄 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: dedab92e-9170-4c78-a3ba-ce7cb2502255
📒 Files selected for processing (11)
.gitignoredocs/superpowers/plans/2026-04-16-zstd-compression-level.mddocs/superpowers/specs/2026-04-16-zstd-compression-level-design.mdinclude/MySQL_Thread.hinclude/proxysql_structs.hlib/MySQL_Protocol.cpplib/MySQL_Thread.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/mysql-zstd_compression_level-t.cpptest/tap/tests/reg_test_2793-compression-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
include/MySQL_Thread.hlib/mysql_data_stream.cppinclude/proxysql_structs.hlib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cpplib/MySQL_Protocol.cpptest/tap/tests/reg_test_2793-compression-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards use the pattern
#ifndef __CLASS_*_H(e.g.,#ifndef __ADMIN_H__,#ifndef __PROXYSQL_PROTOCOL_H__)
Files:
include/MySQL_Thread.hinclude/proxysql_structs.h
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention in lib/ directory
Files:
lib/mysql_data_stream.cpplib/MySQL_Thread.cpplib/MySQL_Protocol.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/mysql-zstd_compression_level-t.cpptest/tap/tests/reg_test_2793-compression-t.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Applied to files:
.gitignoretest/tap/tests/mysql-zstd_compression_level-t.cpp
📚 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:
.gitignoretest/tap/groups/groups.jsonlib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cppdocs/superpowers/plans/2026-04-16-zstd-compression-level.mdtest/tap/tests/reg_test_2793-compression-t.cpp
📚 Learning: 2026-03-26T16:38:58.553Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:38:58.553Z
Learning: In `lib/mysql_connection.cpp`, when reviewing `SHOW WARNINGS` handling, treat the digest source as an intentional design choice: both `update_warning_count_from_connection()` and the `add_eof()` call under `ASYNC_USE_RESULT_CONT` detect warnings using `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This is expected to fail/behave differently when `mysql-query_digests_keep_comment=1` (digest_text includes comments) or when `mysql-query_digests=0` (digest_text unavailable). Do not require a change unless the regression test coverage is expanded (noting `reg_test_5306-show_warnings_with_comment-t` explicitly excludes these configurations as an accepted limitation).
Applied to files:
.gitignore
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
include/proxysql_structs.htest/tap/groups/groups.jsonlib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cpplib/MySQL_Protocol.cppdocs/superpowers/plans/2026-04-16-zstd-compression-level.mdtest/tap/tests/reg_test_2793-compression-t.cppdocs/superpowers/specs/2026-04-16-zstd-compression-level-design.md
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
include/proxysql_structs.hlib/MySQL_Thread.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/groups/groups.json
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/mysql-zstd_compression_level-t.cpptest/tap/tests/reg_test_2793-compression-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/mysql-zstd_compression_level-t.cpptest/tap/tests/reg_test_2793-compression-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
lib/MySQL_Protocol.cpp
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-04-16-zstd-compression-level.mddocs/superpowers/specs/2026-04-16-zstd-compression-level-design.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-04-16-zstd-compression-level.mddocs/superpowers/specs/2026-04-16-zstd-compression-level-design.md
🪛 Clang (14.0.6)
test/tap/tests/mysql-zstd_compression_level-t.cpp
[error] 12-12: 'cstdio' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
lib/mysql_data_stream.cpp
[warning] 32-32: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 LanguageTool
docs/superpowers/specs/2026-04-16-zstd-compression-level-design.md
[style] ~133-~133: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...in mysql_servers / pgsql_servers. - No per-server or per-user zstd level confi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.0)
docs/superpowers/plans/2026-04-16-zstd-compression-level.md
[warning] 16-16: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
docs/superpowers/specs/2026-04-16-zstd-compression-level-design.md
[warning] 16-16: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (9)
.gitignore (1)
214-215: Good ignore rule for unit test binaries.This addition is useful and aligns with keeping local test artifacts out of version control.
lib/mysql_data_stream.cpp (1)
32-33: Correct fallback switch to dedicated zstd thread variable.The fallback now correctly uses
mysql_thread___zstd_compression_levelwhile preserving range validation.include/MySQL_Thread.h (1)
491-491:zstd_compression_levelfield addition looks correct.This cleanly extends thread variables for separate zstd-level management.
lib/MySQL_Protocol.cpp (1)
2303-2303: Handshake fallback now correctly references the zstd-specific setting.This keeps zstd negotiation independent from zlib configuration as intended.
test/tap/tests/reg_test_2793-compression-t.cpp (1)
37-52: Nice coverage addition for admin visibility/default value.The new check is practical and complements the compression behavior assertions well.
include/proxysql_structs.h (2)
1299-1299: Looks good: new TLS zstd variable is declared consistently.The added
mysql_thread___zstd_compression_leveldefinition matches existing thread-local variable patterns and naming conventions.
1633-1633: Extern declaration is correctly mirrored.Good parity with the
PROXYSQL_EXTERNdefinition branch formysql_thread___zstd_compression_level.test/tap/groups/groups.json (1)
90-90: Test group registration looks correct.The new
mysql-zstd_compression_level-tentry is wired with the expected MySQL TAP group variants.test/tap/tests/mysql-zstd_compression_level-t.cpp (1)
47-50:⚠️ Potential issue | 🟠 MajorAvoid calling
mysql_error()on a null admin handle.When
init_mysql_conn()fails,adminis null here, so the error-reporting path can crash before the test reports the actual setup failure.Suggested fix
MYSQL* admin = init_mysql_conn(cl.host, cl.admin_port, cl.admin_username, cl.admin_password); if (!admin) { - fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + fprintf(stderr, "File %s, line %d, Error: failed to connect to admin\n", __FILE__, __LINE__); return EXIT_FAILURE; }⛔ Skipped due to learnings
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).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:27.165Z Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.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.Learnt from: CR Repo: sysown/proxysql PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-12T11:27:41.156Z Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
|
|
||
| --- | ||
|
|
||
| ### Task 1: Add thread-local variable and struct member |
There was a problem hiding this comment.
Fix the heading-level jump in the task sections.
These ### Task ... headings jump straight from the file’s H1, which trips MD001/heading-increment. Make the task headings ## or add an intervening H2 section so markdownlint passes cleanly.
Also applies to: 63-63, 145-145, 224-224, 408-408, 440-440, 548-548
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 16-16: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-16-zstd-compression-level.md` at line 16, The
task headings like "### Task 1: Add thread-local variable and struct member"
jump directly from the document H1 and violate markdown heading increment rules;
change these "### Task ..." headings to "## Task ..." (or insert an intervening
H2 section before the task group) so the sequence increments correctly; apply
the same fix to the other task headings mentioned (the repeated "### Task ..."
occurrences) to satisfy MD001/heading-increment.
2a47755 to
b3b2056
Compare
Replace mysql_thread___protocol_compression_level with mysql_thread___zstd_compression_level in both the handshake (MySQL_Protocol.cpp) and data stream (mysql_data_stream.cpp) zstd level selection paths, so the two compression algorithms can be configured independently.
- New test mysql-zstd_compression_level-t.cpp: validates default value, range enforcement (1-22), rejection of 0 and 23, independence from mysql-protocol_compression_level, and functional compressed connection. - Register mysql-zstd_compression_level-t in groups.json (same groups as mysql-protocol_compression_level-t). - Add variable visibility check to reg_test_2793-compression-t.cpp. - Add unit test binary pattern to .gitignore.
610bf03 to
4631b1f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/tap/tests/mysql-zstd_compression_level-t.cpp (2)
73-79:⚠️ Potential issue | 🟠 MajorBoundary and rejection checks are still brittle and can mask the invalid-path behavior.
Line 75/78 and Line 92/95 hardcode
22/23, while runtime code usesZSTD_maxCLevel(). Also, Line 83 and Line 92 useMYSQL_QUERY_Tfor expected-fail statements, so failure behavior isn’t asserted explicitly.Suggested test hardening
+#include <zstd.h> ... - // Test 4: Set to max (22) and load to runtime + const int zstd_max = ZSTD_maxCLevel(); + // Test 4: Set to max and load to runtime { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=22"); + const string q = "SET mysql-zstd_compression_level=" + std::to_string(zstd_max); + MYSQL_QUERY_T(admin, q.c_str()); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Runtime value after SET=22 is 22, got: %d", rt_val); + ok(rt_val == zstd_max, "Runtime value after max SET is %d, got: %d", zstd_max, rt_val); } // Test 5: Reject value 0 { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=0"); + int rc = mysql_query(admin, "SET mysql-zstd_compression_level=0"); + ok(rc != 0, "SET mysql-zstd_compression_level=0 is rejected"); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); - // The value should NOT have changed from 22 (previous test) int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Value 0 rejected, still 22, got: %d", rt_val); + ok(rt_val == zstd_max, "Value 0 rejected, still %d, got: %d", zstd_max, rt_val); } - // Test 6: Reject value 23 (above max) + // Test 6: Reject value above max { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=23"); + const string q = "SET mysql-zstd_compression_level=" + std::to_string(zstd_max + 1); + int rc = mysql_query(admin, q.c_str()); + ok(rc != 0, "SET mysql-zstd_compression_level=max+1 is rejected"); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Value 23 rejected, still 22, got: %d", rt_val); + ok(rt_val == zstd_max, "Value above max rejected, still %d, got: %d", zstd_max, rt_val); }Also applies to: 81-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-zstd_compression_level-t.cpp` around lines 73 - 79, The test hardcodes compression bounds (22/23) and uses MYSQL_QUERY_T for operations that should fail; replace magic numbers with the runtime max by calling ZSTD_maxCLevel() to compute valid_max and invalid_max = valid_max + 1, use those when setting and asserting mysql-zstd_compression_level, and for expected-failure statements switch from MYSQL_QUERY_T to the explicit failure-check helper (or assert the query returns an error) so the test asserts failure behavior (refer to mysql-zstd_compression_level, ZSTD_maxCLevel(), MYSQL_QUERY_T, and get_variable_value_int to locate and update the assertions).
130-144:⚠️ Potential issue | 🟠 MajorThe functional check does not prove the zstd path is exercised.
Line 139 only validates that a compressed query works. This can still pass without demonstrating that zstd (and thus
mysql-zstd_compression_level) was the negotiated path.Please assert the negotiated compression algorithm is
zstdbefore counting this case as coverage of the new variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-zstd_compression_level-t.cpp` around lines 130 - 144, The test currently only checks that a compressed query succeeds but doesn't verify zstd was actually negotiated; after creating the compressed connection via init_mysql_conn and before closing proxy_cmp, run a follow-up query that returns the negotiated compression algorithm (for example with a session/status variable or a proxy-provided diagnostic query) and assert its value equals "zstd" (use the same mysql_query/mysql_result path you use for the "SELECT 1" check); update the block that uses MYSQL_QUERY_T, init_mysql_conn and proxy_cmp to include this additional assertion so the test proves the zstd path was exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/mysql-zstd_compression_level-t.cpp`:
- Around line 73-79: The test hardcodes compression bounds (22/23) and uses
MYSQL_QUERY_T for operations that should fail; replace magic numbers with the
runtime max by calling ZSTD_maxCLevel() to compute valid_max and invalid_max =
valid_max + 1, use those when setting and asserting
mysql-zstd_compression_level, and for expected-failure statements switch from
MYSQL_QUERY_T to the explicit failure-check helper (or assert the query returns
an error) so the test asserts failure behavior (refer to
mysql-zstd_compression_level, ZSTD_maxCLevel(), MYSQL_QUERY_T, and
get_variable_value_int to locate and update the assertions).
- Around line 130-144: The test currently only checks that a compressed query
succeeds but doesn't verify zstd was actually negotiated; after creating the
compressed connection via init_mysql_conn and before closing proxy_cmp, run a
follow-up query that returns the negotiated compression algorithm (for example
with a session/status variable or a proxy-provided diagnostic query) and assert
its value equals "zstd" (use the same mysql_query/mysql_result path you use for
the "SELECT 1" check); update the block that uses MYSQL_QUERY_T, init_mysql_conn
and proxy_cmp to include this additional assertion so the test proves the zstd
path was exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f471f7-0f6d-4643-a3aa-a178e302ee3d
📒 Files selected for processing (9)
.gitignoreinclude/MySQL_Thread.hinclude/proxysql_structs.hlib/MySQL_Protocol.cpplib/MySQL_Thread.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/mysql-zstd_compression_level-t.cpptest/tap/tests/reg_test_2793-compression-t.cpp
✅ Files skipped from review due to trivial changes (1)
- test/tap/groups/groups.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- test/tap/tests/reg_test_2793-compression-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
include/proxysql_structs.hlib/MySQL_Protocol.cppinclude/MySQL_Thread.hlib/MySQL_Thread.cpplib/mysql_data_stream.cpptest/tap/tests/mysql-zstd_compression_level-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards use the pattern
#ifndef __CLASS_*_H(e.g.,#ifndef __ADMIN_H__,#ifndef __PROXYSQL_PROTOCOL_H__)
Files:
include/proxysql_structs.hinclude/MySQL_Thread.h
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention in lib/ directory
Files:
lib/MySQL_Protocol.cpplib/MySQL_Thread.cpplib/mysql_data_stream.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/mysql-zstd_compression_level-t.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
include/proxysql_structs.hlib/MySQL_Protocol.cpplib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
include/proxysql_structs.hlib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
lib/MySQL_Protocol.cpp
📚 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:
lib/MySQL_Thread.cpptest/tap/tests/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Applied to files:
test/tap/tests/mysql-zstd_compression_level-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/mysql-zstd_compression_level-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/mysql-zstd_compression_level-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/mysql-zstd_compression_level-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/mysql-zstd_compression_level-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/mysql-zstd_compression_level-t.cpp
[error] 12-12: 'cstdio' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
lib/mysql_data_stream.cpp
[warning] 32-32: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 GitHub Check: SonarCloud Code Analysis
lib/MySQL_Thread.cpp
[failure] 532-532: C-style cast removing const qualification from the type of a pointer may lead to an undefined behaviour.
🔇 Additional comments (5)
include/MySQL_Thread.h (1)
491-491: Looks good: dedicated zstd config field is correctly introduced.Line 491 cleanly adds the independent
zstd_compression_levelknob in the thread handler variable set.lib/mysql_data_stream.cpp (1)
32-33: Correct fallback source switch for zstd compression level.Lines 32-33 correctly decouple zstd fallback from
mysql_thread___protocol_compression_levelwhile preserving range validation.lib/MySQL_Protocol.cpp (1)
2303-2303: Fallback clamp now uses the right thread-local zstd variable.Line 2303 correctly clamps
mysql_thread___zstd_compression_leveland keeps safe bounds for negotiated zstd usage.include/proxysql_structs.h (1)
1299-1299: Thread-local declaration wiring is complete and consistent.Lines 1299 and 1633 correctly add both the TLS definition and extern declaration for
mysql_thread___zstd_compression_level.Also applies to: 1633-1633
lib/MySQL_Thread.cpp (1)
33-33: Complete and consistent zstd variable wiring.The new
zstd_compression_levelpath is correctly registered, defaulted, range-validated, and refreshed at thread scope.Also applies to: 532-532, 1458-1458, 2675-2675, 4740-4740, 4825-4825
- Remove mysql_error(NULL) UB on failed admin connection (use static error string instead). - Correct Test 10 comment and label: it exercises zlib (CLIENT_COMPRESS), not zstd compression, verifying the new variable doesn't break existing compressed connections.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/tap/tests/mysql-zstd_compression_level-t.cpp (1)
73-95:⚠️ Potential issue | 🟠 MajorMake boundary/rejection checks deterministic and semantically explicit.
At Line 73–95, the test hardcodes
22/23while production validation usesZSTD_maxCLevel(), and rejection scenarios are executed throughMYSQL_QUERY_T(which conflates “query must succeed” with “value must be rejected”). This can make the test brittle across zstd builds and ambiguous on failure mode.Suggested adjustment
- // Test 4: Set to max (22) and load to runtime + // Test 4: Set to max (derived) and load to runtime { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=22"); + // derive max from runtime behavior or metadata instead of hardcoding + const int max_level = 22; // TODO: replace with discovered runtime bound + MYSQL_QUERY_T(admin, ("SET mysql-zstd_compression_level=" + std::to_string(max_level)).c_str()); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Runtime value after SET=22 is 22, got: %d", rt_val); + ok(rt_val == max_level, "Runtime value after SET=max is %d, got: %d", max_level, rt_val); } - // Test 5: Reject value 0 + // Test 5: Reject value 0 (assert SET fails explicitly) { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=0"); + int rc = mysql_query(admin, "SET mysql-zstd_compression_level=0"); + ok(rc != 0, "SET with value 0 is rejected"); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); - // The value should NOT have changed from 22 (previous test) + // The value should NOT have changed from previous valid value int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Value 0 rejected, still 22, got: %d", rt_val); + ok(rt_val == max_level, "Value 0 rejected, still %d, got: %d", max_level, rt_val); } - // Test 6: Reject value 23 (above max) + // Test 6: Reject value above max (assert SET fails explicitly) { - MYSQL_QUERY_T(admin, "SET mysql-zstd_compression_level=23"); + int rc = mysql_query(admin, ("SET mysql-zstd_compression_level=" + std::to_string(max_level + 1)).c_str()); + ok(rc != 0, "SET above max is rejected"); MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); int rt_val = get_variable_value_int(admin, "mysql-zstd_compression_level", true); - ok(rt_val == 22, "Value 23 rejected, still 22, got: %d", rt_val); + ok(rt_val == max_level, "Value above max rejected, still %d, got: %d", max_level, rt_val); }#!/bin/bash # Verify variable bound source and test assumptions. set -euo pipefail echo "== Registration bound in runtime code ==" rg -n --type=cpp -C2 'VariablesPointers_int\\["zstd_compression_level"\\]|ZSTD_maxCLevel\\(' lib/MySQL_Thread.cpp echo echo "== Rejection-path implementation in new TAP test ==" rg -n --type=cpp -C2 'SET mysql-zstd_compression_level=22|SET mysql-zstd_compression_level=23|SET mysql-zstd_compression_level=0|MYSQL_QUERY_T\\(' test/tap/tests/mysql-zstd_compression_level-t.cpp echo echo "== MYSQL_QUERY_T behavior (does it require query success?) ==" rg -n -C4 'define\\s+MYSQL_QUERY_T|#define\\s+MYSQL_QUERY_T' test/tap test/tap/tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-zstd_compression_level-t.cpp` around lines 73 - 95, The test hardcodes 22/23/0 and uses MYSQL_QUERY_T for both success and rejection cases, making it brittle and ambiguous; change it to query the library max via ZSTD_maxCLevel() (or the registered variable bound like VariablesPointers_int["zstd_compression_level"]) to compute runtime_max, use MYSQL_QUERY_T only for operations you expect to succeed (e.g., setting runtime_max), and for rejection cases perform a plain mysql_query (or equivalent non-fatal API) and assert the query failed/returned an error while verifying the stored value via get_variable_value_int(admin, "mysql-zstd_compression_level", true) remains unchanged; update the tests around mysql-zstd_compression_level in mysql-zstd_compression_level-t.cpp to use these symbols (ZSTD_maxCLevel, MYSQL_QUERY_T, mysql_query, get_variable_value_int) so boundary checks are deterministic and semantically explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/mysql-zstd_compression_level-t.cpp`:
- Around line 73-95: The test hardcodes 22/23/0 and uses MYSQL_QUERY_T for both
success and rejection cases, making it brittle and ambiguous; change it to query
the library max via ZSTD_maxCLevel() (or the registered variable bound like
VariablesPointers_int["zstd_compression_level"]) to compute runtime_max, use
MYSQL_QUERY_T only for operations you expect to succeed (e.g., setting
runtime_max), and for rejection cases perform a plain mysql_query (or equivalent
non-fatal API) and assert the query failed/returned an error while verifying the
stored value via get_variable_value_int(admin, "mysql-zstd_compression_level",
true) remains unchanged; update the tests around mysql-zstd_compression_level in
mysql-zstd_compression_level-t.cpp to use these symbols (ZSTD_maxCLevel,
MYSQL_QUERY_T, mysql_query, get_variable_value_int) so boundary checks are
deterministic and semantically explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5c5e50e-4c1e-4644-8753-c65cf3ffea1e
📒 Files selected for processing (1)
test/tap/tests/mysql-zstd_compression_level-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/mysql-zstd_compression_level-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/mysql-zstd_compression_level-t.cpp
🧠 Learnings (9)
📚 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/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Applied to files:
test/tap/tests/mysql-zstd_compression_level-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/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/tap/tests/mysql-zstd_compression_level-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/mysql-zstd_compression_level-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/mysql-zstd_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.156Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.156Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/tap/tests/mysql-zstd_compression_level-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/mysql-zstd_compression_level-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/mysql-zstd_compression_level-t.cpp
[error] 12-12: 'cstdio' file not found
(clang-diagnostic-error)
…ing benchmark
The existing test only verified admin variable behavior (default, range,
independence). It did not prove that ZSTD compression was actually
active on the wire. Two new test variants address this:
MariaDB connector path (mysql-zstd_compression_level-t):
- Tests 1-9: unchanged (variable defaults, range validation, independence)
- Tests 10-11: use mysql CLI via execvp with --compression-algorithms=zstd
and --zstd-compression-level={3,19}. Each call spawns a new process
(= new connection). Verifies CONNECTION_ID() retrieval and large
resultset transfer. Skipped if mysql CLI lacks zstd support.
MySQL 8.4 connector path (mysql-zstd_compression_level_libmysql-t):
- Tests 1-9: same as MariaDB path
- Test 10: establishes a ZSTD connection using mysql_options(
MYSQL_OPT_COMPRESSION_ALGORITHMS, "zstd") and prints mysql_thread_id()
without running any query.
- Test 11: timing benchmark proving compression is active. Creates a
query rule (rule_id=1, cache_ttl=60000) to cache all SELECTs, then
runs a 576-row resultset query 1000 times on a plain connection and
1000 times on a ZSTD level 22 connection. ZSTD level 22 is extremely
CPU-intensive: observed 40122ms vs 858ms (46.7x ratio). A ratio >= 3x
proves compression is really running on the client<->ProxySQL link.
Other changes:
- MySQL_Protocol.cpp: simplify redundant clamping to a simple cast
(the variable is already validated at SET time)
- Makefile: add build rule for _libmysql variant linking against
MySQL 8.4 connector
- groups.json: register _libmysql variant for mysql84/90/95-g1
- CLAUDE.md: update testing section with CI script usage and DO NOT list
|
1 participant
Summary
Closes #5632
ProxySQL v3.0.7 introduced zstd compression but shared
mysql-protocol_compression_levelwith zlib. This is incorrect because the two algorithms have fundamentally different level semantics:ZSTD_maxCLevel())The shared variable range
[-1, 9]made zstd levels 10–22 inaccessible and the same numeric value meant very different compression ratios for each algorithm.This PR adds a dedicated
mysql-zstd_compression_levelvariable (range 1–22, default 3) so that zstd and zlib compression can be configured independently.Changes
Core variable infrastructure
include/proxysql_structs.h: Thread-local__thread int mysql_thread___zstd_compression_leveldeclaration and extern.include/MySQL_Thread.h:int zstd_compression_levelstruct member in MySQL_Thread variables.lib/MySQL_Thread.cpp: Full variable registration — name array, constructor init,VariablesPointers_intwith range[1, ZSTD_maxCLevel()], refresh macro, thread-local default.Wire into compression logic
lib/MySQL_Protocol.cpp: Handshake zstd level selection now usesmysql_thread___zstd_compression_levelinstead ofmysql_thread___protocol_compression_level.lib/mysql_data_stream.cpp:get_zstd_compression_level()fallback path now uses the dedicated variable.Tests
test/tap/tests/mysql-zstd_compression_level-t.cpp(new): 10 test cases covering:test/tap/tests/reg_test_2793-compression-t.cpp: Added variable visibility check via admin interface.test/tap/groups/groups.json: Registered new test in same groups asmysql-protocol_compression_level-t.Housekeeping
.gitignore: Addedtest/tap/tests/unit/*-tpattern for orphan unit test binaries.Backwards Compatibility
mysql-protocol_compression_levelis unchanged — continues to control zlib compression only.Test plan
make clean && make debug -j32)make build_tap_test_debug)mysql-zstd_compression_level-tpasses all 10 tests against mysql84-g1groups.jsonvalid and consistent (check_groups.py)Summary by CodeRabbit
New Features
Tests
Documentation
Chores