Per-server SSL parameters for PostgreSQL backends#5583
Conversation
…ommit integration
… includes Servers_SslParams, MySQLServers_SslParams, and PgSQLServers_SslParams are now in Servers_SslParams.h with minimal dependencies (<string> only). This avoids the circular include issue between cpp.h, mysql_connection.h, and Base_HostGroups_Manager.h that caused incomplete type errors.
…r fallback - Add pgsql_servers_ssl_params to pgsql_servers_tablenames so SAVE/LOAD PGSQL SERVERS TO/FROM DISK correctly persists and restores SSL params. - Fix monitor SSL fallback to use per-server params directly (empty fields omitted) instead of per-field fallback to globals, matching the backend connection behavior.
Covers table schema, column reference, ssl_protocol_version_range format (range, pin, empty default), lookup hierarchy, usage examples (basic, per-user, TLS restriction, multi-server), admin commands, and prerequisites.
|
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 Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin Interface
participant HGM as PgSQL_HostGroups_Manager
participant Map as SSL_Params_Map
participant Conn as PgSQL_Connection
participant libpq as libpq
Admin->>HGM: load/save pgsql_servers_ssl_params (SQLite3_result)
HGM->>HGM: generate_pgsql_servers_ssl_params_table()
HGM->>Map: populate (key = hostname:port:username) -> PgSQLServers_SslParams
Conn->>HGM: get_Server_SSL_Params(hostname, port, username)
HGM->>Map: lookup by computed key
Map-->>HGM: return params or NULL
HGM-->>Conn: return heap copy or NULL
alt per-server params found
Note over Conn: override global pgsql-ssl_p2s_* with per-server fields
Conn->>Conn: include ssl_min_protocol_version / ssl_max_protocol_version if non-empty
else fallback to globals
Note over Conn: use global pgsql-ssl_p2s_* values
end
Conn->>libpq: PQconnectdb(conninfo with SSL params)
libpq-->>Conn: connection result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
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 |
rahim-kanji
changed the title
![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 per-server SSL configuration for PostgreSQL backend connections via the new pgsql_servers_ssl_params table, mirroring existing MySQL functionality. The changes include the addition of a Servers_SslParams base class, updates to the PostgreSQL hostgroup manager for parameter lookup, and logic in PgSQL_Connection to apply these settings during connection establishment. Feedback highlights an incorrect constructor implementation in the new SSL params class that fails to initialize members, potential performance overhead from heap allocations during lookups, and fragile parsing logic for TLS version ranges.
| Servers_SslParams(string _h, int _p, string _u) { | ||
| Servers_SslParams(_h, _p, _u, "", "", "", "", "", "", "", "", ""); | ||
| } |
There was a problem hiding this comment.
This constructor implementation is incorrect. In C++, calling a constructor from the body of another constructor creates a temporary object of that type and immediately destroys it; it does not initialize the current instance's members. You should use a delegating constructor in the initializer list instead.
Servers_SslParams(string _h, int _p, string _u) : Servers_SslParams(_h, _p, _u, "", "", "", "", "", "", "", "", "") {}| std::lock_guard<std::mutex> lock(PgSQL_Servers_SSL_Params_map_mutex); | ||
| auto it = PgSQL_Servers_SSL_Params_map.find(MapKey); | ||
| if (it != PgSQL_Servers_SSL_Params_map.end()) { | ||
| PgSQLServers_SslParams * PSSP = new PgSQLServers_SslParams(it->second); |
There was a problem hiding this comment.
The get_Server_SSL_Params method performs a heap allocation (new PgSQLServers_SslParams) for every lookup. Since this is called during the connection establishment phase, it adds unnecessary overhead. Consider returning the object by value or returning a pointer to a persistent object if thread safety can be ensured, although returning by value is likely the cleanest approach here given the small size of the object.
| string min_ver = tls_ver.substr(0, dash_pos); | ||
| string max_ver = tls_ver.substr(dash_pos + 1); |
There was a problem hiding this comment.
The parsing of ssl_protocol_version_range is fragile. If the string contains a trailing or leading dash (e.g., TLSv1.2-), substr will return an empty string for one of the versions. It would be safer to verify that both min_ver and max_ver are non-empty before appending them to the connection info string.
…groups.json Renames test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp to pgsql-servers_ssl_params-t.cpp to drop the duplicated "pgsql-" prefix, updates the @file header to match, and registers both the integration test and the existing pgsql_servers_ssl_params_unit-t in test/tap/groups/groups.json so CI actually runs them. The integration test joins the same group set used by other PgSQL admin/SSL tests (legacy-g4 and the four mysql-*-g4 variants); the unit test joins unit-tests-g1 alongside the other pgsql_*_unit-t entries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/PgSQL_Connection.cpp (1)
2975-3005:⚠️ Potential issue | 🟠 MajorThe kill path does not honor the same override semantics as
connect_start().Here
ssl_configis seeded from the globalpgsql_thread___ssl_p2s_*values first, then only non-empty per-server fields overwrite them. That leaks global SSL material intopg_terminate_backend()for rows that intentionally leave fields empty, which is the opposite of the all-or-nothing row semantics used inconnect_start(). This path also still drops per-server TLS policy fields likessl_protocol_version_range, so terminate-connection can use a different TLS configuration than the live backend session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Connection.cpp` around lines 2975 - 3005, The kill/terminate path must match connect_start() semantics: do not pre-seed ssl_config from globals (pgsql_thread___ssl_p2s_*) and then only overwrite non-empty per-server fields; instead call PgHGM->get_Server_SSL_Params(hostname, port, username) and if it returns a params row, replace ssl_config entirely with that row's values (including empty strings) so per-server rows are all-or-nothing; also copy over the additional TLS policy fields (e.g., ssl_protocol_version_range and any other policy members on PgSQLServers_SslParams) into ssl_config so pg_terminate_backend() uses the same TLS settings as connect_start(). Ensure you update the logic around ssl_config, PgSQLServers_SslParams usage, and refrain from using strdup/free on globals before deciding which source to apply.
🧹 Nitpick comments (8)
docs/pgsql_servers_ssl_params.md (1)
79-87: Make the non-merge behavior explicit.These sections still leave room to read the global
pgsql-ssl_p2s_*values as per-column fallbacks after a row match. If the implementation is the all-or-nothing behavior described in the PR, please say that directly here so operators do not create partial rows expecting inheritance.📝 Suggested wording
-- Empty fields in `pgsql_servers_ssl_params` are omitted from the libpq connection string (libpq defaults apply for those fields). +- Empty fields in a matched `pgsql_servers_ssl_params` row are omitted from the libpq connection string (libpq defaults apply for those fields). +- ProxySQL does not merge empty columns from a matched row with `pgsql-ssl_p2s_*`; the global settings are only consulted when no row matches.Also applies to: 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pgsql_servers_ssl_params.md` around lines 79 - 87, Clarify that SSL parameter lookup is all-or-nothing: when a row matches (either Exact match (hostname, port, username) or Wildcard fallback (hostname, port, '')), ProxySQL uses the entire matching row and does not merge or inherit individual columns from the global pgsql-ssl_p2s_* variables; only if no row matches at all are the global pgsql-ssl_p2s_* variables applied. Update the Lookup Hierarchy wording (and the equivalent text near the later pgsql-ssl_p2s_* section) to state this non-merge behavior explicitly so operators know partial/column-level inheritance does not occur.lib/PgSQL_HostGroups_Manager.cpp (1)
3988-3989: Use the project's pthread mutexes for the new SSL params map.These new synchronization points extend the
std::mutex/std::lock_guardpattern, but the repository standard for new synchronization in C++ code ispthread_mutex_*.As per coding guidelines "Use pthread mutexes for synchronization and std::atomic<> for counters".
Also applies to: 4029-4030
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_HostGroups_Manager.cpp` around lines 3988 - 3989, Replace the std::mutex/std::lock_guard usage with the project's pthread mutex pattern: change the PgSQL_Servers_SSL_Params_map_mutex to a pthread_mutex_t (and ensure pthread.h is included/initialized as the project expects), then replace the std::lock_guard<std::mutex> lock(...) block around PgSQL_Servers_SSL_Params_map.clear() with explicit pthread_mutex_lock(&PgSQL_Servers_SSL_Params_map_mutex) before the clear and pthread_mutex_unlock(...) after; apply the same change for the analogous block at the second occurrence (around lines 4029-4030) so both synchronization points use pthread_mutex_* APIs instead of std::mutex.include/PgSQL_HostGroups_Manager.h (3)
49-49: Use UPPER_SNAKE_CASE for the new table macro name.
MYHGM_PgSQL_SERVERS_SSL_PARAMSbreaks the macro naming rule; please rename toMYHGM_PGSQL_SERVERS_SSL_PARAMSfor consistency with the enforced convention.As per coding guidelines, "
Constants and macros must use UPPER_SNAKE_CASE."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgSQL_HostGroups_Manager.h` at line 49, Rename the macro MYHGM_PgSQL_SERVERS_SSL_PARAMS to follow UPPER_SNAKE_CASE by changing its identifier to MYHGM_PGSQL_SERVERS_SSL_PARAMS; update the `#define` that currently declares the CREATE TABLE string (the macro value stays the same) so all references to MYHGM_PgSQL_SERVERS_SSL_PARAMS in the codebase compile against the new macro name and maintain consistency with the project's macro naming convention.
512-513: Align SSL params cache members with synchronization and naming guidelines.The new members use
std::mutexand non-snake-case member names. Please switch topthread_mutex_tand snake_case names for both fields.As per coding guidelines, "
Use pthread mutexes for synchronization" and "Member variables must use snake_case."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgSQL_HostGroups_Manager.h` around lines 512 - 513, Replace the std::mutex and non-snake-case member names with pthread mutex and snake_case names: change PgSQL_Servers_SSL_Params_map_mutex (std::mutex) to pthread_mutex_t pgsql_servers_ssl_params_map_mutex and change PgSQL_Servers_SSL_Params_map to pgsql_servers_ssl_params_map (std::unordered_map<std::string, PgSQLServers_SslParams>); ensure the pthread mutex is properly initialized (e.g., PTHREAD_MUTEX_INITIALIZER or pthread_mutex_init in the class constructor) and destroyed (pthread_mutex_destroy in the destructor) where the class that declares these members is defined.
781-781: Clarify ownership for returned SSL params and standardize tostd::unique_ptrfor consistency.This API returns a raw pointer to newly allocated objects, requiring explicit caller cleanup. While current call sites do include cleanup paths (mysql_connection.cpp:970-971, MySQL_Session.cpp:347-350), some have already migrated to
std::unique_ptr(MySQL_Monitor.cpp:1579, PgSQL_Monitor.cpp:447, PgSQL_Connection.cpp:968/2982). Standardizing onstd::unique_ptrwithconst char*parameters would make ownership explicit across all callers and prevent accidental leaks in future maintenance.💡 Suggested API change
- PgSQLServers_SslParams * get_Server_SSL_Params(char *hostname, int port, char *username); + std::unique_ptr<PgSQLServers_SslParams> get_Server_SSL_Params(const char *hostname, int port, const char *username);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgSQL_HostGroups_Manager.h` at line 781, Change the API so ownership is explicit: update the declaration of get_Server_SSL_Params to return std::unique_ptr<PgSQLServers_SslParams> and take const char* for hostname and username (and keep int port); update the corresponding implementation/definition to match the new signature and include <memory>; then update all callers (e.g., places referenced in mysql_connection.cpp, MySQL_Session.cpp, MySQL_Monitor.cpp, PgSQL_Monitor.cpp, PgSQL_Connection.cpp) to accept the returned std::unique_ptr, constructing/consuming it directly or moving it where needed and remove any manual delete calls.lib/ProxySQL_Admin.cpp (1)
7718-7760: Consider deduplicating the PgSQL/MySQL SSL-param round-trip logic.The new
pgsql_servers_ssl_paramssave/load blocks are almost a copy of the existing MySQL*_servers_ssl_paramspath. Pulling the shared insert/bind/load flow into a small helper would make future schema changes much harder to miss in one protocol path.Also applies to: 8114-8128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_Admin.cpp` around lines 7718 - 7760, The pgsql/mysql SSL param blocks duplicate the same insert/bind/iterate flow; create a small helper (e.g., save_server_ssl_params or dump_and_save_ssl_params) that accepts the destination table base name ("pgsql_servers_ssl_params" / "mysql_servers_ssl_params"), a function to obtain the source resultset (e.g., PgHGM->dump_table_pgsql or the MySQL equivalent), and the admindb prepare/execute helpers; move the prepare_v2/statement, the loop that uses proxy_sqlite3_bind_text/proxy_sqlite3_bind_int64, SAFE_SQLITE3_STEP2, proxy_sqlite3_clear_bindings and proxy_sqlite3_reset into that helper and call it for both paths (replace the current uses of resultset, dump_table_pgsql, the prepared StrQuery, and the bind loop with a single helper invocation).test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp (1)
27-73: Cover the 3-argument constructor too.The constructor section never exercises
Servers_SslParams(string, int, string), so regressions in the default-empty SSL initialization path will still leave this binary green. Add one small case for that overload and bumpplan(45)accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp` around lines 27 - 73, Add a test that exercises the 3-argument Servers_SslParams(string, int, string) constructor (e.g., create Servers_SslParams p("host", 5432, "user")) and assert that SSL-related members (ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher, tls_version) are default/empty and comment is empty; place it alongside test_base_constructor_string/test_base_constructor_charptr (e.g., as test_three_arg_constructor) and update the test plan count from 45 to 46.include/Servers_SslParams.h (1)
23-23: RenameMapKeytomap_keybefore this API spreads.This new field is the only member here not using snake_case, so it bakes an avoidable inconsistency into every caller of the shared SSL params type. As per coding guidelines, "
**/*.{cpp,h,hpp}: Member variables must use snake_case`".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Servers_SslParams.h` at line 23, Rename the struct/class member MapKey to snake_case map_key: update the declaration of MapKey to map_key in Servers_SslParams (the header member currently declared as "string MapKey"), then update all uses (constructors, initializers, assignments, serialization/deserialization, accessors, tests and any caller code) to reference map_key instead of MapKey to keep naming consistent; ensure any generated bindings or macros that refer to the old name are also adjusted and run a build to catch remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/Servers_SslParams.h`:
- Around line 60-62: The 3-arg constructor currently creates a temporary instead
of delegating, leaving the object uninitialized; replace the body with a
delegating-constructor initializer so the current object is initialized. Change
Servers_SslParams(string _h, int _p, string _u) { Servers_SslParams(...); } to
use constructor delegation: Servers_SslParams(string _h, int _p, string _u) :
Servers_SslParams(_h, _p, _u, "", "", "", "", "", "", "", "", "") {} so the real
12-arg constructor initializes this instance.
In `@lib/PgSQL_Connection.cpp`:
- Around line 967-1004: The per-server SSL columns ssl_capath and ssl_cipher are
never added to the connection string when get_Server_SSL_Params() returns a row;
update the builder in PgSQL_Connection.cpp to check ssl_params->ssl_capath and
ssl_params->ssl_cipher (like the other ssl_* fields) and call
append_conninfo_param(conninfo, "ssl_capath", ...) and
append_conninfo_param(conninfo, "ssl_cipher", ...) when non-empty, and also
ensure the existing fallback branch uses the global
pgsql_thread___ssl_p2s_capath and pgsql_thread___ssl_p2s_cipher variables to
append the same keys when ssl_params is null; refer to ssl_params,
get_Server_SSL_Params, and append_conninfo_param to locate where to add these
checks.
In `@lib/PgSQL_HostGroups_Manager.cpp`:
- Around line 1521-1526: The pgsql_servers_ssl_params table regeneration isn't
folded into the module checksum; update gen_global_pgsql_servers_v2_checksum()
to include pgsql_servers_ssl_params in the checksum computation (alongside
pgsql_replication_hostgroups and pgsql_hostgroup_attributes) so changes to SSL
params affect global_checksum_v2; ensure the function reads/serializes the
pgsql_servers_ssl_params contents the same way other tables are folded (same
ordering and fields) and update any associated comments/tests that assert the
checksum composition (references: gen_global_pgsql_servers_v2_checksum,
pgsql_servers_ssl_params, global_checksum_v2,
generate_pgsql_servers_ssl_params_table).
In `@lib/PgSQL_Monitor.cpp`:
- Around line 443-470: The monitor path is discarding per-server TLS version
info; update the lambda that returns mon_srv_t::ssl_opts_t to include
ssl_params->tls_version when ssl_params is present and propagate the same
default source for non-SSL rows, then extend the mon_srv_t::ssl_opts_t structure
to add a tls_version field and update mon_srv_t::build_conn_str() to parse and
apply tls_version using the same ssl_min_protocol_version /
ssl_max_protocol_version range-parsing and enforcement logic used in
PgSQL_Connection.cpp (reuse the same parsing functions or logic to avoid
divergence).
In `@test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp`:
- Around line 426-441: The test_monitor_ssl_with_per_server_params currently
only toggles the global use_ssl and never inserts a per-server record, so it
doesn't exercise the per-server lookup; update
test_monitor_ssl_with_per_server_params to insert a row into
pgsql_servers_ssl_params for the backend under test (matching host/port or
server id used by the test), setting the per-server use_ssl and any required
ssl_* fields before calling "LOAD PGSQL SERVERS TO RUNTIME" (use exec_ok to run
the INSERT and then the LOAD), then sample PgSQL_Monitor_ssl_connections_OK as
before to verify the counter increases; keep references to cleanup_ssl_params,
pgsql_servers_ssl_params, use_ssl, test_monitor_ssl_with_per_server_params, and
PgSQL_Monitor_ssl_connections_OK so the change is localized and clearly tests
the per-server path.
- Line 449: The TAP plan is one too high—update the planned assertion count from
plan(31) to plan(30) (and similarly adjust the other plan instances around the
478-486 range) so the test harness matches the actual assertions run;
alternatively, if you intended to exercise the monitor, call
test_monitor_ssl_with_per_server_params(a) from main(), but the minimal fix is
to change plan(31) to plan(30) to match the 30 assertions actually executed.
- Around line 286-287: The SELECT queries use existing pooled connections and
must force a fresh backend connection; update the PQexec calls that run "SELECT
1" (e.g., the PGresult* res = PQexec(backend.get(), "SELECT 1") instances in
this test and the similar occurrences around lines 417-418) to include the proxy
hint to create a new connection by prefixing the SQL with "/*
create_new_connection=1 */" so the test validates the just-loaded SSL config
rather than a pooled session.
---
Outside diff comments:
In `@lib/PgSQL_Connection.cpp`:
- Around line 2975-3005: The kill/terminate path must match connect_start()
semantics: do not pre-seed ssl_config from globals (pgsql_thread___ssl_p2s_*)
and then only overwrite non-empty per-server fields; instead call
PgHGM->get_Server_SSL_Params(hostname, port, username) and if it returns a
params row, replace ssl_config entirely with that row's values (including empty
strings) so per-server rows are all-or-nothing; also copy over the additional
TLS policy fields (e.g., ssl_protocol_version_range and any other policy members
on PgSQLServers_SslParams) into ssl_config so pg_terminate_backend() uses the
same TLS settings as connect_start(). Ensure you update the logic around
ssl_config, PgSQLServers_SslParams usage, and refrain from using strdup/free on
globals before deciding which source to apply.
---
Nitpick comments:
In `@docs/pgsql_servers_ssl_params.md`:
- Around line 79-87: Clarify that SSL parameter lookup is all-or-nothing: when a
row matches (either Exact match (hostname, port, username) or Wildcard fallback
(hostname, port, '')), ProxySQL uses the entire matching row and does not merge
or inherit individual columns from the global pgsql-ssl_p2s_* variables; only if
no row matches at all are the global pgsql-ssl_p2s_* variables applied. Update
the Lookup Hierarchy wording (and the equivalent text near the later
pgsql-ssl_p2s_* section) to state this non-merge behavior explicitly so
operators know partial/column-level inheritance does not occur.
In `@include/PgSQL_HostGroups_Manager.h`:
- Line 49: Rename the macro MYHGM_PgSQL_SERVERS_SSL_PARAMS to follow
UPPER_SNAKE_CASE by changing its identifier to MYHGM_PGSQL_SERVERS_SSL_PARAMS;
update the `#define` that currently declares the CREATE TABLE string (the macro
value stays the same) so all references to MYHGM_PgSQL_SERVERS_SSL_PARAMS in the
codebase compile against the new macro name and maintain consistency with the
project's macro naming convention.
- Around line 512-513: Replace the std::mutex and non-snake-case member names
with pthread mutex and snake_case names: change
PgSQL_Servers_SSL_Params_map_mutex (std::mutex) to pthread_mutex_t
pgsql_servers_ssl_params_map_mutex and change PgSQL_Servers_SSL_Params_map to
pgsql_servers_ssl_params_map (std::unordered_map<std::string,
PgSQLServers_SslParams>); ensure the pthread mutex is properly initialized
(e.g., PTHREAD_MUTEX_INITIALIZER or pthread_mutex_init in the class constructor)
and destroyed (pthread_mutex_destroy in the destructor) where the class that
declares these members is defined.
- Line 781: Change the API so ownership is explicit: update the declaration of
get_Server_SSL_Params to return std::unique_ptr<PgSQLServers_SslParams> and take
const char* for hostname and username (and keep int port); update the
corresponding implementation/definition to match the new signature and include
<memory>; then update all callers (e.g., places referenced in
mysql_connection.cpp, MySQL_Session.cpp, MySQL_Monitor.cpp, PgSQL_Monitor.cpp,
PgSQL_Connection.cpp) to accept the returned std::unique_ptr,
constructing/consuming it directly or moving it where needed and remove any
manual delete calls.
In `@include/Servers_SslParams.h`:
- Line 23: Rename the struct/class member MapKey to snake_case map_key: update
the declaration of MapKey to map_key in Servers_SslParams (the header member
currently declared as "string MapKey"), then update all uses (constructors,
initializers, assignments, serialization/deserialization, accessors, tests and
any caller code) to reference map_key instead of MapKey to keep naming
consistent; ensure any generated bindings or macros that refer to the old name
are also adjusted and run a build to catch remaining references.
In `@lib/PgSQL_HostGroups_Manager.cpp`:
- Around line 3988-3989: Replace the std::mutex/std::lock_guard usage with the
project's pthread mutex pattern: change the PgSQL_Servers_SSL_Params_map_mutex
to a pthread_mutex_t (and ensure pthread.h is included/initialized as the
project expects), then replace the std::lock_guard<std::mutex> lock(...) block
around PgSQL_Servers_SSL_Params_map.clear() with explicit
pthread_mutex_lock(&PgSQL_Servers_SSL_Params_map_mutex) before the clear and
pthread_mutex_unlock(...) after; apply the same change for the analogous block
at the second occurrence (around lines 4029-4030) so both synchronization points
use pthread_mutex_* APIs instead of std::mutex.
In `@lib/ProxySQL_Admin.cpp`:
- Around line 7718-7760: The pgsql/mysql SSL param blocks duplicate the same
insert/bind/iterate flow; create a small helper (e.g., save_server_ssl_params or
dump_and_save_ssl_params) that accepts the destination table base name
("pgsql_servers_ssl_params" / "mysql_servers_ssl_params"), a function to obtain
the source resultset (e.g., PgHGM->dump_table_pgsql or the MySQL equivalent),
and the admindb prepare/execute helpers; move the prepare_v2/statement, the loop
that uses proxy_sqlite3_bind_text/proxy_sqlite3_bind_int64, SAFE_SQLITE3_STEP2,
proxy_sqlite3_clear_bindings and proxy_sqlite3_reset into that helper and call
it for both paths (replace the current uses of resultset, dump_table_pgsql, the
prepared StrQuery, and the bind loop with a single helper invocation).
In `@test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp`:
- Around line 27-73: Add a test that exercises the 3-argument
Servers_SslParams(string, int, string) constructor (e.g., create
Servers_SslParams p("host", 5432, "user")) and assert that SSL-related members
(ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher,
tls_version) are default/empty and comment is empty; place it alongside
test_base_constructor_string/test_base_constructor_charptr (e.g., as
test_three_arg_constructor) and update the test plan count from 45 to 46.
🪄 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: 1818ad6a-bf2a-44fb-a78e-9d99645ecc3a
📒 Files selected for processing (16)
docs/pgsql_servers_ssl_params.mdinclude/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.hinclude/PgSQL_HostGroups_Manager.hinclude/ProxySQL_Admin_Tables_Definitions.hinclude/Servers_SslParams.hinclude/mysql_connection.hinclude/proxysql_admin.hlib/Admin_Bootstrap.cpplib/PgSQL_Connection.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Monitor.cpplib/ProxySQL_Admin.cpptest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
💤 Files with no reviewable changes (2)
- include/MySQL_HostGroups_Manager.h
- include/Base_HostGroups_Manager.h
📜 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 (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/Admin_Bootstrap.cppinclude/ProxySQL_Admin_Tables_Definitions.hinclude/mysql_connection.hlib/PgSQL_Monitor.cpplib/PgSQL_Connection.cppinclude/proxysql_admin.hlib/PgSQL_HostGroups_Manager.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cppinclude/PgSQL_HostGroups_Manager.htest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpplib/ProxySQL_Admin.cppinclude/Servers_SslParams.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/Admin_Bootstrap.cpplib/PgSQL_Monitor.cpplib/PgSQL_Connection.cpplib/PgSQL_HostGroups_Manager.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpptest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpplib/ProxySQL_Admin.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/Admin_Bootstrap.cpplib/PgSQL_Monitor.cpplib/PgSQL_Connection.cpplib/PgSQL_HostGroups_Manager.cpplib/ProxySQL_Admin.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/ProxySQL_Admin_Tables_Definitions.hinclude/mysql_connection.hinclude/proxysql_admin.hinclude/PgSQL_HostGroups_Manager.hinclude/Servers_SslParams.h
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp
🧠 Learnings (11)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
lib/Admin_Bootstrap.cppdocs/pgsql_servers_ssl_params.mdinclude/ProxySQL_Admin_Tables_Definitions.hinclude/proxysql_admin.hinclude/PgSQL_HostGroups_Manager.hlib/ProxySQL_Admin.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
lib/Admin_Bootstrap.cppinclude/ProxySQL_Admin_Tables_Definitions.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ 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/unit/Makefiletest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpptest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
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/unit/Makefiletest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
include/ProxySQL_Admin_Tables_Definitions.hinclude/mysql_connection.htest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpplib/ProxySQL_Admin.cppinclude/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
include/mysql_connection.hlib/ProxySQL_Admin.cppinclude/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Use pthread mutexes for synchronization and std::atomic<> for counters
Applied to files:
lib/PgSQL_Connection.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.cpp : Use RAII for resource management and jemalloc for memory allocation
Applied to files:
lib/PgSQL_Connection.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpptest/tap/tests/pgsql-pgsql_servers_ssl_params-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/unit/pgsql_servers_ssl_params_unit-t.cpptest/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 7-7: 'tap.h' file not found
(clang-diagnostic-error)
test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp
[error] 11-11: 'unistd.h' file not found
(clang-diagnostic-error)
include/Servers_SslParams.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (7)
include/ProxySQL_Admin_Tables_Definitions.h (1)
318-320: Schema split is consistent with the rest of the admin/runtime tables.The new persisted and runtime definitions mirror the existing SSL params shape cleanly, which should keep
LOAD/SAVEhandling predictable.include/proxysql_admin.h (1)
206-210:incoming_pgsql_servers_tis plumbed cleanly.Adding the extra resultset handle to both the struct and constructor keeps the new table bundled with the rest of the PgSQL server snapshot, and stale call sites will fail loudly at compile time.
lib/Admin_Bootstrap.cpp (1)
820-821: Bootstrap registration looks complete.Registering the table in both
tables_defs_adminandtables_defs_configcovers the runtime activation and disk persistence paths.Also applies to: 847-847
test/tap/tests/unit/Makefile (1)
291-292: Good to have the new unit test in the default target.Wiring
pgsql_servers_ssl_params_unit-tintoUNIT_TESTSkeeps the new SSL lookup coverage in the normal unit-test build.lib/PgSQL_HostGroups_Manager.cpp (1)
2925-2939: No action needed. The member variableincoming_pgsql_servers_ssl_paramsis properly initialized with an in-class initializer= nullptrin the class declaration (PgSQL_HostGroups_Manager.h, line 564), so the code at lines 2925–2939 is safe.> Likely an incorrect or invalid review comment.include/PgSQL_HostGroups_Manager.h (1)
562-564: Good defensive initialization for incoming SSL params resultset.Declaring the generator and initializing
incoming_pgsql_servers_ssl_paramstonullptris clean and safe.lib/ProxySQL_Admin.cpp (1)
149-157: Good wiring for the new PgSQL SSL params table.Adding
pgsql_servers_ssl_paramsto the generic PgSQL server table set and threadingincoming_pgsql_servers_ssl_paramsthroughincoming_pgsql_servers_tkeeps the disk/main copy helpers and the explicit runtime load path aligned with the existing MySQL flow.Also applies to: 886-897
| std::unique_ptr<PgSQLServers_SslParams> ssl_params { | ||
| PgHGM->get_Server_SSL_Params(parent->address, parent->port, userinfo->username) | ||
| }; | ||
| if (ssl_params != nullptr) { | ||
| // Use per-server SSL params | ||
| if (ssl_params->ssl_key.length() > 0) | ||
| append_conninfo_param(conninfo, "sslkey", (char*)ssl_params->ssl_key.c_str()); | ||
| if (ssl_params->ssl_cert.length() > 0) | ||
| append_conninfo_param(conninfo, "sslcert", (char*)ssl_params->ssl_cert.c_str()); | ||
| if (ssl_params->ssl_ca.length() > 0) | ||
| append_conninfo_param(conninfo, "sslrootcert", (char*)ssl_params->ssl_ca.c_str()); | ||
| if (ssl_params->ssl_crl.length() > 0) | ||
| append_conninfo_param(conninfo, "sslcrl", (char*)ssl_params->ssl_crl.c_str()); | ||
| if (ssl_params->ssl_crlpath.length() > 0) | ||
| append_conninfo_param(conninfo, "sslcrldir", (char*)ssl_params->ssl_crlpath.c_str()); | ||
| // Parse ssl_protocol_version_range (format: "TLSv1.2-TLSv1.3" or "TLSv1.3") | ||
| if (ssl_params->tls_version.length() > 0) { | ||
| string tls_ver = ssl_params->tls_version; | ||
| size_t dash_pos = tls_ver.find('-'); | ||
| if (dash_pos != string::npos) { | ||
| string min_ver = tls_ver.substr(0, dash_pos); | ||
| string max_ver = tls_ver.substr(dash_pos + 1); | ||
| append_conninfo_param(conninfo, "ssl_min_protocol_version", (char*)min_ver.c_str()); | ||
| append_conninfo_param(conninfo, "ssl_max_protocol_version", (char*)max_ver.c_str()); | ||
| } else { | ||
| // Single version = pin to that version | ||
| append_conninfo_param(conninfo, "ssl_min_protocol_version", (char*)tls_ver.c_str()); | ||
| append_conninfo_param(conninfo, "ssl_max_protocol_version", (char*)tls_ver.c_str()); | ||
| } | ||
| } | ||
| } else { | ||
| // Fall back to global SSL settings | ||
| append_conninfo_param(conninfo, "sslkey", pgsql_thread___ssl_p2s_key); | ||
| append_conninfo_param(conninfo, "sslcert", pgsql_thread___ssl_p2s_cert); | ||
| append_conninfo_param(conninfo, "sslrootcert", pgsql_thread___ssl_p2s_ca); | ||
| append_conninfo_param(conninfo, "sslcrl", pgsql_thread___ssl_p2s_crl); | ||
| append_conninfo_param(conninfo, "sslcrldir", pgsql_thread___ssl_p2s_crlpath); | ||
| } |
There was a problem hiding this comment.
ssl_capath and ssl_cipher are silently ignored on the main connect path.
Once get_Server_SSL_Params() finds a row, global fallback is disabled, but this builder still never consumes the per-server ssl_capath and ssl_cipher columns. A row that only sets either of those values becomes a no-op on the actual backend connection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Connection.cpp` around lines 967 - 1004, The per-server SSL columns
ssl_capath and ssl_cipher are never added to the connection string when
get_Server_SSL_Params() returns a row; update the builder in
PgSQL_Connection.cpp to check ssl_params->ssl_capath and ssl_params->ssl_cipher
(like the other ssl_* fields) and call append_conninfo_param(conninfo,
"ssl_capath", ...) and append_conninfo_param(conninfo, "ssl_cipher", ...) when
non-empty, and also ensure the existing fallback branch uses the global
pgsql_thread___ssl_p2s_capath and pgsql_thread___ssl_p2s_cipher variables to
append the same keys when ssl_params is null; refer to ssl_params,
get_Server_SSL_Params, and append_conninfo_param to locate where to add these
checks.
| PGresult* res = PQexec(backend.get(), "SELECT 1"); | ||
| ok(PQresultStatus(res) == PGRES_TUPLES_OK, "Baseline: SELECT 1 succeeds"); |
There was a problem hiding this comment.
Force a fresh backend connection in these positive-path checks.
Unlike Line 361 and Line 396, these queries do not use /* create_new_connection=1 */. ProxySQL can satisfy them from a pooled backend connection opened under earlier settings, so they can pass without proving the just-loaded SSL config was applied.
🔁 Proposed fix
- PGresult* res = PQexec(backend.get(), "SELECT 1");
+ PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1");
...
- PGresult* res = PQexec(backend.get(), "SELECT 1");
+ PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1");Also applies to: 417-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp` around lines 286 - 287,
The SELECT queries use existing pooled connections and must force a fresh
backend connection; update the PQexec calls that run "SELECT 1" (e.g., the
PGresult* res = PQexec(backend.get(), "SELECT 1") instances in this test and the
similar occurrences around lines 417-418) to include the proxy hint to create a
new connection by prefixing the SQL with "/* create_new_connection=1 */" so the
test validates the just-loaded SSL config rather than a pooled session.
Calls sqlite3_config(SQLITE_CONFIG_URI, 1) at the top of
test_globals_init(), mirroring src/main.cpp. Without this,
SQLite3DB::open("file:mem_mydb?mode=memory", ...) used by both
HostGroups Manager constructors falls back to literal-filename
behavior in unit tests and creates a stray on-disk file in the
test cwd instead of an in-memory database. The daemon already
sets this flag before any component is constructed; the test
harness now does the same.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/tap/tests/pgsql-servers_ssl_params-t.cpp (2)
286-287:⚠️ Potential issue | 🟠 MajorForce a fresh backend connection in these success-path checks.
These
SELECT 1calls can be satisfied by a pooled backend session, so they do not necessarily prove the just-loaded SSL state was used. Prefix them with/* create_new_connection=1 */like the negative-path checks.🔁 Minimal fix
- PGresult* res = PQexec(backend.get(), "SELECT 1"); + PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1"); ... - PGresult* res = PQexec(backend.get(), "SELECT 1"); + PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1");Also applies to: 417-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 286 - 287, The success-path checks calling PQexec via the existing backend connection (PGresult* res = PQexec(backend.get(), "SELECT 1")) must force a fresh backend by prefixing the SQL with the same connection-forcing comment used in negative-path checks; change the call to use "/* create_new_connection=1 */ SELECT 1" so PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1") is used instead, and apply the same change to the other identical success check later in the file (the other PQexec(backend.get(), "SELECT 1") occurrence).
426-441:⚠️ Potential issue | 🟠 MajorThe monitor path is still untested.
test_monitor_ssl_with_per_server_params()is never called frommain(), and the helper itself clearspgsql_servers_ssl_paramsand only toggles globaluse_ssl. Even if you wire it in, it still will not cover the new per-server monitor lookup. Please insert a matchingpgsql_servers_ssl_paramsrow for the selected backend, load it to runtime, and then invoke this helper frommain().Also applies to: 478-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 426 - 441, The helper test_monitor_ssl_with_per_server_params currently clears pgsql_servers_ssl_params and only toggles the global use_ssl flag but is never invoked from main(), so the per-server monitor path is untested; modify the test to insert a matching row into the pgsql_servers_ssl_params table for the backend under test (use exec_ok to run an INSERT INTO pgsql_servers_ssl_params ... referencing the target server), call exec_ok to load those SSL params into runtime (e.g., "LOAD PGSQL SERVERS SSL_PARAMS TO RUNTIME"), then invoke test_monitor_ssl_with_per_server_params() from main(); repeat the same insertion+load+call for the duplicate case referenced around lines 478-486 so the per-server monitor lookup is exercised for both tests.
🤖 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/pgsql-servers_ssl_params-t.cpp`:
- Around line 102-122: Add an end-to-end check in
test_per_server_params_promoted_to_runtime() that actually exercises file-based
SSL params: call create_bogus_cert_file() before forcing a new client
connection, configure the server's runtime_pgsql_servers_ssl_params to point
ssl_ca/ssl_cert/ssl_key at BOGUS_CERT_PATH (the strings used by
PgSQL_Connection.cpp), then force a fresh connection (not a reused one) and run
a simple query asserting it fails due to the invalid cert; finally call
remove_bogus_cert_file() to clean up. Ensure the test forces a new connection
instance (so PgSQL_Connection.cpp builds the libpq conninfo) and asserts the
expected failure rather than exiting earlier.
---
Duplicate comments:
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp`:
- Around line 286-287: The success-path checks calling PQexec via the existing
backend connection (PGresult* res = PQexec(backend.get(), "SELECT 1")) must
force a fresh backend by prefixing the SQL with the same connection-forcing
comment used in negative-path checks; change the call to use "/*
create_new_connection=1 */ SELECT 1" so PQexec(backend.get(), "/*
create_new_connection=1 */ SELECT 1") is used instead, and apply the same change
to the other identical success check later in the file (the other
PQexec(backend.get(), "SELECT 1") occurrence).
- Around line 426-441: The helper test_monitor_ssl_with_per_server_params
currently clears pgsql_servers_ssl_params and only toggles the global use_ssl
flag but is never invoked from main(), so the per-server monitor path is
untested; modify the test to insert a matching row into the
pgsql_servers_ssl_params table for the backend under test (use exec_ok to run an
INSERT INTO pgsql_servers_ssl_params ... referencing the target server), call
exec_ok to load those SSL params into runtime (e.g., "LOAD PGSQL SERVERS
SSL_PARAMS TO RUNTIME"), then invoke test_monitor_ssl_with_per_server_params()
from main(); repeat the same insertion+load+call for the duplicate case
referenced around lines 478-486 so the per-server monitor lookup is exercised
for both tests.
🪄 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: 06204c9f-25db-4921-8884-c8c3ad03a8bb
📒 Files selected for processing (3)
test/tap/groups/groups.jsontest/tap/test_helpers/test_globals.cpptest/tap/tests/pgsql-servers_ssl_params-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 (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: claude-review
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/test_helpers/test_globals.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/tap/test_helpers/test_globals.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/pgsql-servers_ssl_params-t.cpp
🧠 Learnings (10)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/test_helpers/test_globals.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
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/test_helpers/test_globals.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
test/tap/test_helpers/test_globals.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
test/tap/test_helpers/test_globals.cpptest/tap/groups/groups.json
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
test/tap/test_helpers/test_globals.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/test_helpers/test_globals.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/pgsql-servers_ssl_params-t.cpp
[error] 11-11: 'unistd.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (1)
test/tap/test_helpers/test_globals.cpp (1)
223-237: Check the return value ofsqlite3_config()for error handling.The stated concern about initialization order in
sqlite3db_unit-t.cppis incorrect—the test correctly callstest_init_minimal()before constructingSQLite3DB(). However,sqlite3_config(SQLITE_CONFIG_URI, 1)intest_globals.cppat line 237 ignores the return value. Unlikesqlite3-t.cppwhich checks forSQLITE_MISUSE, this silently discards configuration failures that could leave URI parsing disabled. Either verify the return value or document why failures are acceptable.> Likely an incorrect or invalid review comment.
| /** | ||
| * @brief Create a bogus cert file that exists but has invalid content. | ||
| * | ||
| * libpq will attempt to parse this file and fail with "no start line", | ||
| * which proves that per-server SSL params are actually being applied. | ||
| * A nonexistent file path is simply ignored by libpq when the backend | ||
| * doesn't require client certificates. | ||
| */ | ||
| static const char* BOGUS_CERT_PATH = "/tmp/proxysql_test_bogus_cert.pem"; | ||
|
|
||
| static void create_bogus_cert_file() { | ||
| FILE* f = fopen(BOGUS_CERT_PATH, "w"); | ||
| if (f) { | ||
| fprintf(f, "this is not a valid certificate\n"); | ||
| fclose(f); | ||
| } | ||
| } | ||
|
|
||
| static void remove_bogus_cert_file() { | ||
| unlink(BOGUS_CERT_PATH); | ||
| } |
There was a problem hiding this comment.
The file-based SSL params never get exercised end-to-end.
create_bogus_cert_file() is defined, but test_per_server_params_promoted_to_runtime() stops at runtime_pgsql_servers_ssl_params. If lib/PgSQL_Connection.cpp stopped appending ssl_ca / ssl_cert / ssl_key to libpq conninfo, this test would still pass because the only end-to-end failures here come from ssl_protocol_version_range. Please add a forced new-connection check that uses a readable bogus cert/key path and asserts the query fails.
Also applies to: 294-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 102 - 122, Add an
end-to-end check in test_per_server_params_promoted_to_runtime() that actually
exercises file-based SSL params: call create_bogus_cert_file() before forcing a
new client connection, configure the server's runtime_pgsql_servers_ssl_params
to point ssl_ca/ssl_cert/ssl_key at BOGUS_CERT_PATH (the strings used by
PgSQL_Connection.cpp), then force a fresh connection (not a reused one) and run
a simple query asserting it fails due to the invalid cert; finally call
remove_bogus_cert_file() to clean up. Ensure the test forces a new connection
instance (so PgSQL_Connection.cpp builds the libpq conninfo) and asserts the
expected failure rather than exiting earlier.
…range - Drop ssl_capath and ssl_cipher columns (no libpq equivalents) - Pre-parse ssl_protocol_version_range into ssl_min/max_protocol_version at construction time instead of on every backend connection - Fix Servers_SslParams 3-arg delegating constructor - Update tests, docs, and all consumers (Connection, Monitor, Kill, Admin)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
test/tap/tests/pgsql-servers_ssl_params-t.cpp (2)
285-288:⚠️ Potential issue | 🟠 MajorForce a fresh backend on these success-path checks.
Both
SELECT 1calls can be satisfied by an already pooled backend connection, so the test can pass without proving the just-loaded SSL settings were applied. Prefix them with/* create_new_connection=1 */.🔁 Minimal fix
- PGresult* res = PQexec(backend.get(), "SELECT 1"); + PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1"); ... - PGresult* res = PQexec(backend.get(), "SELECT 1"); + PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1");Also applies to: 416-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 285 - 288, The success-path SELECT 1 checks may use a pooled backend and not verify newly applied SSL settings; update the PQexec calls that run "SELECT 1" (where `backend` is used and `PQexec(backend.get(), "SELECT 1")`) to force a fresh connection by prefixing the query string with the comment "/* create_new_connection=1 */" (i.e. call PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1")); apply the same change to the other analogous CHECK block around the `SELECT 1` at the later occurrence.
304-330:⚠️ Potential issue | 🟠 MajorExercise the file-based SSL params through a real connection.
This helper only proves the row is copied into
runtime_pgsql_servers_ssl_params. If the connection path stops forwardingssl_ca/ssl_cert/ssl_keyinto libpq conninfo, this test still passes. Usecreate_bogus_cert_file(), point one of the file params atBOGUS_CERT_PATH, force a fresh backend connection, and assert the query fails.
🧹 Nitpick comments (3)
include/PgSQL_HostGroups_Manager.h (1)
782-782: Make the SSL lookup ownership explicit in the API.This returns a heap-allocated copy, and the new callers already wrap it in
std::unique_ptr. Returningstd::unique_ptr<PgSQLServers_SslParams>directly would encode the ownership contract in the type system and remove an easy leak footgun for future callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgSQL_HostGroups_Manager.h` at line 782, Change the API to convey ownership by returning a smart pointer: modify the declaration of get_Server_SSL_Params to return std::unique_ptr<PgSQLServers_SslParams> instead of a raw PgSQLServers_SslParams* (i.e., std::unique_ptr<PgSQLServers_SslParams> get_Server_SSL_Params(char *hostname, int port, char *username);), update the corresponding implementation to allocate and return a std::unique_ptr (or use std::make_unique) and include <memory>, and update all callers to accept the std::unique_ptr result (removing manual delete/ownership code) so ownership is explicit in the type system.lib/PgSQL_Monitor.cpp (1)
446-477: Salt the monitor pool key with the SSL policy.
ssl_opts_tnow changes the conninfo, but pooled monitor connections are still reused byaddr:portonly. That means a runtime CA/cert/TLS-policy change won't take effect until the old pooled connection ages out. Consider including a stable SSL-options fingerprint in the pool key, or flushingmon_conn_poolwhen SSL params are refreshed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Monitor.cpp` around lines 446 - 477, The monitor pool key currently only uses addr:port so SSL changes aren't reflected; modify the mon_conn_pool keying to include a stable fingerprint derived from mon_srv_t::ssl_opts_t (e.g., hash of ssl_key/ssl_cert/ssl_ca/ssl_crl/ssl_crlpath/min/max versions) or, alternatively, flush relevant entries from mon_conn_pool when SSL params are refreshed; update the code that builds the pool key (where ssl_opts_t is constructed) to compute and attach this fingerprint and/or add a hook in the SSL refresh path to remove connections for that server from mon_conn_pool so new connections use the updated TLS policy.include/Servers_SslParams.h (1)
9-23: These exposed names don't match the repo's C++ naming rules.The new public API introduces
Servers_SslParams,MySQLServers_SslParams,PgSQLServers_SslParams, andMapKey, but the repository requires protocol-prefixed class names and snake_case member names. This is easiest to fix before more call sites depend on the current names.As per coding guidelines "Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)" and "Member variables must use snake_case".
Also applies to: 71-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Servers_SslParams.h` around lines 9 - 23, The public names violate naming rules: rename class Servers_SslParams and its protocol-specific variants to include protocol prefixes and PascalCase (e.g., MySQL_SslParams, PgSQL_SslParams or per repo convention MySQL_SslParams / PgSQL_SslParams), and convert all public member variables (hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher, tls_version, comment, MapKey) to snake_case (e.g., hostname -> hostname, port -> port, username -> username, ssl_ca -> ssl_ca, ssl_cert -> ssl_cert, ssl_key -> ssl_key, ssl_capath -> ssl_ca_path, ssl_crl -> ssl_crl, ssl_crlpath -> ssl_crl_path, ssl_cipher -> ssl_cipher, tls_version -> tls_version, comment -> comment, MapKey -> map_key) and update all references; ensure class declarations and any derived/related classes (MySQLServers_SslParams, PgSQLServers_SslParams) are renamed consistently to the protocol-prefixed PascalCase form and run a project-wide refactor to update call sites and headers/forward declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/PgSQL_HostGroups_Manager.h`:
- Around line 513-514: Rename the two members to follow snake_case and replace
std::mutex with the project-standard pthread_mutex_t: change
PgSQL_Servers_SSL_Params_map_mutex -> pgsql_servers_ssl_params_map_mutex (type
pthread_mutex_t) and PgSQL_Servers_SSL_Params_map ->
pgsql_servers_ssl_params_map (keep the same unordered_map type), and ensure
pthread_mutex_init is called where the class is constructed and
pthread_mutex_destroy in the destructor (or appropriate init/teardown function)
so callers using pgsql_servers_ssl_params_map_mutex will compile and follow
project conventions.
In `@include/Servers_SslParams.h`:
- Line 23: getMapKey() currently returns a cached MapKey which can become stale
when delimiter or identity fields change; either remove the MapKey member and
compute/return the key on every call in getMapKey(), or implement proper cache
invalidation by storing the delimiter (and/or a generation/version) alongside
MapKey and clearing/updating MapKey whenever hostname, port, or username are
modified; apply the same change to the other cached key usage referenced around
lines 63-67 so the cached value cannot be returned stale.
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp`:
- Around line 478-489: The test currently compares ok_after to ok_before but
getMonitorValue() can return -1 on failure, making the assertion vacuously pass;
update the test to first verify both ok_before and ok_after are valid by
checking they are not -1 (e.g., assert ok_before != -1 and ok_after != -1 with a
TAP failure message indicating monitor read failure) and only then perform the
existing ok(ok_after == ok_before, ...) assertion about monitor behavior using
the validated values from getMonitorValue().
- Around line 393-401: The test currently initializes query_failed=true so a
nullptr backend can falsely make the override test pass; change the logic by (1)
adding an explicit ok(backend != nullptr, "createNewConnection(BACKEND) returned
a connection") immediately after obtaining backend to count the connection
assertion, and (2) initialize query_failed=false and only set query_failed =
(PQresultStatus(res) != PGRES_TUPLES_OK) after calling PQexec inside the
existing if (backend) block so a missing backend cannot satisfy the later
ok(query_failed, ...) check; reference createNewConnection, backend,
query_failed, PQexec and the surrounding ok(...) assertion.
In `@test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp`:
- Around line 316-317: The TAP plan count in main() is off by one: update the
plan(...) call (the plan(67) invocation in main) to the correct total of 68 so
the test harness expects all 68 assertions; locate the plan(...) call in the
main function and change its argument from 67 to 68.
---
Duplicate comments:
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp`:
- Around line 285-288: The success-path SELECT 1 checks may use a pooled backend
and not verify newly applied SSL settings; update the PQexec calls that run
"SELECT 1" (where `backend` is used and `PQexec(backend.get(), "SELECT 1")`) to
force a fresh connection by prefixing the query string with the comment "/*
create_new_connection=1 */" (i.e. call PQexec(backend.get(), "/*
create_new_connection=1 */ SELECT 1")); apply the same change to the other
analogous CHECK block around the `SELECT 1` at the later occurrence.
---
Nitpick comments:
In `@include/PgSQL_HostGroups_Manager.h`:
- Line 782: Change the API to convey ownership by returning a smart pointer:
modify the declaration of get_Server_SSL_Params to return
std::unique_ptr<PgSQLServers_SslParams> instead of a raw PgSQLServers_SslParams*
(i.e., std::unique_ptr<PgSQLServers_SslParams> get_Server_SSL_Params(char
*hostname, int port, char *username);), update the corresponding implementation
to allocate and return a std::unique_ptr (or use std::make_unique) and include
<memory>, and update all callers to accept the std::unique_ptr result (removing
manual delete/ownership code) so ownership is explicit in the type system.
In `@include/Servers_SslParams.h`:
- Around line 9-23: The public names violate naming rules: rename class
Servers_SslParams and its protocol-specific variants to include protocol
prefixes and PascalCase (e.g., MySQL_SslParams, PgSQL_SslParams or per repo
convention MySQL_SslParams / PgSQL_SslParams), and convert all public member
variables (hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_capath,
ssl_crl, ssl_crlpath, ssl_cipher, tls_version, comment, MapKey) to snake_case
(e.g., hostname -> hostname, port -> port, username -> username, ssl_ca ->
ssl_ca, ssl_cert -> ssl_cert, ssl_key -> ssl_key, ssl_capath -> ssl_ca_path,
ssl_crl -> ssl_crl, ssl_crlpath -> ssl_crl_path, ssl_cipher -> ssl_cipher,
tls_version -> tls_version, comment -> comment, MapKey -> map_key) and update
all references; ensure class declarations and any derived/related classes
(MySQLServers_SslParams, PgSQLServers_SslParams) are renamed consistently to the
protocol-prefixed PascalCase form and run a project-wide refactor to update call
sites and headers/forward declarations.
In `@lib/PgSQL_Monitor.cpp`:
- Around line 446-477: The monitor pool key currently only uses addr:port so SSL
changes aren't reflected; modify the mon_conn_pool keying to include a stable
fingerprint derived from mon_srv_t::ssl_opts_t (e.g., hash of
ssl_key/ssl_cert/ssl_ca/ssl_crl/ssl_crlpath/min/max versions) or, alternatively,
flush relevant entries from mon_conn_pool when SSL params are refreshed; update
the code that builds the pool key (where ssl_opts_t is constructed) to compute
and attach this fingerprint and/or add a hook in the SSL refresh path to remove
connections for that server from mon_conn_pool so new connections use the
updated TLS policy.
🪄 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: 9c5c5ccd-169c-4260-9f7c-62609fd44f38
📒 Files selected for processing (11)
docs/pgsql_servers_ssl_params.mdinclude/PgSQL_Connection.hinclude/PgSQL_HostGroups_Manager.hinclude/ProxySQL_Admin_Tables_Definitions.hinclude/Servers_SslParams.hlib/PgSQL_Connection.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Monitor.cpplib/ProxySQL_Admin.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
✅ Files skipped from review due to trivial changes (2)
- docs/pgsql_servers_ssl_params.md
- include/ProxySQL_Admin_Tables_Definitions.h
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/PgSQL_HostGroups_Manager.cpp
- lib/ProxySQL_Admin.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 (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/PgSQL_Connection.hinclude/PgSQL_HostGroups_Manager.hlib/PgSQL_Connection.cpplib/PgSQL_Monitor.cppinclude/Servers_SslParams.htest/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/PgSQL_Connection.hinclude/PgSQL_HostGroups_Manager.hinclude/Servers_SslParams.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/PgSQL_Connection.cpplib/PgSQL_Monitor.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/PgSQL_Connection.cpplib/PgSQL_Monitor.cpp
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/pgsql-servers_ssl_params-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
include/PgSQL_HostGroups_Manager.h
📚 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:
lib/PgSQL_Connection.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Use pthread mutexes for synchronization and std::atomic<> for counters
Applied to files:
lib/PgSQL_Connection.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.cpp : Use RAII for resource management and jemalloc for memory allocation
Applied to files:
lib/PgSQL_Connection.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
include/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
include/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ 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/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-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:
test/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-t.cpptest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
🪛 Clang (14.0.6)
include/Servers_SslParams.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
test/tap/tests/pgsql-servers_ssl_params-t.cpp
[error] 11-11: 'unistd.h' file not found
(clang-diagnostic-error)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 7-7: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
| std::mutex PgSQL_Servers_SSL_Params_map_mutex; | ||
| std::unordered_map<std::string, PgSQLServers_SslParams> PgSQL_Servers_SSL_Params_map; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the project-standard mutex and member naming here.
These new members introduce std::mutex and mixed-case member names in a header. Please switch them to the repository's pthread_mutex_t + snake_case pattern before new call sites and helpers build on top of this shape. As per coding guidelines Member variables must use snake_case and Use pthread mutexes for synchronization and std::atomic<> for counters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/PgSQL_HostGroups_Manager.h` around lines 513 - 514, Rename the two
members to follow snake_case and replace std::mutex with the project-standard
pthread_mutex_t: change PgSQL_Servers_SSL_Params_map_mutex ->
pgsql_servers_ssl_params_map_mutex (type pthread_mutex_t) and
PgSQL_Servers_SSL_Params_map -> pgsql_servers_ssl_params_map (keep the same
unordered_map type), and ensure pthread_mutex_init is called where the class is
constructed and pthread_mutex_destroy in the destructor (or appropriate
init/teardown function) so callers using pgsql_servers_ssl_params_map_mutex will
compile and follow project conventions.
| string ssl_cipher; | ||
| string tls_version; | ||
| string comment; | ||
| string MapKey; |
There was a problem hiding this comment.
getMapKey() can return a stale key.
The first call permanently fixes MapKey, but the API still accepts a delimiter and the identity fields remain mutable. A later call with a different del — or a post-construction change to hostname / port / username — returns the wrong key.
Also applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/Servers_SslParams.h` at line 23, getMapKey() currently returns a
cached MapKey which can become stale when delimiter or identity fields change;
either remove the MapKey member and compute/return the key on every call in
getMapKey(), or implement proper cache invalidation by storing the delimiter
(and/or a generation/version) alongside MapKey and clearing/updating MapKey
whenever hostname, port, or username are modified; apply the same change to the
other cached key usage referenced around lines 63-67 so the cached value cannot
be returned stale.
| auto backend = createNewConnection(BACKEND); | ||
| bool query_failed = true; | ||
| if (backend) { | ||
| PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1"); | ||
| query_failed = (PQresultStatus(res) != PGRES_TUPLES_OK); | ||
| PQclear(res); | ||
| } | ||
|
|
||
| ok(query_failed, |
There was a problem hiding this comment.
Don't let a failed client connection count as a passing override test.
query_failed starts as true, so if createNewConnection(BACKEND) returns nullptr the final assertion still passes without exercising the backend path. Add an explicit ok(backend != nullptr, ...) here; that's also the reason main() is currently one assertion short of plan(34).
✅ Minimal fix
auto backend = createNewConnection(BACKEND);
+ ok(backend != nullptr, "Override: client connection to ProxySQL established");
bool query_failed = true;
if (backend) {
PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 393 - 401, The
test currently initializes query_failed=true so a nullptr backend can falsely
make the override test pass; change the logic by (1) adding an explicit
ok(backend != nullptr, "createNewConnection(BACKEND) returned a connection")
immediately after obtaining backend to count the connection assertion, and (2)
initialize query_failed=false and only set query_failed = (PQresultStatus(res)
!= PGRES_TUPLES_OK) after calling PQexec inside the existing if (backend) block
so a missing backend cannot satisfy the later ok(query_failed, ...) check;
reference createNewConnection, backend, query_failed, PQexec and the surrounding
ok(...) assertion.
| long ok_before = getMonitorValue(admin, "PgSQL_Monitor_ssl_connections_OK"); | ||
| diag("With TLSv1 per-server pin, ssl OK before wait: %ld", ok_before); | ||
|
|
||
| usleep(3000000); // 3 seconds — multiple monitor cycles | ||
|
|
||
| long ok_after = getMonitorValue(admin, "PgSQL_Monitor_ssl_connections_OK"); | ||
| diag("With TLSv1 per-server pin, ssl OK after wait: %ld (delta=%ld)", | ||
| ok_after, ok_after - ok_before); | ||
|
|
||
| ok(ok_after == ok_before, | ||
| "Monitor per-server: SSL OK counter does NOT advance when " | ||
| "per-server row pins ssl_protocol_version_range to TLSv1"); |
There was a problem hiding this comment.
Fail if the monitor counter cannot be read.
getMonitorValue() returns -1 on query failure. In that case ok_after == ok_before is true and this negative-path assertion passes without proving anything about monitor behavior.
🩹 Preserve a single TAP assertion while rejecting read failures
- ok(ok_after == ok_before,
+ ok(ok_before >= 0 && ok_after >= 0 && ok_after == ok_before,
"Monitor per-server: SSL OK counter does NOT advance when "
"per-server row pins ssl_protocol_version_range to TLSv1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 478 - 489, The
test currently compares ok_after to ok_before but getMonitorValue() can return
-1 on failure, making the assertion vacuously pass; update the test to first
verify both ok_before and ok_after are valid by checking they are not -1 (e.g.,
assert ok_before != -1 and ok_after != -1 with a TAP failure message indicating
monitor read failure) and only then perform the existing ok(ok_after ==
ok_before, ...) assertion about monitor behavior using the validated values from
getMonitorValue().
- Define CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS macro for remote node fetch - Add Admin_Handler interception so cluster peers can query the table - Add pgsql_servers_ssl_params to get_current_pgsql_table() lookup - Checksum computation was already in place via CUCFT1 in commit()
Tests the PROXY_SELECT cluster query via MySQL admin port: - Inserts SSL params via PgSQL admin, loads to runtime - Connects to MySQL admin port and executes the cluster query - Verifies returned row has correct hostname, ssl_ca, and ssl_protocol_version_range values
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
test/tap/tests/pgsql-servers_ssl_params-t.cpp (3)
416-425:⚠️ Potential issue | 🟠 MajorDon't let a failed client connection satisfy the override test.
query_failedstarts astrue, and there is no assertion thatcreateNewConnection(BACKEND)succeeded. A frontend connect failure therefore makes this negative-path check pass without exercising the backend path; it's also whyplan(37)is currently one assertion too high.💡 Tighten the assertion and fix the plan mismatch at the root cause
auto backend = createNewConnection(BACKEND); - bool query_failed = true; + ok(backend != nullptr, "Override: client connection to ProxySQL established"); + bool query_failed = false; if (backend) { PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1"); query_failed = (PQresultStatus(res) != PGRES_TUPLES_OK); PQclear(res); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 416 - 425, The test currently lets a failed createNewConnection(BACKEND) make query_failed remain true and falsely satisfy the override negative-path; add an explicit assertion that the backend connection succeeded (e.g. ok(backend, "Connected to backend for override test")) before executing PQexec, and only compute query_failed from the PQexec/PQresultStatus path when backend is non-null (or initialize query_failed to false and set it true on PQexec failure). Also adjust the test plan count (plan(...)) to remove the spurious passing assertion so the total assertions match the real checks.
502-513:⚠️ Potential issue | 🟠 MajorValidate the monitor reads before comparing them.
getMonitorValue()returns-1on query failure. If both reads fail,ok_after == ok_beforestill passes without proving that the per-server monitor path was exercised.💡 Keep this as one TAP assertion
- ok(ok_after == ok_before, + ok(ok_before >= 0 && ok_after >= 0 && ok_after == ok_before, "Monitor per-server: SSL OK counter does NOT advance when " "per-server row pins ssl_protocol_version_range to TLSv1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp` around lines 502 - 513, Replace the separate equality check with a single TAP assertion that also verifies both monitor reads succeeded: ensure you use getMonitorValue to populate ok_before and ok_after and then assert ok(ok_before != -1 && ok_after != -1 && ok_after == ok_before, "Monitor per-server: SSL OK counter does NOT advance when per-server row pins ssl_protocol_version_range to TLSv1 and monitor reads succeeded"); keep the diag(...) calls to print ok_before/ok_after for debugging but change the ok(...) call to reference ok_before, ok_after and getMonitorValue so failed reads (-1) cause the assertion to fail rather than silently passing.
111-123:⚠️ Potential issue | 🟠 MajorExercise the file-based SSL params end-to-end, not just the runtime table.
create_bogus_cert_file()is never used, and this test stops after asserting that the row landed inruntime_pgsql_servers_ssl_params. IfPgSQL_Connectionstopped appendingssl_ca/ssl_cert/ssl_keyto libpq conninfo, this suite would still pass.🧪 Minimal extension
exec_ok(admin, "LOAD PGSQL SERVERS TO RUNTIME"); // Verify the params appear in the runtime table for the correct server std::stringstream verify; @@ } PQclear(res); + + create_bogus_cert_file(); + std::stringstream bad_paths; + bad_paths << "UPDATE pgsql_servers_ssl_params SET " + "ssl_ca='" << BOGUS_CERT_PATH << "', " + "ssl_cert='" << BOGUS_CERT_PATH << "', " + "ssl_key='" << BOGUS_CERT_PATH << "' " + "WHERE hostname='" << hostname << "' AND port=" << port; + exec_ok(admin, bad_paths.str().c_str()); + exec_ok(admin, "LOAD PGSQL SERVERS TO RUNTIME"); + + auto backend = createNewConnection(BACKEND); + ok(backend != nullptr, "Per-server file params: client connection to ProxySQL established"); + if (backend) { + PGresult* res = PQexec(backend.get(), "/* create_new_connection=1 */ SELECT 1"); + ok(PQresultStatus(res) != PGRES_TUPLES_OK, + "Per-server file params: bogus ssl_ca/ssl_cert/ssl_key fail end-to-end"); + PQclear(res); + } + remove_bogus_cert_file(); }Also applies to: 317-353
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/ProxySQL_Cluster.hpp`:
- Around line 49-50: The cluster query macro
CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS currently omits the ssl_capath and
ssl_cipher columns, causing changes to those fields to be ignored; update the
macro so the SELECT from runtime_pgsql_servers_ssl_params includes ssl_capath
and ssl_cipher (in the same ordering scheme as the other ssl fields) so the
fetched rowset/checksum reflects all PgSQL SSL columns stored in
pgsql_servers_ssl_params.
In `@lib/Admin_Handler.cpp`:
- Around line 3453-3464: The code is deleting resultset unconditionally which
frees a shared "current table" snapshot returned by
PgHGM->get_current_pgsql_table(); change the ownership handling to only delete
the fallback dump returned by PgHGM->dump_table_pgsql() while leaving the
resultset returned by get_current_pgsql_table() intact. Specifically, when
calling PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params") keep that
pointer alive (do not delete), but if you had to call
PgHGM->dump_table_pgsql("pgsql_servers_ssl_params") as a fallback, delete that
dump after sess->SQLite3_to_MySQL(...) as the MySQL-path does; update the logic
around resultset, sess->SQLite3_to_MySQL(...), and delete resultset so only the
dump_table_pgsql() result is freed and run_query handling remains correct.
---
Duplicate comments:
In `@test/tap/tests/pgsql-servers_ssl_params-t.cpp`:
- Around line 416-425: The test currently lets a failed
createNewConnection(BACKEND) make query_failed remain true and falsely satisfy
the override negative-path; add an explicit assertion that the backend
connection succeeded (e.g. ok(backend, "Connected to backend for override
test")) before executing PQexec, and only compute query_failed from the
PQexec/PQresultStatus path when backend is non-null (or initialize query_failed
to false and set it true on PQexec failure). Also adjust the test plan count
(plan(...)) to remove the spurious passing assertion so the total assertions
match the real checks.
- Around line 502-513: Replace the separate equality check with a single TAP
assertion that also verifies both monitor reads succeeded: ensure you use
getMonitorValue to populate ok_before and ok_after and then assert ok(ok_before
!= -1 && ok_after != -1 && ok_after == ok_before, "Monitor per-server: SSL OK
counter does NOT advance when per-server row pins ssl_protocol_version_range to
TLSv1 and monitor reads succeeded"); keep the diag(...) calls to print
ok_before/ok_after for debugging but change the ok(...) call to reference
ok_before, ok_after and getMonitorValue so failed reads (-1) cause the assertion
to fail rather than silently passing.
🪄 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: c7031fe9-f80c-4de1-9ea7-036784ee69dd
📒 Files selected for processing (4)
include/ProxySQL_Cluster.hpplib/Admin_Handler.cpplib/PgSQL_HostGroups_Manager.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
✅ Files skipped from review due to trivial changes (1)
- lib/PgSQL_HostGroups_Manager.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/ProxySQL_Cluster.hpplib/Admin_Handler.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/ProxySQL_Cluster.hpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/Admin_Handler.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/Admin_Handler.cpp
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/pgsql-servers_ssl_params-t.cpp
🧠 Learnings (11)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
include/ProxySQL_Cluster.hpptest/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
include/ProxySQL_Cluster.hpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
include/ProxySQL_Cluster.hpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
include/ProxySQL_Cluster.hpplib/Admin_Handler.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:
test/tap/tests/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ 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/pgsql-servers_ssl_params-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-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/pgsql-servers_ssl_params-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/pgsql-servers_ssl_params-t.cpp
[error] 11-11: 'unistd.h' file not found
(clang-diagnostic-error)
| /* @brief Query to be intercepted by 'ProxySQL_Admin' for 'runtime_pgsql_servers_ssl_params'. See top comment for details. */ | ||
| #define CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS "PROXY_SELECT hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_crl, ssl_crlpath, ssl_protocol_version_range, comment FROM runtime_pgsql_servers_ssl_params ORDER BY hostname, port, username" |
There was a problem hiding this comment.
Include all PgSQL SSL columns in the cluster query.
This query omits ssl_capath and ssl_cipher, so changes isolated to those fields never affect the fetched resultset/checksum and can be skipped by cluster sync even though pgsql_servers_ssl_params stores them.
💡 Expand the fetched rowset
-#define CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS "PROXY_SELECT hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_crl, ssl_crlpath, ssl_protocol_version_range, comment FROM runtime_pgsql_servers_ssl_params ORDER BY hostname, port, username"
+#define CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS "PROXY_SELECT hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher, ssl_protocol_version_range, comment FROM runtime_pgsql_servers_ssl_params ORDER BY hostname, port, username"📝 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.
| /* @brief Query to be intercepted by 'ProxySQL_Admin' for 'runtime_pgsql_servers_ssl_params'. See top comment for details. */ | |
| #define CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS "PROXY_SELECT hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_crl, ssl_crlpath, ssl_protocol_version_range, comment FROM runtime_pgsql_servers_ssl_params ORDER BY hostname, port, username" | |
| /* `@brief` Query to be intercepted by 'ProxySQL_Admin' for 'runtime_pgsql_servers_ssl_params'. See top comment for details. */ | |
| `#define` CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS "PROXY_SELECT hostname, port, username, ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher, ssl_protocol_version_range, comment FROM runtime_pgsql_servers_ssl_params ORDER BY hostname, port, username" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/ProxySQL_Cluster.hpp` around lines 49 - 50, The cluster query macro
CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS currently omits the ssl_capath and
ssl_cipher columns, causing changes to those fields to be ignored; update the
macro so the SELECT from runtime_pgsql_servers_ssl_params includes ssl_capath
and ssl_cipher (in the same ordering scheme as the other ssl fields) so the
fetched rowset/checksum reflects all PgSQL SSL columns stored in
pgsql_servers_ssl_params.
| GloAdmin->pgsql_servers_wrlock(); | ||
| resultset = PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params"); | ||
| GloAdmin->pgsql_servers_wrunlock(); | ||
|
|
||
| if (resultset == nullptr) { | ||
| resultset = PgHGM->dump_table_pgsql("pgsql_servers_ssl_params"); | ||
| } | ||
|
|
||
| if (resultset) { | ||
| sess->SQLite3_to_MySQL(resultset, error, affected_rows, &sess->client_myds->myprot); | ||
| delete resultset; | ||
| run_query=false; |
There was a problem hiding this comment.
Don't delete the current-table resultset here.
PgHGM->get_current_pgsql_table() looks like the PgSQL twin of the MySQL path a few lines above, and that path only deletes the dump_table_*() fallback. Unconditionally freeing resultset here can tear down the shared runtime snapshot on the first cluster query and make later reads hit UAF/double-free.
💡 Mirror the existing ownership pattern
if (sess->session_type == PROXYSQL_SESSION_ADMIN) {
if (!strncasecmp(CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS, query_no_space, strlen(CLUSTER_QUERY_PGSQL_SERVERS_SSL_PARAMS))) {
+ bool owns_resultset = false;
GloAdmin->pgsql_servers_wrlock();
resultset = PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params");
GloAdmin->pgsql_servers_wrunlock();
if (resultset == nullptr) {
resultset = PgHGM->dump_table_pgsql("pgsql_servers_ssl_params");
+ owns_resultset = true;
}
if (resultset) {
sess->SQLite3_to_MySQL(resultset, error, affected_rows, &sess->client_myds->myprot);
- delete resultset;
+ if (owns_resultset) {
+ delete resultset;
+ }
run_query=false;
goto __run_query;
}
}
}📝 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.
| GloAdmin->pgsql_servers_wrlock(); | |
| resultset = PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params"); | |
| GloAdmin->pgsql_servers_wrunlock(); | |
| if (resultset == nullptr) { | |
| resultset = PgHGM->dump_table_pgsql("pgsql_servers_ssl_params"); | |
| } | |
| if (resultset) { | |
| sess->SQLite3_to_MySQL(resultset, error, affected_rows, &sess->client_myds->myprot); | |
| delete resultset; | |
| run_query=false; | |
| bool owns_resultset = false; | |
| GloAdmin->pgsql_servers_wrlock(); | |
| resultset = PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params"); | |
| GloAdmin->pgsql_servers_wrunlock(); | |
| if (resultset == nullptr) { | |
| resultset = PgHGM->dump_table_pgsql("pgsql_servers_ssl_params"); | |
| owns_resultset = true; | |
| } | |
| if (resultset) { | |
| sess->SQLite3_to_MySQL(resultset, error, affected_rows, &sess->client_myds->myprot); | |
| if (owns_resultset) { | |
| delete resultset; | |
| } | |
| run_query=false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Admin_Handler.cpp` around lines 3453 - 3464, The code is deleting
resultset unconditionally which frees a shared "current table" snapshot returned
by PgHGM->get_current_pgsql_table(); change the ownership handling to only
delete the fallback dump returned by PgHGM->dump_table_pgsql() while leaving the
resultset returned by get_current_pgsql_table() intact. Specifically, when
calling PgHGM->get_current_pgsql_table("pgsql_servers_ssl_params") keep that
pointer alive (do not delete), but if you had to call
PgHGM->dump_table_pgsql("pgsql_servers_ssl_params") as a fallback, delete that
dump after sess->SQLite3_to_MySQL(...) as the MySQL-path does; update the logic
around resultset, sess->SQLite3_to_MySQL(...), and delete resultset so only the
dump_table_pgsql() result is freed and run_query handling remains correct.
parse_tls_version() silently discarded partially-specified ranges like "TLSv1.2-" or "-TLSv1.3", leaving both min and max empty. This meant the user's intent (e.g. "at least TLSv1.2") was lost without any indication. Now one-sided ranges set the specified bound and leave the other side to libpq defaults. A bare "-" logs a proxy_warning.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
include/Servers_SslParams.h (2)
24-41: Consider passing strings by const reference to avoid copies.Constructors take
stringparameters by value, causing an extra copy when callers pass lvalues. Usingconst string&(orstring_viewin C++17) avoids this overhead.♻️ Suggested change for one constructor (apply similarly to others)
- Servers_SslParams(string _h, int _p, string _u, - string ca, string cert, string key, string capath, - string crl, string crlpath, string cipher, string tls, - string c) { + Servers_SslParams(const string& _h, int _p, const string& _u, + const string& ca, const string& cert, const string& key, const string& capath, + const string& crl, const string& crlpath, const string& cipher, const string& tls, + const string& c) {Also applies to: 42-59, 89-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Servers_SslParams.h` around lines 24 - 41, The Servers_SslParams constructor (the one taking parameters _h, _p, _u, ca, cert, key, capath, crl, crlpath, cipher, tls, c) takes many std::string parameters by value causing unnecessary copies; change those string parameters to const std::string& (or std::string_view if you prefer C++17) and apply the same change to the other Servers_SslParams overloads/constructors referenced elsewhere so all string inputs are passed by const reference to avoid copies while keeping assignments to members (e.g., hostname, ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher, tls_version, comment, MapKey) unchanged.
119-120: Implicit dependency on proxy logging header.
proxy_warningis used here butServers_SslParams.hdoesn't include the header that defines it. Any translation unit including this header must ensure the proxy logging macros are available beforehand.💡 Consider adding forward declaration or include
Either add an include for the header defining
proxy_warning, or add a forward declaration/comment documenting the prerequisite:`#include` <string> + +// Requires proxy logging macros (proxy_warning) to be available; +// include "proxysql.h" or equivalent before this header.Alternatively, move
parse_tls_version()out-of-line into a.cppfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Servers_SslParams.h` around lines 119 - 120, The header uses proxy_warning inside parse_tls_version (and references tls_version, hostname, port) but does not include the header that defines the proxy logging macros, creating an implicit dependency; fix by either adding an `#include` for the header that defines proxy_warning to Servers_SslParams.h, or move parse_tls_version out-of-line into a .cpp and include the logging header there, or add a clear comment/forward-declaration documenting that any TU including Servers_SslParams.h must include the proxy logging header first; update references in parse_tls_version accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@include/Servers_SslParams.h`:
- Around line 24-41: The Servers_SslParams constructor (the one taking
parameters _h, _p, _u, ca, cert, key, capath, crl, crlpath, cipher, tls, c)
takes many std::string parameters by value causing unnecessary copies; change
those string parameters to const std::string& (or std::string_view if you prefer
C++17) and apply the same change to the other Servers_SslParams
overloads/constructors referenced elsewhere so all string inputs are passed by
const reference to avoid copies while keeping assignments to members (e.g.,
hostname, ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath,
ssl_cipher, tls_version, comment, MapKey) unchanged.
- Around line 119-120: The header uses proxy_warning inside parse_tls_version
(and references tls_version, hostname, port) but does not include the header
that defines the proxy logging macros, creating an implicit dependency; fix by
either adding an `#include` for the header that defines proxy_warning to
Servers_SslParams.h, or move parse_tls_version out-of-line into a .cpp and
include the logging header there, or add a clear comment/forward-declaration
documenting that any TU including Servers_SslParams.h must include the proxy
logging header first; update references in parse_tls_version accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f874b392-a406-46ca-b303-048fe9ef2584
📒 Files selected for processing (3)
docs/pgsql_servers_ssl_params.mdinclude/Servers_SslParams.htest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
✅ Files skipped from review due to trivial changes (1)
- docs/pgsql_servers_ssl_params.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/Servers_SslParams.htest/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/Servers_SslParams.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
🧠 Learnings (6)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
include/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
include/Servers_SslParams.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ 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/unit/pgsql_servers_ssl_params_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/pgsql_servers_ssl_params_unit-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/unit/pgsql_servers_ssl_params_unit-t.cpp
🪛 Clang (14.0.6)
include/Servers_SslParams.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 7-7: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (5)
include/Servers_SslParams.h (1)
63-67:getMapKey()can return a stale key.The cached
MapKeyis built with the first delimiter and never invalidated. Subsequent calls with a different delimiter return the originally-cached value. Since identity fields are public and mutable, changes after construction also go unnoticed.test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp (4)
1-21: LGTM!The test file correctly includes
test_globals.handtest_init.has required for unit tests that depend on ProxySQL runtime globals. The file naming follows the*-t.cpppattern, and the structure with section comments is well-organized.
336-337: TAP plan count verified.The
plan(71)correctly matches the total assertion count across all test functions (accounting for conditionalok()andskip()paths).
126-192: Thorough coverage of TLS version parsing edge cases.Good test coverage for the
parse_tls_version()logic including full range, single-token pin, empty input, one-sided ranges (TLSv1.2-,-TLSv1.3), and the malformed bare dash case.
255-330: Well-structured lookup tests with proper cleanup.The lookup tests correctly verify exact match, username fallback, miss behavior, copy semantics, and cross-host retrieval. Memory management is handled properly with
deletecalls on returned pointers, andskip()is used appropriately for conditional test paths.
|
2 participants
Summary
Adds per-server SSL configuration for PostgreSQL backends, closing the long-standing gap between the MySQL and PostgreSQL backend SSL stories. Mirrors the existing
mysql_servers_ssl_paramsdesign as closely as possible so the two protocol sides stay symmetric.Problem
Until now, every PostgreSQL backend that ProxySQL connects to with
use_ssl=1was forced to share a single global SSL configuration (thepgsql-ssl_p2s_*variables) — one CA, one client cert, one key, one cipher list, one protocol range, for the entire instance. That made it impossible to:The MySQL side already solved this with
mysql_servers_ssl_params. PostgreSQL had no equivalent.What this PR adds
A new admin table
pgsql_servers_ssl_params, fully integrated into the PgSQL admin → runtime → connection → monitor pipeline.Schema — keyed by
(hostname, port, username)with an empty-username fallback row, exactly like the MySQL table:hostname, port (default 5432), username,
ssl_ca, ssl_cert, ssl_key, ssl_capath, ssl_crl, ssl_crlpath, ssl_cipher,
ssl_protocol_version_range, comment
PRIMARY KEY (hostname, port, username)
ssl_protocol_version_rangeaccepts either a range (TLSv1.2-TLSv1.3) or a single pin (TLSv1.3), and is mapped to libpq'sssl_min_protocol_version/ssl_max_protocol_version.Lookup semantics — exact
(host, port, username)match → empty-username fallback for the same(host, port)→ falls back to the existing globalpgsql-ssl_p2s_*variables. The override is all-or-nothing per row, matching what the backend connection path actually does (the monitor's earlier per-field fallback was inconsistent and has
been aligned).
Changes
New files
include/Servers_SslParams.h— standalone header with the baseServers_SslParamsclass (virtual destructor) plusMySQLServers_SslParamsandPgSQLServers_SslParamsderived classes. Extracted into its own header to break a circular include between
cpp.h,mysql_connection.h, andBase_HostGroups_Manager.h. Removes the duplicatedefinition that previously lived in
MySQL_HostGroups_Manager.h.docs/pgsql_servers_ssl_params.md— user documentation: schema, column reference, range/pin format, lookup hierarchy, usage examples, admin commands, prerequisites, notes onconnection pooling and
/* create_new_connection=1 */.Schema definitions
include/PgSQL_HostGroups_Manager.h— addsMYHGM_PgSQL_SERVERS_SSL_PARAMS(HGM-side in-memory table), thePgSQL_Servers_SSL_Params_map+ mutex, theincoming_pgsql_servers_ssl_paramspointer, the privategenerate_pgsql_servers_ssl_params_table()and the publicget_Server_SSL_Params(hostname, port, username).include/ProxySQL_Admin_Tables_Definitions.h— addsADMIN_SQLITE_TABLE_PGSQL_SERVERS_SSL_PARAMSandADMIN_SQLITE_TABLE_RUNTIME_PGSQL_SERVERS_SSL_PARAMS(admin-sidedefinitions; same schema as the HGM-side, duplicated by the same convention every other pgsql_* table follows).
HostGroups Manager
lib/PgSQL_HostGroups_Manager.cppmydbalongside the existing pgsql tables.generate_pgsql_servers_ssl_params_table()— populatesPgSQL_Servers_SSL_Params_mapfrom the staged incoming rows, mirroring the MySQL implementation.get_Server_SSL_Params()— exact match → empty-username fallback →nullptr.save_incoming_pgsql_table()anddump_table_pgsql()— handlepgsql_servers_ssl_params.commit()— promotes SSL params to runtime after the hostgroup_attributes block.Connection path
lib/PgSQL_Connection.cppconnect_start()— looks up per-server SSL params for the resolved(hostname, port, username), applies all libpq fields if a row is found, otherwise falls back to theglobal
pgsql-ssl_p2s_*variables. Parsesssl_protocol_version_rangefor range vs pin.PgSQL_Backend_Kill_Argsconstructor — applies the same per-server override after the global init so cancel/terminate connections honor per-server SSL too.Monitor
lib/PgSQL_Monitor.cpp—mon_srv_tpopulation now consults per-server SSL params first via a small lambda, then falls back to globals. Aligned with the backend connection'sall-or-nothing behavior (was previously per-field).
Admin plumbing
include/proxysql_admin.h— extendsincoming_pgsql_servers_twithincoming_pgsql_servers_ssl_paramsand bumps the constructor from 4 to 5SQLite3_result*parameters.lib/ProxySQL_Admin.cpppgsql_servers_ssl_paramsto thepgsql_servers_tablenamesvector — without this,LOAD/SAVE PGSQL SERVERS TO/FROM DISKwould silently ignore the new table. (Caughtby code review.)
load_pgsql_servers_to_runtime().save_pgsql_servers_runtime_to_database().incoming_pgsql_servers_twith the new parameter.lib/Admin_Bootstrap.cpp— registerspgsql_servers_ssl_paramsandruntime_pgsql_servers_ssl_paramsin the admin tables list, andpgsql_servers_ssl_paramsin the configtables list.
Tests
Unit tests —
test/tap/tests/unit/pgsql_servers_ssl_params_unit-t.cpp(new, 45 tests)Covers the standalone class layer and the HGM lookup layer end to end:
char*overloads),getMapKey()formatting,get_Server_SSL_Params()lookup paths: exact match, empty-username fallback, total miss, copy semantics on the returned object.Uses
test_init_query_processor()beforetest_init_hostgroups()(GloPTH must exist beforecommit()is called — discovered the hard way via a segfault).Integration tests —
test/tap/tests/pgsql-pgsql_servers_ssl_params-t.cpp(new, 31 tests)Two parts, run against a real ProxySQL connected to a real PostgreSQL backend:
LOAD PGSQL SERVERS TO RUNTIME, multi-row handling,SAVE/LOAD ... TO/FROM DISK,DELETE, UPDATE, default-port behavior.
per-server row exists.
The E2E tests use the
/* create_new_connection=1 */query annotation to force ProxySQL to open a fresh backend connection, bypassing the connection pool — without this, pooledconnections from earlier tests masked the override behavior. (This was the key reliability fix; thanks to the pointer at ProxySQL's own annotation docs.)
Connects via the PgSQL admin port (6132) using libpq, per project convention (PgSQL tests use libpq, never the MySQL client).
Closes #5296
Closes #5582
Summary by CodeRabbit
Documentation
New Features
Tests