Milestone 3 Phase 3.5: Extract hostgroup routing and locking decisions from session#5529
Milestone 3 Phase 3.5: Extract hostgroup routing and locking decisions from session#5529renecannao wants to merge 4 commits intov3.0from
Conversation
…hDecision.cpp The int64_t cast in can_unshun_server() requires <cstdint>, which was not included, causing build failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions
This commit completes Milestone 3 Phase 3.5 by moving hostgroup routing
and locking decisions out of the MySQL_Session and PgSQL_Session classes.
The logic is now encapsulated in standalone, pure functions designed for
easier testing and better maintainability.
Key changes:
- Created include/MySQL_HostGroup_Routing.h and lib/MySQL_HostGroup_Routing.cpp:
- Implements resolve_hostgroup_routing(), a pure function that handles:
- Mirroring logic (highest priority).
- Intercepting SHOW WARNINGS and routing to the previous HG.
- Routing SELECT LAST_INSERT_ID() / @@IDENTITY queries to the HG
where the last affected rows occurred.
- Applying destination_hostgroup from Query Processor Output (QPO).
- Enforcing hostgroup locking (mysql-set_query_lock_on_hostgroup).
- Created include/PgSQL_HostGroup_Routing.h and lib/PgSQL_HostGroup_Routing.cpp:
- Implements resolve_pgsql_hostgroup_routing() with similar logic adapted
for PostgreSQL sessions.
- Refactored lib/MySQL_Session.cpp and lib/PgSQL_Session.cpp:
- Replaced scattered inline logic with calls to the new routing functions.
- Integration points include STATE_SLEEP, STMT_PREPARE, STMT_EXECUTE,
and the main query execution path.
- Enhanced Unit Testing:
- Added test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp (15 test cases).
- Added test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp (10 test cases).
- Verified all 25 test cases pass successfully.
- Updated Build System:
- Modified lib/Makefile to include the new object files.
- Updated test/tap/tests/unit/Makefile to include unit test targets.
This refactoring significantly reduces coupling between the session state
and routing logic, providing a foundation for more robust session
orchestration and simplified unit testing of critical ProxySQL features.
Issue: #5493
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
lib/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/tap/tests/unit/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (5)📚 Learning: 2026-03-22T14:38:16.082ZApplied to files:
📚 Learning: 2026-03-22T14:38:16.082ZApplied to files:
📚 Learning: 2026-01-20T09:34:19.124ZApplied to files:
📚 Learning: 2026-03-22T14:38:16.082ZApplied to files:
📚 Learning: 2026-01-20T07:40:34.938ZApplied to files:
🪛 Clang (14.0.6)test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp[error] 1-1: 'tap.h' file not found (clang-diagnostic-error) test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp[error] 1-1: 'tap.h' file not found (clang-diagnostic-error) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughExtracted hostgroup routing and locking into standalone resolver functions and headers for MySQL and PostgreSQL. Session code now builds routing state, calls resolvers, applies their results (including lock enforcement and error handling). Added unit tests and build targets for both routing implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Session as DB_Session
participant Router as Hostgroup_Router
participant Backend as Backend_Selection
Client->>Session: send query (QPO + flags)
Session->>Session: build Routing_Session_State, Routing_QPO_State
Session->>Router: resolve_hostgroup_routing(sess_state, qpo_state, cfg)
Router-->>Session: Routing_Result (new_current_hostgroup, new_locked_on_hostgroup, lock_hostgroup, error, error_msg)
alt error
Session->>Client: send ERR (error_msg)
Session->>Session: increment locked_queries counter
else success
Session->>Session: apply new_current_hostgroup / new_locked_on_hostgroup
alt lock_hostgroup
Session->>Session: increment lock counters
end
Session->>Backend: route request to selected hostgroup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the hostgroup routing and locking decision-making process for both MySQL and PostgreSQL sessions. By extracting this complex logic into standalone, pure functions, the change aims to enhance the system's testability and maintainability. The refactoring centralizes routing rules, including special handling for mirroring, specific query types, and hostgroup locking, ensuring a more robust and modular architecture. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring. Moving the hostgroup routing and locking logic out of the session classes into pure, standalone functions is an excellent improvement for maintainability and testability. The addition of comprehensive unit tests for the new routing logic is also a great step forward.
I've identified a couple of areas for improvement:
- A potential issue in how
SHOW WARNINGSqueries are detected, which might not cover all variants. - A recurring pattern of using
sprintfwith manually calculated buffer sizes, which poses a risk of buffer overflows.
Overall, this is a high-quality contribution that improves the codebase structure. Addressing the points in the review comments will make it even more robust.
lib/MySQL_Session.cpp
Outdated
| char *buf = (char *)malloc(strlen(err_msg)+strlen(nqn.c_str())+strlen(end)+64); | ||
| sprintf(buf, err_msg, res.new_current_hostgroup, locked_on_hostgroup, nqn.c_str(), end); |
There was a problem hiding this comment.
Using sprintf with a manually calculated buffer size is risky and can lead to buffer overflows. It's safer to use snprintf, first to determine the required buffer size, and then to format the string. This would prevent potential security vulnerabilities.
This comment also applies to similar malloc and sprintf usage on lines 3575-3576, 5443-5444, and in lib/PgSQL_Session.cpp on lines 2180-2181.
lib/MySQL_Session.cpp
Outdated
| if ((dig_len == 13) && (strncasecmp(dig_text, "SHOW WARNINGS", 13) == 0)) { | ||
| qpo_state.is_show_warnings = true; | ||
| } |
There was a problem hiding this comment.
The check for SHOW WARNINGS is very specific and will not match variants like SHOW COUNT(*) WARNINGS, which seems to be an intended use case according to the comment in MySQL_HostGroup_Routing.cpp. This could lead to incorrect routing for such queries. Using strcasestr would be more robust, similar to how LAST_INSERT_ID is checked.
This comment also applies to similar checks on lines 3550, 5423, and 8366.
if (strcasestr(dig_text, "SHOW WARNINGS")) {
qpo_state.is_show_warnings = true;
}There was a problem hiding this comment.
Pull request overview
This PR aims to complete Milestone 3 Phase 3.5 by extracting MySQL/PgSQL hostgroup routing + hostgroup-locking decisions into standalone pure functions, then refactoring session code paths to use them, with added unit tests and build-system updates.
Changes:
- Added new pure routing/locking helpers for MySQL and PgSQL hostgroup selection.
- Refactored
MySQL_SessionandPgSQL_Sessionto call the extracted helpers at several routing decision points. - Added new unit tests and updated unit/lib Makefiles to build the new code.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
include/MySQL_HostGroup_Routing.h |
Declares MySQL routing/locking state structs and resolver API |
lib/MySQL_HostGroup_Routing.cpp |
Implements MySQL pure routing/locking resolver |
include/PgSQL_HostGroup_Routing.h |
Declares PgSQL routing/locking state structs and resolver API |
lib/PgSQL_HostGroup_Routing.cpp |
Implements PgSQL pure routing/locking resolver |
lib/MySQL_Session.cpp |
Replaces inline routing/locking logic with calls to extracted MySQL resolver |
lib/PgSQL_Session.cpp |
Replaces inline routing/locking logic with calls to extracted PgSQL resolver |
test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp |
Adds MySQL routing unit tests |
test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp |
Adds PgSQL routing unit tests |
test/tap/tests/unit/Makefile |
Registers new unit tests and adds custom build rules |
lib/Makefile |
Adds new routing objects to libproxysql.a build |
lib/MonitorHealthDecision.cpp |
Adds missing <cstdint> include |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/MySQL_HostGroup_Routing.cpp
Outdated
| // 4. Default routing from QPO | ||
| if (qpo_state.destination_hostgroup >= 0) { | ||
| if (sess_state.transaction_persistent_hostgroup == -1) { | ||
| result.new_current_hostgroup = qpo_state.destination_hostgroup; | ||
| } | ||
| } | ||
|
|
||
| // 5. Hostgroup Locking Decisions (mysql-set_query_lock_on_hostgroup) | ||
| if (set_query_lock_on_hostgroup == 1) { | ||
| // Algorithm introduced in ProxySQL 2.0.6 | ||
| if (result.new_locked_on_hostgroup < 0) { | ||
| if (qpo_state.is_set_statement) { | ||
| // If it's a SET statement that caused locking (determined by parser) | ||
| // In a pure function, we assume qpo_state.is_set_statement implies it should lock | ||
| result.lock_hostgroup = true; | ||
| result.new_locked_on_hostgroup = result.new_current_hostgroup; | ||
| } | ||
| } | ||
|
|
||
| if (result.new_locked_on_hostgroup >= 0) { | ||
| if (result.new_current_hostgroup != result.new_locked_on_hostgroup) { | ||
| result.error = true; | ||
| result.error_msg = "ProxySQL Error: connection is locked to hostgroup " + | ||
| std::to_string(result.new_locked_on_hostgroup) + | ||
| " but trying to reach hostgroup " + | ||
| std::to_string(result.new_current_hostgroup); | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new pure routing function doesn’t reproduce the existing STATE_SLEEP hostgroup reset behavior. When set_query_lock_on_hostgroup==1 and qpo_state.destination_hostgroup<0, this function leaves new_current_hostgroup unchanged (and can even return an error if current_hostgroup != locked_on_hostgroup). In MySQL_Session::STATE_SLEEP the result is used to set current_hostgroup without checking res.error, so sessions can keep a stale hostgroup instead of resetting to default_hostgroup/locked_on_hostgroup as before.
Suggested fix: add an explicit “sleep/reset” mode/parameter (or a dedicated helper) that implements the old STATE_SLEEP logic: if transaction_persistent_hostgroup==-1 then new_current_hostgroup = (locked_on_hostgroup>=0 ? locked_on_hostgroup : default_hostgroup) when locking is enabled, otherwise default_hostgroup. Use that at the STATE_SLEEP call site(s).
lib/PgSQL_HostGroup_Routing.cpp
Outdated
| // Default routing from QPO | ||
| if (qpo_state.destination_hostgroup >= 0) { | ||
| if (sess_state.transaction_persistent_hostgroup == -1) { | ||
| result.new_current_hostgroup = qpo_state.destination_hostgroup; | ||
| } | ||
| } | ||
|
|
||
| // Hostgroup Locking Decisions (pgsql-set_query_lock_on_hostgroup) | ||
| if (set_query_lock_on_hostgroup == 1) { | ||
| // Algorithm introduced in ProxySQL 2.0.6 | ||
| if (result.new_locked_on_hostgroup < 0) { | ||
| if (qpo_state.lock_hostgroup) { | ||
| result.lock_hostgroup = true; | ||
| result.new_locked_on_hostgroup = result.new_current_hostgroup; | ||
| } | ||
| } | ||
|
|
||
| if (result.new_locked_on_hostgroup >= 0) { | ||
| if (result.new_current_hostgroup != result.new_locked_on_hostgroup) { | ||
| result.error = true; | ||
| result.error_msg = "ProxySQL Error: connection is locked to hostgroup " + | ||
| std::to_string(result.new_locked_on_hostgroup) + | ||
| " but trying to reach hostgroup " + | ||
| std::to_string(result.new_current_hostgroup); | ||
| return result; | ||
| } | ||
| } | ||
| } else { | ||
| // Legacy behavior before 2.0.6 | ||
| if (sess_state.transaction_persistent_hostgroup == -1) { | ||
| if (qpo_state.destination_hostgroup < 0) { | ||
| result.new_current_hostgroup = sess_state.default_hostgroup; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as MySQL: this routing helper doesn’t implement the STATE_SLEEP hostgroup reset semantics. With set_query_lock_on_hostgroup==1 and destination_hostgroup<0, it returns current_hostgroup unchanged and may set error if it differs from locked_on_hostgroup. PgSQL_Session::STATE_SLEEP calls this helper and assigns current_hostgroup from the result without checking res.error, so a session can keep a stale hostgroup (or silently ignore a lock mismatch) instead of selecting default_hostgroup/locked_on_hostgroup as before.
Suggested fix: add a mode/parameter (or separate helper) for the STATE_SLEEP reset decision and use that from the STATE_SLEEP path(s), keeping the current “no-destination means keep current hostgroup” behavior for actual query-routing paths.
| #include <mutex> | ||
| #include <vector> | ||
| #include "tap.h" | ||
| #include "MySQL_HostGroup_Routing.h" | ||
|
|
||
| // TAP noise tool stubs | ||
| extern "C" int get_noise_tools_count() { return 0; } | ||
| extern "C" void stop_noise_tools() {} | ||
| std::mutex noise_failure_mutex; | ||
| std::vector<std::string> noise_failures; | ||
|
|
There was a problem hiding this comment.
The new unit tests bypass the repository’s unit-test harness and duplicate symbols that are already provided by test_globals.cpp (noise stubs + global vectors/mutex). Because the Makefile rules added for these tests don’t link $(TEST_HELPERS_OBJ) / libproxysql.a, the tests end up recompiling production .cpp files directly and re-defining harness globals, which can mask integration issues and diverges from how every other unit test in this directory is built.
Suggested fix: remove these stubs and use the standard harness includes (test_globals.h, test_init.h), call test_init_minimal()/test_cleanup_minimal(), and link against libproxysql.a (i.e., build like the other unit tests in this directory).
| #include <mutex> | ||
| #include <vector> | ||
| #include "tap.h" | ||
| #include "PgSQL_HostGroup_Routing.h" | ||
|
|
||
| // TAP noise tool stubs | ||
| extern "C" int get_noise_tools_count() { return 0; } | ||
| extern "C" void stop_noise_tools() {} | ||
| std::mutex noise_failure_mutex; | ||
| std::vector<std::string> noise_failures; | ||
|
|
There was a problem hiding this comment.
Same as the MySQL variant: this test file duplicates TAP noise stubs and harness globals (already defined in test_globals.cpp) and doesn’t use the standard unit-test harness initialization/cleanup. With the current Makefile rule, it recompiles production sources directly instead of exercising the symbols from libproxysql.a.
Suggested fix: drop the local stubs, include test_globals.h + test_init.h, call test_init_minimal()/test_cleanup_minimal(), and link against libproxysql.a via the standard unit-test build pattern used in this directory.
test/tap/tests/unit/Makefile
Outdated
| MySQL_HostGroup_Routing-t: MySQL_HostGroup_Routing-t.cpp $(PROXYSQL_PATH)/lib/MySQL_HostGroup_Routing.cpp $(ODIR)/tap.o | ||
| $(CXX) $^ $(IDIRS) $(OPT) -o $@ | ||
|
|
||
| PgSQL_HostGroup_Routing-t: PgSQL_HostGroup_Routing-t.cpp $(PROXYSQL_PATH)/lib/PgSQL_HostGroup_Routing.cpp $(ODIR)/tap.o | ||
| $(CXX) $^ $(IDIRS) $(OPT) -o $@ |
There was a problem hiding this comment.
The new test targets are built differently from the rest of the unit tests: they compile lib/*_HostGroup_Routing.cpp directly and only link tap.o, skipping $(TEST_HELPERS_OBJ) and libproxysql.a. This can hide integration/ODR issues and also explains why the new test .cpp files had to redefine TAP noise stubs.
Suggested fix: remove these custom build rules and build these tests like the existing ones in this Makefile (linking
| MySQL_HostGroup_Routing-t: MySQL_HostGroup_Routing-t.cpp $(PROXYSQL_PATH)/lib/MySQL_HostGroup_Routing.cpp $(ODIR)/tap.o | |
| $(CXX) $^ $(IDIRS) $(OPT) -o $@ | |
| PgSQL_HostGroup_Routing-t: PgSQL_HostGroup_Routing-t.cpp $(PROXYSQL_PATH)/lib/PgSQL_HostGroup_Routing.cpp $(ODIR)/tap.o | |
| $(CXX) $^ $(IDIRS) $(OPT) -o $@ | |
| MySQL_HostGroup_Routing-t: MySQL_HostGroup_Routing-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) | |
| $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ | |
| $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ | |
| $(ALLOW_MULTI_DEF) -o $@ | |
| PgSQL_HostGroup_Routing-t: PgSQL_HostGroup_Routing-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) | |
| $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ | |
| $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ | |
| $(ALLOW_MULTI_DEF) -o $@ |
include/MySQL_HostGroup_Routing.h
Outdated
| #ifndef __MYSQL_HOSTGROUP_ROUTING_H | ||
| #define __MYSQL_HOSTGROUP_ROUTING_H |
There was a problem hiding this comment.
The include guard uses a reserved identifier (__MYSQL_HOSTGROUP_ROUTING_H). Identifiers beginning with double underscores are reserved to the implementation and can cause undefined behavior or conflicts with system headers.
Suggested fix: rename the guard to a non-reserved, project-scoped macro (e.g., PROXYSQL_MYSQL_HOSTGROUP_ROUTING_H) and update the closing comment accordingly.
include/PgSQL_HostGroup_Routing.h
Outdated
| #ifndef __PGSQL_HOSTGROUP_ROUTING_H | ||
| #define __PGSQL_HOSTGROUP_ROUTING_H |
There was a problem hiding this comment.
The include guard uses a reserved identifier (__PGSQL_HOSTGROUP_ROUTING_H). Identifiers beginning with double underscores are reserved to the implementation and can cause undefined behavior or conflicts with system headers.
Suggested fix: rename the guard to a non-reserved, project-scoped macro (e.g., PROXYSQL_PGSQL_HOSTGROUP_ROUTING_H) and update the closing comment accordingly.
| struct MySQL_Routing_QPO_State { | ||
| int destination_hostgroup; | ||
| bool is_set_statement; // Derived from query parsing | ||
| bool is_show_warnings; // Derived from query parsing | ||
| bool is_last_insert_id; // Derived from query parsing | ||
| bool is_version_query; // Derived from query parsing | ||
| }; |
There was a problem hiding this comment.
In this extracted API, MySQL_Routing_QPO_State::is_set_statement is used as the “should lock hostgroup” signal (call sites assign it from the lock_hostgroup flag computed by query parsing). The current name reads like a generic query classification, but its semantics are specifically about hostgroup locking (and PgSQL uses lock_hostgroup for the same concept), which makes the API confusing and easy to misuse.
Suggested fix: rename this field to something semantics-based like lock_hostgroup (or should_lock_hostgroup) to match the actual meaning and align with the PgSQL routing API.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
lib/Makefile (1)
91-102: Remove the duplicatePgSQL_Session.ooentry.
PgSQL_Session.oois already listed again in the PgSQL object block below, so keeping it twice just makes_OBJ_CXXharder to audit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Makefile` around lines 91 - 102, The _OBJ_CXX variable contains a duplicate entry for PgSQL_Session.oo; remove the earlier occurrence so PgSQL_Session.oo only appears once in the PgSQL object block. Edit the _OBJ_CXX assignment and delete the duplicate "PgSQL_Session.oo" token (leaving the later PgSQL block intact) to keep the object list unique and easier to audit.test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp (1)
1-10: Use the shared unit-test harness instead of local noise stubs.These file-local
get_noise_tools_count/stop_noise_toolsshims should disappear once the target is built like the rest oftest/tap/tests/unit/*; otherwise this test carries a private copy of the harness contract.As per coding guidelines, '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'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp` around lines 1 - 10, Remove the local TAP noise-tool stubs and test-local globals (delete the functions get_noise_tools_count and stop_noise_tools and the symbols noise_failure_mutex and noise_failures) and instead use the shared test harness by including the harness headers (e.g., add the common test_globals.h and test_init.h includes at the top) so this unit test links against the shared libproxysql test harness rather than carrying a private copy of the contract; keep the test code that uses MySQL_HostGroup_Routing unchanged and rely on the harness-provided noise tools implementations.include/MySQL_HostGroup_Routing.h (1)
15-15: Prefer snake_case for this new public member.
last_HG_affected_rowsis a brand-new API field, so this is the cheapest point to normalize the name before more call sites and tests depend on it.As per coding guidelines, '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/MySQL_HostGroup_Routing.h` at line 15, Rename the new public member last_HG_affected_rows to snake_case last_hg_affected_rows everywhere (declaration in include/MySQL_HostGroup_Routing.h and all uses in implementation, tests, and call sites) to comply with the "Member variables must use snake_case" guideline; update any serialization/marshalling, header comments, and public API references to the new name and run/adjust unit tests and consumers to the renamed symbol to avoid breaking builds.test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp (1)
1-10: Use the shared unit-test harness instead of local noise stubs.These file-local
get_noise_tools_count/stop_noise_toolsshims should disappear once the target is built like the rest oftest/tap/tests/unit/*; otherwise this test carries a private copy of the harness contract.As per coding guidelines, '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'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp` around lines 1 - 10, Remove the file-local noise stubs and related globals (get_noise_tools_count, stop_noise_tools, noise_failure_mutex, noise_failures) and instead use the shared unit-test harness by including test_globals.h and test_init.h at the top of PgSQL_HostGroup_Routing-t.cpp so the test links against libproxysql.a via the common harness; update any initialization or teardown in the test to rely on the harness-provided noise tools functions rather than the local shims.lib/MySQL_Session.cpp (1)
3333-3387: Collapse the extra post-QPO routing blocks.
handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo()now owns the routing step at its__exit_set_destination_hostgrouppath, but these three callers rebuild and re-apply the same state again. The copies are already drifting — for example, theSTMT_EXECUTEvariant has astmt_info->queryfallback that the central block does not — so keeping all of them makes routing behavior order-dependent and harder to audit.Also applies to: 3532-3591, 5405-5459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 3333 - 3387, Duplicate post-QPO routing logic is present in callers (the block building MySQL_Routing_Session_State/MySQL_Routing_QPO_State and calling resolve_hostgroup_routing) causing drift; remove these duplicated blocks and centralize routing in handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo's __exit_set_destination_hostgroup path. Specifically, delete the repeated construction/usage of MySQL_Routing_Session_State, MySQL_Routing_QPO_State and resolve_hostgroup_routing in this snippet (and the other occurrences noted) and instead ensure the central routing code is enhanced to include the STMT_EXECUTE fallback (use stmt_info->query when CurrentQuery.QueryParserArgs.digest_text is absent) so behavior remains identical after consolidation. Ensure updates to status_variables and error handling (generation of ERR packet / RequestEnd) remain invoked from the single centralized routing outcome.
🤖 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/MySQL_HostGroup_Routing.h`:
- Around line 10-44: The structs MySQL_Routing_Session_State,
MySQL_Routing_QPO_State, and MySQL_Routing_Result must get explicit member
initializers so unset hostgroup/int fields default to the safe sentinel -1
(e.g., current_hostgroup, default_hostgroup, locked_on_hostgroup,
transaction_persistent_hostgroup, autocommit_on_hostgroup, last_hg_affected_rows
and destination_hostgroup, new_current_hostgroup, new_locked_on_hostgroup) while
booleans default to false and strings to empty; update the field name
last_HG_affected_rows → last_hg_affected_rows in the struct
MySQL_Routing_Session_State and convert all usages (e.g., where
MySQL_Routing_Session_State is instantiated like MySQL_Session.cpp with {0}) to
rely on the new default initializers or explicitly set values, ensuring call
sites compile and semantics remain the same.
In `@include/PgSQL_HostGroup_Routing.h`:
- Around line 10-36: The structs default-initialize integer members to 0 which
is a valid hostgroup id; update PgSQL_Routing_Session_State,
PgSQL_Routing_QPO_State, and PgSQL_Routing_Result to use in-class initializers
so unset fields default to the -1 sentinel and booleans default to false (e.g.
set PgSQL_Routing_Session_State::current_hostgroup, ::default_hostgroup,
::locked_on_hostgroup, ::transaction_persistent_hostgroup = -1; set
PgSQL_Routing_QPO_State::destination_hostgroup = -1 and ::lock_hostgroup =
false; set PgSQL_Routing_Result::new_current_hostgroup = -1,
::new_locked_on_hostgroup = -1, ::lock_hostgroup = false, ::error = false and
ensure ::error_msg is default-initialized) to prevent accidental routing to
hostgroup 0.
In `@lib/MySQL_HostGroup_Routing.cpp`:
- Around line 37-73: The default-hostgroup fallback should run regardless of
locking mode: when qpo_state.destination_hostgroup < 0 and
sess_state.transaction_persistent_hostgroup == -1, set
result.new_current_hostgroup = sess_state.default_hostgroup; move this check out
of the set_query_lock_on_hostgroup == 1 / else branches so it executes in both
cases (ensure the logic referencing qpo_state.destination_hostgroup,
sess_state.transaction_persistent_hostgroup, sess_state.default_hostgroup, and
result.new_current_hostgroup is preserved and only applied when no explicit
destination exists).
In `@lib/MySQL_Session.cpp`:
- Around line 8376-8379: The SQLSTATE string passed to myprot.generate_pkt_ERR
is mistyped as "Y0000"; update the call in the error branch that checks
res.error so that myprot.generate_pkt_ERR(true, NULL, NULL,
client_myds->pkt_sid+1, 9006, (char *)"HY000", (char*)res.error_msg.c_str())
uses "HY000" instead of "Y0000" (the change should be made where
client_myds->DSS is set to STATE_QUERY_SENT_NET and the
st_var_hostgroup_locked_queries counter is incremented).
- Around line 8360-8372: The code only sets qpo_state.is_show_warnings for the
exact 13-char "SHOW WARNINGS" digest and only when
CurrentQuery.QueryParserArgs.digest_text is present, causing prepared forms like
"SHOW COUNT(*) WARNINGS" to be missed; update the check to (a) consult
CurrentQuery.stmt_info (if available) to detect a SHOW/WARNINGS statement type
or flag, and (b) when falling back to digest_text, use a case-insensitive
substring test for "SHOW" and "WARNINGS" (or just "WARNINGS") rather than the
strict length/equality test; set MySQL_Routing_QPO_State::is_show_warnings
accordingly so routing (e.g., warning_in_hg) works for prepared variants (refer
to MySQL_Routing_QPO_State, qpo_state, CurrentQuery.QueryParserArgs.digest_text,
and CurrentQuery.stmt_info).
In `@lib/PgSQL_Session.cpp`:
- Around line 6126-6151: The extended-protocol paths call
resolve_pgsql_hostgroup_routing (e.g., in the block using
PgSQL_Routing_Session_State/PgSQL_Routing_QPO_State and
pgsql_thread___set_query_lock_on_hostgroup) and may set res.lock_hostgroup but
currently do not increment the per-thread gauges; add the same increment logic
used by the simple-query path so that when res.lock_hostgroup is true you
increment st_var_hostgroup_locked and st_var_hostgroup_locked_set_cmds (and any
related counters) immediately after assigning
current_hostgroup/locked_on_hostgroup; apply the same fix to the other similar
blocks that call resolve_pgsql_hostgroup_routing (the other
Parse/Describe/Bind/Execute handlers) so the destructor’s decrement stays
balanced.
In `@test/tap/tests/unit/Makefile`:
- Around line 293-297: The Makefile targets MySQL_HostGroup_Routing-t and
PgSQL_HostGroup_Routing-t incorrectly link the .cpp sources directly; change
them to link against the proxysql static archive (libproxysql.a /
$(LIBPROXYSQL)) and the test harness objects instead of the .cpp files so the
routing objects are pulled from the library and test_globals.o/test_init.o are
used; update the target dependencies to depend on the test harness
variables/rules (e.g. $(LIBPROXYSQL), $(ODIR)/test_globals.o,
$(ODIR)/test_init.o or the existing test harness macro) and invoke the same
linker flags/IDIRS/OPT as other unit tests so the new tests follow the project
convention.
In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp`:
- Around line 98-112: The second assertion in test_legacy_behavior is vacuous
because current_hostgroup and transaction_persistent_hostgroup are both 10;
change the test to set current_hostgroup to a different value (e.g., 20) while
setting transaction_persistent_hostgroup to 10 so that calling
resolve_hostgroup_routing(sess, qpo, 0) must actively choose
transaction_persistent_hostgroup; update the expectation to check
res.new_current_hostgroup == 10. Refer to test_legacy_behavior,
MySQL_Routing_Session_State.transaction_persistent_hostgroup, current_hostgroup,
and resolve_hostgroup_routing to locate and fix the test.
In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp`:
- Around line 64-80: The second assertion is vacuous because
session.current_hostgroup and session.transaction_persistent_hostgroup are both
10; change the test_legacy_behavior setup so transaction_persistent_hostgroup is
different from current_hostgroup (e.g., set current_hostgroup to 10 and
transaction_persistent_hostgroup to a different value like 20) before calling
resolve_pgsql_hostgroup_routing, then assert the resolver returns the
transaction_persistent_hostgroup value; update references in the test that use
PgSQL_Routing_Session_State, transaction_persistent_hostgroup, current_hostgroup
and the resolve_pgsql_hostgroup_routing call accordingly.
---
Nitpick comments:
In `@include/MySQL_HostGroup_Routing.h`:
- Line 15: Rename the new public member last_HG_affected_rows to snake_case
last_hg_affected_rows everywhere (declaration in
include/MySQL_HostGroup_Routing.h and all uses in implementation, tests, and
call sites) to comply with the "Member variables must use snake_case" guideline;
update any serialization/marshalling, header comments, and public API references
to the new name and run/adjust unit tests and consumers to the renamed symbol to
avoid breaking builds.
In `@lib/Makefile`:
- Around line 91-102: The _OBJ_CXX variable contains a duplicate entry for
PgSQL_Session.oo; remove the earlier occurrence so PgSQL_Session.oo only appears
once in the PgSQL object block. Edit the _OBJ_CXX assignment and delete the
duplicate "PgSQL_Session.oo" token (leaving the later PgSQL block intact) to
keep the object list unique and easier to audit.
In `@lib/MySQL_Session.cpp`:
- Around line 3333-3387: Duplicate post-QPO routing logic is present in callers
(the block building MySQL_Routing_Session_State/MySQL_Routing_QPO_State and
calling resolve_hostgroup_routing) causing drift; remove these duplicated blocks
and centralize routing in
handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo's
__exit_set_destination_hostgroup path. Specifically, delete the repeated
construction/usage of MySQL_Routing_Session_State, MySQL_Routing_QPO_State and
resolve_hostgroup_routing in this snippet (and the other occurrences noted) and
instead ensure the central routing code is enhanced to include the STMT_EXECUTE
fallback (use stmt_info->query when CurrentQuery.QueryParserArgs.digest_text is
absent) so behavior remains identical after consolidation. Ensure updates to
status_variables and error handling (generation of ERR packet / RequestEnd)
remain invoked from the single centralized routing outcome.
In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp`:
- Around line 1-10: Remove the local TAP noise-tool stubs and test-local globals
(delete the functions get_noise_tools_count and stop_noise_tools and the symbols
noise_failure_mutex and noise_failures) and instead use the shared test harness
by including the harness headers (e.g., add the common test_globals.h and
test_init.h includes at the top) so this unit test links against the shared
libproxysql test harness rather than carrying a private copy of the contract;
keep the test code that uses MySQL_HostGroup_Routing unchanged and rely on the
harness-provided noise tools implementations.
In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp`:
- Around line 1-10: Remove the file-local noise stubs and related globals
(get_noise_tools_count, stop_noise_tools, noise_failure_mutex, noise_failures)
and instead use the shared unit-test harness by including test_globals.h and
test_init.h at the top of PgSQL_HostGroup_Routing-t.cpp so the test links
against libproxysql.a via the common harness; update any initialization or
teardown in the test to rely on the harness-provided noise tools functions
rather than the local shims.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4392107a-957d-4333-8f7c-f994dccdcd0c
📒 Files selected for processing (11)
include/MySQL_HostGroup_Routing.hinclude/PgSQL_HostGroup_Routing.hlib/Makefilelib/MonitorHealthDecision.cpplib/MySQL_HostGroup_Routing.cpplib/MySQL_Session.cpplib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-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). (1)
- GitHub Check: Agent
🧰 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:
lib/MonitorHealthDecision.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cppinclude/PgSQL_HostGroup_Routing.htest/tap/tests/unit/MySQL_HostGroup_Routing-t.cppinclude/MySQL_HostGroup_Routing.hlib/MySQL_HostGroup_Routing.cpplib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/MonitorHealthDecision.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpplib/MySQL_HostGroup_Routing.cpplib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/MonitorHealthDecision.cpplib/MySQL_HostGroup_Routing.cpplib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpplib/MySQL_Session.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_HostGroup_Routing-t.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/PgSQL_HostGroup_Routing.hinclude/MySQL_HostGroup_Routing.h
🧠 Learnings (6)
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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:
lib/Makefiletest/tap/tests/unit/Makefilelib/PgSQL_Session.cpplib/MySQL_Session.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
lib/Makefilelib/MySQL_Session.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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:
lib/Makefiletest/tap/tests/unit/Makefiletest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpplib/MySQL_Session.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/Makefiletest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-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_HostGroup_Routing-t.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/MySQL_Session.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
[error] 1-1: 'mutex' file not found
(clang-diagnostic-error)
include/PgSQL_HostGroup_Routing.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp
[error] 1-1: 'mutex' file not found
(clang-diagnostic-error)
include/MySQL_HostGroup_Routing.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
lib/MonitorHealthDecision.cpp (1)
14-14: Good fix: required header added forint64_tusage.Including
<cstdint>is correct here sinceint64_tis used in this translation unit (Line 46).lib/MySQL_Session.cpp (1)
5243-5255: LGTM: the idle-state hostgroup initialization is much easier to follow now.Pushing the
STATE_SLEEPdefault/current hostgroup selection throughresolve_hostgroup_routing()keeps this entry point aligned with the extracted routing rules.
| void test_legacy_behavior() { | ||
| MySQL_Routing_Session_State sess = {0}; | ||
| sess.current_hostgroup = 10; | ||
| sess.default_hostgroup = 5; | ||
| sess.transaction_persistent_hostgroup = -1; | ||
|
|
||
| MySQL_Routing_QPO_State qpo = {0}; | ||
| qpo.destination_hostgroup = -1; // No rule match | ||
|
|
||
| MySQL_Routing_Result res = resolve_hostgroup_routing(sess, qpo, 0); // Legacy mode | ||
| ok(res.new_current_hostgroup == 5, "Legacy: falls back to default_hostgroup when no QPO destination"); | ||
|
|
||
| sess.transaction_persistent_hostgroup = 10; | ||
| res = resolve_hostgroup_routing(sess, qpo, 0); | ||
| ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup"); |
There was a problem hiding this comment.
Make the transaction-persistent assertion non-vacuous.
Here both current_hostgroup and transaction_persistent_hostgroup are 10, so the second assertion still passes if the resolver ignores transaction_persistent_hostgroup and just returns the unchanged current hostgroup.
💡 Tighten the assertion
- sess.transaction_persistent_hostgroup = 10;
+ sess.current_hostgroup = 99;
+ sess.transaction_persistent_hostgroup = 10;
res = resolve_hostgroup_routing(sess, qpo, 0);
ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup");📝 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.
| void test_legacy_behavior() { | |
| MySQL_Routing_Session_State sess = {0}; | |
| sess.current_hostgroup = 10; | |
| sess.default_hostgroup = 5; | |
| sess.transaction_persistent_hostgroup = -1; | |
| MySQL_Routing_QPO_State qpo = {0}; | |
| qpo.destination_hostgroup = -1; // No rule match | |
| MySQL_Routing_Result res = resolve_hostgroup_routing(sess, qpo, 0); // Legacy mode | |
| ok(res.new_current_hostgroup == 5, "Legacy: falls back to default_hostgroup when no QPO destination"); | |
| sess.transaction_persistent_hostgroup = 10; | |
| res = resolve_hostgroup_routing(sess, qpo, 0); | |
| ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup"); | |
| void test_legacy_behavior() { | |
| MySQL_Routing_Session_State sess = {0}; | |
| sess.current_hostgroup = 10; | |
| sess.default_hostgroup = 5; | |
| sess.transaction_persistent_hostgroup = -1; | |
| MySQL_Routing_QPO_State qpo = {0}; | |
| qpo.destination_hostgroup = -1; // No rule match | |
| MySQL_Routing_Result res = resolve_hostgroup_routing(sess, qpo, 0); // Legacy mode | |
| ok(res.new_current_hostgroup == 5, "Legacy: falls back to default_hostgroup when no QPO destination"); | |
| sess.current_hostgroup = 99; | |
| sess.transaction_persistent_hostgroup = 10; | |
| res = resolve_hostgroup_routing(sess, qpo, 0); | |
| ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp` around lines 98 - 112, The
second assertion in test_legacy_behavior is vacuous because current_hostgroup
and transaction_persistent_hostgroup are both 10; change the test to set
current_hostgroup to a different value (e.g., 20) while setting
transaction_persistent_hostgroup to 10 so that calling
resolve_hostgroup_routing(sess, qpo, 0) must actively choose
transaction_persistent_hostgroup; update the expectation to check
res.new_current_hostgroup == 10. Refer to test_legacy_behavior,
MySQL_Routing_Session_State.transaction_persistent_hostgroup, current_hostgroup,
and resolve_hostgroup_routing to locate and fix the test.
| void test_legacy_behavior() { | ||
| PgSQL_Routing_Session_State sess = {0}; | ||
| sess.current_hostgroup = 10; | ||
| sess.default_hostgroup = 5; | ||
| sess.locked_on_hostgroup = -1; | ||
| sess.transaction_persistent_hostgroup = -1; | ||
|
|
||
| PgSQL_Routing_QPO_State qpo = {0}; | ||
| qpo.destination_hostgroup = -1; // No rule match | ||
| qpo.lock_hostgroup = false; | ||
|
|
||
| PgSQL_Routing_Result res = resolve_pgsql_hostgroup_routing(sess, qpo, 0); // Legacy mode | ||
| ok(res.new_current_hostgroup == 5, "Legacy: falls back to default_hostgroup when no QPO destination"); | ||
|
|
||
| sess.transaction_persistent_hostgroup = 10; | ||
| res = resolve_pgsql_hostgroup_routing(sess, qpo, 0); | ||
| ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup"); |
There was a problem hiding this comment.
Make the transaction-persistent assertion non-vacuous.
Here both current_hostgroup and transaction_persistent_hostgroup are 10, so the second assertion still passes if the resolver ignores transaction_persistent_hostgroup and just returns the unchanged current hostgroup.
💡 Tighten the assertion
- sess.transaction_persistent_hostgroup = 10;
+ sess.current_hostgroup = 99;
+ sess.transaction_persistent_hostgroup = 10;
res = resolve_pgsql_hostgroup_routing(sess, qpo, 0);
ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp` around lines 64 - 80, The
second assertion is vacuous because session.current_hostgroup and
session.transaction_persistent_hostgroup are both 10; change the
test_legacy_behavior setup so transaction_persistent_hostgroup is different from
current_hostgroup (e.g., set current_hostgroup to 10 and
transaction_persistent_hostgroup to a different value like 20) before calling
resolve_pgsql_hostgroup_routing, then assert the resolver returns the
transaction_persistent_hostgroup value; update references in the test that use
PgSQL_Routing_Session_State, transaction_persistent_hostgroup, current_hostgroup
and the resolve_pgsql_hostgroup_routing call accordingly.
|
@claude review |
|
Claude finished @renecannao's task in 2m 38s —— View job Code ReviewGood structural improvement — extracting routing decisions into pure, testable functions is the right direction. However, there are several issues to address before merge, including one regression in the build system. 🔴 Blocker
The PR replaces the single Removed from the test suite:
These tests exist in the directory but are no longer built or run. This looks unintentional and is a regression. The original pattern rule ( 🟠 Bugs1. // Current (wrong):
client_myds->myprot.generate_pkt_ERR(true,NULL,NULL,client_myds->pkt_sid+1,9006,(char *)"Y0000", ...);
// All other hostgroup-lock rejections in this file use "HY000"This makes client-side error handling inconsistent for the COM_STMT_EXECUTE lock-rejection path. 2. The extended-query Parse path has a comment acknowledging a gap but doesn't actually increment counters: if (res.lock_hostgroup) {
// PgSQL session doesn't seem to update status variables here like MySQL does,
// but we keep the logic consistent.
}However, 3. The 🟡 Important Issues4. The two new test targets compile the
The consequence is that the noise stubs ( 5. The detection at lines 3353, 3550, 5423, 8366 uses an exact 13-char match for 6. Vacuous test assertions ( In 📝 Minor Issues7. Include guards use reserved identifiers (
8. Project convention requires 9. This field means "should lock hostgroup" (set from the 10. Struct members should have safe default initializers Both headers define structs where integer fields default to ✅ Positives
|
- Restored existing unit tests in test/tap/tests/unit/Makefile that were accidentally removed. - Refactored new unit tests to use standard ProxySQL test harness (test_init_minimal()) and pattern build rule. - Fixed MySQL routing logic to correctly fallback to default_hostgroup when no QPO destination is provided. - Improved SHOW WARNINGS detection using strcasestr. - Added hostgroup lock counter increments in PgSQL extended-query paths to ensure consistency and prevent negative gauges. - Improved safety by replacing sprintf with snprintf for error messages. - Fixed SQLSTATE typo (Y0000 -> HY000). - Renamed struct members and variables to match project conventions. - Standardized include guards. Verified all 25 new test cases and restored existing unit tests pass.
…ions - Updated resolve_hostgroup_routing and resolve_pgsql_hostgroup_routing to explicitly honor transaction_persistent_hostgroup if set. - Improved test_legacy_behavior in unit tests to use non-overlapping hostgroup IDs, ensuring assertions are not vacuous and truly verify persistence logic. - Verified all unit tests pass after logic updates.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp (1)
70-72:⚠️ Potential issue | 🟡 MinorMake the transaction-persistent assertion non-vacuous.
Line 70 sets
transaction_persistent_hostgroupto the same value ascurrent_hostgroup, so this still passes if the resolver just echoes the unchanged current hostgroup.💡 Tighten the test
- sess.transaction_persistent_hostgroup = 10; + sess.current_hostgroup = 99; + sess.transaction_persistent_hostgroup = 10; res = resolve_pgsql_hostgroup_routing(sess, qpo, 0); ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp` around lines 70 - 72, The test is vacuous because sess.transaction_persistent_hostgroup is set equal to sess.current_hostgroup, so change the setup to give transaction_persistent_hostgroup a different value than current_hostgroup (e.g., current_hostgroup=10, transaction_persistent_hostgroup=20) before calling resolve_pgsql_hostgroup_routing(sess, qpo, 0), and then assert that res.new_current_hostgroup equals the transaction_persistent_hostgroup value (and not the original current_hostgroup) to prove the resolver honors the transaction-persistent override; reference symbols: sess.transaction_persistent_hostgroup, sess.current_hostgroup, resolve_pgsql_hostgroup_routing, res.new_current_hostgroup.test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp (1)
104-106:⚠️ Potential issue | 🟡 MinorMake the transaction-persistent assertion non-vacuous.
Line 104 sets
transaction_persistent_hostgroupto the same value ascurrent_hostgroup, so this still passes if the resolver just returns the unchanged current hostgroup.💡 Tighten the test
- sess.transaction_persistent_hostgroup = 10; + sess.current_hostgroup = 99; + sess.transaction_persistent_hostgroup = 10; res = resolve_hostgroup_routing(sess, qpo, 0); ok(res.new_current_hostgroup == 10, "Legacy: remains on transaction_persistent_hostgroup");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp` around lines 104 - 106, The test sets sess.transaction_persistent_hostgroup to the same value as current_hostgroup, making the assertion vacuous; change sess.transaction_persistent_hostgroup to a different value (e.g., 11) before calling resolve_hostgroup_routing and assert that res.new_current_hostgroup equals that transaction-persistent value (sess.transaction_persistent_hostgroup) rather than simply remaining equal to the original current_hostgroup, using resolve_hostgroup_routing, sess.transaction_persistent_hostgroup, and res.new_current_hostgroup to locate and update the assertion.
🧹 Nitpick comments (3)
include/PgSQL_HostGroup_Routing.h (1)
1-2: Use the repo-standard include-guard form.Please rename this guard to the
__CLASS_*_Hstyle used underinclude/so it matches the project convention, e.g.__CLASS_PGSQL_HOSTGROUP_ROUTING_H.♻️ Suggested change
-#ifndef PGSQL_HOSTGROUP_ROUTING_H -#define PGSQL_HOSTGROUP_ROUTING_H +#ifndef __CLASS_PGSQL_HOSTGROUP_ROUTING_H +#define __CLASS_PGSQL_HOSTGROUP_ROUTING_H @@ -#endif // PGSQL_HOSTGROUP_ROUTING_H +#endif // __CLASS_PGSQL_HOSTGROUP_ROUTING_HAs per coding guidelines, "Include guards must follow the pattern
#ifndef_CLASS*_H".Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgSQL_HostGroup_Routing.h` around lines 1 - 2, The include guard in PgSQL_HostGroup_Routing.h uses PGSQL_HOSTGROUP_ROUTING_H but must follow the project convention; rename the guard to __CLASS_PGSQL_HOSTGROUP_ROUTING_H (both the `#ifndef` and corresponding `#define` and the closing `#endif` comment if present) so it matches the repo-standard __CLASS_*_H pattern used across include/ headers.include/MySQL_HostGroup_Routing.h (1)
1-2: Use the repo-standard include-guard form.Please rename this guard to the
__CLASS_*_Hstyle used underinclude/so it matches the project convention, e.g.__CLASS_MYSQL_HOSTGROUP_ROUTING_H.♻️ Suggested change
-#ifndef MYSQL_HOSTGROUP_ROUTING_H -#define MYSQL_HOSTGROUP_ROUTING_H +#ifndef __CLASS_MYSQL_HOSTGROUP_ROUTING_H +#define __CLASS_MYSQL_HOSTGROUP_ROUTING_H @@ -#endif // MYSQL_HOSTGROUP_ROUTING_H +#endif // __CLASS_MYSQL_HOSTGROUP_ROUTING_HAs per coding guidelines, "Include guards must follow the pattern
#ifndef_CLASS*_H".Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_HostGroup_Routing.h` around lines 1 - 2, The current include guard MYSQL_HOSTGROUP_ROUTING_H must be renamed to the repo-standard form __CLASS_MYSQL_HOSTGROUP_ROUTING_H; update both the `#ifndef` and the matching `#define` (and any duplicate occurrence in this header) to use __CLASS_MYSQL_HOSTGROUP_ROUTING_H so the guard follows the pattern required by the coding guidelines and matches other headers like __CLASS_*. Ensure no other macros in this file reference the old name.lib/PgSQL_Session.cpp (1)
2179-2186: Use RAII for the new locked-hostgroup error message.This branch adds another manual
malloc/freepair inlib/*.cpp; building the message withstd::string/string_format()would keep ownership local in an already error-heavy path.As per coding guidelines, `**/*.cpp`: Use RAII for resource management and jemalloc for memory allocation.♻️ Suggested refactor
- char* buf = (char*)malloc(strlen(err_msg) + strlen(nqn.c_str()) + strlen(end) + 64); - sprintf(buf, err_msg, res.new_current_hostgroup, locked_on_hostgroup, nqn.c_str(), end); - client_myds->myprot.generate_error_packet(true, true, buf, PGSQL_ERROR_CODES::ERRCODE_RAISE_EXCEPTION, + std::string buf{}; + string_format(err_msg, buf, res.new_current_hostgroup, locked_on_hostgroup, nqn.c_str(), end); + client_myds->myprot.generate_error_packet(true, true, buf.c_str(), PGSQL_ERROR_CODES::ERRCODE_RAISE_EXCEPTION, false, true); thread->status_variables.stvar[st_var_hostgroup_locked_queries]++; RequestEnd(NULL, true); - free(buf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 2179 - 2186, Replace the manual malloc/free string construction for the locked-hostgroup error with an RAII-managed std::string (or the project's string_format utility) so ownership is automatic and exception-safe: build the message using std::string msg = string_format(err_msg, res.new_current_hostgroup, locked_on_hostgroup, nqn.c_str(), end); then pass msg.c_str() to client_myds->myprot.generate_error_packet(...) and remove the malloc/free and raw buf variable; keep the existing increments (thread->status_variables.stvar[st_var_hostgroup_locked_queries]++) and RequestEnd(NULL, true) calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/PgSQL_HostGroup_Routing.cpp`:
- Around line 8-25: The code seeds result.new_current_hostgroup from
sess_state.current_hostgroup and only avoids overwriting it when
sess_state.transaction_persistent_hostgroup == -1, which can leave a stale
hostgroup; change the selection so that if
sess_state.transaction_persistent_hostgroup != -1 you set
result.new_current_hostgroup = sess_state.transaction_persistent_hostgroup (take
the transaction-persistent hostgroup as the authoritative target), otherwise
apply qpo_state.destination_hostgroup or sess_state.default_hostgroup as
currently done; update any related lock/mismatch logic that relies on
result.new_current_hostgroup to use this value (references:
result.new_current_hostgroup, sess_state.transaction_persistent_hostgroup,
qpo_state.destination_hostgroup).
In `@lib/PgSQL_Session.cpp`:
- Around line 6130-6142: The post-Sync handlers call
resolve_pgsql_hostgroup_routing() a second time but force
PgSQL_Routing_QPO_State.destination_hostgroup = -1 which can overwrite the
previously chosen destination (or previous_hostgroup override); change each
post-Sync handler to preserve the already selected destination by setting
qpo_state.destination_hostgroup to the previously chosen hostgroup (e.g.,
previous_hostgroup or the earlier QPO result) instead of -1 so
resolve_pgsql_hostgroup_routing() keeps the chosen hostgroup; update all four
post-Sync blocks that build PgSQL_Routing_QPO_State (the occurrences around
resolve_pgsql_hostgroup_routing) to use the saved destination rather than -1.
---
Duplicate comments:
In `@test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp`:
- Around line 104-106: The test sets sess.transaction_persistent_hostgroup to
the same value as current_hostgroup, making the assertion vacuous; change
sess.transaction_persistent_hostgroup to a different value (e.g., 11) before
calling resolve_hostgroup_routing and assert that res.new_current_hostgroup
equals that transaction-persistent value (sess.transaction_persistent_hostgroup)
rather than simply remaining equal to the original current_hostgroup, using
resolve_hostgroup_routing, sess.transaction_persistent_hostgroup, and
res.new_current_hostgroup to locate and update the assertion.
In `@test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp`:
- Around line 70-72: The test is vacuous because
sess.transaction_persistent_hostgroup is set equal to sess.current_hostgroup, so
change the setup to give transaction_persistent_hostgroup a different value than
current_hostgroup (e.g., current_hostgroup=10,
transaction_persistent_hostgroup=20) before calling
resolve_pgsql_hostgroup_routing(sess, qpo, 0), and then assert that
res.new_current_hostgroup equals the transaction_persistent_hostgroup value (and
not the original current_hostgroup) to prove the resolver honors the
transaction-persistent override; reference symbols:
sess.transaction_persistent_hostgroup, sess.current_hostgroup,
resolve_pgsql_hostgroup_routing, res.new_current_hostgroup.
---
Nitpick comments:
In `@include/MySQL_HostGroup_Routing.h`:
- Around line 1-2: The current include guard MYSQL_HOSTGROUP_ROUTING_H must be
renamed to the repo-standard form __CLASS_MYSQL_HOSTGROUP_ROUTING_H; update both
the `#ifndef` and the matching `#define` (and any duplicate occurrence in this
header) to use __CLASS_MYSQL_HOSTGROUP_ROUTING_H so the guard follows the
pattern required by the coding guidelines and matches other headers like
__CLASS_*. Ensure no other macros in this file reference the old name.
In `@include/PgSQL_HostGroup_Routing.h`:
- Around line 1-2: The include guard in PgSQL_HostGroup_Routing.h uses
PGSQL_HOSTGROUP_ROUTING_H but must follow the project convention; rename the
guard to __CLASS_PGSQL_HOSTGROUP_ROUTING_H (both the `#ifndef` and corresponding
`#define` and the closing `#endif` comment if present) so it matches the
repo-standard __CLASS_*_H pattern used across include/ headers.
In `@lib/PgSQL_Session.cpp`:
- Around line 2179-2186: Replace the manual malloc/free string construction for
the locked-hostgroup error with an RAII-managed std::string (or the project's
string_format utility) so ownership is automatic and exception-safe: build the
message using std::string msg = string_format(err_msg,
res.new_current_hostgroup, locked_on_hostgroup, nqn.c_str(), end); then pass
msg.c_str() to client_myds->myprot.generate_error_packet(...) and remove the
malloc/free and raw buf variable; keep the existing increments
(thread->status_variables.stvar[st_var_hostgroup_locked_queries]++) and
RequestEnd(NULL, true) calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cca66ac5-79cb-429c-8873-c910bed195f5
📒 Files selected for processing (9)
include/MySQL_HostGroup_Routing.hinclude/PgSQL_HostGroup_Routing.hlib/MySQL_HostGroup_Routing.cpplib/MySQL_Session.cpplib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/MySQL_HostGroup_Routing.cpp
👮 Files not reviewed due to content moderation or server errors (1)
- lib/MySQL_Session.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:
lib/PgSQL_HostGroup_Routing.cppinclude/MySQL_HostGroup_Routing.hlib/PgSQL_Session.cpplib/MySQL_Session.cppinclude/PgSQL_HostGroup_Routing.htest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/PgSQL_HostGroup_Routing.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/MySQL_HostGroup_Routing.hinclude/PgSQL_HostGroup_Routing.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/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Dual-protocol design uses parallel class hierarchies for MySQL and PostgreSQL with the same architecture but protocol-specific implementations
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/Makefilelib/MySQL_Session.cpptest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/tests/unit/Makefileinclude/MySQL_HostGroup_Routing.hlib/PgSQL_Session.cpplib/MySQL_Session.cppinclude/PgSQL_HostGroup_Routing.htest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/Makefiletest/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/tests/unit/Makefile
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
test/tap/tests/unit/Makefileinclude/MySQL_HostGroup_Routing.hlib/MySQL_Session.cppinclude/PgSQL_HostGroup_Routing.h
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to include/**/*.{h,hpp} : Include guards must follow the pattern `#ifndef` __CLASS_*_H
Applied to files:
include/MySQL_HostGroup_Routing.hinclude/PgSQL_HostGroup_Routing.h
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
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/MySQL_Session.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-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/unit/MySQL_HostGroup_Routing-t.cpptest/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
🪛 Clang (14.0.6)
include/MySQL_HostGroup_Routing.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
include/PgSQL_HostGroup_Routing.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp
[error] 1-1: 'tap.h' file not found
(clang-diagnostic-error)
test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp
[error] 1-1: 'tap.h' file not found
(clang-diagnostic-error)
| { | ||
| PgSQL_Routing_Session_State sess_state = {0}; | ||
| sess_state.current_hostgroup = current_hostgroup; | ||
| sess_state.default_hostgroup = default_hostgroup; | ||
| sess_state.locked_on_hostgroup = locked_on_hostgroup; | ||
| sess_state.transaction_persistent_hostgroup = transaction_persistent_hostgroup; | ||
|
|
||
| PgSQL_Routing_QPO_State qpo_state = {0}; | ||
| qpo_state.destination_hostgroup = -1; // Not relevant here as current_hostgroup was already reset to previous | ||
| qpo_state.lock_hostgroup = lock_hostgroup; | ||
|
|
||
| PgSQL_Routing_Result res = resolve_pgsql_hostgroup_routing(sess_state, qpo_state, pgsql_thread___set_query_lock_on_hostgroup); | ||
|
|
There was a problem hiding this comment.
Preserve the chosen hostgroup on the post-Sync resolver pass.
These handlers invoke resolve_pgsql_hostgroup_routing() a second time after the destination has already been selected, but they hardcode destination_hostgroup = -1. The same no-destination path is also used at Line 1978 to fall back to the session/default routing, so this second pass can clobber either the earlier QPO decision or the previous_hostgroup override and send the rest of the Parse/Describe/Bind/Execute cycle to the default HG instead.
💡 Suggested fix
- qpo_state.destination_hostgroup = -1; // Not relevant here as current_hostgroup was already reset to previous
+ qpo_state.destination_hostgroup = current_hostgroup; // Preserve the HG chosen above while still enforcing lock checksApply the same change in all four post-Sync handlers.
Also applies to: 6387-6399, 6544-6556, 6696-6708
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Session.cpp` around lines 6130 - 6142, The post-Sync handlers call
resolve_pgsql_hostgroup_routing() a second time but force
PgSQL_Routing_QPO_State.destination_hostgroup = -1 which can overwrite the
previously chosen destination (or previous_hostgroup override); change each
post-Sync handler to preserve the already selected destination by setting
qpo_state.destination_hostgroup to the previously chosen hostgroup (e.g.,
previous_hostgroup or the earlier QPO result) instead of -1 so
resolve_pgsql_hostgroup_routing() keeps the chosen hostgroup; update all four
post-Sync blocks that build PgSQL_Routing_QPO_State (the occurrences around
resolve_pgsql_hostgroup_routing) to use the saved destination rather than -1.
This PR completes Milestone 3 Phase 3.5 by moving hostgroup routing and locking decisions out of the MySQL_Session and PgSQL_Session classes.
The logic is now encapsulated in standalone, pure functions designed for easier testing and better maintainability.
Key changes:
include/MySQL_HostGroup_Routing.handlib/MySQL_HostGroup_Routing.cpp.resolve_hostgroup_routing(), a pure function that handles:SHOW WARNINGSand routing to the previous HG.SELECT LAST_INSERT_ID()/@@IDENTITYqueries to the HG where the last affected rows occurred.destination_hostgroupfrom Query Processor Output (QPO).mysql-set_query_lock_on_hostgroup).include/PgSQL_HostGroup_Routing.handlib/PgSQL_HostGroup_Routing.cpp.resolve_pgsql_hostgroup_routing()with similar logic adapted for PostgreSQL sessions.lib/MySQL_Session.cppandlib/PgSQL_Session.cppto use the new pure functions.STATE_SLEEP,handler_STMT_PREPARE,handler_STMT_EXECUTE, and the main query execution path.test/tap/tests/unit/MySQL_HostGroup_Routing-t.cpp(15 test cases).test/tap/tests/unit/PgSQL_HostGroup_Routing-t.cpp(10 test cases).lib/Makefileandtest/tap/tests/unit/Makefile.Issue: #5493
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores