Add destination_schema support to mysql_query_rules attributes - #5925
Add destination_schema support to mysql_query_rules attributes#5925peterlyoo wants to merge 4 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🪛 Cppcheck (2.21.0)lib/MySQL_Query_Processor.cpp[warning] 781-781: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) [warning] 919-919: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) lib/PgSQL_Query_Processor.cpp[warning] 373-373: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) [warning] 509-509: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) 🔇 Additional comments (4)
📝 WalkthroughWalkthroughThis PR adds ChangesDestination schema routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 logical database routing via a new destination_schema attribute in mysql_query_rules, allowing the session schema to be switched before backend connection selection. A critical issue was identified in lib/MySQL_Session.cpp where applying the destination_schema at the end of the handler function can cause query cache key mismatches on the first query and bypasses the schema switch entirely on query cache hits. It is recommended to apply the schema switch earlier in the function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (qpo->destination_schema) { | ||
| // switch the session schema before backend connection selection: the | ||
| // connection pool matches on (username, schemaname) and issues | ||
| // COM_INIT_DB on schema mismatch, so the query lands on this schema | ||
| client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema)); | ||
| } |
There was a problem hiding this comment.
Applying destination_schema at the very end of handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo has two significant drawbacks:
- Query Cache Key Mismatch on First Query: The query cache lookup (which happens earlier in the same function at line 7533 using
client_myds->myconn->userinfo->hash) will use the pre-remapped schema's hash for the first query of a connection, leading to a cache miss/mismatch. - Missing Schema Switch on Cache Hits: If a query is a cache hit, the function returns
trueearly (line 7556) and bypasses the rest of the function, meaning the session's schema is never switched todestination_schemafor subsequent queries.
To resolve both issues, we should apply destination_schema earlier in the function (e.g., right before the cache_ttl check or before the ps_type_execute_stmt check). This ensures the cache lookup uses the correct remapped schema hash and that the schema is consistently switched even on cache hits.
There was a problem hiding this comment.
Fixed in 652ffa1 — both points were valid:
- The schema switch now happens right after the
error_msg/OK_msghandling and before the query cache lookup, so the cache key (userinfo->hash) is computed with the remapped schema from the first query. - Since the switch precedes the
cache_ttlblock, a cache hit no longer bypasses it — the session stays on the remapped schema even when the resultset is served from cache.
Added a query-cache interaction case to the TAP test (rule with cache_ttl + destination_schema; asserts the second query is a real cache hit via Query_Cache_count_GET_OK and that DATABASE() still reports the remapped schema afterwards). 12/12 assertions pass.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/query_processor.h (1)
242-242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
destination_schemainQuery_Processor_Output::get_info_json.destination_schemais populated and used for routing, but this JSON dump omits it, so the field never appears in observability output.🤖 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 `@include/query_processor.h` at line 242, Query_Processor_Output::get_info_json currently omits the populated destination_schema field from its JSON output, so observability data is incomplete. Update get_info_json to serialize destination_schema alongside the other Query_Processor_Output fields, keeping the existing JSON structure consistent and ensuring the field is included whenever the object is dumped.
🧹 Nitpick comments (1)
include/query_processor.h (1)
234-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet
attributesandcommentto NULL after free for consistency.The new
destination_schemacorrectly sets the pointer toNULLafterfree(), but the pre-existingattributes(line 231-233) andcomment(line 238-240) do not. Ifdestroy()is called more than once, those fields become double-free risks whiledestination_schemaremains safe. Aligning all three prevents that class of bug.♻️ Proposed fix
if (attributes) { free(attributes); + attributes=NULL; } if (destination_schema) { free(destination_schema); destination_schema=NULL; } if (comment) { // `#643` free(comment); + comment=NULL; }🤖 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 `@include/query_processor.h` around lines 234 - 237, In `destroy()`, the `destination_schema` cleanup already clears the pointer after `free()`, but `attributes` and `comment` should be handled the same way for consistency and double-free safety. Update the cleanup blocks for `attributes`, `destination_schema`, and `comment` so each pointer is set to `NULL` immediately after `free()`, matching the existing `destination_schema` pattern.
🤖 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/MySQL_Query_Processor.cpp`:
- Around line 825-835: The handling of destination_schema in the
MySQL_Query_Processor logic silently ignores empty string values, so update both
overloads that parse this field to emit a proxy_warning when dest_schema is a
string but s.length() == 0. Keep the existing proxy_error behavior for
non-string types, and use the destination_schema parsing block in
MySQL_Query_Processor (the branch around j_attributes.find("destination_schema")
and the matching overload) to locate and mirror the fix consistently.
In `@lib/MySQL_Session.cpp`:
- Around line 7570-7575: The schema update in MySQL_Session::... is happening
too early and can persist even when the hostgroup lock check rejects the query.
Move the qpo->destination_schema handling so it only runs after the
locked_on_hostgroup validation succeeds, and keep it aligned with the existing
destination_hostgroup transaction-persistence guard. Use the existing
client_myds->myconn->userinfo->set_schemaname path, but apply it only after the
session is confirmed eligible to switch hostgroups/schemas.
---
Outside diff comments:
In `@include/query_processor.h`:
- Line 242: Query_Processor_Output::get_info_json currently omits the populated
destination_schema field from its JSON output, so observability data is
incomplete. Update get_info_json to serialize destination_schema alongside the
other Query_Processor_Output fields, keeping the existing JSON structure
consistent and ensuring the field is included whenever the object is dumped.
---
Nitpick comments:
In `@include/query_processor.h`:
- Around line 234-237: In `destroy()`, the `destination_schema` cleanup already
clears the pointer after `free()`, but `attributes` and `comment` should be
handled the same way for consistency and double-free safety. Update the cleanup
blocks for `attributes`, `destination_schema`, and `comment` so each pointer is
set to `NULL` immediately after `free()`, matching the existing
`destination_schema` pattern.
🪄 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: 87ac576d-a956-4b4a-83bf-767a640f65d3
📒 Files selected for processing (7)
include/query_processor.hlib/MySQL_Query_Processor.cpplib/MySQL_Session.cpplib/PgSQL_Query_Processor.cpplib/Query_Processor.cpptest/tap/groups/groups.jsontest/tap/tests/mysql-dest_schema_routing-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 usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
lib/PgSQL_Query_Processor.cpplib/Query_Processor.cpplib/MySQL_Session.cppinclude/query_processor.hlib/MySQL_Query_Processor.cpptest/tap/tests/mysql-dest_schema_routing-t.cpp
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/query_processor.h
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/mysql-dest_schema_routing-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/mysql-dest_schema_routing-t.cpp
🪛 Cppcheck (2.21.0)
lib/PgSQL_Query_Processor.cpp
[warning] 373-373: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 509-509: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/MySQL_Query_Processor.cpp
[warning] 755-755: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 903-903: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (7)
lib/MySQL_Session.cpp (1)
7570-7575: No change neededset_schemanamecopies the schema into owned storage and already short-circuits when the value is unchanged.> Likely an incorrect or invalid review comment.include/query_processor.h (1)
134-134: LGTM!Also applies to: 178-178, 217-217
lib/Query_Processor.cpp (1)
392-393: LGTM!Also applies to: 1911-1917
lib/MySQL_Query_Processor.cpp (1)
755-755: LGTM!Also applies to: 903-903
lib/PgSQL_Query_Processor.cpp (1)
373-373: LGTM!Also applies to: 509-509
test/tap/groups/groups.json (1)
84-84: LGTM!test/tap/tests/mysql-dest_schema_routing-t.cpp (1)
1-9: LGTM!Also applies to: 11-17, 19-23, 25-31, 33-51, 53-64, 66-188
| if (qpo->destination_schema) { | ||
| // switch the session schema before backend connection selection: the | ||
| // connection pool matches on (username, schemaname) and issues | ||
| // COM_INIT_DB on schema mismatch, so the query lands on this schema | ||
| client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Schema mutation before hostgroup lock validation can corrupt session state.
The set_schemaname call at line 7574 mutates the client session's schema before the locked_on_hostgroup check at lines 7577-7590. If that check fails (e.g., qpo->destination_hostgroup routes to a different hostgroup than the one the session is locked to), the function returns an error at line 7587 — but the schema change has already persisted on client_myds->myconn->userinfo. The next query from the client will see the mutated schema, even though the current query was rejected.
Additionally, unlike destination_hostgroup (which is guarded by transaction_persistent_hostgroup == -1 at line 7566), destination_schema is applied unconditionally. If a transaction is active and the session is pinned to a hostgroup, switching the schema can cause the backend pool to issue COM_INIT_DB on a connection that should remain sticky, potentially breaking transaction semantics.
🔒 Proposed fix: move schema switch after the lock check
__exit_set_destination_hostgroup:
if ( qpo->next_query_flagIN >= 0 ) {
next_query_flagIN=qpo->next_query_flagIN;
}
if ( qpo->destination_hostgroup >= 0 ) {
if (transaction_persistent_hostgroup == -1) {
current_hostgroup=qpo->destination_hostgroup;
}
}
- if (qpo->destination_schema) {
- // switch the session schema before backend connection selection: the
- // connection pool matches on (username, schemaname) and issues
- // COM_INIT_DB on schema mismatch, so the query lands on this schema
- client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema));
- }
if (mysql_thread___set_query_lock_on_hostgroup == 1) { // algorithm introduced in 2.0.6
if (locked_on_hostgroup >= 0) {
if (current_hostgroup != locked_on_hostgroup) {
client_myds->DSS=STATE_QUERY_SENT_NET;
char buf[140];
sprintf(buf,"ProxySQL Error: connection is locked to hostgroup %d but trying to reach hostgroup %d", locked_on_hostgroup, current_hostgroup);
client_myds->myprot.generate_pkt_ERR(true,NULL,NULL,client_myds->pkt_sid+1,9006,(char *)"Y0000",buf);
thread->status_variables.stvar[st_var_hostgroup_locked_queries]++;
RequestEnd(NULL, 9006, buf);
l_free(pkt->size,pkt->ptr);
return true;
}
}
}
+ if (qpo->destination_schema) {
+ // switch the session schema before backend connection selection: the
+ // connection pool matches on (username, schemaname) and issues
+ // COM_INIT_DB on schema mismatch, so the query lands on this schema
+ client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema));
+ }
return false;📝 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.
| if (qpo->destination_schema) { | |
| // switch the session schema before backend connection selection: the | |
| // connection pool matches on (username, schemaname) and issues | |
| // COM_INIT_DB on schema mismatch, so the query lands on this schema | |
| client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema)); | |
| } | |
| __exit_set_destination_hostgroup: | |
| if ( qpo->next_query_flagIN >= 0 ) { | |
| next_query_flagIN=qpo->next_query_flagIN; | |
| } | |
| if ( qpo->destination_hostgroup >= 0 ) { | |
| if (transaction_persistent_hostgroup == -1) { | |
| current_hostgroup=qpo->destination_hostgroup; | |
| } | |
| } | |
| if (mysql_thread___set_query_lock_on_hostgroup == 1) { // algorithm introduced in 2.0.6 | |
| if (locked_on_hostgroup >= 0) { | |
| if (current_hostgroup != locked_on_hostgroup) { | |
| client_myds->DSS=STATE_QUERY_SENT_NET; | |
| char buf[140]; | |
| sprintf(buf,"ProxySQL Error: connection is locked to hostgroup %d but trying to reach hostgroup %d", locked_on_hostgroup, current_hostgroup); | |
| client_myds->myprot.generate_pkt_ERR(true,NULL,NULL,client_myds->pkt_sid+1,9006,(char *)"Y0000",buf); | |
| thread->status_variables.stvar[st_var_hostgroup_locked_queries]++; | |
| RequestEnd(NULL, 9006, buf); | |
| l_free(pkt->size,pkt->ptr); | |
| return true; | |
| } | |
| } | |
| } | |
| if (qpo->destination_schema) { | |
| // switch the session schema before backend connection selection: the | |
| // connection pool matches on (username, schemaname) and issues | |
| // COM_INIT_DB on schema mismatch, so the query lands on this schema | |
| client_myds->myconn->userinfo->set_schemaname(qpo->destination_schema, strlen(qpo->destination_schema)); | |
| } | |
| return false; |
🤖 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/MySQL_Session.cpp` around lines 7570 - 7575, The schema update in
MySQL_Session::... is happening too early and can persist even when the
hostgroup lock check rejects the query. Move the qpo->destination_schema
handling so it only runs after the locked_on_hostgroup validation succeeds, and
keep it aligned with the existing destination_hostgroup transaction-persistence
guard. Use the existing client_myds->myconn->userinfo->set_schemaname path, but
apply it only after the session is confirmed eligible to switch
hostgroups/schemas.
There was a problem hiding this comment.
Partially applied in 652ffa1, with reasoning for the part I did not take as-is:
What changed: the switch now runs right after the error_msg/OK_msg early-returns — so a query blocked by a rule no longer mutates the session schema, which addresses the "rejected query should have no side effects" principle for the rule-level rejection paths.
Why not after the locked_on_hostgroup check: that placement conflicts with the query-cache correctness issue raised in the other review thread — the cache lookup (userinfo->hash) happens before the label, and on a cache hit the function returns early and would never reach a switch placed after the lock check. The two placements are mutually exclusive, and the cache-hit bypass is the more severe defect (silent wrong-schema persistence for every cached query). On the lock-rejection edge: destination_schema is session-persistent by design (same semantics as a client-issued USE), and since query rules re-apply on every subsequent matching query, the post-rejection state is the same state every accepted query would produce anyway.
On the transaction concern: switching the default schema mid-transaction via COM_INIT_DB does not commit or invalidate the transaction in MySQL — it is exactly what happens today when a client issues USE inside a transaction through ProxySQL. The sticky backend connection is preserved; only its default schema changes. Guarding on transaction_persistent_hostgroup would make the rule silently inert mid-transaction, which seems more surprising than the current behavior. Happy to add the guard if the maintainers prefer symmetry with destination_hostgroup.
There was a problem hiding this comment.
@peterlyoo Thanks for the detailed write-up — this makes sense, and I agree with your reasoning on both points.
- Cache-hit bypass: you're right that the
GloMyQC->get(...)lookup (keyed onuserinfo->hash) happens before the__exit_set_destination_hostgrouplabel and returns early on a hit. Placing the schema switch after thelocked_on_hostgroupcheck would indeed skip it entirely on cache hits, silently serving cached results without ever applying the intended schema remap for that path. That's a more severe correctness issue than the rejection-side-effect concern I raised, so keeping the switch before the label (but afterOK_msg/error_msgshort-circuits) is the right trade-off. - Mid-transaction schema switch: agreed that a
USE/COM_INIT_DB-equivalent schema change on the same sticky connection doesn't affect transaction state — that's consistent with how ProxySQL already handles client-issuedUSEinside a transaction. Guarding ontransaction_persistent_hostgroupwould only make the rule inconsistently inert, so I won't push for that guard.
The current placement (after OK_msg/error_msg, before the hostgroup-lock check) looks like the right balance given the cache-lookup ordering constraint.
(∪ ᐢ ‿ ᐢ ∪) sounds like a solid trade-off
✏️ Learnings added
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:34.305Z
Learning: In lib/MySQL_Session.cpp, MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo() has an early-return path for query cache hits (GloMyQC->get(...) keyed on client_myds->myconn->userinfo->hash) that occurs before the `__exit_set_destination_hostgroup` label. Any per-query session state mutation driven by qpo (e.g. qpo->destination_schema) that is placed after that label will be skipped entirely on a cache hit. The destination_schema switch (client_myds->myconn->userinfo->set_schemaname) is therefore applied right after the qpo->OK_msg/qpo->error_msg early-return checks (before the __exit_set_destination_hostgroup label and before the locked_on_hostgroup rejection check), not after the hostgroup-lock validation, specifically to avoid this cache-hit bypass. This placement was decided in PR `#5925` (commit 652ffa124) after discussion.
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:34.305Z
Learning: In lib/MySQL_Session.cpp, mysql_query_rules.attributes.destination_schema (query-rule-driven session schema switching) is applied unconditionally, without the `transaction_persistent_hostgroup == -1` guard used for `destination_hostgroup`. This is intentional: switching a session's default schema mid-transaction via COM_INIT_DB has the same semantics as a client issuing `USE <schema>` mid-transaction through ProxySQL — it does not commit or invalidate the transaction and the sticky backend connection is preserved. Guarding on `transaction_persistent_hostgroup` was considered but rejected because it would make the destination_schema rule silently inert during an active transaction, which was judged more surprising than the current behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Pushed 652ffa1 addressing all review feedback:
|
…n#4880) A query rule whose attributes JSON contains {"destination_schema": "name"} switches the session schema before backend connection selection. The connection pool matches on (username, schemaname) and issues COM_INIT_DB on schema mismatch, so matched queries transparently execute against the remapped schema. This enables per-user / per-rule logical database routing on the same cluster (multi-tenant style redirection). MySQL only; PgSQL rule creation initializes the field to NULL.
Verifies the handshake, COM_INIT_DB and USE schema-selection paths are all remapped by a destination_schema rule, and that behavior reverts after the rule is removed.
f1b3092 to
31bd993
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
- Apply the schema switch before the query cache lookup so cache keys use the remapped schema and a cache hit no longer bypasses the switch - Warn when destination_schema is an empty string in rule attributes - Serialize destination_schema in Query_Processor_Output::get_info_json - Set attributes/comment to NULL after free in destroy() for consistency - Extend the TAP test with a query-cache interaction case (12 assertions)
SonarCloud cpp:S134 flagged >3 levels of nesting at both new_query_rule() parsing sites, where the block was also duplicated verbatim. Behavior is unchanged; mysql-dest_schema_routing-t still passes 12/12.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/tap/tests/mysql-dest_schema_routing-t.cpp (1)
36-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse RAII owners for MySQL client resources.
fetch_single(),connect_proxy(), andmain()manually ownMYSQL_RES*andMYSQL*resources. Use an RAII owner withmysql_free_resultandmysql_closedeleters. This removes cleanup-state tracking across thegoto cleanuppaths.As per coding guidelines,
**/*.{cpp,h,hpp}requires “Use RAII for resource management.”Also applies to: 72-75, 202-216
🤖 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/mysql-dest_schema_routing-t.cpp` around lines 36 - 64, Introduce RAII owner types with mysql_free_result and mysql_close deleters, then update fetch_single(), connect_proxy(), and main() to store MYSQL_RES* and MYSQL* in those owners. Replace manual cleanup and goto cleanup resource handling with automatic destruction while preserving existing error diagnostics and connection behavior.Source: Coding guidelines
🤖 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/mysql-dest_schema_routing-t.cpp`:
- Around line 104-105: Replace the fixed sleep(2) in the schema-routing test
with bounded polling through a proxy connection, repeatedly checking for the
inserted marker to become visible on the reader hostgroup. Continue to the
baseline assertions only after visibility is confirmed, and fail clearly when
the timeout expires.
- Line 67: Update the TAP test plan from 12 to 14 and add an ok() assertion
immediately after each mysql_select_db() command covering the COM_INIT_DB and
USE paths. Stop or return from the test when either assertion fails, ensuring
the subsequent schema-routing checks cannot pass without executing the
corresponding schema reset.
---
Nitpick comments:
In `@test/tap/tests/mysql-dest_schema_routing-t.cpp`:
- Around line 36-64: Introduce RAII owner types with mysql_free_result and
mysql_close deleters, then update fetch_single(), connect_proxy(), and main() to
store MYSQL_RES* and MYSQL* in those owners. Replace manual cleanup and goto
cleanup resource handling with automatic destruction while preserving existing
error diagnostics and connection behavior.
🪄 Autofix
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 Plus
Run ID: 91120c86-9829-4817-bfed-fa14e9548bfe
📒 Files selected for processing (7)
include/query_processor.hlib/MySQL_Query_Processor.cpplib/MySQL_Session.cpplib/PgSQL_Query_Processor.cpplib/Query_Processor.cpptest/tap/groups/groups.jsontest/tap/tests/mysql-dest_schema_routing-t.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/Query_Processor.cpp
- test/tap/groups/groups.json
- lib/MySQL_Session.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/query_processor.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/query_processor.htest/tap/tests/mysql-dest_schema_routing-t.cpplib/MySQL_Query_Processor.cpplib/PgSQL_Query_Processor.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/mysql-dest_schema_routing-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/mysql-dest_schema_routing-t.cpp
🪛 Cppcheck (2.21.0)
lib/MySQL_Query_Processor.cpp
[warning] 781-781: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 919-919: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/PgSQL_Query_Processor.cpp
[warning] 373-373: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 509-509: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (4)
lib/MySQL_Query_Processor.cpp (1)
704-728: LGTM!Also applies to: 781-781, 851-851, 919-919, 989-989
lib/PgSQL_Query_Processor.cpp (1)
373-373: LGTM!Also applies to: 509-509
include/query_processor.h (2)
218-218: LGTM!
135-135: 🩺 Stability & AvailabilityNo change needed.
| } | ||
|
|
||
| int main() { | ||
| plan(12); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make COM_INIT_DB and USE command failures fail the TAP test.
If mysql_select_db() fails, Line 142 still reads the DST_DB schema from the handshake remap. If USE fails, Line 150 also reads the prior remap. diag() does not fail an assertion, so both path checks can pass without executing their schema-reset command. Add one ok() assertion for each command, stop on failure, and increase the plan to 14.
Proposed fix
- plan(12);
+ plan(14);
@@
- if (mysql_select_db(conn, SRC_DB)) {
- diag("mysql_select_db failed: %s", mysql_error(conn));
+ const bool init_db_succeeded = mysql_select_db(conn, SRC_DB) == 0;
+ ok(init_db_succeeded, "COM_INIT_DB should succeed");
+ if (!init_db_succeeded) {
+ goto cleanup;
}
@@
- if (mysql_query(conn, query)) {
- diag("USE failed: %s", mysql_error(conn));
+ const bool use_succeeded = mysql_query(conn, query) == 0;
+ ok(use_succeeded, "USE %s should succeed", SRC_DB);
+ if (!use_succeeded) {
+ goto cleanup;
}Also applies to: 139-151
🤖 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/mysql-dest_schema_routing-t.cpp` at line 67, Update the TAP
test plan from 12 to 14 and add an ok() assertion immediately after each
mysql_select_db() command covering the COM_INIT_DB and USE paths. Stop or return
from the test when either assertion fails, ensuring the subsequent
schema-routing checks cannot pass without executing the corresponding schema
reset.
| // let replicas catch up: reads may be routed to a reader hostgroup | ||
| sleep(2); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wait for reader visibility instead of using a fixed delay.
The next reads can route to a reader hostgroup. A two-second delay does not prove that the replica applied the inserts. Poll the marker through a proxy connection with a bounded timeout before the baseline assertions.
🤖 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/mysql-dest_schema_routing-t.cpp` around lines 104 - 105,
Replace the fixed sleep(2) in the schema-routing test with bounded polling
through a proxy connection, repeatedly checking for the inserted marker to
become visible on the reader hostgroup. Continue to the baseline assertions only
after visibility is confirmed, and fail clearly when the timeout expires.
31bd993 to
67b6e08
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
1 participant
What
Adds a new optional key to
mysql_query_rules.attributes:{"destination_schema": "<schema>"}.When a query matches a rule carrying this attribute, ProxySQL switches the session schema to the given value before backend connection selection. The connection pool already matches backend connections on
(username, schemaname)and issuesCOM_INIT_DBon schema mismatch, so matched queries transparently execute against the remapped schema — no client-side change required.Why
This enables per-user / per-rule logical database routing on the same cluster, e.g. multi-tenant setups where an application connects to a fixed database name but the operator wants to redirect specific users (or query patterns) to a different schema at the proxy layer.
Requested in #4880 ("Redirect database based on hostgroup?") — this implements the MySQL side of that request, driven by query rules (which can match on username, schemaname, hostgroup annotations, digest, etc., so it covers the hostgroup-driven case and more). Related to the long-standing schema-control discussion in #1133.
Existing alternatives don't cover this:
mysql_query_rules.schemanameis match-only input,mysql_users.default_schemaonly applies when the client omits the database at handshake, andUSE/COM_INIT_DBare handled internally before query rules run — so rewritingUSEviareplace_patterncannot work.How
Small, self-contained change (no admin table schema change — reuses the JSON-validated
attributescolumn, following the existingflagOUTspattern):include/query_processor.h— newdestination_schemafield onQP_rule_tandQuery_Processor_Output(init/destroy follow the same lifecycle aserror_msg/attributes).lib/MySQL_Query_Processor.cpp— parsedestination_schemafrom the rule attributes JSON in bothnew_query_rule()variants.lib/Query_Processor.cpp— free on rule delete; copy rule → output on match.lib/MySQL_Session.cpp— apply viauserinfo->set_schemaname()at__exit_set_destination_hostgroup(covers both COM_QUERY and prepared-statement paths, before hostgroup lock check and connection selection).lib/PgSQL_Query_Processor.cpp— initialize the field toNULL(rules aremalloc'd; PgSQL support intentionally out of scope for this PR).Usage
Any query from
app_usernow executes againsttenant1regardless of the database requested at connect time. Withapply=0the rule composes with subsequent routing rules (e.g. read/write split).Notes / limitations
SELECT DATABASE()reflects the remapped schema (it executes on the backend), as do backend error messages — intentionally, since that is where the query really runs.schemanamesee the remapped value for subsequent queries; matching byusername(or digest/annotations) is the recommended pairing.Testing
test/tap/tests/mysql-dest_schema_routing-t.cpp(registered ingroups.json): verifies all three schema-selection paths — handshake db,COM_INIT_DB(mysql_select_db), and textualUSE— are remapped while a rule is active, and that behavior reverts after rule removal. 8/8 assertions pass viatest/infra/control/run-tests-isolated.bash(mysql84-g1) on this branch (v3.0base):PROXYSQL31=1 make debugonbuild-ubuntu24(aarch64). Full group run deferred to CI.Questions for reviewers
USE-switchable), so it likely deserves its own design pass.__exit_set_destination_hostgroupinMySQL_Session.cppso it covers both the COM_QUERY and prepared-statement paths after all rules are evaluated — happy to move it if there is a preferred spot.cache_ttlin the same ruleset, the first query of a fresh connection computes its cache key with the pre-remap schema (the remap becomes effective at rule-application time); subsequent queries use the remapped schema consistently. Worst case is one extra cache miss — flagging for awareness.Summary by CodeRabbit
New Features
USEcommands.Bug Fixes
Tests