MySQL: Track session-specific system variables changes#5166
Conversation
Summary of ChangesHello @wazir-ahmed, 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 introduces comprehensive support for tracking session-specific system variable changes within MySQL backend connections. It establishes a robust mechanism to configure the necessary session tracking settings, process notifications from the backend when variables change, and synchronize these changes across the internal client and server variable states. This enhancement improves the accuracy of ProxySQL's understanding of the backend session state, particularly for variables that are not easily parsed from SQL statements. Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 tracking for session-specific system variables in MySQL by leveraging session_track_system_variables and session_track_state_change. The changes include new handlers and logic to set and process these variables, ensuring that changes are captured and synchronized between the client and server. My review focuses on improving code safety, clarity, and performance in the new implementations. I've suggested using safer string comparison functions, reducing variable scopes for better readability, and making minor performance optimizations in string handling. I also pointed out a commented-out line that should be removed.
d817bda to
346030e
Compare
a22aa90 to
e059a67
Compare
f1f57ac to
7709911
Compare
826fd70 to
c5aed61
Compare
PR 5166: Comprehensive Review and Concerns - Detailed AnalysisFirst, excellent work on implementing session variable tracking! This is a valuable enhancement that addresses real limitations in ProxySQL's session state management. The architectural approach leveraging MySQL's native session tracking capabilities is sound and well-designed. However, I have identified several critical issues and areas for improvement that should be addressed before merge. This analysis provides detailed technical background for each concern. 🔴 Critical Issues (Blockers)1. Missing Capability Detection - Protocol Level Compatibility GapProblem: The implementation lacks proper capability detection for Technical Background:
Current Implementation Gap: // ✅ GTID tracking correctly checks capability:
bool MySQL_Session::handler_again___verify_backend_session_track_gtids() {
if ((mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_SESSION_TRACK) == 0) {
// the backend doesn't support CLIENT_SESSION_TRACK
return ret; // Gracefully handles unsupported backends
}
// ... proceed with configuration
}
// ❌ Variable tracking attempts SET commands without capability check:
bool MySQL_Session::handler_again___verify_backend_session_track_variables() {
if (mysql_thread___session_track_variables == session_track_variables::DISABLED) {
return false;
}
// ❌ No CLIENT_SESSION_TRACK check - will try to SET on unsupported servers!
if (mybe->server_myds->myconn->options.session_track_variables_sent == false) {
// This will fail on MySQL < 5.7, MariaDB, SQLite3, etc.
NEXT_IMMEDIATE_NEW(SETTING_SESSION_TRACK_VARIABLES);
}
}Impact: This will cause connection failures on:
Fix Required: Add the same capability check that GTID tracking uses before attempting any SET commands. 2. CLIENT_DEPRECATE_EOF Dependency Not HandledTechnical Background:
When Current State: The implementation assumes backends support session tracking without verifying both required capabilities. Impact: Even if a server supports 🟡 Major Concerns3. TAP Test Modifications Need ExplanationObservation: Two TAP tests were modified to explicitly disable session tracking: // test_binlog_reader-t.cpp:
MYSQL_QUERY_T(proxysql_admin, "SET mysql-session_track_variables=0");
MYSQL_QUERY_T(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME");
// reg_test_4264-commit_rollback-t.cpp:
MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0");Possible Root Causes:
Action Required: We need to understand the exact failure modes before merge. These test modifications suggest issues were encountered but worked around them instead of fixing the root cause. 4. Fast Forward Mode CompatibilityTechnical Background: Session Tracking Impact:
Current Gap: No checking of client capabilities when assigning connections in fast forward mode. Impact: Clients without session tracking support may receive packets they cannot parse or unexpected protocol overhead. 🔵 Enhancement Opportunities5. Enhanced Configuration Modes (Production-Ready Design)Current Limitation: Binary enable/disable (0/1) is too simplistic for production environments with mixed backends. Proposed Three-Tier Configuration: enum mode {
DISABLED = 0, // Current behavior: feature completely disabled
OPTIONAL = 1, // Graceful fallback with logging for mixed environments
ENFORCED = 2 // Strict mode: fail if requirements cannot be met
};Detailed Mode Behavior: OPTIONAL Mode (1):
ENFORCED Mode (2):
6. Frontend Variable Propagation (Complete Implementation)Current Gap: The implementation tracks variables internally for ProxySQL's state management but doesn't propagate changes to clients that support session tracking. Technical Requirements:
Implementation Complexity:
📋 Required Testing7. Comprehensive CI Testing with Feature EnabledCurrent State: Tests run with feature disabled (see TAP test modifications above) Required Testing Matrix: A. Version Compatibility with mysql-session_track_variables=1 and mysql-session_track_variables=2:
B. Protocol-Level Testing:
🎯 Recommended Implementation OrderPhase 1 - Critical Fixes (Merge Blockers)
Phase 2 - Compatibility Hardening
Phase 3 - Production Enhancements
📝 Additional Technical Implementation DetailsError Handling Strategy Example:bool MySQL_Session::handler_again___verify_backend_session_track_variables() {
if (mysql_thread___session_track_variables == session_track_variables::DISABLED) {
return false;
}
// Check if backend supports session tracking
if ((mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_SESSION_TRACK) == 0) {
if (mysql_thread___session_track_variables == ENFORCED) {
proxy_error("Backend %s:%d does not support required session tracking capabilities (CLIENT_SESSION_TRACK)",
mybe->host, mybe->port);
return true; // Trigger connection failure
} else if (mysql_thread___session_track_variables == OPTIONAL) {
proxy_info("Backend %s:%d does not support session tracking, continuing without it",
mybe->host, mybe->port);
return false; // Skip session tracking setup
}
return false;
}
// Check CLIENT_DEPRECATE_EOF requirement
if ((mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_DEPRECATE_EOF) == 0) {
if (mysql_thread___session_track_variables == ENFORCED) {
proxy_error("Backend %s:%d does not support CLIENT_DEPRECATE_EOF required for session tracking",
mybe->host, mybe->port);
return true; // Trigger connection failure
} else if (mysql_thread___session_track_variables == OPTIONAL) {
proxy_info("Backend %s:%d does not support CLIENT_DEPRECATE_EOF, session tracking will be limited",
mybe->host, mybe->port);
// Could proceed with limited functionality or skip entirely
}
}
// Proceed with session tracking setup...
}SummaryThis PR implements valuable functionality that addresses real production needs. However, the current implementation has critical gaps that could cause connection failures in production environments. Critical Issues Summary:
Recommendation:
The architectural foundation is solid and the approach is sound - we just need to harden the implementation for production use across diverse backend environments. |
eb5a10c to
3f1c89f
Compare
3f1c89f to
47f344f
Compare
- Add new mysql variable `mysql-session_track_variables`. - Configure `session_track_system_variables` and `session_track_state_change` on backend connections if the mysql variable is enabled. - Utilize notifications from backend servers to capture system variable changes that cannot be handled by `MySQL_Set_Stmt_Parser` - Update both client and server variable maps based on backend responses. - TAP test to verify this patch. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
…acking Add detailed architectural documentation for PR 5166 session variable tracking: - High-level architecture overview in session_track_variables struct explaining the 3-phase workflow - Detailed documentation for handler_rc0_Process_Variables explaining the core processing workflow - Technical implementation details for MySQL_Connection::get_variables protocol interface - Configuration logic documentation for handler_again___verify_backend_session_track_variables - Added inline comments explaining why session tracking is needed and performance considerations This documentation provides a complete understanding of how MySQL session variable tracking integrates with ProxySQL's existing state machine and leverages MySQL's native session tracking capabilities.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
415dc07 to
6e6a9bb
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
retest this please |
Signed-off-by: René Cannaò <rene@proxysql.com>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
include/MySQL_Thread.h (1)
374-384: Naming collision:struct session_track_variablesshares its identifier withvariables.session_track_variables(int).The struct tag and the integer field share the same name, which compiles but creates ambiguous-looking sites:
if (mysql_thread___session_track_variables == session_track_variables::DISABLED)Here the left-hand side is an
intvariable and the right-hand side resolves to the struct type — readable only once the pattern is known. Consider eliminating the ambiguity:Suggested alternative — direct enum class
-struct session_track_variables { - enum mode { - DISABLED = 0, - OPTIONAL, - ENFORCED - }; -}; +enum class session_track_variables_mode { + DISABLED = 0, + OPTIONAL, + ENFORCED +};Then update the
intfield declaration and all comparison sites to usesession_track_variables_mode::DISABLEDetc. The thread-localmysql_thread___session_track_variables(anint) still stores the underlying integer;static_cast<session_track_variables_mode>(...)makes conversions explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_Thread.h` around lines 374 - 384, The struct tag session_track_variables conflicts with the int variable variables.session_track_variables and makes comparisons like mysql_thread___session_track_variables == session_track_variables::DISABLED ambiguous; rename the type to a clear enum (e.g. session_track_variables_mode) and make it an enum class or plain enum with named constants, update the field declaration (variables.session_track_variables) uses to store the underlying int (keep mysql_thread___session_track_variables as int) and replace all comparison sites (e.g. mysql_thread___session_track_variables == session_track_variables::DISABLED) to use session_track_variables_mode::DISABLED with explicit static_casts where converting between int and the enum; update any switch/case or function signatures referencing session_track_variables to the new enum name.lib/mysql_connection.cpp (1)
3150-3150: Parametervariablesshadows the class memberVariable variables[].The parameter
std::unordered_map<string, string>& variableshas the same identifier as the class memberVariable variables[SQL_NAME_LAST_HIGH_WM]. Inside the method body,variables[var_name] = val;resolves to the parameter due to shadowing — functionally correct because the types are unambiguous, but confusing for readers who may conflate the two.♻️ Suggested rename
-bool MySQL_Connection::get_variables(std::unordered_map<string, string>& variables) { +bool MySQL_Connection::get_variables(std::unordered_map<string, string>& out_vars) { ... - variables[var_name] = val; + out_vars[var_name] = val;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mysql_connection.cpp` at line 3150, The method MySQL_Connection::get_variables shadows the class member Variable variables[] by using the same name for the parameter std::unordered_map<string, string>& variables; rename the parameter (for example to out_variables or variables_map) and update all uses inside get_variables to refer to the new name (e.g., out_variables[var_name] = val;) so there is no ambiguity with the class member Variable variables[], keeping class member access unchanged.lib/MySQL_Thread.cpp (1)
2683-2684: Use enum bounds to avoid drift.Hard‑coding
0..2risks mismatching the enum if new modes are added later. Consider tying the max to the enum constant.♻️ Proposed tweak
- VariablesPointers_int["session_track_variables"] = make_tuple(&variables.session_track_variables, 0, 2, false); + VariablesPointers_int["session_track_variables"] = make_tuple( + &variables.session_track_variables, + static_cast<int>(session_track_variables::DISABLED), + static_cast<int>(session_track_variables::ENFORCED), + false + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Thread.cpp` around lines 2683 - 2684, The hard-coded range "0..2" for VariablesPointers_int["session_track_variables"] risks drifting if the session tracking enum changes; update the max bound to derive from the enum constant (e.g., use the enum's MAX/COUNT value or SESSION_TRACK_MODE_COUNT) and replace the literal 2 with that enum-based constant when constructing the tuple for VariablesPointers_int and ensure any code using variables.session_track_variables uses the same enum bound for consistency.
🤖 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_Thread.h`:
- Around line 380-382: The comment on ENFORCED in include/MySQL_Thread.h
incorrectly states "connection fails" — update the comment to reflect that
unsupported session tracking causes the connection to be returned/destroyed and
the server backoff logic is applied (see handle_session_track_capabilities in
lib/MySQL_Session.cpp and server_backoff_time) so no client-visible error is
raised and the session will be retried on a compatible server; change wording to
explicitly mention "returned to pool or destroyed and server backoff applied"
instead of "connection fails".
In `@lib/mysql_connection.cpp`:
- Around line 900-903: The ENFORCED guard (check of
mysql_thread___session_track_variables == session_track_variables::ENFORCED
which sets mysql->options.client_flag |= CLIENT_DEPRECATE_EOF) is being silently
overridden by the fast-forward code that clears CLIENT_DEPRECATE_EOF; move the
ENFORCED handling so it runs after the fast-forward block (or merge its logic
into that block) so ENFORCED can actually take effect, and if you must keep the
current ordering instead log a proxy_warning when session_fast_forward is true
and ENFORCED was requested but cannot be honored; update references to
mysql->options.client_flag and CLIENT_DEPRECATE_EOF accordingly.
- Around line 3160-3184: The code currently sets ret = true unconditionally
after mysql_session_track_get_first even if no name/value pairs were stored or
the last name is unpaired; change it so ret is set only when at least one
complete pair was inserted (e.g., check variables.empty() or that expect_value
== false before assigning ret = true), ensuring
mysql_session_track_get_next/expect_value/var_name/val logic remains the same so
unpaired trailing names are not treated as a successful result.
---
Duplicate comments:
In `@include/MySQL_HostGroups_Manager.h`:
- Around line 238-244: The field server_backoff_time in
MySQL_HostGroups_Manager.h has a data race because it is written by
MySQL_Session::handle_session_track_capabilities() and read concurrently by
MyHGC::get_random_MySrvC() and code in MySQL_Thread.cpp; change its type to
std::atomic<unsigned long long>, add `#include` <atomic>, and update all accesses
(writes in handle_session_track_capabilities and reads in get_random_MySrvC /
MySQL_Thread.cpp) to use atomic store/load (or fetch/store with appropriate
memory_order) so updates are synchronized and the race is eliminated.
In `@include/MySQL_Thread.h`:
- Around line 646-655: The doc comment for the int member
session_track_variables incorrectly references a non-existent enum constant
ENABLED; update the text in the comment above session_track_variables to
reference the correct enum values (DISABLED (0), OPTIONAL (1), ENFORCED (2)) and
replace the phrase "When set to session_track_variables::ENABLED (1)" with the
appropriate wording (e.g., "When set to session_track_variables::OPTIONAL (1) or
session_track_variables::ENFORCED (2)" or specify the intended behavior for
OPTIONAL vs ENFORCED) so it matches the actual enum constants DISABLED,
OPTIONAL, and ENFORCED.
In `@lib/MySQL_Session.cpp`:
- Around line 1976-2004: The retry flags session_track_variables_sent and
session_track_state_sent are being set unconditionally in
handler_again___verify_backend_session_track_variables which can block retries
after soft SQL errors; modify the flow so that if the subsequent SET operations
(SETTING_SESSION_TRACK_VARIABLES / SETTING_SESSION_TRACK_STATE dispatched via
NEXT_IMMEDIATE_NEW) fail with a soft error the flags are cleared/reset and
previous state restored (undo session_track_*_sent and call
set_previous_status_mode3 if needed) so retries remain possible; ensure the
reset logic is invoked wherever soft SQL errors are handled for those settings
(the error/rollback path that handles SETTING_SESSION_TRACK_VARIABLES or
SETTING_SESSION_TRACK_STATE) and use the same option fields and helper functions
(myconn->options.session_track_variables_sent,
myconn->options.session_track_state_sent, set_previous_status_mode3) to revert
state.
---
Nitpick comments:
In `@include/MySQL_Thread.h`:
- Around line 374-384: The struct tag session_track_variables conflicts with the
int variable variables.session_track_variables and makes comparisons like
mysql_thread___session_track_variables == session_track_variables::DISABLED
ambiguous; rename the type to a clear enum (e.g. session_track_variables_mode)
and make it an enum class or plain enum with named constants, update the field
declaration (variables.session_track_variables) uses to store the underlying int
(keep mysql_thread___session_track_variables as int) and replace all comparison
sites (e.g. mysql_thread___session_track_variables ==
session_track_variables::DISABLED) to use session_track_variables_mode::DISABLED
with explicit static_casts where converting between int and the enum; update any
switch/case or function signatures referencing session_track_variables to the
new enum name.
In `@lib/mysql_connection.cpp`:
- Line 3150: The method MySQL_Connection::get_variables shadows the class member
Variable variables[] by using the same name for the parameter
std::unordered_map<string, string>& variables; rename the parameter (for example
to out_variables or variables_map) and update all uses inside get_variables to
refer to the new name (e.g., out_variables[var_name] = val;) so there is no
ambiguity with the class member Variable variables[], keeping class member
access unchanged.
In `@lib/MySQL_Thread.cpp`:
- Around line 2683-2684: The hard-coded range "0..2" for
VariablesPointers_int["session_track_variables"] risks drifting if the session
tracking enum changes; update the max bound to derive from the enum constant
(e.g., use the enum's MAX/COUNT value or SESSION_TRACK_MODE_COUNT) and replace
the literal 2 with that enum-based constant when constructing the tuple for
VariablesPointers_int and ensure any code using
variables.session_track_variables uses the same enum bound for consistency.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Keep `CLIENT_DEPRECATE_EOF` enabled during frontend handshake when `session_track_variables=ENFORCED`. - Avoid unsafe mysql_error() calls in session-track TAP tests. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
@coderabbitai Do a full review of this PR. Report if you find any new issues. Don't repeat previous review comments. |
|
🔴 New Critical Issues1. 2.
This should at minimum log a warning, and ideally the ENFORCED mode's capability forcing should be opt-in with clear documentation. 🟡 New Major Concerns3. Duplicate 4. Duplicate Backoff Check in Two Locations 5. Undefined Backoff Duration and Reset 6.
7. Naming Ambiguity: Struct and Field Share the Same Name variables.session_track_variables = session_track_variables::DISABLED;Here 🔵 New Enhancement Observations8. No 9. MySQL 5.6 Test Groups Include OPTIONAL/ENFORCED Triggering a full automated review now for any additional findings from the full diff. 🧠 Learnings used✅ Actions performedFull review triggered. |
🧩 Analysis chain
🏁 Script executed: Length of output: 22540 🏁 Script executed: Length of output: 29631 🏁 Script executed: Length of output: 3739 🏁 Script executed: Length of output: 10173
New Issues Found🔴 Critical: Thread Safety —
|
| Severity | Issue | File |
|---|---|---|
| 🔴 | server_backoff_time not atomic — data race across threads |
MySQL_HostGroups_Manager.h, MySQL_Session.cpp |
| 🟡 | handler_rc0_Process_Variables called unconditionally in hot path |
MySQL_Session.cpp:6404 |
| 🟡 | Missing null guard in handler_again___verify_backend_session_track_variables |
MySQL_Session.cpp |
| 🟡 | OPTIONAL mode silent failure with no log output | MySQL_Session.cpp |
| 🟡 | session_track_system_variables='*' performance impact undocumented |
MySQL_Session.cpp, MySQL_Thread.h |
| 🔵 | set_session_variable() misleading set_value out-param documentation |
session_track_variables.h |
| 🔵 | Commented-out alternative backoff value left in code | MySQL_Session.cpp |
🧠 Learnings used
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/infra/control/run-tests-isolated.bash (1)
73-94:⚠️ Potential issue | 🟠 MajorPopulate
INFRAS_TO_CHECKafter derivingINFRA_TYPE.For
mysql56-single*groups without aninfras.lst, line 75 captures an emptyINFRA_TYPEbefore line 82 derives it. That leavesINFRAS_TO_CHECKempty, so backend verification at lines 131-153 is skipped.Proposed fix
# Derive INFRA_TYPE from TAP_GROUP if not set # This is simple and deterministic - matches group naming convention if [ -z "${INFRA_TYPE}" ]; then case "${TAP_GROUP}" in mysql56-single*) export INFRA_TYPE="${DEFAULT_MYSQL_INFRA:-infra-dbdeployer-mysql56-single}" ;; @@ if [ -n "${INFRA_TYPE}" ]; then echo ">>> Derived INFRA_TYPE from TAP_GROUP: ${INFRA_TYPE}" fi fi + +# If no list found, use the derived INFRA_TYPE as single requirement. +if [ -z "${INFRAS_TO_CHECK}" ]; then + INFRAS_TO_CHECK="${INFRA_TYPE}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/control/run-tests-isolated.bash` around lines 73 - 94, INFRAS_TO_CHECK is only initialized before INFRA_TYPE may be derived from TAP_GROUP, causing INFRAS_TO_CHECK to stay empty for groups like mysql56-single* and skip backend checks; after the case block that sets INFRA_TYPE (the block referencing TAP_GROUP and values like mysql56-single*, mysql84*, etc.), add logic to populate INFRAS_TO_CHECK from INFRA_TYPE when INFRAS_TO_CHECK is empty (i.e., if INFRAS_TO_CHECK is unset/null and INFRA_TYPE is set, assign INFRAS_TO_CHECK=INFRA_TYPE) so subsequent backend verification uses the derived infra.
♻️ Duplicate comments (8)
include/MySQL_Thread.h (1)
374-384:⚠️ Potential issue | 🟡 MinorClarify
ENFORCED: unsupported backends are backed off, not simply failed.Both comments still say the connection “fails” when required capabilities are missing. The current flow rejects/returns the backend connection and applies temporary server backoff, so the docs should avoid implying an immediate client-visible hard failure.
Suggested wording
- // Enforce session tracking; connection fails if backend does - // not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING + // Enforce session tracking; backend connections that lack + // CLIENT_DEPRECATE_EOF or CLIENT_SESSION_TRACKING are rejected + // and the server is put into temporary backoff. ENFORCED- * - ENFORCED (2): Enforce session tracking; connection fails if backend - * does not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING. + * - ENFORCED (2): Enforce session tracking; backend connections that + * do not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING + * are rejected and the server is put into temporary backoff.Also applies to: 656-670
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_Thread.h` around lines 374 - 384, The comment for session_track_variables::mode::ENFORCED is misleading: it says the connection "fails" when backends lack CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING, but the code actually rejects that backend and applies a temporary server backoff rather than an immediate client-visible hard failure; update the comment on enum mode (struct session_track_variables) for ENFORCED to state that unsupported backends are rejected and the server applies a temporary backoff (not an instant client-visible failure), and make the same wording change in the duplicate comment block elsewhere (the other session tracking enum comment).test/tap/tests/reg_test_4264-commit_rollback-t.cpp (1)
1156-1160:⚠️ Potential issue | 🟠 MajorDon’t mask session-tracking traffic by disabling the feature in this regression test.
reg_test_4264should establish its counter baseline after connection/session-track setup, or subtract known internal setup queries, so it still validates transaction routing with the new feature enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/reg_test_4264-commit_rollback-t.cpp` around lines 1156 - 1160, This test currently disables session-tracking (via the MYSQL_QUERY_T calls setting mysql-default_session_track_gtids and mysql-session_track_variables and LOAD MYSQL VARIABLES TO RUNTIME), which masks the behavior under test; either remove those statements so session-tracking remains enabled, or move them so they run before establishing the test's transaction-routing baseline (or adjust the baseline calculation to subtract these known internal setup queries). Update the reg_test_4264 test flow around the MYSQL_QUERY_T(admin, ...) calls and the point where the baseline is captured so the session/connection tracking is active when you establish the counter baseline used to validate routing.include/MySQL_HostGroups_Manager.h (1)
209-215:⚠️ Potential issue | 🟠 MajorDeclare
server_backoff_timeas atomic.This timestamp is read from hostgroup selection/cache paths and written by session handling, so the plain
unsigned long longkeeps the data race already noted in prior review.Proposed fix
- unsigned long long server_backoff_time; + std::atomic<unsigned long long> server_backoff_time { 0 };#!/bin/bash # Confirm all reads/writes can be migrated to atomic load/store. rg -n -C3 'server_backoff_time\s*(=|>|\.load|\.store)' --type=cpp --type=hAs per coding guidelines, use pthread mutexes for synchronization and
std::atomic<>for counters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_HostGroups_Manager.h` around lines 209 - 215, Declare server_backoff_time as std::atomic<unsigned long long> and include <atomic>; update all producers/consumers to use .store()/ .load() (e.g., uses in MyHGC::get_random_MySrvC and the session handling code that writes the timestamp) instead of raw reads/writes, choosing an appropriate memory_order (memory_order_relaxed is acceptable for simple timestamp updates unless stronger ordering is required); scan the repo for direct accesses to server_backoff_time and replace them with atomic ops so reads/writes are race-free.lib/MyHGC.cpp (1)
41-43:⚠️ Potential issue | 🟠 MajorGuard
sessand make the backoff timestamp atomic.The new check dereferences
sess->threadunconditionally, while this function already has nullable-session guards later. Also, theserver_backoff_timeread is still unsynchronized; that part was already flagged in prior review.Proposed fix
- if (mysrvc->server_backoff_time > sess->thread->curtime) + if (sess && mysrvc->server_backoff_time.load(std::memory_order_relaxed) > sess->thread->curtime) continue;And update the field declaration:
- unsigned long long server_backoff_time; + std::atomic<unsigned long long> server_backoff_time { 0 };#!/bin/bash # Verify nullable-session hints and all server_backoff_time access sites. rg -n -C3 'sess &&|get_random_MySrvC|server_backoff_time\s*(=|>|\.load|\.store)' --type=cpp --type=hAs per coding guidelines, use pthread mutexes for synchronization and
std::atomic<>for counters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MyHGC.cpp` around lines 41 - 43, The code dereferences sess->thread unconditionally when checking mysrvc->server_backoff_time; update the check to first ensure sess is non-null and sess->thread is non-null before accessing curtime, and replace the unsynchronized plain access to mysrvc->server_backoff_time with a thread-safe read (e.g., change the field in the MySrvC struct to std::atomic<time_t> server_backoff_time or protect reads/writes with the appropriate pthread mutex), then use server_backoff_time.load() (or the mutex-protected read) in the if condition; update all other access sites to server_backoff_time accordingly and follow the project guideline to use pthread mutexes for compound operations and std::atomic<> for simple counters/flags.test/infra/infra-mysql56-single/docker-compose-destroy.bash (1)
6-8:⚠️ Potential issue | 🟠 MajorSame
INFRA_IDpreservation gap as the dbdeployer destroy script.
docker-compose-init.bashfor this infra preserves a caller-providedINFRA_IDacross.envsourcing; this destroy script does not, so a.env-definedINFRA_IDcan override the caller's and makeCOMPOSE_PROJECTdiverge from what init created, causingdocker compose down -vto no-op and leave the real cluster/volumes behind. Apply the sameSAVED_INFRA_IDpreservation pattern used indocker-compose-init.bash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/docker-compose-destroy.bash` around lines 6 - 8, The destroy script fails to preserve a caller-provided INFRA_ID before sourcing .env, so a .env INFRA_ID can override the caller and make COMPOSE_PROJECT mismatch; mimic the pattern from docker-compose-init.bash by saving the existing INFRA_ID into a temporary SAVED_INFRA_ID (or similar) before the "set -a; . .env; set +a" block and restore INFRA_ID from that saved variable after sourcing, then compute/export COMPOSE_PROJECT="${INFRA}-${INFRA_ID}" so it matches the init-created project and docker compose down -v targets the correct resources.lib/mysql_connection.cpp (2)
3177-3200:⚠️ Potential issue | 🟡 MinorReturn success only after storing a complete variable pair.
Line 3200 reports success even if
get_first()returned only a variable name and no value was inserted. Moveret = trueinto the completed-pair branch so callers do not process an empty/incomplete tracking map.Proposed fix
if (expect_value) { val = string(data, length); variables[var_name] = val; + ret = true; // got a value in this iteration // in the next iteration, we have to expect a variable_name expect_value = false; @@ - - ret = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mysql_connection.cpp` around lines 3177 - 3200, The code currently sets ret = true unconditionally after calling mysql_session_track_get_first, which can indicate success even if no value was paired with the variable name; modify the loop handling mysql_session_track_get_next so that ret is only set to true when a complete name/value pair has been stored into the variables map (i.e., move the assignment of ret = true into the branch where you assign variables[var_name] = val after constructing val and before toggling expect_value), and ensure ret remains false if get_first() returns a name but no subsequent value was read by mysql_session_track_get_next.
904-920:⚠️ Potential issue | 🟠 MajorENFORCED mode is still cleared for fast-forward backend connects.
Line 906 sets
CLIENT_DEPRECATE_EOF, but Line 915 clears it for fast-forward sessions and only restores it when the frontend/client capability match succeeds. That means ENFORCED can still be silently bypassed for fast-forward connections.Proposed direction
if (c->options.client_flag & CLIENT_DEPRECATE_EOF) { if (c->options.server_capabilities & CLIENT_DEPRECATE_EOF) { mysql->options.client_flag |= CLIENT_DEPRECATE_EOF; } } + if (mysql_thread___session_track_variables == session_track_variables::ENFORCED + && !(mysql->options.client_flag & CLIENT_DEPRECATE_EOF)) { + proxy_warning("session_track_variables=ENFORCED but CLIENT_DEPRECATE_EOF " + "could not be enabled for fast_forward connection\n"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mysql_connection.cpp` around lines 904 - 920, The ENFORCED setting can be bypassed for fast-forward connections because code clears CLIENT_DEPRECATE_EOF for myds->sess->session_fast_forward then conditionally restores it from the backend client; change this so that when mysql_thread___session_track_variables == session_track_variables::ENFORCED you never clear CLIENT_DEPRECATE_EOF for fast-forward backends. Concretely, in the fast-forward branch (check using myds->sess->session_fast_forward and MySQL_Connection *c = myds->sess->client_myds->myconn) add a guard so mysql->options.client_flag &= ~(CLIENT_DEPRECATE_EOF) is skipped if mysql_thread___session_track_variables == session_track_variables::ENFORCED, or alternatively set CLIENT_DEPRECATE_EOF unconditionally when ENFORCED before touching it; reference the symbols mysql_thread___session_track_variables, session_track_variables::ENFORCED, myds->sess->session_fast_forward, mysql->options.client_flag and CLIENT_DEPRECATE_EOF.lib/MySQL_Session.cpp (1)
2685-2705:⚠️ Potential issue | 🟠 MajorKeep optional session-track setup errors non-fatal.
The bounds check prevents the OOB write, but
session_track_system_variables/session_track_state_changeare still not entries inmysql_tracked_variables[]. If a backend returns a permitted setup error for these variables,idxremains the sentinel and this block falls through to the generic client ERR path. In OPTIONAL mode that turns a compatibility fallback into a user-visible query failure.Handle these two setup variables explicitly: free the async result, clear the corresponding
*_sentflag or mark tracking unsupported for this backend, and resume the previous status without touchingvar_absent.Also applies to: 2820-2831
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 2685 - 2705, When a permitted setup error occurs for session_track_system_variables or session_track_state_change, do not treat it as a generic fatal client error; instead detect these two variable names (session_track_system_variables and session_track_state_change) in the error handling around mysql_tracked_variables lookup and, if matched, call myds->myconn->async_free_result(), call myconn->compute_unknown_transaction_status(), clear the appropriate "<variable>_sent" flag (or set a tracking-unsupported flag for this backend) rather than setting myconn->var_absent[idx], and then restore previous_status with previous_status.pop() + NEXT_IMMEDIATE_NEW(st) and set myds->revents |= POLLOUT and myds->DSS = STATE_MARIADB_GENERIC so processing resumes; ensure this logic is applied in the same places as the var_absent handling (including the other occurrence at the 2820–2831 block).
🟡 Minor comments (8)
test/infra/infra-mysql56-single/.env-1-7 (1)
1-7:⚠️ Potential issue | 🟡 MinorRemove or relocate this
.envfile; it will not be loaded by the mysql56-single test group.The test runner uses
infra-dbdeployer-mysql56-single(set inrun-tests-isolated.bash:82formysql56-single*groups), so this.envininfra-mysql56-singlewill be ignored. Theinfra-dbdeployer-mysql56-singledirectory already has its own.envwith complete configuration. If these variables are needed only forinfra-mysql56-single, clarify its role; otherwise, this file should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/.env` around lines 1 - 7, This .env contains variables (MYSQL_VERSION, PREFIX, WHG, TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP) that will be ignored by the mysql56-single test group; remove this file from infra-mysql56-single or move these variables into the infra-dbdeployer-mysql56-single/.env (where the test runner actually loads env) or a shared config location if they are needed by multiple groups, ensuring you keep the same variable names and values when relocating.test/tap/groups/mysql56-single/env.sh-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorAdd a ShellCheck source directive for the dynamic include.
This avoids SC1090 for the intentionally dynamic infra
.envsource.Proposed fix
if [ -n "${WORKSPACE}" ]; then _INFRA_ENV="${WORKSPACE}/test/infra/${DEFAULT_MYSQL_INFRA}/.env" + # shellcheck source=test/infra/infra-dbdeployer-mysql56-single/.env [ -f "${_INFRA_ENV}" ] && . "${_INFRA_ENV}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/mysql56-single/env.sh` around lines 5 - 7, Add a ShellCheck source directive to silence SC1090 for the dynamic include: insert a comment like "# shellcheck source=/dev/null" immediately above the line that sources "${_INFRA_ENV}" (the block using WORKSPACE, DEFAULT_MYSQL_INFRA and _INFRA_ENV) so ShellCheck knows the runtime-driven file include is intentional.test/infra/infra-mysql56-single/docker-compose.yml-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorData and log volumes bind-mounted to the same host path.
Both
/var/lib/mysql(datadir) and/var/log/mysql(error/general/slow log dir permy.cnf) are mapped to the same host directory${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1. MySQL will create log files next to tablespace files,configure_ssl.shwritesca.pem/server-*.peminto the same tree, and any cleanup of logs risks touching the datadir. Please split them into distinct subdirectories, e.g..../mysql1/dataand.../mysql1/log.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/docker-compose.yml` around lines 11 - 12, The compose file mounts both /var/lib/mysql and /var/log/mysql to the same host path (${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1); change the mounts to use distinct host subdirectories so data and logs/certs are separated (e.g., map ${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1/data -> /var/lib/mysql and ${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1/log -> /var/log/mysql) to avoid MySQL and configure_ssl.sh (which writes ca.pem/server-*.pem) placing logs or certificates into the datadir.test/infra/infra-mysql56-single/bin/docker-proxy-post.bash-14-18 (1)
14-18:⚠️ Potential issue | 🟡 MinorDead error branch:
set -eaborts before$?is checked.With
set -eat the top of the script, a failure of thedocker exec … <<SQLheredoc exits the script immediately, so line 18'selsebranch never runs — errors will surface only as bare non-zero exit without the intended message. Either dropset -earound this call, capture the status into a variable before theif, or useif ! docker exec ...; then ...; fi:Proposed change
-docker exec -i "${PROXY_CONTAINER}" mysql -uadmin -padmin -h127.0.0.1 -P6032 <<SQL -${SQL_CONTENT} -SQL - -if [ $? -eq 0 ]; then echo "Cluster ${INFRA} registered in ProxySQL."; else echo "ERROR: ProxySQL configuration FAILED for ${INFRA}"; exit 1; fi +if docker exec -i "${PROXY_CONTAINER}" mysql -uadmin -padmin -h127.0.0.1 -P6032 <<SQL +${SQL_CONTENT} +SQL +then + echo "Cluster ${INFRA} registered in ProxySQL." +else + echo "ERROR: ProxySQL configuration FAILED for ${INFRA}" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/bin/docker-proxy-post.bash` around lines 14 - 18, The current docker exec heredoc invocation (docker exec -i "${PROXY_CONTAINER}" mysql -uadmin -padmin -h127.0.0.1 -P6032 <<SQL ... ${SQL_CONTENT} SQL) is run under set -e so failures abort before the subsequent if [$?] check; change the block to reliably handle failure by either (a) capturing the command result into a variable immediately after the heredoc (rc=$?) and then testing rc in the if/else that prints "Cluster ${INFRA} registered..." or the error message, or (b) replacing the heredoc invocation with an if/then construct (if docker exec -i "${PROXY_CONTAINER}" mysql ... <<SQL; then echo "Cluster ${INFRA} registered in ProxySQL."; else echo "ERROR: ProxySQL configuration FAILED for ${INFRA}"; exit 1; fi) so the error branch runs even with set -e; reference the docker exec heredoc, ${PROXY_CONTAINER}, ${SQL_CONTENT}, and ${INFRA} when applying the change.test/infra/infra-dbdeployer-mysql56-single/bin/docker-proxy-post.bash-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorLoad the SQL template relative to the script directory.
Line 11 ignores
SCRIPT_DIR, so invoking this hook from any directory other than the infra root reads the wrong path or fails before configuring ProxySQL.Proposed fix
-SQL_TEMPLATE=$(cat ./conf/proxysql/infra-config.sql) +SQL_TEMPLATE=$(cat "${SCRIPT_DIR}/../conf/proxysql/infra-config.sql") SQL_CONTENT=$(eval "echo \"${SQL_TEMPLATE}\"")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql56-single/bin/docker-proxy-post.bash` around lines 11 - 12, The SQL template is being read with a relative path (SQL_TEMPLATE) which ignores SCRIPT_DIR; update the SQL_TEMPLATE assignment to load the file using SCRIPT_DIR (e.g., point to "${SCRIPT_DIR}/conf/proxysql/infra-config.sql") so the script works when invoked from any CWD, and ensure SCRIPT_DIR is defined (or computed from the script location like dirname of BASH_SOURCE) before SQL_TEMPLATE is evaluated; leave SQL_CONTENT evaluation as-is.test/tap/tests/session_track_variables.h-99-130 (1)
99-130:⚠️ Potential issue | 🟡 MinorDocstring does not match implementation.
The doc on line 108 says
set_valueis "The actual value of innodb_lock_wait_timeout after the procedure call", but the function body only assignsset_value = -1at line 112 and never queries the server to populate it. Callers must invokeselect_sess_var_value()separately (whichtest_session_variables_ffdoes at line 227). Either:
- Call
select_sess_var_value()insideset_session_variable()before returning, or- Update the doc to say
set_valueis reset to -1 and the caller must retrieve the actual value.Misleading docs on shared test helpers tend to cause subtle test flakes down the line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/session_track_variables.h` around lines 99 - 130, The docstring for set_session_variable is incorrect because the function never populates set_value; modify set_session_variable(MYSQL* proxy, int& set_value) to call select_sess_var_value(proxy, "innodb_lock_wait_timeout", set_value) (or the existing helper used elsewhere) after the CALL test.set_innodb_lock_wait_timeout(), check its return and propagate EXIT_FAILURE on error (otherwise leave/return EXIT_SUCCESS), so callers no longer need to separately invoke select_sess_var_value; update or remove the initial set_value = -1 behavior as appropriate and keep the docstring consistent with this behavior.test/tap/tests/session_track_variables.h-164-201 (1)
164-201:⚠️ Potential issue | 🟡 MinorUnhandled JSON exceptions will abort the test process.
nlohmann::json::parse(row[0])at line 180 throws on malformed input, andstd::stoi(...)at lines 187 and 196 throws on empty/non-numeric strings. Without atry/catch, any anomaly in thePROXYSQL INTERNAL SESSIONoutput kills the test with an uninformative abort rather than producing a proper TAP failure. Consider wrapping the JSON access intry { ... } catch (const std::exception& e) { diag("JSON parse error: %s", e.what()); ... return EXIT_FAILURE; }so tests report a clean failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/session_track_variables.h` around lines 164 - 201, Wrap the JSON parsing and numeric conversions inside extract_sess_var_proxy_internal in a try/catch block: catch std::exception (or const std::exception& e) around nlohmann::json::parse(row[0]) and the std::stoi(...) calls for backend["conn"]["innodb_lock_wait_timeout"] and j_session["conn"]["innodb_lock_wait_timeout"], call diag("JSON parse/convert error: %s", e.what()), ensure mysql_free_result(result) is called before returning, and return EXIT_FAILURE instead of allowing exceptions to propagate and abort the test.test/infra/infra-dbdeployer-mysql56-single/docker-compose-init.bash-102-102 (1)
102-102:⚠️ Potential issue | 🟡 MinorBrittle hardcoded container name.
CONTAINER="${COMPOSE_PROJECT}-dbdeployer1-1"hardcodes the Compose V2 dash-style naming convention and replica index1. This breaks if:
- Docker Compose V1 (underscore separator) is used (the script still supports
docker-composeat line 31).COMPOSE_COMPATIBILITY=1is set or service name includes a project prefix override.You already resolve container names via
$COMPOSE_CMD -p "${COMPOSE_PROJECT}" ps --format '{{.Name}}'at line 86 — reuse that approach.🔧 Proposed fix
-CONTAINER="${COMPOSE_PROJECT}-dbdeployer1-1" +CONTAINER=$($COMPOSE_CMD -p "${COMPOSE_PROJECT}" ps --format '{{.Name}}' dbdeployer1 | head -n1) +if [ -z "${CONTAINER}" ]; then + echo "ERROR: Could not resolve dbdeployer1 container name" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql56-single/docker-compose-init.bash` at line 102, The script currently hardcodes CONTAINER="${COMPOSE_PROJECT}-dbdeployer1-1", which is brittle; instead derive CONTAINER from the Compose ps output you already generate with $COMPOSE_CMD -p "${COMPOSE_PROJECT}" ps --format '{{.Name}}' (used earlier) — filter that output for the dbdeployer service (e.g., match "dbdeployer" or the service base name) and assign the matching entry to CONTAINER so it works with both Compose V1/V2 and compatibility mode; update the assignment of CONTAINER to use that ps-derived value and remove the hardcoded string.
🧹 Nitpick comments (6)
test/infra/infra-mysql56-single/conf/mysql/mysql1/my.cnf (1)
26-31: Enabling the general log plus slow-log-without-indexes will be very noisy.
general_log=1writes every statement executed against this test MySQL, andlog_queries_not_using_indexes=1combined withlong_query_time=2will spam the slow log from many TAP tests. On longer CI runs this can fill${INFRA_LOGS_PATH}quickly and perceptibly slow query throughput. Consider leavinggeneral_log=0by default and flipping it on only when debugging a specific failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/conf/mysql/mysql1/my.cnf` around lines 26 - 31, The configuration enables both general_log and noisy slow-query logging which will flood logs; change general_log=1 to general_log=0 by default in the MySQL config and keep slow_query_log and log_queries_not_using_indexes as-is (or increase long_query_time) so normal TAP runs aren’t spammed; ensure the change is made to the my.cnf entry containing general_log, slow_query_log_file, log_queries_not_using_indexes and long_query_time so general query logging is only enabled manually when debugging.test/infra/infra-mysql56-single/bin/docker-proxy-post.bash (1)
11-12:eval "echo ..."on the SQL template is fragile and unsafe.Passing the contents of
infra-config.sqlthroughevalmeans any`...`or$(...)that ends up in the SQL (now or in future edits — e.g. a password containing$, a comment with backticks) will be executed as a shell command, and any literal\,",!etc. must also be shell-escaped. Preferenvsubst(or a whitelist of variables) for pure variable substitution:Proposed change
-SQL_TEMPLATE=$(cat ./conf/proxysql/infra-config.sql) -SQL_CONTENT=$(eval "echo \"${SQL_TEMPLATE}\"") +SQL_CONTENT=$(envsubst '${INFRA} ${INFRA_ID} ${WHG} ${ROOT_PASSWORD}' \ + < ./conf/proxysql/infra-config.sql)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/bin/docker-proxy-post.bash` around lines 11 - 12, The current code reads infra-config.sql into SQL_TEMPLATE and then uses unsafe eval in SQL_CONTENT (SQL_TEMPLATE and SQL_CONTENT) which will execute any shell metacharacters; instead replace the eval "echo ..." step with a safe variable substitution approach such as piping the template through envsubst or using a controlled whitelist-based replacement: load the file into SQL_TEMPLATE as before, then run envsubst on that content (or implement explicit parameter replacement for known variables) to produce SQL_CONTENT, ensuring infra-config.sql is not executed by the shell.test/infra/infra-dbdeployer-mysql56-single/docker/Dockerfile (1)
22-31: Consider verifying the MySQL tarball checksum.The MySQL 5.6.49 tarball is downloaded over the network and consumed without any integrity check. A network MITM or a future URL/redirect change would silently produce a compromised image. Since the version is pinned via
ARG, pinning the SHA256 alongside it is cheap and makes the build reproducible.Proposed change
ARG DBDEPLOYER_VERSION=2.2.1 ARG MYSQL_TARBALL_URL=https://dev.mysql.com/get/Downloads/MySQL-5.6/mysql-5.6.49-linux-glibc2.12-x86_64.tar.gz +ARG MYSQL_TARBALL_SHA256=<sha256-of-5.6.49-tarball> ... && curl -fsSL -o /root/downloads/mysql-5.6.49-linux-glibc2.12-x86_64.tar.gz "${MYSQL_TARBALL_URL}" \ + && echo "${MYSQL_TARBALL_SHA256} /root/downloads/mysql-5.6.49-linux-glibc2.12-x86_64.tar.gz" | sha256sum -c - \ && dbdeployer unpack /root/downloads/mysql-5.6.49-linux-glibc2.12-x86_64.tar.gz \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql56-single/docker/Dockerfile` around lines 22 - 31, Add a pinned SHA256 variable and validate the downloaded tarball before unpacking: introduce an ARG or ENV like MYSQL_TARBALL_SHA256 and after downloading via curl (the current use of MYSQL_TARBALL_URL) compute the tarball checksum (e.g., sha256sum) and compare it to MYSQL_TARBALL_SHA256, failing the build if it does not match; only proceed to call dbdeployer unpack on successful verification and remove the tarball on mismatch/after verification. Reference the existing symbols MYSQL_TARBALL_URL, the downloaded file path /root/downloads/mysql-5.6.49-linux-glibc2.12-x86_64.tar.gz, and the dbdeployer unpack invocation to locate where to insert the check.test/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpp (1)
82-98: Magic error code9001and directproxy->optionsmanipulation.Two nits worth addressing:
Line 84 — directly writing
proxy->options.client_flag |= CLIENT_DEPRECATE_EOF;touches internal client-library struct fields. The supported API is to pass client flags via the last argument ofmysql_real_connect():mysql_real_connect(proxy, cl.host, "sbtest4", "sbtest4", NULL, cl.port, NULL, CLIENT_DEPRECATE_EOF);This is more portable across libmysqlclient/libmariadb versions.
Line 95 —
error_code == 9001is a magic number for the ProxySQL-generated "backend connection unavailable" error. If the error code is redefined, this test breaks silently with a misleading diff. Consider referencing a named constant (e.g.,ER_PROXYSQL_NO_BACKEND_CONNECTIONor equivalent) or adding a comment citing the ProxySQL source where 9001 is defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpp` around lines 82 - 98, Don't write into the internal client struct via proxy->options.client_flag; instead pass CLIENT_DEPRECATE_EOF through the mysql_real_connect() client_flag parameter (update the mysql_real_connect call that currently passes 0 as the last arg). Also avoid the magic literal 9001: replace the direct comparison error_code == 9001 with a named constant or at minimum add a clear comment referencing the ProxySQL source/name (e.g., ER_PROXYSQL_NO_BACKEND_CONNECTION) and use that named symbol where available so the check in the ok(...) assertion refers to the constant instead of the hardcoded 9001.include/MySQL_Session.h (1)
246-274: Remove PR-specific markers from doc comments before merge.The
=== PR 5166: Backend Configuration Entry Point ===and=== PR 5166: Variable Processing Workflow ===annotations (also at line 359) embed transient PR metadata into permanent header documentation. This noise obscures the intent and ages poorly—git history is the right place for PR provenance. Consider trimming these blocks to a concise purpose statement and moving operational detail to the.cppimplementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_Session.h` around lines 246 - 274, Remove the transient PR annotations (e.g., "=== PR 5166: Backend Configuration Entry Point ===" and the similar marker at the other doc block around line 359) from the header comment for the session-tracking function in include/MySQL_Session.h; replace them with a concise purpose sentence for the function (e.g., "Configure session variable tracking on backend connections") and move the operational/configuration details (state machine steps, configuration logic) into the corresponding .cpp implementation comments (or trim them to a brief summary in the header), ensuring the header only documents the API contract and return semantics for the function that orchestrates session tracking.lib/MySQL_Session.cpp (1)
6077-6091: Mirror charset/collation side effects when applying backend notifications.The SET parser keeps
collation_connectionandcharacter_set_connectionin sync, but this backend-notification path only updates the notified index. If the server reports only one side of that implicit change, ProxySQL’s client/server maps diverge and later backend verification can issue unnecessary or wrong charset resets.Suggested direction
if (ci) { value = std::to_string(ci->nr); } + if (variable == "collation_connection") { + mysql_variables.client_set_value(this, SQL_CHARACTER_SET_CONNECTION, value.c_str()); + mysql_variables.server_set_value(this, SQL_CHARACTER_SET_CONNECTION, value.c_str()); + } else if (variable == "character_set_connection") { + mysql_variables.client_set_value(this, SQL_COLLATION_CONNECTION, value.c_str()); + mysql_variables.server_set_value(this, SQL_COLLATION_CONNECTION, value.c_str()); + } + - mysql_variables.client_set_value(this, idx, value); + mysql_variables.client_set_value(this, idx, value.c_str()); mysql_variables.server_set_value(this, idx, value.c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 6077 - 6091, The backend-notification path only updates the notified variable index, causing client/server maps to diverge for charset/collation pairs; update the mirrored counterpart when handling "collation_connection" or any "character_set_*" change. After resolving ci with proxysql_find_charset_name or proxysql_find_charset_collate, determine the paired variable's index (the complementary "collation_connection" or the appropriate "character_set_connection"/"character_set_client"/"character_set_results"/"character_set_database" mapping) and call mysql_variables.client_set_value(...) and mysql_variables.server_set_value(...) for both the original idx and the paired index so both sides stay in sync. Use the same value conversion (std::to_string(ci->nr)) for the mirrored update and ensure you only mirror when ci is non-null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c675f26-decd-4e52-8517-6d31f80a4091
📒 Files selected for processing (45)
include/MySQL_HostGroups_Manager.hinclude/MySQL_Session.hinclude/MySQL_Thread.hinclude/mysql_connection.hinclude/proxysql_structs.hlib/MyHGC.cpplib/MySQL_Protocol.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/MySrvC.cpplib/mysql_connection.cpptest/infra/control/run-tests-isolated.bashtest/infra/infra-dbdeployer-mysql56-single/.envtest/infra/infra-dbdeployer-mysql56-single/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql56-single/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql56-single/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql56-single/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql56-single/docker-compose.ymltest/infra/infra-dbdeployer-mysql56-single/docker/Dockerfiletest/infra/infra-dbdeployer-mysql56-single/docker/build.shtest/infra/infra-dbdeployer-mysql56-single/docker/entrypoint.shtest/infra/infra-mysql56-single/.envtest/infra/infra-mysql56-single/bin/docker-mysql-post.bashtest/infra/infra-mysql56-single/bin/docker-proxy-post.bashtest/infra/infra-mysql56-single/conf/mysql/Dockerfiletest/infra/infra-mysql56-single/conf/mysql/docker-entrypoint.shtest/infra/infra-mysql56-single/conf/mysql/mysql1/my.cnftest/infra/infra-mysql56-single/conf/mysql/ssl/configure_ssl.shtest/infra/infra-mysql56-single/conf/proxysql/infra-config.sqltest/infra/infra-mysql56-single/docker-compose-destroy.bashtest/infra/infra-mysql56-single/docker-compose-init.bashtest/infra/infra-mysql56-single/docker-compose.ymltest/tap/groups/groups.jsontest/tap/groups/mysql56-single/env.shtest/tap/groups/mysql56-single/infras.lsttest/tap/groups/mysql56-single/pre-proxysql.bashtest/tap/tap/utils.htest/tap/tests/mysql-session_track_variables_enforced-t.cpptest/tap/tests/mysql-session_track_variables_ff-t.cpptest/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpptest/tap/tests/mysql-session_track_variables_optional-t.cpptest/tap/tests/reg_test_4264-commit_rollback-t.cpptest/tap/tests/session_track_variables.htest/tap/tests/test_binlog_reader-t.cpp
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (12)
lib/MyHGC.cpp (1)
41-43:⚠️ Potential issue | 🟠 MajorUse an atomic/locked read for the backoff timestamp.
server_backoff_timecan be updated by another session thread while this selection loop reads it. Keep the backoff check, but make the member atomic and use.load(std::memory_order_relaxed)here, or guard all reads/writes with the same mutex. As per coding guidelines,**/*.{cpp,h,hpp}should “Use pthread mutexes for synchronization andstd::atomic<>for counters.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MyHGC.cpp` around lines 41 - 43, The backoff check reads mysrvc->server_backoff_time concurrently without synchronization; make server_backoff_time a std::atomic<int64_t> (or appropriate integer type) in the service struct/header and include <atomic>, then change the read here to mysrvc->server_backoff_time.load(std::memory_order_relaxed) (or alternatively protect both reads/writes with the same pthread mutex if you prefer mutexing); update any writes to use .store(...) or atomic ops so all accesses are consistently synchronized.test/tap/tests/reg_test_4264-commit_rollback-t.cpp (1)
1156-1160:⚠️ Potential issue | 🟠 MajorDon’t mask the new session-tracking traffic in this regression test.
This test depends on exact query/connection deltas, but disabling
mysql-session_track_variablesglobally sidesteps the behavior added by this PR. Prefer taking baselines after connection-init/session-track setup, or explicitly subtract/ignore the internal setup queries, so transaction routing is still validated with tracking enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/reg_test_4264-commit_rollback-t.cpp` around lines 1156 - 1160, The test currently disables session tracking by issuing MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0") which masks the new session-tracking traffic; remove or avoid that SET and instead collect the baseline after performing connection-init/session-track setup (or explicitly subtract/ignore the known internal setup queries like "LOAD MYSQL VARIABLES TO RUNTIME" and any connection-init commands) so the test can validate transaction routing with session tracking enabled; update the test around the MYSQL_QUERY_T calls (e.g., the block containing MYSQL_QUERY_T(admin, "SET mysql-auto_increment_delay_multiplex=0"), MYSQL_QUERY_T(admin, "SET mysql-default_session_track_gtids='OFF'"), MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0"), and MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME")) to either not set mysql-session_track_variables to 0 or to take baselines after setup and explicitly ignore the setup queries in the delta comparison.lib/MySrvC.cpp (1)
34-34:⚠️ Potential issue | 🟠 MajorMake
server_backoff_timethread-safe before using it across session threads.This field is written from session handling and read during server/connection selection, but the backing member is still a plain
unsigned long long; initialize/store/load it throughstd::atomic<unsigned long long>or guard all accesses with the hostgroup lock. As per coding guidelines,**/*.{cpp,h,hpp}should “Use pthread mutexes for synchronization andstd::atomic<>for counters.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySrvC.cpp` at line 34, The field server_backoff_time is accessed by session threads and the server/connection selection path but is a plain unsigned long long; make it thread-safe by changing the backing member to std::atomic<unsigned long long> (or, if you prefer locking, ensure every read/write is protected by the existing hostgroup mutex), then update all uses (writes in the session handling code and reads in the server/connection selection code) to use atomic store/load semantics or to acquire the hostgroup lock before accessing server_backoff_time; search for the symbol server_backoff_time and the session/selection code paths to update all accesses consistently.test/tap/tests/test_binlog_reader-t.cpp (1)
464-465:⚠️ Potential issue | 🟠 MajorRestore the global session-tracking setting in cleanup.
This disables
mysql-session_track_variablesat runtime but never restores the previous value, so later tests in the same ProxySQL instance can inherit the modified behavior. Save the original value before changing it and restore it at thecleanup:path after the assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/test_binlog_reader-t.cpp` around lines 464 - 465, Before calling MYSQL_QUERY_T(proxysql_admin, "SET mysql-session_track_variables=0"), query and store the current value (e.g., run MYSQL_QUERY_T(proxysql_admin, "SELECT @@mysql-session_track_variables") and save into a variable) and then after the test assertions in the cleanup: block restore it by executing MYSQL_QUERY_T(proxysql_admin, "SET mysql-session_track_variables=<saved_value>"); ensure the saved value is used even if the test fails so cleanup: always resets the global setting.include/MySQL_Thread.h (1)
380-382:⚠️ Potential issue | 🟡 MinorUpdate
ENFORCEDdocs to describe backoff/retry behavior, not hard failure.The implementation path uses backend rejection/backoff for unsupported servers; “connection fails” reads like a client-visible hard failure and is misleading.
Proposed fix
- // Enforce session tracking; connection fails if backend does - // not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING + // Enforce session tracking; backends that do not support + // CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING are rejected + // and put in temporary backoff.- * - ENFORCED (2): Enforce session tracking; connection fails if backend - * does not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING. + * - ENFORCED (2): Enforce session tracking; unsupported backends are + * rejected and put in temporary backoff.Verification:
#!/bin/bash # Verify comments match the implementation behavior around capability handling and backoff. rg -n -C4 'handle_session_track_capabilities|server_backoff_time|connection fails if backend|temporary backoff' include libAlso applies to: 663-664
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_Thread.h` around lines 380 - 382, Update the ENFORCED comment to reflect that unsupported server session-tracking capabilities trigger a client-side backoff/retry rather than an immediate hard failure: change wording around the ENFORCED marker in include/MySQL_Thread.h to describe that when CLIENT_DEPRECATE_EOF or CLIENT_SESSION_TRACKING are rejected the implementation uses server_backoff_time and handle_session_track_capabilities to perform a temporary backoff and retry connection attempts, not a fatal client-visible failure; reference those symbols (handle_session_track_capabilities, server_backoff_time, ENFORCED) in the docstring so the behavior matches the code.include/MySQL_HostGroups_Manager.h (1)
209-215:⚠️ Potential issue | 🟠 MajorMake
server_backoff_timeatomic before sharing it across session threads.This field is read during host/cached-connection selection (
lib/MyHGC.cpp,lib/MySQL_Thread.cpp) and is written by session handling, so the plainunsigned long longintroduces an unsynchronized data race. Usestd::atomic<unsigned long long>and update call sites toload()/store().Proposed fix
- unsigned long long server_backoff_time; + std::atomic<unsigned long long> server_backoff_time { 0 };Then update readers/writers, for example:
mysrvc->server_backoff_time.load(std::memory_order_relaxed) mysrvc->server_backoff_time.store(backoff_until, std::memory_order_relaxed)Verification:
#!/bin/bash # Verify all server_backoff_time access sites are atomic load/store or otherwise lock-protected. rg -n -C3 '\bserver_backoff_time\b' include libAs per coding guidelines, use pthread mutexes for synchronization and
std::atomic<>for counters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_HostGroups_Manager.h` around lines 209 - 215, Change the plain field server_backoff_time to std::atomic<unsigned long long> in the class declaration and update every access site to use atomic load/store (e.g. in MyHGC::get_random_MySrvC, lib/MyHGC.cpp, lib/MySQL_Thread.cpp and any session handling code that writes it) instead of direct reads/writes; use mysrvc->server_backoff_time.load(std::memory_order_relaxed) for readers and mysrvc->server_backoff_time.store(backoff_until, std::memory_order_relaxed) for writers (or stronger ordering where logically required) and ensure no remaining unsynchronized accesses remain.test/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bash (1)
4-8:⚠️ Potential issue | 🟠 MajorMirror init’s
INFRA_IDhandling before destroying the compose project.
docker-compose-init.bashpreserves a caller-providedINFRA_ID; this script should do the same before derivingCOMPOSE_PROJECT, otherwisedown -vcan target the wrong project and leave the real infra running.Proposed fix
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null trap 'popd &>/dev/null' EXIT -set -a; . .env; set +a +if [ ! -f .env ]; then echo "Error: .env not found"; exit 1; fi +SAVED_INFRA_ID="${INFRA_ID}" +set -a; . .env; set +a +[ -n "${SAVED_INFRA_ID}" ] && export INFRA_ID="${SAVED_INFRA_ID}" export INFRA=${PWD##*/} export COMPOSE_PROJECT="${INFRA}-${INFRA_ID}"Verification:
#!/bin/bash # Compare init/destroy project derivation. Expect both scripts to preserve caller-provided INFRA_ID # before computing COMPOSE_PROJECT. rg -n -C2 'SAVED_INFRA_ID|INFRA_ID|COMPOSE_PROJECT|set -a|pushd' \ test/infra/infra-dbdeployer-mysql56-single/docker-compose-init.bash \ test/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bash` around lines 4 - 8, The destroy script must mirror init's INFRA_ID handling: if a caller provided INFRA_ID, preserve it across sourcing .env before deriving COMPOSE_PROJECT. Before "set -a; . .env; set +a" save any existing INFRA_ID (e.g., SAVED_INFRA_ID="$INFRA_ID"), then source .env, then if SAVED_INFRA_ID is non-empty restore INFRA_ID from it, and only after that compute COMPOSE_PROJECT="${INFRA}-${INFRA_ID}" so docker-compose down -v targets the same project as init.test/tap/tests/mysql-session_track_variables_enforced-t.cpp (1)
16-20:⚠️ Potential issue | 🟠 MajorGuard cleanup handles so failed reconnect still resets ENFORCED mode.
When Line 61 returns
NULL,cleanup(admin, proxy)still runs and Line 19 closesproxyunconditionally. Keep the admin reset, but only close non-null handles.Proposed fix
int cleanup(MYSQL* admin, MYSQL* proxy) { - MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0"); - MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); - mysql_close(proxy); - mysql_close(admin); + if (admin) { + MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0"); + MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); + } + if (proxy) { + mysql_close(proxy); + } + if (admin) { + mysql_close(admin); + } return EXIT_SUCCESS; }Also applies to: 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-session_track_variables_enforced-t.cpp` around lines 16 - 20, The cleanup function currently unconditionally calls mysql_close on both proxy and admin which can crash when proxy is NULL after a failed reconnect; update the cleanup(MYSQL* admin, MYSQL* proxy) routine to always execute the admin reset queries (MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0"); and MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME");) but only call mysql_close(proxy) if proxy is non-NULL and only call mysql_close(admin) if admin is non-NULL, guarding both closes with null checks to avoid closing null pointers.lib/MySQL_Thread.cpp (1)
6394-6397:⚠️ Potential issue | 🟠 MajorKeep
server_backoff_timeaccess synchronized.This read still races with writes from the session path unless
server_backoff_timehas been converted to an atomic or guarded accessor everywhere. Use one synchronization strategy for the declaration, writes, and this read. Based on learnings, Applies to **/*.{cpp,h,hpp} : Use pthread mutexes for synchronization andstd::atomic<>for counters.Proposed atomic fix
- if (c->parent->server_backoff_time > curtime) + if (c->parent->server_backoff_time.load(std::memory_order_acquire) > curtime) continue;And update the backing field/write sites consistently:
- unsigned long long server_backoff_time; + std::atomic<unsigned long long> server_backoff_time {0};- be_conn->parent->server_backoff_time = thread->curtime + (30 * 1000000); + be_conn->parent->server_backoff_time.store( + thread->curtime + (30 * 1000000), + std::memory_order_release + );Run this read-only check to verify all access sites use the same synchronization model:
#!/bin/bash set -euo pipefail # Expectation: declaration is std::atomic<...> or all reads/writes go through a mutex-protected accessor. rg -n -C3 '\bserver_backoff_time\b' \ include/MySQL_HostGroups_Manager.h \ lib/MySQL_Thread.cpp \ lib/MySQL_Session.cpp \ lib/MyHGC.cpp # Expectation: no direct plain assignments/reads remain if the field is atomic. rg -n -C2 '\bserver_backoff_time\s*(=|\.store|\.load)\b|server_backoff_time\s*[<>!=]' \ include/MySQL_HostGroups_Manager.h \ lib/MySQL_Thread.cpp \ lib/MySQL_Session.cpp \ lib/MyHGC.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Thread.cpp` around lines 6394 - 6397, The read of server_backoff_time (seen at c->parent->server_backoff_time in MySQL_Thread::...) races with writers; make server_backoff_time access consistent by choosing one strategy (preferred: protect the field with a pthread mutex on the host/group object and add guarded accessor methods, or convert the declaration to std::atomic<time_t> and update all writes/reads to use .store/.load). Update the declaration in MySQL_HostGroups_Manager (or the host struct/class that defines server_backoff_time), change all write sites in MySQL_Session.cpp and MyHGC.cpp to use the chosen synchronization (mutex lock + accessor or atomic store), and change this read in MySQL_Thread.cpp to use the same accessor or atomic load; then run the provided grep-based verification script to ensure no remaining unsynchronized direct accesses.lib/MySQL_Session.cpp (3)
2006-2016:⚠️ Potential issue | 🟡 MinorSet the session-track “sent” flags only after the
SETsucceeds.
session_track_variables_sentandsession_track_state_sentare flipped before the asyncSETcompletes. If a soft error/retry path returns to the previous status, the next verification skips the SET forever for that connection.🛠️ Suggested direction
Move these flags to the success path of the corresponding
SETTING_SESSION_TRACK_*handlers, or clear the specific flag before popping back toprevious_statuson retryable/ignored errors.- mybe->server_myds->myconn->options.session_track_variables_sent = true; set_previous_status_mode3(); NEXT_IMMEDIATE_NEW(SETTING_SESSION_TRACK_VARIABLES);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 2006 - 2016, The flags session_track_variables_sent and session_track_state_sent are currently set before the async SET completes; change this so the flags are only set on successful completion of the corresponding async SETs (i.e., move the assignments out of the current block and into the success/commit path of the SETTING_SESSION_TRACK_VARIABLES and SETTING_SESSION_TRACK_STATE handlers), and/or ensure any retry/pop-to-previous_status paths (invocations of set_previous_status_mode3() / NEXT_IMMEDIATE_NEW) clear the appropriate flag before returning so a failed/soft-retry does not permanently skip the SET for that connection.
1940-1942:⚠️ Potential issue | 🟠 MajorUse negotiated
CLIENT_DEPRECATE_EOF, not advertised server capabilities.These checks still use
mysql->server_capabilitiesforCLIENT_DEPRECATE_EOF. For pooled/reused backend connections, validatebe_conn->options.client_flag & CLIENT_DEPRECATE_EOFso ENFORCED/GTID/session-variable tracking only runs when EOF deprecation was actually negotiated. KeepCLIENT_SESSION_TRACKINGagainst server-advertised capabilities.🔎 Read-only verification
#!/bin/bash # Verify where backend negotiated flags are copied and where session tracking still checks advertised capabilities. # Expectation: CLIENT_DEPRECATE_EOF checks for backend session tracking should use options.client_flag. rg -n -C3 --type=cpp --type=h \ 'options\.client_flag\s*=\s*mysql->client_flag|server_capabilities.*CLIENT_DEPRECATE_EOF|CLIENT_DEPRECATE_EOF.*server_capabilities|CLIENT_SESSION_TRACKING' \ lib include🛠️ Suggested direction
- if ((mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_SESSION_TRACKING) == 0 - || (mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_DEPRECATE_EOF) == 0) { + MySQL_Connection* be_conn = mybe->server_myds->myconn; + if ((be_conn->mysql->server_capabilities & CLIENT_SESSION_TRACKING) == 0 + || (be_conn->options.client_flag & CLIENT_DEPRECATE_EOF) == 0) {Also applies to: 1998-2004, 9751-9757
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 1940 - 1942, The check is using the server's advertised server_capabilities for CLIENT_DEPRECATE_EOF but must use the negotiated client flags on the backend connection; change the second conditional to test be_conn->options.client_flag & CLIENT_DEPRECATE_EOF (leave CLIENT_SESSION_TRACKING check against mysql->server_capabilities) and ensure mysql_thread___enable_server_deprecate_eof is still respected; update the analogous checks at the other occurrences (around the blocks identified near lines ~1998-2004 and ~9751-9757) so ENFORCED/GTID/session-variable tracking only runs when CLIENT_DEPRECATE_EOF was actually negotiated on be_conn.
2678-2706:⚠️ Potential issue | 🟠 MajorKeep permitted
session_track_*setup errors non-fatal even when they are not tracked variables.The bounds check avoids the out-of-bounds write, but
session_track_system_variablesandsession_track_state_changeare still not inmysql_tracked_variables. For 1193/permitted tracking errors,idxremains out of range and execution falls through to sending an ERR packet to the client, which breaks OPTIONAL fallback.🛠️ Suggested direction
- if (idx >= 0 && idx < SQL_NAME_LAST_HIGH_WM) { + const bool known_tracked_variable = idx >= 0 && idx < SQL_NAME_LAST_HIGH_WM; + const bool ignorable_session_track_error = is_perm_track_err(myerr, var_name) + || strcasecmp(var_name, "session_track_system_variables") == 0 + || strcasecmp(var_name, "session_track_state_change") == 0; + + if (known_tracked_variable || ignorable_session_track_error) { + if (known_tracked_variable) { myconn->var_absent[idx] = true; + } myds->myconn->async_free_result(); myconn->compute_unknown_transaction_status();Also applies to: 2820-2831
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_Session.cpp` around lines 2678 - 2706, The code currently only handles permitted tracking errors (is_perm_track_err(var_name)) if the variable is found in mysql_tracked_variables, causing an ERR to be sent when a permitted session_track_* error refers to a variable that isn't tracked; fix by treating permitted tracking errors as non-fatal even when idx is not within [0, SQL_NAME_LAST_HIGH_WM): after computing idx, if is_perm_track_err(myerr, var_name) and idx is out of bounds, do not send an error—perform the same non-fatal flow (set myds->revents |= POLLOUT, set myds->DSS = STATE_MARIADB_GENERIC, pop previous_status and call NEXT_IMMEDIATE_NEW) without touching myconn->var_absent to avoid out-of-bounds writes; use mysql_tracked_variables, is_perm_track_err, myconn->var_absent, SQL_NAME_LAST_HIGH_WM, myds->revents, myds->DSS, previous_status, and NEXT_IMMEDIATE_NEW to locate and implement the change.
🧹 Nitpick comments (7)
include/mysql_connection.h (1)
269-298: Add explicit#include <unordered_map>for clarity and self-contained inclusion.The new
get_variables()method at line 298 exposesstd::unordered_mapin the public API. While this type is available through a transitive include viacpp.h, adding an explicit include makes the dependency clear and improves header self-containment.🛠️ Proposed fix
`#include` "proxysql.h" `#include` "cpp.h" +#include <unordered_map>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/mysql_connection.h` around lines 269 - 298, Add an explicit `#include` <unordered_map> to the header so the public API type used by get_variables(std::unordered_map<std::string, std::string>&) is directly visible; modify include/mysql_connection.h to include <unordered_map> near other standard headers so the declaration of bool get_variables(...) is self-contained and does not rely on transitive includes like cpp.h.test/tap/groups/mysql56-single/env.sh (1)
5-8: Use[[instead of[and suppress SC1090 for the dynamic.envsource.The dynamic source on line 7 triggers ShellCheck SC1090. Replace single brackets with
[[, and add a# shellcheck source=/dev/nulldirective to silence the warning. These are bash best practices for this context.Proposed fix
-if [ -n "${WORKSPACE}" ]; then +if [[ -n "${WORKSPACE}" ]]; then _INFRA_ENV="${WORKSPACE}/test/infra/${DEFAULT_MYSQL_INFRA}/.env" - [ -f "${_INFRA_ENV}" ] && . "${_INFRA_ENV}" + if [[ -f "${_INFRA_ENV}" ]]; then + # shellcheck source=/dev/null + . "${_INFRA_ENV}" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/mysql56-single/env.sh` around lines 5 - 8, Replace the POSIX test brackets and silence ShellCheck for the dynamic env source: change the conditionals using [ -n "${WORKSPACE}" ] and [ -f "${_INFRA_ENV}" ] to bash conditional syntax [[ -n "${WORKSPACE}" ]] and [[ -f "${_INFRA_ENV}" ]], and add a directive comment "# shellcheck source=/dev/null" immediately before the dynamic source line that evaluates . "${_INFRA_ENV}" (referencing variables WORKSPACE, _INFRA_ENV, DEFAULT_MYSQL_INFRA) so SC1090 is suppressed.test/infra/infra-mysql56-single/bin/docker-proxy-post.bash (1)
11-12: Use the resolved script directory and avoidevalfor the SQL template.Line 11 ignores
SCRIPT_DIR, so this script depends on the caller’s working directory. Line 12 also evaluates the SQL as shell code; prefer reading the file via an absolute script-relative path and only substituting known variables if needed.Proposed hardening
-SQL_TEMPLATE=$(cat ./conf/proxysql/infra-config.sql) -SQL_CONTENT=$(eval "echo \"${SQL_TEMPLATE}\"") +SQL_TEMPLATE_PATH="${SCRIPT_DIR}/../conf/proxysql/infra-config.sql" +SQL_CONTENT=$(cat "${SQL_TEMPLATE_PATH}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/bin/docker-proxy-post.bash` around lines 11 - 12, The current code reads the SQL template relative to the CWD and uses eval to expand it; change it to load the file using the resolved SCRIPT_DIR (use the SCRIPT_DIR variable) to build an absolute path for ./conf/proxysql/infra-config.sql, and replace the use of eval for SQL_CONTENT with a safe read/templating approach (e.g., read the file contents into SQL_TEMPLATE and perform controlled variable substitution with a safe tool or parameter expansion rather than eval) so SQL_TEMPLATE and SQL_CONTENT no longer depend on the caller's working directory or execute arbitrary shell code.test/infra/infra-mysql56-single/bin/docker-mysql-post.bash (1)
88-94: Optional: align CA extraction with the dbdeployer variant.The sibling script uses
docker exec ... cat ca.pem | tee -a ...while this one usesdocker cp ... - | tar -Ox | tee -a .... Both work, but unifying ondocker exec catis simpler, avoids the tar-stream dependency, and keeps the two post-scripts symmetric.♻️ Suggested change
- docker cp "${CONTAINER}:/var/lib/mysql/ca.pem" - | tar -Ox | sudo tee -a "${DB_BUNDLE}" | sudo tee -a "${CA_BUNDLE}" >/dev/null + docker exec "${CONTAINER}" cat /var/lib/mysql/ca.pem | sudo tee -a "${DB_BUNDLE}" | sudo tee -a "${CA_BUNDLE}" >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql56-single/bin/docker-mysql-post.bash` around lines 88 - 94, Replace the tar-stream-based CA extraction with a simpler docker exec/cat pipeline used by the sibling script: instead of using docker cp "${CONTAINER}:/var/lib/mysql/ca.pem" - | tar -Ox | sudo tee -a "${DB_BUNDLE}" | sudo tee -a "${CA_BUNDLE}", run docker exec "${CONTAINER}" cat /var/lib/mysql/ca.pem and pipe its output into sudo tee -a "${DB_BUNDLE}" and sudo tee -a "${CA_BUNDLE}" (preserving the existing sudo chmod 666 on "${DB_BUNDLE}" and "${CA_BUNDLE}" and the surrounding if test -f /var/lib/mysql/ca.pem check).test/tap/groups/groups.json (1)
95-98: Missing MySQL 8.x test-group coverage for new session-tracking tests.
mysql-session_track_variables_enforced-tandmysql-session_track_variables_optional-tare registered only forlegacy-g4andmysql56-single-g1, andmysql-session_track_variables_ff*only forlegacy-g4/mysql56-single-g1. Most comparable tests in this file (e.g.,deprecate_eof_cache-t,eof_conn_options_check-t) also run onmysql84-g4/mysql90-g4/mysql95-g4. Given that ENFORCED mode specifically depends onCLIENT_SESSION_TRACK+CLIENT_DEPRECATE_EOFwhich differ across MySQL 5.7/8.0/8.4/9.x, running against those groups would catch protocol-level regressions the 5.6 and legacy backends cannot exercise.♻️ Suggested coverage expansion
- "mysql-session_track_variables_enforced-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql56-single-g1" ], - "mysql-session_track_variables_ff-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4" ], - "mysql-session_track_variables_ff_mysql56-t" : [ "mysql56-single-g1" ], - "mysql-session_track_variables_optional-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql56-single-g1" ], + "mysql-session_track_variables_enforced-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql84-g4","mysql90-g4","mysql95-g4","mysql56-single-g1" ], + "mysql-session_track_variables_ff-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql84-g4","mysql90-g4","mysql95-g4" ], + "mysql-session_track_variables_ff_mysql56-t" : [ "mysql56-single-g1" ], + "mysql-session_track_variables_optional-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql84-g4","mysql90-g4","mysql95-g4","mysql56-single-g1" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/groups.json` around lines 95 - 98, The test groups for session-tracking lack MySQL 8+ coverage; update the arrays for "mysql-session_track_variables_enforced-t", "mysql-session_track_variables_optional-t", and "mysql-session_track_variables_ff-t" to also include "mysql84-g4", "mysql90-g4", and "mysql95-g4" (keep "mysql-session_track_variables_ff_mysql56-t" as-is for mysql56-specific runs). This ensures ENFORCED/OPTIONAL/FF variants run against MySQL 8.x/9.x groups to catch protocol-level regressions.include/MySQL_Session.h (1)
246-304: Keep header comments durable and concise.The declarations look fine, but the new header docs include PR-specific labels and detailed workflow narration that will age quickly. Prefer a short API contract here and keep implementation workflow in the
.cppor design notes.Also applies to: 356-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_Session.h` around lines 246 - 304, The header comments are too verbose and include PR-specific workflow; shorten them to a concise API contract for the declared functions (e.g., handler_again___verify_backend_session_track_variables, handler_again___verify_backend_multi_statement, handler_again___verify_backend_user_schema, handler_again___verify_multiple_variables, handler_again___status_SETTING_INIT_CONNECT, handler_again___status_SETTING_LDAP_USER_VARIABLE, handler_again___status_SETTING_SQL_MODE, handler_again___status_SETTING_SESSION_TRACK_GTIDS, handler_again___status_SETTING_SESSION_TRACK_VARIABLES, handler_again___status_SETTING_SESSION_TRACK_STATE): remove PR labels, step-by-step state machine narration and implementation details, leaving a one- or two-sentence description of purpose, parameters, and return semantics; move any detailed workflow or rationale into the .cpp implementation or design notes.test/tap/tests/session_track_variables.h (1)
111-228: Hardcodedinnodb_lock_wait_timeoutlimits portability.Every helper (
set_session_variable,select_sess_var_value,extract_sess_var_proxy_internal, andtest_session_variables_ff) is pinned toinnodb_lock_wait_timeout. As already raised in the PR discussion, that variable is InnoDB-specific and may not behave identically on Aurora or MyRocks deployments. Parameterise the variable name (or pick a server-generic one likewait_timeout/net_read_timeoutthat is writable at session scope) so the same helper can be reused across infra targets without reimplementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/session_track_variables.h` around lines 111 - 228, The helpers are hardcoded to "innodb_lock_wait_timeout", breaking portability; update set_session_variable, select_sess_var_value, extract_sess_var_proxy_internal, and test_session_variables_ff to accept a variable-name parameter (e.g., const std::string& var_name or const char* var_name) and use that name when building the CREATE PROCEDURE, CALL, SELECT @@..., and when extracting JSON keys from PROXYSQL INTERNAL SESSION and mysql packet tracking; keep current behavior by allowing a default value (e.g., "wait_timeout") when none is provided so tests can run against non-InnoDB backends without duplicating logic.
🤖 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/MySQL_Session.cpp`:
- Around line 9764-9767: The write to backend-wide server_backoff_time from
session logic (see MySrvC::server_backoff_time being set via
be_conn->parent->server_backoff_time = thread->curtime + (30 * 1000000)) must be
made thread-safe to avoid races with concurrent hostgroup/server reads; either
change MySrvC::server_backoff_time to an atomic type (e.g.,
std::atomic<uint64_t>) and use atomic stores/loads where referenced, or protect
both writes and reads with the existing hostgroup/server pthread mutex around
the assignment and all accesses. Also replace the magic constant (30 * 1000000)
and the stale 10-minute comment with a named constexpr (e.g.,
BACKOFF_MICROSECONDS) so the intention is clear and maintainable; update any
code that reads server_backoff_time to use atomic load or hold the mutex
accordingly.
In `@test/infra/infra-dbdeployer-mysql56-single/bin/docker-mysql-post.bash`:
- Around line 5-8: The script currently only sources .env before expanding
COMPOSE_PROJECT, INFRA_LOGS_PATH and ROOT_PASSWORD, so CONTAINER and BUNDLE_DIR
can be empty; update the startup sequence to first source the isolated env file
(.env.isolated.${INFRA_ID}) if it exists (falling back to .env) before any use
of COMPOSE_PROJECT, INFRA_LOGS_PATH or ROOT_PASSWORD, ensuring variables used to
build CONTAINER and BUNDLE_DIR and passed as -p"${ROOT_PASSWORD}" are populated;
reference the variable names CONTAINER, BUNDLE_DIR, COMPOSE_PROJECT,
INFRA_LOGS_PATH and ROOT_PASSWORD when adding the conditional source.
In `@test/infra/infra-dbdeployer-mysql56-single/bin/docker-proxy-post.bash`:
- Around line 14-18: The current docker exec invocation that runs mysql (the
here-doc block invoking ${SQL_CONTENT}) can abort the script under set -e before
the failure echo runs; move the docker exec mysql command into the conditional
check (i.e., run docker exec -i "${PROXY_CONTAINER}" mysql ... <<SQL inside an
if ...; then echo success in the then branch and the error/fail and exit 1 in
the else branch) so the failure branch becomes reachable; update the conditional
that references $? to instead test the command result directly around the docker
exec invocation (use the same ${PROXY_CONTAINER}, ${SQL_CONTENT}, and ${INFRA}
symbols).
- Around line 11-12: The current expansion uses eval on SQL_TEMPLATE then
assigns to SQL_CONTENT which is unsafe; instead, read the template via a
script-relative path (resolve with dirname "$0") into SQL_TEMPLATE and perform a
bounded, non-evaluating substitution to produce SQL_CONTENT (e.g. run the
template through envsubst with an explicit variable whitelist like
'${ROOT_PASSWORD}' or use a sed/awk replacement for specific placeholders) so
shell syntax is not executed and credentials with $, backticks or backslashes
are preserved; update the SQL_TEMPLATE/SQL_CONTENT assignment to use this safe
substitution approach rather than eval "echo ...".
In `@test/infra/infra-dbdeployer-mysql56-single/conf/proxysql/infra-config.sql`:
- Line 8: The DELETE statement targeting mysql_servers uses LIKE 'test server'
which is misleading and won't match variants; update the WHERE clause on the
comment column to either use equality (WHERE comment = 'test server') if you
only mean that exact value, or add the intended wildcard(s) (e.g. WHERE comment
LIKE 'test server%' or LIKE '%test server%') so variants like 'test
server-${INFRA}' are captured; modify the DELETE FROM mysql_servers ... WHERE
comment ... accordingly.
In `@test/infra/infra-dbdeployer-mysql56-single/docker/Dockerfile`:
- Around line 22-35: The Dockerfile hard-codes 5.6.49 in the downloaded filename
and ENV PATH which will break if MYSQL_TARBALL_URL points to a different
version; update the Dockerfile so the MySQL version is derived and used
consistently: introduce or use an ARG (e.g., MYSQL_VERSION) or parse the tarball
basename from MYSQL_TARBALL_URL before calling dbdeployer unpack (references:
the RUN line that invokes curl and dbdeployer unpack and the ENV
PATH="/root/opt/mysql/5.6.49/bin:${PATH}"), then replace the hard-coded 5.6.49
with the variable (e.g., /root/opt/mysql/${MYSQL_VERSION}/bin) so the created
installation path and PATH env match the downloaded tarball.
In `@test/infra/infra-dbdeployer-mysql56-single/docker/entrypoint.sh`:
- Around line 26-57: Detect if SANDBOX_DIR exists and, instead of re-running
dbdeployer deploy single, start the existing sandbox by invoking its generated
start script (use ${SANDBOX_DIR}/start) and skip deploy; then determine the
correct root password for already-initialized instances and set
DBDEPLOYER_ROOT_PASS accordingly (try connecting with the current
DBDEPLOYER_ROOT_PASS first, and if that fails, read the stored credential/config
created by dbdeployer inside ${SANDBOX_DIR}—e.g. sandbox credential or
my.sandbox.cnf/log files—and update MYSQL_CMD to use the discovered password);
finally, keep the readiness loop but use the adjusted MYSQL_CMD so restarts are
idempotent and work with existing sandboxes.
- Around line 16-24: The script currently only checks for /root/certs/ca.pem
before skipping certificate creation; change the existence check to require the
full SSL set (ca.pem, server-key.pem, server-cert.pem) so generation runs if any
of those files are missing. Update the conditional around the openssl generation
block in entrypoint.sh to test all three files (ca.pem, server-key.pem,
server-cert.pem) and only skip when all exist, and ensure the subsequent chmod
/root/certs/server-key.pem remains executed after generation.
In `@test/infra/infra-mysql56-single/bin/docker-proxy-post.bash`:
- Around line 14-18: The current docker exec invocation with here-doc will abort
the script under set -e before the subsequent conditional runs, so change the
logic to test the command directly: run the docker exec mysql -uadmin -padmin
-h127.0.0.1 -P6032 <<SQL ${SQL_CONTENT} SQL inside an if statement (e.g. if
docker exec ... <<SQL ...; then ...) so the success and failure branches (echo
"Cluster ${INFRA} registered in ProxySQL." and the ERROR/exit branch referencing
${INFRA}) are executed reliably; adjust to reference the existing variables
PROXY_CONTAINER, SQL_CONTENT and INFRA so the error message is reachable under
set -e.
In `@test/infra/infra-mysql56-single/docker-compose-destroy.bash`:
- Line 4: The pushd call uses an unquoted command substitution pushd $(dirname
$0) which can break on spaces or glob characters; update the invocation to quote
both the command substitution and the $0 argument so dirname receives a quoted
$0 and pushd receives a quoted result (i.e., use quoted forms around dirname and
the substitution) to prevent word-splitting and globbing when resolving the
script directory.
In `@test/infra/infra-mysql56-single/docker-compose.yml`:
- Around line 9-12: The docker-compose volumes mount both /var/lib/mysql and
/var/log/mysql to the same host path, mixing data and logs; update the volumes
under the volumes: block so /var/lib/mysql maps to a dedicated data directory
(e.g. ${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1/data) and /var/log/mysql maps
to a separate logs directory (e.g.
${INFRA_LOGS_PATH}/${COMPOSE_PROJECT}/mysql1/logs), leaving the my.cnf mount
unchanged; adjust references to the environment variables INFRA_LOGS_PATH and
COMPOSE_PROJECT in the docker-compose.yml volumes section for mysql1 so data and
logs are stored in distinct subdirectories.
In `@test/tap/tap/utils.h`:
- Around line 184-190: The fprintf in the MYSQL_QUERY_T macro uses the wrong
format for mysql_errno(): change the "%d" specifier to "%u" so the call in
MYSQL_QUERY_T (which logs mysql_errno(mysql) and mysql_error(mysql) after
mysql_query_t fails) prints the unsigned int correctly; locate the MYSQL_QUERY_T
macro and update the format string in the fprintf accordingly.
In `@test/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpp`:
- Around line 82-84: Null-check the result of mysql_init() before accessing
proxy->options.client_flag: after calling mysql_init(NULL) assign to proxy and
immediately test if (proxy == NULL) and handle the error (e.g., emit a TAP
diagnostic or gracefully abort the test) instead of dereferencing proxy; update
the code around the mysql_init call and the CLIENT_DEPRECATE_EOF line so the
option is only set when proxy is non-NULL to avoid the segfault in mysql_init.
- Around line 90-97: The test currently asserts an exact ProxySQL error code
9001 immediately after calling mysql_query(proxy, ...) which is fragile and
lacks a timeout; update the test to set a read timeout on the proxy MYSQL handle
(use MYSQL_OPT_READ_TIMEOUT on proxy) before calling mysql_query to avoid hangs,
and replace the hardcoded 9001 check with a robust assertion that either (a)
uses a named constant from a shared test helper (e.g.
PROXY_ERR_FAST_FORWARD_UNSUPPORTED) or (b) accepts any ProxySQL-side error in
the 9xxx range (error_code >= 9000 && error_code < 10000) while printing the
actual code in the diag message; reference mysql_query, mysql_errno,
mysql_error, proxy, and MYSQL_OPT_READ_TIMEOUT when making these changes.
- Around line 28-64: The test function test_ff_optional_mysql56 leaks runtime
state on early returns: if mysql_real_connect or test_session_variables_ff fails
the function returns before the cleanup that resets mysql_users.fast_forward and
mysql-session_track_variables; change control flow so both early-error paths
jump to a shared cleanup/exit label (or use a small RAII-style guard) that
always executes the same cleanup SQL (the UPDATE/LOAD and SET/LOAD sequence
currently in the end of test_ff_optional_mysql56) before closing proxy and
returning, apply the same fix to test_ff_enforced_mysql56 early returns, and
update main() to wrap the calls to test_ff_optional_mysql56 and
test_ff_enforced_mysql56 with ok(...) assertions so their return values produce
TAP failures instead of being ignored.
In `@test/tap/tests/mysql-session_track_variables_ff-t.cpp`:
- Around line 34-40: Ensure mysql_init(NULL) is null-checked before
dereferencing proxy->options.client_flag: after calling mysql_init() and
assigning to proxy, test if proxy == NULL and if so print an error (e.g., using
fprintf with mysql_error(NULL) or a descriptive message) and return
EXIT_FAILURE; only then modify proxy->options.client_flag to set
CLIENT_DEPRECATE_EOF so you avoid a potential segfault in the test when
mysql_init fails.
In `@test/tap/tests/session_track_variables.h`:
- Around line 139-153: The helper select_sess_var_value currently always returns
EXIT_SUCCESS even when mysql_store_result returns NULL or when mysql_fetch_row
yields no rows, leaving set_value at -1 and masking failures; update
select_sess_var_value to check the result of mysql_store_result and
mysql_fetch_row and return EXIT_FAILURE (or a non-zero error code) when result
is NULL or no row is fetched, only setting set_value from atoi(row[0]) and
returning EXIT_SUCCESS when a valid row was retrieved; reference
MYSQL_QUERY_T(proxy, ...), mysql_store_result(proxy), mysql_fetch_row(result),
and set_value in your changes.
- Around line 164-201: In extract_sess_var_proxy_internal replace the
inconsistent MYSQL_QUERY call with the standard MYSQL_QUERY_T macro used by
other helpers, and wrap the nlohmann::json::parse(row[0]) call in a
try/catch(nlohmann::json::exception&) block so that if parsing fails (or row[0]
is null/malformed) you catch the exception, mysql_free_result(result) if needed
and return EXIT_FAILURE instead of letting the exception abort the test process.
- Around line 111-130: The helper set_session_variable initializes set_value to
-1 but never writes the actual innodb_lock_wait_timeout, causing a
docstring/behaviour mismatch; fix by either (A) populating set_value after the
CALL by running a query (e.g., SELECT @@session.innodb_lock_wait_timeout or
invoke/select_sess_var_value) and parsing the result into set_value, or (B)
update the function's docstring/param comment to state that set_value is left as
-1 and the caller must call select_sess_var_value to obtain the real value;
reference set_session_variable and select_sess_var_value when applying the
change.
---
Duplicate comments:
In `@include/MySQL_HostGroups_Manager.h`:
- Around line 209-215: Change the plain field server_backoff_time to
std::atomic<unsigned long long> in the class declaration and update every access
site to use atomic load/store (e.g. in MyHGC::get_random_MySrvC, lib/MyHGC.cpp,
lib/MySQL_Thread.cpp and any session handling code that writes it) instead of
direct reads/writes; use
mysrvc->server_backoff_time.load(std::memory_order_relaxed) for readers and
mysrvc->server_backoff_time.store(backoff_until, std::memory_order_relaxed) for
writers (or stronger ordering where logically required) and ensure no remaining
unsynchronized accesses remain.
In `@include/MySQL_Thread.h`:
- Around line 380-382: Update the ENFORCED comment to reflect that unsupported
server session-tracking capabilities trigger a client-side backoff/retry rather
than an immediate hard failure: change wording around the ENFORCED marker in
include/MySQL_Thread.h to describe that when CLIENT_DEPRECATE_EOF or
CLIENT_SESSION_TRACKING are rejected the implementation uses server_backoff_time
and handle_session_track_capabilities to perform a temporary backoff and retry
connection attempts, not a fatal client-visible failure; reference those symbols
(handle_session_track_capabilities, server_backoff_time, ENFORCED) in the
docstring so the behavior matches the code.
In `@lib/MyHGC.cpp`:
- Around line 41-43: The backoff check reads mysrvc->server_backoff_time
concurrently without synchronization; make server_backoff_time a
std::atomic<int64_t> (or appropriate integer type) in the service struct/header
and include <atomic>, then change the read here to
mysrvc->server_backoff_time.load(std::memory_order_relaxed) (or alternatively
protect both reads/writes with the same pthread mutex if you prefer mutexing);
update any writes to use .store(...) or atomic ops so all accesses are
consistently synchronized.
In `@lib/MySQL_Session.cpp`:
- Around line 2006-2016: The flags session_track_variables_sent and
session_track_state_sent are currently set before the async SET completes;
change this so the flags are only set on successful completion of the
corresponding async SETs (i.e., move the assignments out of the current block
and into the success/commit path of the SETTING_SESSION_TRACK_VARIABLES and
SETTING_SESSION_TRACK_STATE handlers), and/or ensure any
retry/pop-to-previous_status paths (invocations of set_previous_status_mode3() /
NEXT_IMMEDIATE_NEW) clear the appropriate flag before returning so a
failed/soft-retry does not permanently skip the SET for that connection.
- Around line 1940-1942: The check is using the server's advertised
server_capabilities for CLIENT_DEPRECATE_EOF but must use the negotiated client
flags on the backend connection; change the second conditional to test
be_conn->options.client_flag & CLIENT_DEPRECATE_EOF (leave
CLIENT_SESSION_TRACKING check against mysql->server_capabilities) and ensure
mysql_thread___enable_server_deprecate_eof is still respected; update the
analogous checks at the other occurrences (around the blocks identified near
lines ~1998-2004 and ~9751-9757) so ENFORCED/GTID/session-variable tracking only
runs when CLIENT_DEPRECATE_EOF was actually negotiated on be_conn.
- Around line 2678-2706: The code currently only handles permitted tracking
errors (is_perm_track_err(var_name)) if the variable is found in
mysql_tracked_variables, causing an ERR to be sent when a permitted
session_track_* error refers to a variable that isn't tracked; fix by treating
permitted tracking errors as non-fatal even when idx is not within [0,
SQL_NAME_LAST_HIGH_WM): after computing idx, if is_perm_track_err(myerr,
var_name) and idx is out of bounds, do not send an error—perform the same
non-fatal flow (set myds->revents |= POLLOUT, set myds->DSS =
STATE_MARIADB_GENERIC, pop previous_status and call NEXT_IMMEDIATE_NEW) without
touching myconn->var_absent to avoid out-of-bounds writes; use
mysql_tracked_variables, is_perm_track_err, myconn->var_absent,
SQL_NAME_LAST_HIGH_WM, myds->revents, myds->DSS, previous_status, and
NEXT_IMMEDIATE_NEW to locate and implement the change.
In `@lib/MySQL_Thread.cpp`:
- Around line 6394-6397: The read of server_backoff_time (seen at
c->parent->server_backoff_time in MySQL_Thread::...) races with writers; make
server_backoff_time access consistent by choosing one strategy (preferred:
protect the field with a pthread mutex on the host/group object and add guarded
accessor methods, or convert the declaration to std::atomic<time_t> and update
all writes/reads to use .store/.load). Update the declaration in
MySQL_HostGroups_Manager (or the host struct/class that defines
server_backoff_time), change all write sites in MySQL_Session.cpp and MyHGC.cpp
to use the chosen synchronization (mutex lock + accessor or atomic store), and
change this read in MySQL_Thread.cpp to use the same accessor or atomic load;
then run the provided grep-based verification script to ensure no remaining
unsynchronized direct accesses.
In `@lib/MySrvC.cpp`:
- Line 34: The field server_backoff_time is accessed by session threads and the
server/connection selection path but is a plain unsigned long long; make it
thread-safe by changing the backing member to std::atomic<unsigned long long>
(or, if you prefer locking, ensure every read/write is protected by the existing
hostgroup mutex), then update all uses (writes in the session handling code and
reads in the server/connection selection code) to use atomic store/load
semantics or to acquire the hostgroup lock before accessing server_backoff_time;
search for the symbol server_backoff_time and the session/selection code paths
to update all accesses consistently.
In `@test/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bash`:
- Around line 4-8: The destroy script must mirror init's INFRA_ID handling: if a
caller provided INFRA_ID, preserve it across sourcing .env before deriving
COMPOSE_PROJECT. Before "set -a; . .env; set +a" save any existing INFRA_ID
(e.g., SAVED_INFRA_ID="$INFRA_ID"), then source .env, then if SAVED_INFRA_ID is
non-empty restore INFRA_ID from it, and only after that compute
COMPOSE_PROJECT="${INFRA}-${INFRA_ID}" so docker-compose down -v targets the
same project as init.
In `@test/tap/tests/mysql-session_track_variables_enforced-t.cpp`:
- Around line 16-20: The cleanup function currently unconditionally calls
mysql_close on both proxy and admin which can crash when proxy is NULL after a
failed reconnect; update the cleanup(MYSQL* admin, MYSQL* proxy) routine to
always execute the admin reset queries (MYSQL_QUERY_T(admin, "SET
mysql-session_track_variables=0"); and MYSQL_QUERY_T(admin, "LOAD MYSQL
VARIABLES TO RUNTIME");) but only call mysql_close(proxy) if proxy is non-NULL
and only call mysql_close(admin) if admin is non-NULL, guarding both closes with
null checks to avoid closing null pointers.
In `@test/tap/tests/reg_test_4264-commit_rollback-t.cpp`:
- Around line 1156-1160: The test currently disables session tracking by issuing
MYSQL_QUERY_T(admin, "SET mysql-session_track_variables=0") which masks the new
session-tracking traffic; remove or avoid that SET and instead collect the
baseline after performing connection-init/session-track setup (or explicitly
subtract/ignore the known internal setup queries like "LOAD MYSQL VARIABLES TO
RUNTIME" and any connection-init commands) so the test can validate transaction
routing with session tracking enabled; update the test around the MYSQL_QUERY_T
calls (e.g., the block containing MYSQL_QUERY_T(admin, "SET
mysql-auto_increment_delay_multiplex=0"), MYSQL_QUERY_T(admin, "SET
mysql-default_session_track_gtids='OFF'"), MYSQL_QUERY_T(admin, "SET
mysql-session_track_variables=0"), and MYSQL_QUERY_T(admin, "LOAD MYSQL
VARIABLES TO RUNTIME")) to either not set mysql-session_track_variables to 0 or
to take baselines after setup and explicitly ignore the setup queries in the
delta comparison.
In `@test/tap/tests/test_binlog_reader-t.cpp`:
- Around line 464-465: Before calling MYSQL_QUERY_T(proxysql_admin, "SET
mysql-session_track_variables=0"), query and store the current value (e.g., run
MYSQL_QUERY_T(proxysql_admin, "SELECT @@mysql-session_track_variables") and save
into a variable) and then after the test assertions in the cleanup: block
restore it by executing MYSQL_QUERY_T(proxysql_admin, "SET
mysql-session_track_variables=<saved_value>"); ensure the saved value is used
even if the test fails so cleanup: always resets the global setting.
---
Nitpick comments:
In `@include/mysql_connection.h`:
- Around line 269-298: Add an explicit `#include` <unordered_map> to the header so
the public API type used by get_variables(std::unordered_map<std::string,
std::string>&) is directly visible; modify include/mysql_connection.h to include
<unordered_map> near other standard headers so the declaration of bool
get_variables(...) is self-contained and does not rely on transitive includes
like cpp.h.
In `@include/MySQL_Session.h`:
- Around line 246-304: The header comments are too verbose and include
PR-specific workflow; shorten them to a concise API contract for the declared
functions (e.g., handler_again___verify_backend_session_track_variables,
handler_again___verify_backend_multi_statement,
handler_again___verify_backend_user_schema,
handler_again___verify_multiple_variables,
handler_again___status_SETTING_INIT_CONNECT,
handler_again___status_SETTING_LDAP_USER_VARIABLE,
handler_again___status_SETTING_SQL_MODE,
handler_again___status_SETTING_SESSION_TRACK_GTIDS,
handler_again___status_SETTING_SESSION_TRACK_VARIABLES,
handler_again___status_SETTING_SESSION_TRACK_STATE): remove PR labels,
step-by-step state machine narration and implementation details, leaving a one-
or two-sentence description of purpose, parameters, and return semantics; move
any detailed workflow or rationale into the .cpp implementation or design notes.
In `@test/infra/infra-mysql56-single/bin/docker-mysql-post.bash`:
- Around line 88-94: Replace the tar-stream-based CA extraction with a simpler
docker exec/cat pipeline used by the sibling script: instead of using docker cp
"${CONTAINER}:/var/lib/mysql/ca.pem" - | tar -Ox | sudo tee -a "${DB_BUNDLE}" |
sudo tee -a "${CA_BUNDLE}", run docker exec "${CONTAINER}" cat
/var/lib/mysql/ca.pem and pipe its output into sudo tee -a "${DB_BUNDLE}" and
sudo tee -a "${CA_BUNDLE}" (preserving the existing sudo chmod 666 on
"${DB_BUNDLE}" and "${CA_BUNDLE}" and the surrounding if test -f
/var/lib/mysql/ca.pem check).
In `@test/infra/infra-mysql56-single/bin/docker-proxy-post.bash`:
- Around line 11-12: The current code reads the SQL template relative to the CWD
and uses eval to expand it; change it to load the file using the resolved
SCRIPT_DIR (use the SCRIPT_DIR variable) to build an absolute path for
./conf/proxysql/infra-config.sql, and replace the use of eval for SQL_CONTENT
with a safe read/templating approach (e.g., read the file contents into
SQL_TEMPLATE and perform controlled variable substitution with a safe tool or
parameter expansion rather than eval) so SQL_TEMPLATE and SQL_CONTENT no longer
depend on the caller's working directory or execute arbitrary shell code.
In `@test/tap/groups/groups.json`:
- Around line 95-98: The test groups for session-tracking lack MySQL 8+
coverage; update the arrays for "mysql-session_track_variables_enforced-t",
"mysql-session_track_variables_optional-t", and
"mysql-session_track_variables_ff-t" to also include "mysql84-g4", "mysql90-g4",
and "mysql95-g4" (keep "mysql-session_track_variables_ff_mysql56-t" as-is for
mysql56-specific runs). This ensures ENFORCED/OPTIONAL/FF variants run against
MySQL 8.x/9.x groups to catch protocol-level regressions.
In `@test/tap/groups/mysql56-single/env.sh`:
- Around line 5-8: Replace the POSIX test brackets and silence ShellCheck for
the dynamic env source: change the conditionals using [ -n "${WORKSPACE}" ] and
[ -f "${_INFRA_ENV}" ] to bash conditional syntax [[ -n "${WORKSPACE}" ]] and [[
-f "${_INFRA_ENV}" ]], and add a directive comment "# shellcheck
source=/dev/null" immediately before the dynamic source line that evaluates .
"${_INFRA_ENV}" (referencing variables WORKSPACE, _INFRA_ENV,
DEFAULT_MYSQL_INFRA) so SC1090 is suppressed.
In `@test/tap/tests/session_track_variables.h`:
- Around line 111-228: The helpers are hardcoded to "innodb_lock_wait_timeout",
breaking portability; update set_session_variable, select_sess_var_value,
extract_sess_var_proxy_internal, and test_session_variables_ff to accept a
variable-name parameter (e.g., const std::string& var_name or const char*
var_name) and use that name when building the CREATE PROCEDURE, CALL, SELECT
@@..., and when extracting JSON keys from PROXYSQL INTERNAL SESSION and mysql
packet tracking; keep current behavior by allowing a default value (e.g.,
"wait_timeout") when none is provided so tests can run against non-InnoDB
backends without duplicating logic.
🪄 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: a55a68a1-9cf9-4e36-a2e3-176cc3787514
📒 Files selected for processing (45)
include/MySQL_HostGroups_Manager.hinclude/MySQL_Session.hinclude/MySQL_Thread.hinclude/mysql_connection.hinclude/proxysql_structs.hlib/MyHGC.cpplib/MySQL_Protocol.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/MySrvC.cpplib/mysql_connection.cpptest/infra/control/run-tests-isolated.bashtest/infra/infra-dbdeployer-mysql56-single/.envtest/infra/infra-dbdeployer-mysql56-single/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql56-single/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql56-single/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql56-single/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql56-single/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql56-single/docker-compose.ymltest/infra/infra-dbdeployer-mysql56-single/docker/Dockerfiletest/infra/infra-dbdeployer-mysql56-single/docker/build.shtest/infra/infra-dbdeployer-mysql56-single/docker/entrypoint.shtest/infra/infra-mysql56-single/.envtest/infra/infra-mysql56-single/bin/docker-mysql-post.bashtest/infra/infra-mysql56-single/bin/docker-proxy-post.bashtest/infra/infra-mysql56-single/conf/mysql/Dockerfiletest/infra/infra-mysql56-single/conf/mysql/docker-entrypoint.shtest/infra/infra-mysql56-single/conf/mysql/mysql1/my.cnftest/infra/infra-mysql56-single/conf/mysql/ssl/configure_ssl.shtest/infra/infra-mysql56-single/conf/proxysql/infra-config.sqltest/infra/infra-mysql56-single/docker-compose-destroy.bashtest/infra/infra-mysql56-single/docker-compose-init.bashtest/infra/infra-mysql56-single/docker-compose.ymltest/tap/groups/groups.jsontest/tap/groups/mysql56-single/env.shtest/tap/groups/mysql56-single/infras.lsttest/tap/groups/mysql56-single/pre-proxysql.bashtest/tap/tap/utils.htest/tap/tests/mysql-session_track_variables_enforced-t.cpptest/tap/tests/mysql-session_track_variables_ff-t.cpptest/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpptest/tap/tests/mysql-session_track_variables_optional-t.cpptest/tap/tests/reg_test_4264-commit_rollback-t.cpptest/tap/tests/session_track_variables.htest/tap/tests/test_binlog_reader-t.cpp
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
…te hot-path read Context ------- PR #5166 introduced a per-'MySrvC' atomic timestamp that prevents the server from being chosen for new connections for a 30-second window. The timestamp is written in exactly one place, 'MySQL_Session::handle_session_track_capabilities()', and only when 'mysql-session_track_variables' is in ENFORCED mode and the backend is missing 'CLIENT_SESSION_TRACKING' or 'CLIENT_DEPRECATE_EOF'. The field was originally named 'server_backoff_time' and its only semantics were that ENFORCED-mode capability mismatches temporarily exclude the server from selection. Two issues with the original shape: 1. The name does not hint that the scope is session-tracking capability enforcement. A reader of 'MyHGC::get_random_MySrvC' or 'MySQL_Thread::get_MyConn_local' had no way to tell that this field is tied to one specific feature and not, say, a generic health backoff. 2. The atomic load was unconditional in both hot selection loops. Even though the atomic is relaxed (effectively a 'mov' on x86), per-iteration atomic reads inhibit hoisting and CSE that the compiler can otherwise apply to thread-local integer reads. 'get_random_MySrvC' runs the loop over every server in the hostgroup on every backend-connection acquisition, so this is the sort of place where small per-iteration costs accumulate. Changes ------- * Rename the field: MySrvC::server_backoff_time -> MySrvC::session_track_backoff_until Naming it after the feature makes it obvious that the field is specific to session-tracking enforcement and should not be overloaded for unrelated purposes (e.g. connect errors, monitor failures). If a future change needs a generic backoff mechanism, a new field -- or a rename at that point -- keeps intent clear rather than piggy-backing on this one silently. * Gate the atomic load on mode: In both 'MyHGC::get_random_MySrvC()' (lib/MyHGC.cpp) and 'MySQL_Thread::get_MyConn_local()' (lib/MySQL_Thread.cpp), the relaxed load is skipped entirely when 'mysql_thread___session_track_variables' is not ENFORCED. In 'get_MyConn_local' the check is computed once before the loop; in 'get_random_MySrvC' the check is inside the loop but is a thread-local int read, which is free. Correctness note: switching out of ENFORCED at runtime makes any stale 'session_track_backoff_until' values in the pool immediately ignored, which is the desired behavior -- servers rejected while ENFORCED was active become eligible again the moment the operator relaxes the mode, matching how ProxySQL handles other runtime-variable-dependent state. No explicit reset is required. * Updated the field documentation in include/MySQL_HostGroups_Manager.h with the monotonic-timestamp unit, the list of sites that read/write it, the explicit scope (ENFORCED mode only), and the reset policy (no explicit reset; deadline ages out by comparison against curtime). No functional change for DISABLED / OPTIONAL configurations (the default for all installations). ENFORCED mode behaviour is unchanged.
…untime backoff Context ------- PR #5166 introduced a three-mode 'mysql-session_track_variables' variable (DISABLED / OPTIONAL / ENFORCED) whose behavior silently depends on two other MySQL capability toggles and on the capabilities of each backend: * In OPTIONAL mode the feature is a no-op when 'mysql-enable_server_deprecate_eof=false' or when the backend does not advertise CLIENT_DEPRECATE_EOF / CLIENT_SESSION_TRACKING. The operator sees the variable set to 1, but 'PROXYSQL INTERNAL SESSION' shows no tracked variables and there is no indication why. * In ENFORCED mode ProxySQL forces CLIENT_DEPRECATE_EOF on both the client and server sides of the protocol even if the operator explicitly set 'mysql-enable_client_deprecate_eof=false' / 'mysql-enable_server_deprecate_eof=false'. This is a protocol-visible behavior change for clients that did not negotiate CLIENT_DEPRECATE_EOF, and it was previously silent. * In ENFORCED mode backends that cannot honor tracking (missing CLIENT_SESSION_TRACKING or CLIENT_DEPRECATE_EOF) are placed in a 30-second selection backoff via 'MySrvC::session_track_backoff_until'. The server is NOT shunned and no connect-error counter is incremented, so in a mixed-version hostgroup an incompatible backend sits idle indefinitely with no log trail. Changes ------- * lib/Admin_FlushVariables.cpp: add a cross-variable validator at the tail of 'ProxySQL_Admin::flush_mysql_variables___database_to_runtime()', right next to the existing 'mysql-use_tcp_keepalive' warning. It runs once per 'LOAD MYSQL VARIABLES TO RUNTIME' and only when the feature is actually requested (mode != DISABLED). Emitted warnings: - ENFORCED + !enable_client_deprecate_eof -> "will force CLIENT_DEPRECATE_EOF on frontend connections; older clients may be affected" - ENFORCED + !enable_server_deprecate_eof -> equivalent for backends - OPTIONAL + !enable_server_deprecate_eof -> "has no effect while mysql-enable_server_deprecate_eof=false; backend session tracking will be skipped" The checks use the global 'GloMTH->get_variable_int()' view (same pattern as the TCP keepalive warning above it), and deliberately do not name any backend -- this is a global-configuration warning; per-backend capability mismatches are handled separately below. * lib/MySQL_Session.cpp: in 'handle_session_track_capabilities()', at the point where the code stores a new 'session_track_backoff_until' deadline for an incompatible backend, add a 'proxy_warning' naming the backend's address:port and the reason. Because this path only runs on new connection establishment -- and is itself gated by the 30-second backoff on the selection path -- the warning is naturally rate-limited to at most once per 30 seconds per backend. In a mixed fleet that rate is just enough for an operator to notice the incompatible server without drowning the log. Behaviour --------- * DISABLED mode (the default): no new log output; validator is skipped entirely. The hot selection path was already skipping the atomic read in the previous commit, so DISABLED remains free of new work. * OPTIONAL / ENFORCED: at most three warnings at 'LOAD MYSQL VARIABLES TO RUNTIME' time (cross-variable validator) plus one periodic warning per incompatible backend (ENFORCED path only). No per-query log output.
…sql_mode coverage
Context
-------
The four session_track_variables TAP tests previously hard-coded a single
variable, 'innodb_lock_wait_timeout', for their round-trip assertion. That
only exercised the integer path in
'MySQL_Session::handler_rc0_Process_Variables', leaving the string-value
path (and the character-set special case elsewhere in the same function)
untested. The stored-procedure helper was also named generically
('set_session_variable') while its body was pinned to InnoDB's lock wait
timeout -- confusing for readers.
Changes
-------
* test/tap/tests/session_track_variables.h:
- Introduce 'struct tracked_var_spec { name, set_expr; }' so the helpers
can exercise arbitrary session variables from a single
spec-driven code path.
- 'set_session_variable()' now takes a spec, builds a per-variable
stored procedure name ('test.set_session_track_<name>') so multiple
specs can coexist without stepping on each other, and guarantees
that the CALL is the last statement on the connection before the
tracker is read (any query in between would reset the session
tracker state).
- Add 'drop_session_variable_proc()' for optional teardown. Kept
separate from the set path so tests that need to preserve the CALL
as the last statement can skip cleanup.
- Switch the whole value surface from 'int' to 'std::string':
'extract_sess_var_mysql_pkt', 'select_sess_var_value', and
'extract_sess_var_proxy_internal' all return strings, which is how
session tracking delivers values on the wire and matches both
integer and string variables without encoding assumptions.
- Provide a default set, 'session_track_default_vars', with three
representative cases:
* 'innodb_lock_wait_timeout' (integer; original coverage)
* 'time_zone' (string, portable small value-space)
* 'sql_mode' (string, representative of production usage)
All three 'set_expr' values are pure server-side SQL (no client-side
randomization), so a CALL alone produces a fresh value per run.
* test/tap/tests/mysql-session_track_variables_optional-t.cpp,
test/tap/tests/mysql-session_track_variables_enforced-t.cpp,
test/tap/tests/mysql-session_track_variables_ff-t.cpp,
test/tap/tests/mysql-session_track_variables_ff_mysql56-t.cpp:
- Iterate over 'session_track_default_vars' and emit one TAP 'ok' per
(spec[, mode]) combination.
- Adjust plans accordingly. The ENFORCED-on-5.6 case still emits a
single 'ok' because the backend is rejected before any per-variable
tracking is attempted, so multiplying by the spec count there would
be misleading.
- Compare values as strings using the new string-returning helpers.
The broadened coverage does not change what 'handler_rc0_Process_Variables'
or 'handle_session_track_capabilities' do; it only exercises more of their
existing behavior, notably the non-integer value path and the
'proxysql_find_charset_*' branches that activate for character-set
variables (not exercised by these three specs but now reachable through
the same harness for follow-up tests).
Context ------- PR #5166 introduced a new TAP group 'mysql56-single-g1' backed by 'test/infra/infra-dbdeployer-mysql56-single/' (a dbdeployer-based MySQL 5.6 fixture). Four new tests register against this group in 'test/tap/groups/groups.json': - mysql-session_track_variables_enforced-t - mysql-session_track_variables_ff_mysql56-t - mysql-session_track_variables_optional-t Without a dedicated CI workflow these tests are never exercised automatically: the generic 'CI-taptests-groups' plumbing hardcodes 'infradb: [ mysql57 ]' in its matrix, and no existing 'CI-mysql*' caller routes to the 'mysql56-single-g1' group. Changes ------- This commit adds the v3.0-branch half of the standard caller/reusable pair, following the architecture documented in 'doc/GH-Actions/README.md' (section "The dedicated-reusable pattern"). The file is modelled on 'CI-mysql84-g1.yml' verbatim -- only the workflow name and the 'uses:' target change. Like every other 'CI-*.yml' caller on v3.0: - Triggers on 'workflow_run' after 'CI-trigger' completes successfully (so the build/test caches are guaranteed populated) plus manual 'workflow_dispatch'. - Uses 'concurrency.cancel-in-progress: true' to cancel any in-flight run when a new push lands on the same branch. - Delegates all real work to 'ci-mysql56-single-g1.yml' on the 'GH-Actions' branch. The reusable half of the pair -- 'ci-mysql56-single-g1.yml' on the 'GH-Actions' branch -- is added in a separate commit on that branch. Until both halves are present this caller will fail with "workflow not found". That is the expected landing order per the project's CI workflow: the reusable ships first on 'GH-Actions', then the caller ships on v3.0, matching how 'CI-mysql84-g1.yml' was introduced (see 'doc/GH-Actions/README.md' section "Adding a new test group end-to-end").
|
@wazir-ahmed quick question on the test infra layout. This PR ships two parallel MySQL 5.6 fixtures:
We're moving away from Docker-Compose for single-instance MySQL fixtures in favor of dbdeployer. If the Docker variant is not serving a purpose I'm missing, it looks like a candidate for removal in this PR (or a follow-up) — can you confirm whether it's safe to drop? If it's kept as an intentional alternative, a note in |
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
@renecannao I have removed the dockerized infra. We now have only one |
|
4 participants
session_track_system_variablesandsession_track_state_changein all backend connections.MySQL_Set_Stmt_ParserSummary by CodeRabbit
New Features
Tests