feat: per-hostgroup query timeouts.#5772
Conversation
|
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 hostgroup-level ChangesHostgroup-level Query Timeout Override
Sequence Diagram (high-level request flow): sequenceDiagram
participant Client
participant ProxySQL
participant Admin as ProxyAdmin
participant DB
Client->>ProxySQL: Execute query
ProxySQL->>ProxySQL: Determine timeout (check per-rule timeout)
alt no per-rule
ProxySQL->>ProxySQL: Check hostgroup default_query_timeout
else no hostgroup override
ProxySQL->>ProxySQL: Use global default timeout
end
ProxySQL->>DB: Forward query with applied timeout
Admin->>ProxySQL: (tests) update hostgroup settings / query_rules / globals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a per-hostgroup default_query_timeout setting for MySQL and PostgreSQL, allowing for more granular control over query execution limits. The changes include updates to hostgroup attribute structures, JSON parsing logic, and session-level timeout enforcement, alongside new TAP tests for both protocols. Review feedback highlighted potential null pointer dereferences in the session logic when accessing hostgroup attributes through the connection chain, recommending defensive null checks to prevent segmentation faults.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/PgSQL_HostGroups_Manager.cpp`:
- Around line 3805-3808: The validation for default_query_timeout is using
PgSQL_j_get_srv_default_int_val which logs errors under
pgsql_hostgroup_attributes.servers_defaults.*, but the JSON key comes from
hostgroup_settings; update the call so validation errors reference
hostgroup_settings instead: modify the invocation of
PgSQL_j_get_srv_default_int_val (or its wrapper) to pass the correct
context/path for logging (or use the overload that accepts a custom json-path
label) when reading "default_query_timeout", keeping the existing
default_query_timeout_check and assignment to
myhgc->attributes.default_query_timeout unchanged.
In `@lib/PgSQL_Session.cpp`:
- Around line 2765-2767: The read of attributes.default_query_timeout in
SetQueryTimeout() races with admin reload writes; either make the field
std::atomic<int32_t> (change myhgc->attributes.default_query_timeout to
std::atomic<int32_t> and use .load(std::memory_order_relaxed) in
SetQueryTimeout() and .store(...) with a matching memory order in
generate_pgsql_hostgroup_attributes_table()) or protect both the reader
(SetQueryTimeout) and the writer (generate_pgsql_hostgroup_attributes_table
called under wrlock()) with the same lock; update the read at the
SetQueryTimeout site to use .load(...) or acquire the lock accordingly and
update the write path to use .store(...) if switching to atomic.
🪄 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: 70f0b5d0-95ac-4fa7-8641-e8db2c57c76b
📒 Files selected for processing (9)
include/Base_HostGroups_Manager.hlib/BaseHGC.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpptest/tap/tests/test_hostgroup_default_query_timeout-t.cpp
📜 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 naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
lib/BaseHGC.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cppinclude/Base_HostGroups_Manager.htest/tap/tests/test_hostgroup_default_query_timeout-t.cpplib/MySQL_HostGroups_Manager.cpptest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in header files must use
#ifndef __CLASS_*_Hpattern
Files:
include/Base_HostGroups_Manager.h
test/tap/tests/{test_*,-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must follow the naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test binaries are built via pattern rule in
test/tap/tests/Makefile:make <testname>-tcompiles<testname>-t.cppinto<testname>-twithout requiring explicit Makefile targets
Files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpptest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpptest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
🔇 Additional comments (8)
include/Base_HostGroups_Manager.h (1)
313-313: LGTM!lib/BaseHGC.cpp (1)
74-74: LGTM!lib/MySQL_HostGroups_Manager.cpp (2)
5912-5914: LGTM!
5938-5941: LGTM!lib/MySQL_Session.cpp (1)
4749-4757: LGTM!test/tap/groups/groups.json (1)
370-371: LGTM!test/tap/tests/test_hostgroup_default_query_timeout-t.cpp (1)
1-323: LGTM!test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp (1)
1-311: LGTM!
|
Hi @burnison . |
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
0fa9734 to
b0c0321
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp (1)
237-261: 💤 Low valueEarly
return EXIT_FAILUREpaths skip cleanup of already-applied runtime mutations.Once
pgsql-default_query_timeouthas been updated and loaded to runtime (lines 237-240), any subsequentadmin_execfailure in lines 243-261 returnsEXIT_FAILUREwithout running the restore block at 308-320, leaving the global variable and (potentially) the hostgroup attributes row modified for the next test. Consider using a small RAII guard or agoto cleanup;pattern so partial-progress failures still hit the restore path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp` around lines 237 - 261, The test currently returns EXIT_FAILURE directly from multiple admin_exec calls (e.g., the calls that set GLOBAL_TIMEOUT_MS, LOAD PGSQL VARIABLES TO RUNTIME, and subsequent INSERT/DELETE statements around hg_override, RULE_ID_BASE, RULE_TIMEOUT_MS), which skips the restore block and leaves runtime state modified; change the flow so any failure after the initial runtime mutation jumps to a single cleanup/restore path (for example add an RAII guard that tracks “applied_runtime_changes” and restores on destruction, or replace early returns with setting an error flag and goto cleanup;) ensuring admin_exec failures still execute the existing restore logic that resets pgsql-default_query_timeout and removes/rewrites pgsql_hostgroup_attributes and pgsql_query_rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp`:
- Around line 227-234: The test only snapshots pgsql-default_query_timeout but
not the pgsql_hostgroup_attributes row for the discovered override hostgroup
(hg_override), so the DELETE removes a pre-existing row and the restore block
does not re-insert it; add code to query and save the entire
pgsql_hostgroup_attributes row for hg_override before the DELETE (e.g., into a
saved_hostgroup_row string), and in the restore/exit block recreate or re-insert
that row if non-empty (or DELETE only if it was absent originally) using the
same admin SQL helper functions used elsewhere; after restoring/re-inserting the
row, call the existing servers reload routine so runtime reflects the restored
state.
---
Nitpick comments:
In `@test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp`:
- Around line 237-261: The test currently returns EXIT_FAILURE directly from
multiple admin_exec calls (e.g., the calls that set GLOBAL_TIMEOUT_MS, LOAD
PGSQL VARIABLES TO RUNTIME, and subsequent INSERT/DELETE statements around
hg_override, RULE_ID_BASE, RULE_TIMEOUT_MS), which skips the restore block and
leaves runtime state modified; change the flow so any failure after the initial
runtime mutation jumps to a single cleanup/restore path (for example add an RAII
guard that tracks “applied_runtime_changes” and restores on destruction, or
replace early returns with setting an error flag and goto cleanup;) ensuring
admin_exec failures still execute the existing restore logic that resets
pgsql-default_query_timeout and removes/rewrites pgsql_hostgroup_attributes and
pgsql_query_rules.
🪄 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: f6494533-c734-4840-b806-b3e05c5c8123
📒 Files selected for processing (9)
include/Base_HostGroups_Manager.hlib/BaseHGC.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpptest/tap/tests/test_hostgroup_default_query_timeout-t.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/BaseHGC.cpp
- lib/MySQL_HostGroups_Manager.cpp
- lib/PgSQL_HostGroups_Manager.cpp
- test/tap/groups/groups.json
- lib/MySQL_Session.cpp
- test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
📜 Review details
🧰 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 naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
include/Base_HostGroups_Manager.hlib/PgSQL_Session.cpptest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in header files must use
#ifndef __CLASS_*_Hpattern
Files:
include/Base_HostGroups_Manager.h
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test binaries are built via pattern rule in
test/tap/tests/Makefile:make <testname>-tcompiles<testname>-t.cppinto<testname>-twithout requiring explicit Makefile targets
Files:
test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
🔇 Additional comments (2)
include/Base_HostGroups_Manager.h (1)
313-313: LGTM!lib/PgSQL_Session.cpp (1)
2766-2770: Unresolved race ondefault_query_timeoutread inSetQueryTimeout().This read path is still unsynchronized while admin reload can update the same field concurrently; this is the same unresolved issue previously reported in this PR thread.
|
@renecannao, the use-case we're trying to solve here is a number of users that share a common hostgroup that has a significantly higher query timeout threshold than the rest of the fleet (eg. a batch job replica pool). This hostgroup also has a non-negligible number of users being routed to it, in some cases by rules, but mostly by default hostgroup. There's no other way we could determine to affect query timeouts to all queries on the hostgroup beside creating rules for each individual user. Managing and maintaining such rules create a maintenance problem and creates risk of forgetting to create related rules for each individual user when new ones are added or when users are replaced. |
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
b0c0321 to
b7784f1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp (2)
223-224: 💤 Low valueConsider escaping
cl.usernamefor defensive coding.Although
cl.usernamecomes from environment variables (presumably trusted), directly concatenating it into SQL is a potential injection risk. For consistency with the restoration code (which usesmysql_real_escape_stringforsaved_hg_settings), consider escaping the username as well.🛡️ Defensive fix using mysql_real_escape_string
+ std::vector<char> escaped_user(strlen(cl.username) * 2 + 1); + mysql_real_escape_string(admin, escaped_user.data(), cl.username, strlen(cl.username)); const string q = string("SELECT default_hostgroup FROM mysql_users WHERE username = '") - + cl.username + "' LIMIT 1"; + + escaped_user.data() + "' LIMIT 1";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/test_hostgroup_default_query_timeout-t.cpp` around lines 223 - 224, The SQL query builds q by concatenating cl.username directly, which risks injection; before constructing q, allocate a buffer and call mysql_real_escape_string on cl.username (using the existing MYSQL* connection used elsewhere), then use the escaped username when building q (replace direct use of cl.username in the string construction). Ensure you size the buffer to at least (strlen(cl.username) * 2 + 1) and reference mysql_real_escape_string and q to locate the change.
292-292: 💤 Low valueComment uses "SELECT SLEEP" but code uses "DO SLEEP".
The comment says "SELECT SLEEP(5)" but the actual query is "DO SLEEP(5)". Update the comment for consistency.
📝 Proposed fix
- // SELECT SLEEP(5) > global 3s but < hostgroup override 8s: must complete. + // DO SLEEP(5) > global 3s but < hostgroup override 8s: must complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/test_hostgroup_default_query_timeout-t.cpp` at line 292, Update the mismatched test comment so it reflects the actual query used: change the comment that says "SELECT SLEEP(5)" to "DO SLEEP(5)" (or otherwise mention "DO SLEEP(5) > global 3s but < hostgroup override 8s") so the comment and the executed query in test_hostgroup_default_query_timeout-t.cpp are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/tap/tests/test_hostgroup_default_query_timeout-t.cpp`:
- Around line 223-224: The SQL query builds q by concatenating cl.username
directly, which risks injection; before constructing q, allocate a buffer and
call mysql_real_escape_string on cl.username (using the existing MYSQL*
connection used elsewhere), then use the escaped username when building q
(replace direct use of cl.username in the string construction). Ensure you size
the buffer to at least (strlen(cl.username) * 2 + 1) and reference
mysql_real_escape_string and q to locate the change.
- Line 292: Update the mismatched test comment so it reflects the actual query
used: change the comment that says "SELECT SLEEP(5)" to "DO SLEEP(5)" (or
otherwise mention "DO SLEEP(5) > global 3s but < hostgroup override 8s") so the
comment and the executed query in test_hostgroup_default_query_timeout-t.cpp are
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 381c78d7-23b8-41e3-9d0f-63424c4e10de
📒 Files selected for processing (9)
include/Base_HostGroups_Manager.hlib/BaseHGC.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpptest/tap/tests/test_hostgroup_default_query_timeout-t.cpp
✅ Files skipped from review due to trivial changes (1)
- lib/BaseHGC.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- include/Base_HostGroups_Manager.h
- lib/PgSQL_HostGroups_Manager.cpp
- lib/PgSQL_Session.cpp
- lib/MySQL_HostGroups_Manager.cpp
- lib/MySQL_Session.cpp
- test/tap/groups/groups.json
- test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
📜 Review details
🧰 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 naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
test/tap/tests/{test_*,-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must follow the naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test binaries are built via pattern rule in
test/tap/tests/Makefile:make <testname>-tcompiles<testname>-t.cppinto<testname>-twithout requiring explicit Makefile targets
Files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
🔇 Additional comments (8)
test/tap/tests/test_hostgroup_default_query_timeout-t.cpp (8)
41-56: LGTM!
58-90: LGTM!
94-109: LGTM!
111-142: LGTM!
144-168: LGTM!
170-201: LGTM!
256-289: LGTM!
309-370: LGTM!
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
b7784f1 to
218796c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/PgSQL_HostGroups_Manager.cpp (1)
3805-3808:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading error source path for
default_query_timeoutvalidation.The validation uses
PgSQL_j_get_srv_default_int_valwhich logs errors aspgsql_hostgroup_attributes.servers_defaults.default_query_timeout, but this field is actually read fromhostgroup_settings(as documented in lines 3784-3786). This makes runtime diagnostics confusing when validation fails.The same issue affects
handle_warningson line 3802.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/PgSQL_HostGroups_Manager.cpp` around lines 3805 - 3808, The validation/error logging for default_query_timeout and handle_warnings incorrectly attributes errors to pgsql_hostgroup_attributes.servers_defaults; update the calls that use PgSQL_j_get_srv_default_int_val (for default_query_timeout) and the call that validates handle_warnings so they reference the correct JSON path "hostgroup_settings.<field>" (e.g., "hostgroup_settings.default_query_timeout" and "hostgroup_settings.handle_warnings") or use the variant/parameter of PgSQL_j_get_srv_default_int_val that allows supplying the explicit source path, and then assign the result to myhgc->attributes.default_query_timeout as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@lib/PgSQL_HostGroups_Manager.cpp`:
- Around line 3805-3808: The validation/error logging for default_query_timeout
and handle_warnings incorrectly attributes errors to
pgsql_hostgroup_attributes.servers_defaults; update the calls that use
PgSQL_j_get_srv_default_int_val (for default_query_timeout) and the call that
validates handle_warnings so they reference the correct JSON path
"hostgroup_settings.<field>" (e.g., "hostgroup_settings.default_query_timeout"
and "hostgroup_settings.handle_warnings") or use the variant/parameter of
PgSQL_j_get_srv_default_int_val that allows supplying the explicit source path,
and then assign the result to myhgc->attributes.default_query_timeout as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 416c3bb3-525a-4335-93b3-06933f12a761
📒 Files selected for processing (9)
include/Base_HostGroups_Manager.hlib/BaseHGC.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpptest/tap/tests/test_hostgroup_default_query_timeout-t.cpp
✅ Files skipped from review due to trivial changes (1)
- lib/BaseHGC.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/MySQL_Session.cpp
- lib/PgSQL_Session.cpp
- test/tap/groups/groups.json
- lib/MySQL_HostGroups_Manager.cpp
- test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
- test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/Base_HostGroups_Manager.hlib/PgSQL_HostGroups_Manager.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards should use
#ifndef_CLASS*_H format
Files:
include/Base_HostGroups_Manager.h
🔇 Additional comments (2)
include/Base_HostGroups_Manager.h (1)
314-314: LGTM!lib/PgSQL_HostGroups_Manager.cpp (1)
3784-3786: LGTM!Also applies to: 3805-3806
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
218796c to
63c3ab7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp`:
- Around line 258-325: Multiple early returns after calling admin_exec leave DB
mutations (global_variables, pgsql_hostgroup_attributes, pgsql_query_rules) in
place and skip the intended restore block; ensure restoration always runs by
routing all exits through a single cleanup path. Implement a scope-guard/RAII or
a restore lambda (call it e.g. restore_runtime) that performs the existing
restore logic and ensure it is invoked in a destructor or explicitly before
every EXIT_FAILURE return (or replace early returns with goto/return to a single
cleanup label). Update code sections that call admin_exec, and the test helpers
run_success_case/run_kill_case, so any failure or test exit triggers the restore
function and guarantees the snapshot is restored.
🪄 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: 75e70ea7-b331-4d9f-b6a9-a5651ee55414
📒 Files selected for processing (9)
include/Base_HostGroups_Manager.hlib/BaseHGC.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpptest/tap/tests/test_hostgroup_default_query_timeout-t.cpp
✅ Files skipped from review due to trivial changes (2)
- lib/BaseHGC.cpp
- test/tap/groups/groups.json
🚧 Files skipped from review as they are similar to previous changes (5)
- include/Base_HostGroups_Manager.h
- lib/PgSQL_HostGroups_Manager.cpp
- lib/MySQL_Session.cpp
- lib/MySQL_HostGroups_Manager.cpp
- test/tap/tests/test_hostgroup_default_query_timeout-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/PgSQL_Session.cpptest/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/pgsql-hostgroup_default_query_timeout-t.cpp
🧠 Learnings (1)
📚 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-hostgroup_default_query_timeout-t.cpp
🔇 Additional comments (2)
lib/PgSQL_Session.cpp (2)
2767-2770: Please confirm this hostgroup timeout read is synchronized with admin reload writes.This appears to be the same unresolved read-path concern already raised earlier; if
attributes.default_query_timeoutis still non-atomic/non-locked, this remains racy.
2771-2779: LGTM!
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
2 times, most recently
from
7efc0dd to
8d264e5
Compare
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
8d264e5 to
acb5742
Compare
We have a use-case where we'd want to set query timeouts on a hostgroup
basis. This is currently only possible globally or by using query rules,
but I'd like to be able to handle this in constant-time and without
having to pin the rule to a specific user.
This change adds a `default_query_timeout` JSON pseudo-attribute in the
`mysql_hostgroup_attributes.hostgroup_settings` column, which will
override the global default, but will, itself, be overridden by a rule.
```
INSERT INTO mysql_hostgroup_attributes (hostgroup_id, hostgroup_settings)
VALUES (100, '{"default_query_timeout": 8000}');
LOAD MYSQL SERVERS TO RUNTIME;
```
burnison
force-pushed
the
hostgroup_attribute_timeouts
branch
from
acb5742 to
52ba60b
Compare
|
|
Hi @burnison , We will try to review this in time for 3.0.10 . Thank you for your patience |
…roup_settings init_myhgc_hostgroup_settings is defined in three translation units (Base_HostGroups_Manager.cpp, MySQL_HostGroups_Manager.cpp, and PgSQL_HostGroups_Manager.cpp), all archived into the same libproxysql.a. PR sysown#5772 updated the MySQL and PgSQL copies to parse the new 'hostgroup_settings.default_query_timeout' field, but not the Base copy. Because the Base copy also defines generate_mysql_hostgroup_attributes_table, the linker could bind the Base (unchanged) variant depending on member resolution order, causing the feature to silently fail to apply. Mirror the parsing block and doc-comment update so all three copies agree, making the feature linker-independent.
|
2 participants
We have a use-case where we'd want to set query timeouts on a per-hostgroup basis. This is currently only possible globally or by using query rules, but I'd like to be able to handle this in constant-time and without having to pin the rule to a specific user.
This change adds a
default_query_timeoutJSON pseudo-attribute in themysql_hostgroup_attributes.hostgroup_settingscolumn, which will override the global default, but will, itself, be overridden by a rule.Summary by CodeRabbit
New Features
Behavior
Tests