MySQL: Track session-specific system variables changes#5166
MySQL: Track session-specific system variables changes#5166wazir-ahmed wants to merge 12 commits intov3.0from
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.
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
from
d817bda to
346030e
Compare
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
2 times, most recently
from
a22aa90 to
e059a67
Compare
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
4 times, most recently
from
f1f57ac to
7709911
Compare
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
3 times, most recently
from
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. |
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
from
eb5a10c to
3f1c89f
Compare
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
from
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>
wazir-ahmed
force-pushed
the
session-track-system-variable
branch
from
415dc07 to
6e6a9bb
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds backend MySQL session-variable tracking (DISABLED/OPTIONAL/ENFORCED), session-tracking state handlers and processing, new connection capability flags and get_variables(), server backoff logic to skip incompatible backends, CLIENT_DEPRECATE_EOF gating adjustments, runtime plumbing, and tests for multiple MySQL versions and fast_forward paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Proxy as ProxySQL
participant Backend as MySQL Backend
Client->>Proxy: Connect + Query
activate Proxy
Proxy->>Proxy: Check session_track_variables mode (thread var)
alt ENFORCED
Proxy->>Backend: Handshake (advertise CLIENT_DEPRECATE_EOF + session-track)
activate Backend
Backend-->>Proxy: Handshake + capabilities
deactivate Backend
Proxy->>Proxy: handle_session_track_capabilities()
alt Backend supports required features
Proxy->>Backend: SET SESSION session_track_system_variables=...
activate Backend
Backend-->>Proxy: OK (+ SERVER_SESSION_STATE_CHANGED packets)
deactivate Backend
Proxy->>Backend: Execute query
activate Backend
Backend-->>Proxy: Result + session-state packets
deactivate Backend
Proxy->>Proxy: handler_rc0_Process_Variables() — extract & propagate
Proxy-->>Client: Result (synchronized variables)
else Backend lacks features
Proxy->>Proxy: set server_backoff_time (exclude server)
Proxy-->>Client: Error / backoff response (e.g., 9001)
end
else OPTIONAL or DISABLED
Proxy->>Backend: Normal connect/execute
Backend-->>Proxy: Result
Proxy-->>Client: Result
end
deactivate Proxy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: René Cannaò <rene@proxysql.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @include/MySQL_Thread.h:
- Around line 637-647: The comment for variables.session_track_variables
incorrectly references session_track_variables::ENABLED; update the docstring
for the int session_track_variables field to list the actual enum variants
(session_track_variables::DISABLED, session_track_variables::OPTIONAL,
session_track_variables::ENFORCED), explain what ENFORCED/OPTIONAL mean wrt
backend configuration tracking, and keep the default noted as
session_track_variables::DISABLED; locate the comment above the
variables.session_track_variables declaration and replace the incorrect enum
name with the correct three values and brief descriptions.
In @lib/MyHGC.cpp:
- Around line 40-44: Replace the plain unsigned long long backoff field with an
atomic and use atomic loads/stores to avoid data races: change the declaration
of server_backoff_time in MySQL_HostGroups_Manager.h to std::atomic<unsigned
long long>; in MyHGC.cpp replace direct reads of mysrvc->server_backoff_time
with mysrvc->server_backoff_time.load() for the backoff comparison (same spot as
the check around get_status() and the existing num_online_servers.load() usage);
and in MySQL_Session.cpp update assignments to use
server_backoff_time.store(new_value) (or assignment which is supported) so
writes are atomic.
In @lib/MySQL_Protocol.cpp:
- Around line 1114-1120: generate_pkt_initial_handshake advertises
CLIENT_DEPRECATE_EOF when mysql_thread___enable_client_deprecate_eof is true OR
mysql_thread___session_track_variables == session_track_variables::ENFORCED, but
PPHR_2 currently unconditionally strips CLIENT_DEPRECATE_EOF when
!mysql_thread___enable_client_deprecate_eof; update PPHR_2 so the removal of
CLIENT_DEPRECATE_EOF mirrors the advertise logic—only clear the flag if neither
mysql_thread___enable_client_deprecate_eof is true nor
mysql_thread___session_track_variables == session_track_variables::ENFORCED
(i.e., strip CLIENT_DEPRECATE_EOF only when both conditions are false) so
capability negotiation remains consistent between generate_pkt_initial_handshake
and PPHR_2.
In @lib/MySQL_Session.cpp:
- Around line 1969-1997: The flags session_track_variables_sent and
session_track_state_sent set in
handler_again___verify_backend_session_track_variables() must be reset when a
soft SQL error triggers a retry (or only set after the SET succeeds); update the
error path in handler_again___status_SETTING_GENERIC_VARIABLE to clear
mybe->server_myds->myconn->options.session_track_variables_sent and
session_track_state_sent when the handler pops back to the previous status (use
the ldap_user_variable_sent reset logic as a guide), or alternatively move
flag-setting to after the SET handlers return success so retries will re-send
the SETs.
- Around line 2798-2810: The handlers can trigger an out-of-bounds write when
handler_again___status_SETTING_GENERIC_VARIABLE() returns error 1193 because the
searched variable name (e.g., "session_track_system_variables" or
"session_track_state_change") is not in mysql_tracked_variables and idx ends up
as SQL_NAME_LAST_HIGH_WM; add a bounds check before writing to var_absent[idx]
(or adjust the search result) so you only write when idx is within [0,
SQL_NAME_LAST_HIGH_WM-1]; specifically update
handler_again___status_SETTING_GENERIC_VARIABLE() (or the two wrappers
handler_again___status_SETTING_SESSION_TRACK_VARIABLES and
handler_again___status_SETTING_SESSION_TRACK_STATE) to verify idx <
SQL_NAME_LAST_HIGH_WM (and != SQL_NAME_LAST_LOW_WM if used as sentinel) before
assigning var_absent[idx], and treat unknown-variable (1193) as a
non-fatal/no-op without touching var_absent.
In @test/tap/groups/groups.json:
- Around line 249-250: The JSON has a missing comma between two object entries
("mysql-session_track_variables_ff_enforced-t" and
"unit-strip_schema_from_query-t"), causing a parse error; add a trailing comma
after the first entry's closing bracket so the object entries are properly
separated (i.e., put a comma after the array for
"mysql-session_track_variables_ff_enforced-t").
In @test/tap/groups/mysql56-single/pre-proxysql.bash:
- Around line 46-52: The INSERT uses an undefined variable `${WHG}` inside the
for MYUSER loop which can produce an empty default_hostgroup; either define WHG
before the loop (e.g., WHG=0 or desired hostgroup) or replace `${WHG}` with the
literal value (e.g., 0) in the mysql -e INSERT statement so default_hostgroup is
explicit; update the for MYUSER ... mysql ${SSLOPT} -h127.0.0.1 -P6032 -uadmin
-padmin -e "INSERT OR IGNORE INTO mysql_users ... default_hostgroup,..."
accordingly.
In @test/tap/tests/mysql-session_track_variables_enforced-t.cpp:
- Around line 116-119: The code calls mysql_error(admin) (and likewise for
proxy) even when init_mysql_conn() returned NULL, causing undefined behavior;
update the admin and proxy error branches so you only call mysql_error(...) when
the MYSQL* is non-NULL—otherwise print a safe fallback (e.g., strerror(errno) or
a generic "failed to init connection" message) and then return exit_status();
ensure this change is applied to both the admin check and the proxy check
referencing init_mysql_conn, admin, proxy, mysql_error, and exit_status.
In @test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp:
- Around line 121-124: The check for a failed connection uses mysql_error(admin)
even when admin is NULL; avoid calling mysql_error with a NULL pointer. Update
the error branch around the admin variable so you only call mysql_error(admin)
when admin is non-NULL; if admin is NULL, print a safe fallback (e.g., a generic
"failed to initialize connection" message or errno/strerror output) instead of
passing NULL to mysql_error.
In @test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp:
- Around line 121-124: The code calls mysql_error(admin) even when admin is
NULL; change it to avoid passing a NULL pointer to mysql_error by checking admin
first and using the original MYSQL handle used to initiate the connection (e.g.,
call mysql_error(&mysql) or mysql_error(mysql_init_handle) when admin is NULL),
or otherwise only call mysql_error(admin) when admin is non-NULL; update the
fprintf accordingly and keep the return exit_status() behavior.
In @test/tap/tests/mysql-session_track_variables_optional-t.cpp:
- Around line 116-119: Avoid calling mysql_error() with a NULL pointer: update
the error-handling for the `admin` (and similarly `proxy`) initialization so you
only call mysql_error(admin) when `admin` is non-NULL; if `admin` is NULL log a
clear fallback message like "mysql_init() returned NULL" (or include
strerror(errno)) instead. Locate the checks around the `admin` and `proxy`
variables in this test and replace the unconditional mysql_error(...) call with
a conditional that prints mysql_error(admin) when admin != NULL, otherwise
prints the fallback message; do the same pattern for `proxy`.
- Around line 1-6: The file header @file tag is incorrect: it says
"mysql-track_system_variables_optional-t.cpp" but the actual file is
"mysql-session_track_variables_optional-t.cpp"; update the @file comment in the
file header (the line starting with "@file") to exactly
"mysql-session_track_variables_optional-t.cpp" so the documentation and filename
match.
In @test/tap/tests/reg_test_4264-commit_rollback-t.cpp:
- Around line 1131-1136: The test disables session tracking (the MYSQL_QUERY_T
setting mysql-session_track_variables=0) which masks new connection-init SET/OK
traffic; instead re-enable session tracking and adjust reg_test_4264 to measure
or ignore init traffic by establishing the baseline counters after connection
initialization (e.g., perform the connection(s) and session-track setup, then
call a helper that reads current query/connection counters and uses those as the
starting point) or explicitly subtract the known internal setup queries from
assertions; update the places using MYSQL_QUERY_T and the LOAD MYSQL VARIABLES
TO RUNTIME step so the test asserts delta values relative to the post-init
baseline rather than absolute counts.
🧹 Nitpick comments (7)
include/MySQL_Thread.h (1)
339-381: Consider moving the long architecture block out of the header (keep the enum here, put narrative docs elsewhere).The mode enum itself is useful, but the multi-paragraph “PR 5166 architecture” comment in
include/MySQL_Thread.his likely to drift over time and adds noise to a widely-included header. A Doxygen page ordoc/markdown would age better, with a short pointer comment here.lib/mysql_connection.cpp (1)
3149-3187: Consider adding exception handling for JSON-like edge cases in session tracking data.The implementation correctly follows the MySQL session tracking API pattern where
get_first()returns a variable name andget_next()alternates between values and names. However, if the backend returns malformed tracking data (odd number of elements), the last variable name would be stored without its value.While this is unlikely given MySQL's protocol guarantees, you may want to verify this is acceptable behavior or add a check.
Also, consider using
std::stoiinstead of relying onstd::stringconstructor behavior for consistency with the rest of the codebase.test/tap/tests/mysql-session_track_variables_enforced-t.cpp (2)
83-84: Consider adding exception handling for JSON parsing.
nlohmann::json::parse()can throwjson::parse_erroron malformed input. WhilePROXYSQL INTERNAL SESSIONshould always return valid JSON, defensive coding would catch parsing errors to provide better diagnostics.💡 Suggested improvement
+ nlohmann::json j_session; + try { + j_session = nlohmann::json::parse(row[0]); + } catch (const nlohmann::json::parse_error& e) { + mysql_free_result(result); + diag("Failed to parse session JSON: %s", e.what()); + return false; + } - auto j_session = nlohmann::json::parse(row[0]); mysql_free_result(result);
18-39: Significant code duplication across test files.The
get_server_version()andtest_session_variables()functions are duplicated across multiple test files (mysql-session_track_variables_enforced-t.cpp,mysql-session_track_variables_optional-t.cpp,mysql-session_track_variables_ff_enforced-t.cpp,mysql-session_track_variables_ff_optional-t.cpp).Consider extracting these into a shared test utility header to reduce maintenance burden and ensure consistent behavior across tests.
Also applies to: 41-104
test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp (1)
77-109: Same issue:test_session_variables()always returnstrue.As noted in the
ff_enforcedtest, this function unconditionally returnstrue. While the OPTIONAL mode test doesn't rely on the return value as heavily (it checkstracked_value == -1directly), the inconsistent return value could cause confusion when the code is maintained.For consistency across test files, consider returning whether tracking extraction succeeded.
lib/MySQL_Session.cpp (2)
2959-2962: Connect-flow integration is sensible, but avoid tight retry loops whenmysql_thread___connect_retries_delay == 0.
On capability failure you setpause_until = curtime + delay*1000and return; if delay is 0 and no servers qualify (e.g., all are backoff’d), this can become a hot loop.
- Consider enforcing a small minimum sleep/yield when capability checks fail (even 1–10ms) or treating this like other “no conn” cases that rely on poll timing.
Also applies to: 2993-2998
8553-8605: Add logging when backing off a server due to missing session tracking capabilities.The 30-second backoff silently excludes servers during host selection with no log output. While the time units are correct (30 * 1000000 microseconds), adding a one-time warning or info-level log when
server_backoff_timeis set would improve observability when a host group suddenly reports "no usable servers."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
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/tap/groups/groups.jsontest/tap/groups/mysql56-single/pre-proxysql.bashtest/tap/tests/mysql-session_track_variables_enforced-t.cpptest/tap/tests/mysql-session_track_variables_ff_enforced-t.cpptest/tap/tests/mysql-session_track_variables_ff_optional-t.cpptest/tap/tests/mysql-session_track_variables_optional-t.cpptest/tap/tests/reg_test_4264-commit_rollback-t.cpptest/tap/tests/test_binlog_reader-t.cpp
🧰 Additional context used
🧬 Code graph analysis (6)
test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp (3)
lib/mysql_connection.cpp (1)
var_name(3160-3160)test/tap/tests/mysql-session_track_variables_enforced-t.cpp (4)
get_server_version(18-39)get_server_version(18-18)test_session_variables(41-104)test_session_variables(41-41)test/tap/tests/mysql-session_track_variables_optional-t.cpp (4)
get_server_version(18-39)get_server_version(18-18)test_session_variables(41-104)test_session_variables(41-41)
test/tap/tests/mysql-session_track_variables_optional-t.cpp (2)
test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp (4)
get_server_version(15-36)get_server_version(15-15)test_session_variables(77-109)test_session_variables(77-77)test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp (4)
get_server_version(15-36)get_server_version(15-15)test_session_variables(77-109)test_session_variables(77-77)
test/tap/tests/mysql-session_track_variables_enforced-t.cpp (1)
test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp (4)
get_server_version(15-36)get_server_version(15-15)test_session_variables(77-109)test_session_variables(77-77)
lib/mysql_connection.cpp (4)
test/tap/tests/set_testing-multi-t.cpp (1)
options(242-242)include/MySQL_Monitor.hpp (1)
mysql(459-459)test/tap/tests/test_session_status_flags-t.cpp (1)
length(253-253)include/PgSQL_Connection.h (1)
string(425-433)
test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp (3)
lib/mysql_connection.cpp (1)
var_name(3160-3160)test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp (8)
current_var_name(48-48)value_str(57-57)extract_session_variable(38-75)extract_session_variable(38-38)get_server_version(15-36)get_server_version(15-15)test_session_variables(77-109)test_session_variables(77-77)test/tap/tests/mysql-session_track_variables_enforced-t.cpp (4)
get_server_version(18-39)get_server_version(18-18)test_session_variables(41-104)test_session_variables(41-41)
include/mysql_connection.h (1)
lib/mysql_connection.cpp (2)
get_variables(3149-3187)get_variables(3149-3149)
🪛 Clang (14.0.6)
test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp
[error] 8-8: 'stdio.h' file not found
(clang-diagnostic-error)
test/tap/tests/mysql-session_track_variables_optional-t.cpp
[error] 8-8: 'stdio.h' file not found
(clang-diagnostic-error)
test/tap/tests/mysql-session_track_variables_enforced-t.cpp
[error] 8-8: 'stdio.h' file not found
(clang-diagnostic-error)
test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp
[error] 8-8: 'stdio.h' file not found
(clang-diagnostic-error)
🪛 Shellcheck (0.11.0)
test/tap/groups/mysql56-single/pre-proxysql.bash
[warning] 39-39: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (100)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
- GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.1,mariadb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb10.6,mariadb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,asyncmy)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.5,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.6,mariadbconnector)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.4,mysqldb)
- GitHub Check: CI-3p-php-pdo-mysql / test (mariadb11.5,mysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.11,mysqldb)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb10.5,pymysql)
- GitHub Check: CI-3p-sqlalchemy / test (mariadb11.2,asyncmy)
🔇 Additional comments (23)
test/tap/tests/test_binlog_reader-t.cpp (1)
262-264: Investigate root cause instead of disabling session tracking.Disabling
session_track_variablesin this test masks potential compatibility issues rather than addressing them. Per the PR reviewer's feedback, the root cause of why binlog reader tests fail with session tracking enabled should be investigated.If session tracking genuinely conflicts with binlog reader functionality, document this as an expected limitation. Otherwise, fix the underlying issue to ensure the test validates the feature properly.
include/mysql_connection.h (2)
91-93: Well-structured tracking flags for session configuration state.The new
session_track_variables_sentandsession_track_state_sentflags provide clear tracking of whether session tracking configuration has been sent to backend connections, following the same pattern as the existingsession_track_gtids_sentflag.
267-296: Comprehensive documentation for the session variable extraction interface.The Doxygen documentation clearly explains the protocol flow, technical details, and integration points for the
get_variablesmethod. The implementation (per the relevant code snippet) correctly handles the variable/value pairing protocol from MySQL's session tracking system.include/proxysql_structs.h (2)
314-316: New session states properly extend the state machine.The
SETTING_SESSION_TRACK_VARIABLESandSETTING_SESSION_TRACK_STATEstates follow the existing pattern for session configuration states (similar toSETTING_SESSION_TRACK_GTIDS) and are correctly positioned before thesession_status___NONEmarker.
1310-1310: Thread-local session tracking variable properly defined with existing enum-based documentation.The
mysql_thread___session_track_variablesthread-local variable is correctly defined alongside other MySQL thread variables and follows established conventions. The valid values are already documented via thesession_track_variablesenum (DISABLED=0, OPTIONAL=1, ENFORCED=2), which is actively used throughout the codebase to avoid magic numbers, including in MySQL_Session.cpp, MySQL_Protocol.cpp, and mysql_connection.cpp.lib/MySrvC.cpp (1)
34-34: Proper initialization of backoff timestamp.The
server_backoff_timemember is correctly initialized to zero, indicating no active backoff. This follows the same initialization pattern as other time-related members in the constructor.lib/mysql_connection.cpp (3)
455-456: LGTM!The new session tracking flags are properly initialized to
falsein the constructor, consistent with the existingsession_track_gtids_sentpattern at line 454.
892-902: LGTM! Good defensive override for ENFORCED mode.When
session_track_variablesis set toENFORCED, forcingCLIENT_DEPRECATE_EOFis correct since session tracking data is transmitted in OK packets (which replace EOF packets when this capability is enabled). This ensures the feature works regardless ofmysql-enable_server_deprecate_eofsetting.
3113-3114: LGTM!The reset of tracking flags in
reset()is consistent with the pattern used forsession_track_gtids_sentat line 3112 and ensures clean state for connection reuse.test/tap/groups/mysql56-single/pre-proxysql.bash (1)
38-39: Non-constant source path - expected for dynamic environment loading.The shellcheck warning (SC1090) about non-constant source is a known limitation when sourcing environment-specific files. This pattern is common in test infrastructure scripts and is acceptable here since
JENKINS_SCRIPTS_PATHis expected to be set by the CI environment.include/MySQL_Session.h (3)
244-273: Excellent documentation for the session tracking orchestration.The doxygen block clearly explains the purpose, configuration logic, state machine integration, and design rationale. This level of documentation will be valuable for future maintainers understanding the session tracking flow.
351-379: Documentation note: Return type inconsistency.The doxygen documentation describes the function but doesn't specify a return value, yet line 379 declares
void handler_rc0_Process_Variables(MySQL_Connection *myconn)which is consistent with a procedure that updates state without returning a status.However, I notice the AI summary mentions
bool handler_rc0_Process_Variables(MySQL_Connection *myconn)- please verify the actual return type matches the declaration.
573-573: LGTM!The
handle_session_track_capabilities()declaration is appropriately placed in the public section and follows the naming convention of other capability-checking methods.test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp (1)
166-175: LGTM! Test assertions are appropriate for OPTIONAL mode.The test correctly verifies:
- MySQL 5.7+: Session tracking works (
set_value == tracked_value)- MySQL 5.6 and below: Tracking gracefully disabled (
tracked_value == -1)This aligns with the expected OPTIONAL mode behavior where the feature is used when available but doesn't fail on unsupported servers.
lib/MySQL_Thread.cpp (5)
519-521: LGTM!The new variable name is correctly added to the array before the NULL terminator, following the established pattern.
1184-1185: LGTM!Safe default initialization with
DISABLEDensures session tracking is off until explicitly enabled by the administrator.
4405-4406: LGTM!Standard refresh pattern correctly propagates the global configuration value to the thread-local variable.
4744-4748: Server backoff check correctly placed.The backoff check efficiently filters out connections from servers in backoff before performing more expensive validation. The
server_backoff_timeis only set whensession_track_variables::ENFORCEDmode is active and the server lacks session tracking capabilities, so the check does not inadvertently exclude servers in other scenarios.
2367-2368: LGTM!Variable registration follows the established pattern. Bounds 0-2 correctly correspond to the
session_track_variables::DISABLED(0),OPTIONAL(1), andENFORCED(2) modes as defined ininclude/MySQL_Thread.h.lib/MySQL_Session.cpp (4)
5197-5199: LGTM: verification chain integration is in the right place.
It’s correctly placed before the general variable verification so the tracking knobs are enabled early.
5281-5282: LGTM: post-query variable processing hook is correctly sequenced.
Running after GTID processing and before result wiring makes sense.
1918-1922: Capability gating forsession_track_gtidsis correct.The code properly requires both
CLIENT_SESSION_TRACKINGandCLIENT_DEPRECATE_EOFcapabilities from the backend server, plus the globalmysql_thread___enable_server_deprecate_eofflag before enabling GTID tracking. The constant names are correct and used consistently throughout the codebase.
4965-4998:character_set_serveris not in tracked variables; charset normalization coverage is complete.The code correctly normalizes only the tracked charset variables:
character_set_{results,connection,client,database}andcollation_connection.character_set_serveris not part ofmysql_tracked_variables[](confirmed in enum at include/proxysql_structs.h:189+), so it is never passed to this handler byget_variables(). Thestd::unordered_mapallocation is already guarded by theif(myconn->get_variables(var_map))early exit and is a reasonable per-connection cost for session variable tracking.
| // 'server_backoff_time' stores a timestamp that prevents the server from being | ||
| // considered for random selection ('MyHGC::get_random_MySrvC') until that time passes. | ||
| // | ||
| // This is primarily used when `session_track_variables::ENFORCED` mode is active. | ||
| // If a server lacks the required capabilities in this mode, it is temporarily | ||
| // excluded from selection for a specified duration. | ||
| unsigned long long server_backoff_time; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "server_backoff_time" .Repository: sysown/proxysql
Length of output: 704
🏁 Script executed:
rg -n "curtime|atomic_curtime" include lib | head -n 100Repository: sysown/proxysql
Length of output: 10049
🏁 Script executed:
# Check the MySrvC class definition and surrounding code
fd "MySQL_HostGroups_Manager.h" --type fRepository: sysown/proxysql
Length of output: 93
🏁 Script executed:
# Check context around the write in MySQL_Session.cpp
sed -n '8580,8605p' lib/MySQL_Session.cppRepository: sysown/proxysql
Length of output: 1180
🏁 Script executed:
# Check context around the read in MyHGC.cpp
sed -n '35,50p' lib/MyHGC.cppRepository: sysown/proxysql
Length of output: 985
🏁 Script executed:
# Check context around the read in MySQL_Thread.cpp
sed -n '5740,5755p' lib/MySQL_Thread.cppRepository: sysown/proxysql
Length of output: 886
🏁 Script executed:
# Check if MySrvC or parent access is protected by locks in the surrounding code
sed -n '8570,8600p' lib/MySQL_Session.cpp | head -40Repository: sysown/proxysql
Length of output: 1413
🏁 Script executed:
# Look for mutex/lock patterns in the function containing the write
rg -B 30 "server_backoff_time = thread->curtime" lib/MySQL_Session.cpp | head -60Repository: sysown/proxysql
Length of output: 1411
Make server_backoff_time thread-safe (atomic or lock-protected).
server_backoff_time is written without synchronization in MySQL_Session::handle_session_track_capabilities() (line 8595) and read without locking in MyHGC::get_random_MySrvC() (line 42) and MySQL_Thread.cpp (line 5746) from different threads. This creates a data race that could cause torn reads on some platforms.
The timebase is microseconds (confirmed by the write: thread->curtime + (30 * 1000000) for 30 seconds), which is consistent with curtime. The comment in the code adequately documents this, but the field itself needs synchronization.
Proposed fix
- unsigned long long server_backoff_time;
+ std::atomic<unsigned long long> server_backoff_time { 0 };| /** | ||
| * @brief Configuration flag to enable/disable session variable tracking. | ||
| * | ||
| * When set to session_track_variables::ENABLED (1), ProxySQL will configure | ||
| * backend connections to track system variable changes using MySQL's | ||
| * session_track_system_variables and session_track_state_change capabilities. | ||
| * | ||
| * Default: session_track_variables::DISABLED (0) | ||
| */ | ||
| int session_track_variables; | ||
| } variables; |
There was a problem hiding this comment.
Fix docs: session_track_variables::ENABLED doesn’t exist (values are DISABLED/OPTIONAL/ENFORCED).
The comment on variables.session_track_variables (Line 640) references a non-existent enum value; this will confuse operators and reviewers.
Proposed fix
- * When set to session_track_variables::ENABLED (1), ProxySQL will configure
+ * When set to session_track_variables::OPTIONAL (1) or
+ * session_track_variables::ENFORCED (2), ProxySQL will configure
* backend connections to track system variable changes using MySQL's
* session_track_system_variables and session_track_state_change capabilities.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @brief Configuration flag to enable/disable session variable tracking. | |
| * | |
| * When set to session_track_variables::ENABLED (1), ProxySQL will configure | |
| * backend connections to track system variable changes using MySQL's | |
| * session_track_system_variables and session_track_state_change capabilities. | |
| * | |
| * Default: session_track_variables::DISABLED (0) | |
| */ | |
| int session_track_variables; | |
| } variables; | |
| /** | |
| * @brief Configuration flag to enable/disable session variable tracking. | |
| * | |
| * When set to session_track_variables::OPTIONAL (1) or | |
| * session_track_variables::ENFORCED (2), ProxySQL will configure | |
| * backend connections to track system variable changes using MySQL's | |
| * session_track_system_variables and session_track_state_change capabilities. | |
| * | |
| * Default: session_track_variables::DISABLED (0) | |
| */ | |
| int session_track_variables; | |
| } variables; |
🤖 Prompt for AI Agents
In @include/MySQL_Thread.h around lines 637 - 647, The comment for
variables.session_track_variables incorrectly references
session_track_variables::ENABLED; update the docstring for the int
session_track_variables field to list the actual enum variants
(session_track_variables::DISABLED, session_track_variables::OPTIONAL,
session_track_variables::ENFORCED), explain what ENFORCED/OPTIONAL mean wrt
backend configuration tracking, and keep the default noted as
session_track_variables::DISABLED; locate the comment above the
variables.session_track_variables declaration and replace the incorrect enum
name with the correct three values and brief descriptions.
| if (mysrvc->get_status() == MYSQL_SERVER_STATUS_ONLINE) { // consider this server only if ONLINE | ||
| // skip servers that are in backoff period | ||
| if (mysrvc->server_backoff_time > sess->thread->curtime) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: sysown/proxysql
Length of output: 799
🏁 Script executed:
find . -name "*.h" -o -name "*.hpp" | grep -i "mysql\|hgc" | head -20Repository: sysown/proxysql
Length of output: 612
🏁 Script executed:
ls -la lib/MyHGC.cpp 2>/dev/null && echo "File exists" || echo "Checking structure..."Repository: sysown/proxysql
Length of output: 134
🏁 Script executed:
# Get codebase structure
fd --type f --name "*.cpp" --name "*.h" | grep -E "(MyHGC|mysql)" | head -30Repository: sysown/proxysql
Length of output: 309
🏁 Script executed:
cat -n lib/MyHGC.cpp | head -60Repository: sysown/proxysql
Length of output: 2925
🏁 Script executed:
# Search for server_backoff_time definition
rg "server_backoff_time" --type-list | head -5
rg "server_backoff_time" -B 2 -A 2 | head -100Repository: sysown/proxysql
Length of output: 2462
🏁 Script executed:
# Find all writes to server_backoff_time
rg "server_backoff_time\s*=" -B 3 -A 3Repository: sysown/proxysql
Length of output: 859
🏁 Script executed:
# Find the header definition with context
rg -A 5 -B 5 "unsigned long long server_backoff_time"Repository: sysown/proxysql
Length of output: 919
🏁 Script executed:
# Check for any mutex/synchronization related to MySrvC
rg "MySrvC" include/MySQL_HostGroups_Manager.h | head -20Repository: sysown/proxysql
Length of output: 915
🏁 Script executed:
# Search for mutex or lock context around MySrvC or server_backoff_time
rg -B 10 "server_backoff_time = " lib/MySQL_Session.cpp | head -40Repository: sysown/proxysql
Length of output: 792
🏁 Script executed:
# Look for threading/locks in relation to MySrvC
rg "pthread_mutex\|lock\|rwlock" include/MySQL_HostGroups_Manager.h -A 2 -B 2Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Check MyHGC context - who calls get_random_MySrvC
rg "get_random_MySrvC" -B 5 | head -50Repository: sysown/proxysql
Length of output: 3145
🏁 Script executed:
# Check if there's a lock protecting MySrvC or mysrvs
rg -B 5 -A 5 "class MySrvC" include/MySQL_HostGroups_Manager.h | head -80Repository: sysown/proxysql
Length of output: 800
🏁 Script executed:
# Look for locks in the hostgroup management
rg "pthread_rwlock\|pthread_mutex\|rwlock_write\|wrlock\|rdlock" include/MySQL_HostGroups_Manager.h | head -20Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Check the broader context of MySQL_Session write
rg -B 20 "server_backoff_time = thread->curtime" lib/MySQL_Session.cpp | head -60Repository: sysown/proxysql
Length of output: 1115
🏁 Script executed:
# Check if there are thread contexts for writes vs reads
rg "MySQL_Session\|MySQL_Thread" lib/MyHGC.cpp | head -20Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Check BaseHGC for any locking mechanism
rg "class BaseHGC" -A 30 include/Base_HostGroups_Manager.h | head -60Repository: sysown/proxysql
Length of output: 1817
🏁 Script executed:
# Look for rwlock or mutex protecting server lists
rg "rwlock\|pthread_mutex" include/MySQL_HostGroups_Manager.h -B 5 -A 5Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Check MySQL_Thread structure and curtime
rg "class MySQL_Thread" -A 50 include/MySQL_Thread.h | head -80Repository: sysown/proxysql
Length of output: 1908
🏁 Script executed:
# Check if there's synchronization around get_random_MySrvC calls
rg -B 10 "myhgc->get_random_MySrvC" lib/Base_HostGroups_Manager.cpp | head -50Repository: sysown/proxysql
Length of output: 456
🏁 Script executed:
# Check MySrvList structure for any locks
rg "class MySrvList\|class BaseSrvList" -A 40 include/MySQL_HostGroups_Manager.h | head -80Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Examine the actual threading - is get_random_MySrvC protected by any lock?
rg "MyHGM\|myhgc\|get_random_MySrvC" lib/MySQL_Session.cpp -B 3 -A 3 | head -80Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Search more broadly for MySrvList
fd "MySQL_HostGroups_Manager" --type fRepository: sysown/proxysql
Length of output: 126
🏁 Script executed:
# Try to find MySrvList definition
rg "class MySrvList" -A 50Repository: sysown/proxysql
Length of output: 26083
🏁 Script executed:
# Check if there's a global lock protecting MyHGM or MyHGC access
rg "MyHGM\|MyHGC_lookup" include/*.h -B 2 -A 2 | head -100Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Look for synchronization in get_random_MySrvC itself
cat -n lib/MyHGC.cpp | sed -n '14,120p'Repository: sysown/proxysql
Length of output: 6052
🏁 Script executed:
# Final check: confirm multiple threads can access same MySrvC by searching for thread pool/worker threads
rg "worker.*thread\|pthread_create" include/MySQL_Thread.h -A 3 -B 3 | head -40Repository: sysown/proxysql
Length of output: 41
Use atomic load to prevent data races in backoff check.
server_backoff_time is a plain unsigned long long that can be written by one thread (MySQL_Session.cpp) and read by another (MyHGC.cpp line 42) without synchronization. This creates a data race. The codebase already uses atomics elsewhere (e.g., num_online_servers.load() on line 45); apply the same pattern here:
Proposed fix
- if (mysrvc->server_backoff_time > sess->thread->curtime)
+ if (mysrvc->server_backoff_time.load(std::memory_order_relaxed) > sess->thread->curtime)
continue;Also update the field declaration in include/MySQL_HostGroups_Manager.h from unsigned long long server_backoff_time; to std::atomic<unsigned long long> server_backoff_time; and ensure the assignment in MySQL_Session.cpp uses .store() or assignment (which works with atomics).
🤖 Prompt for AI Agents
In @lib/MyHGC.cpp around lines 40 - 44, Replace the plain unsigned long long
backoff field with an atomic and use atomic loads/stores to avoid data races:
change the declaration of server_backoff_time in MySQL_HostGroups_Manager.h to
std::atomic<unsigned long long>; in MyHGC.cpp replace direct reads of
mysrvc->server_backoff_time with mysrvc->server_backoff_time.load() for the
backoff comparison (same spot as the check around get_status() and the existing
num_online_servers.load() usage); and in MySQL_Session.cpp update assignments to
use server_backoff_time.store(new_value) (or assignment which is supported) so
writes are atomic.
| if (deprecate_eof_active) { | ||
| if (mysql_thread___enable_client_deprecate_eof | ||
| || mysql_thread___session_track_variables == session_track_variables::ENFORCED) { | ||
| extended_capabilities |= CLIENT_DEPRECATE_EOF; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MySQL_Protocol.cpp" -type fRepository: sysown/proxysql
Length of output: 83
🏁 Script executed:
# Get context around the lines mentioned in the review
grep -n "deprecate_eof_active" . -r --include="*.cpp" | head -20Repository: sysown/proxysql
Length of output: 2205
🏁 Script executed:
# Search for PPHR_2 and the strip logic mentioned
grep -n "PPHR_2" . -r --include="*.cpp" | head -10Repository: sysown/proxysql
Length of output: 309
🏁 Script executed:
# Find the full function context around line 1114-1120
sed -n '1100,1130p' lib/MySQL_Protocol.cppRepository: sysown/proxysql
Length of output: 1899
🏁 Script executed:
# Search for the CLIENT_DEPRECATE_EOF strip logic mentioned
grep -n "CLIENT_DEPRECATE_EOF" lib/MySQL_Protocol.cppRepository: sysown/proxysql
Length of output: 967
🌐 Web query:
MySQL CLIENT_DEPRECATE_EOF capability flag semantics protocol specification
💡 Result:
Summary of CLIENT_DEPRECATE_EOF semantics (protocol-level)
-
What it is: a client capability bit (CLIENT_DEPRECATE_EOF = 1 << 24 = 16777216) advertised in the Handshake Response so the server will use OK_Packet instead of EOF_Packet to terminate text resultsets. [1][2]
-
Why it exists: EOF_Packet is not extensible (overlaps with resultset row bytes); OK_Packet is extensible (needed for session-state / CLIENT_SESSION_TRACK). EOF was deprecated in MySQL 5.7.5 for this reason. [2][3]
-
Effect on Text Resultset framing:
- If CLIENT_DEPRECATE_EOF is NOT set: column defs and rows are terminated by EOF_Packet.
- If CLIENT_DEPRECATE_EOF is set: column defs and rows are terminated by OK_Packet (OK replaces EOF as the terminator). [4]
-
How to distinguish OK vs EOF bytes (backwards compatibility): OK and EOF share header values; the rules used are:
- OK: header = 0x00 (or 0xFE) and packet length > 7 → OK.
- EOF: header = 0xFE and packet length < 9 → EOF.
New clients advertise CLIENT_DEPRECATE_EOF so new servers may send OK-as-terminator; old servers (that don’t know the flag) won’t. [2]
-
Interaction with session tracking and server-status:
- Using OK as terminator allows the server to include session-state and other status fields (SERVER_SESSION_STATE_CHANGED, info, session-state-info) in the terminator packet; EOF could not carry that reliably. [2][3][4]
-
Implementation notes / caveats:
- The capability is part of the 32-bit capability mask; both sides must agree (server may choose behavior based on presence). [1]
- Some client/connector behavior (e.g., code that inspects server_status immediately after a resultset EOF) changed because status flags and "more results" indicators move into the terminating OK; this has caused connector issues in practice. See related bug/discussion. [5]
Sources
- MySQL server protocol capability flags (CLIENT_DEPRECATE_EOF description). [1]
- OK_Packet (OK vs EOF rules; CLIENT_DEPRECATE_EOF background). [2]
- EOF_Packet (deprecation note). [3]
- Text Resultset framing (when EOF vs OK used). [4]
- MySQL bug / discussion about connector issues with DEPRECATE_EOF. [5]
🌐 Web query:
MySQL session tracking ENFORCED mode CLIENT_DEPRECATE_EOF handling
💡 Result:
Do you mean the server/session-tracking "ENFORCED" SQL mode or something else? (I ask because I can’t find an authoritative MySQL doc that uses the exact phrase "session tracking ENFORCED mode".)
Meanwhile: brief summary of how CLIENT_DEPRECATE_EOF interacts with session-state tracking
- New clients advertise CLIENT_DEPRECATE_EOF to tell the server they accept OK packets (not EOF packets) as end-of-result markers; when that flag is present, modern servers use OK packets to carry session-state / tracker info at result end. [1][2].
- Session-state tracking (session_track_) data is sent in OK packets (when session tracking is enabled), so a client that uses CLIENT_DEPRECATE_EOF will receive session-state notifications inside OK packets rather than in legacy EOF packets. Enable/configure trackers with the session_track_ variables (e.g., session_track_state_change, session_track_system_variables, session_track_schema, session_track_gtids). [3][4].
- Caveat / known issue: changing from EOF to OK-as-EOF affects where status flags (SERVER_MORE_RESULTS_EXIST, SERVER_PS_OUT_PARAMS, etc.) and intermediate EOF semantics appear; some connectors/clients experienced behavioral issues when CLIENT_DEPRECATE_EOF is set because formerly-available EOF-based signals are moved into the final OK packet. See MySQL bug reports/discussions. [2][5].
Sources
- MySQL OK_Packet documentation (explains OK used to indicate EOF and CLIENT_DEPRECATE_EOF flag). [dev.mysql.com]. [1]
- MySQL EOF_Packet / protocol notes. [dev.mysql.com]. [2]
- MySQL Session-State Tracking overview and session_track_* variables. [dev.mysql.com / Oracle MySQL manual]. [3][4]
- MySQL bug report re: CLIENT_DEPRECATE_EOF and connector behavior. [bugs.mysql.com]. [5]
If you want, I can: (a) show the exact packet layout examples, (b) point to where connectors (e.g., libmysqlclient, Connector/J, Connector/C) handle CLIENT_DEPRECATE_EOF, or (c) help debug a specific client/connector problem you’re seeing.
🏁 Script executed:
sed -n '1595,1615p' lib/MySQL_Protocol.cppRepository: sysown/proxysql
Length of output: 1225
🏁 Script executed:
# Get more context to understand the full condition
sed -n '1580,1620p' lib/MySQL_Protocol.cppRepository: sysown/proxysql
Length of output: 2270
ENFORCED mode advertises CLIENT_DEPRECATE_EOF, but PPHR_2 still strips it when mysql_thread___enable_client_deprecate_eof is false — breaks protocol negotiation.
In generate_pkt_initial_handshake(), you advertise CLIENT_DEPRECATE_EOF when either mysql_thread___enable_client_deprecate_eof is enabled OR mysql_thread___session_track_variables == ENFORCED (lines 1116–1117). However, in PPHR_2(), the flag is unconditionally stripped if !mysql_thread___enable_client_deprecate_eof (line 1606), without checking for ENFORCED mode.
This causes a protocol violation: the handshake advertises support for OK packets (not EOF packets) as result terminators, but the client handshake response processing removes that flag regardless. Since session-state tracking (ENFORCED mode) requires OK packets to carry state data, this inconsistency breaks the negotiated capability set.
The strip logic in PPHR_2 should mirror the advertise logic:
Proposed fix (in PPHR_2, around line 1606)
- if (!mysql_thread___enable_client_deprecate_eof) {
+ if (
+ !mysql_thread___enable_client_deprecate_eof
+ && mysql_thread___session_track_variables != session_track_variables::ENFORCED
+ ) {
vars1.capabilities &= ~CLIENT_DEPRECATE_EOF;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (deprecate_eof_active) { | |
| if (mysql_thread___enable_client_deprecate_eof | |
| || mysql_thread___session_track_variables == session_track_variables::ENFORCED) { | |
| extended_capabilities |= CLIENT_DEPRECATE_EOF; | |
| } | |
| } | |
| if ( | |
| !mysql_thread___enable_client_deprecate_eof | |
| && mysql_thread___session_track_variables != session_track_variables::ENFORCED | |
| ) { | |
| vars1.capabilities &= ~CLIENT_DEPRECATE_EOF; | |
| } |
🤖 Prompt for AI Agents
In @lib/MySQL_Protocol.cpp around lines 1114 - 1120,
generate_pkt_initial_handshake advertises CLIENT_DEPRECATE_EOF when
mysql_thread___enable_client_deprecate_eof is true OR
mysql_thread___session_track_variables == session_track_variables::ENFORCED, but
PPHR_2 currently unconditionally strips CLIENT_DEPRECATE_EOF when
!mysql_thread___enable_client_deprecate_eof; update PPHR_2 so the removal of
CLIENT_DEPRECATE_EOF mirrors the advertise logic—only clear the flag if neither
mysql_thread___enable_client_deprecate_eof is true nor
mysql_thread___session_track_variables == session_track_variables::ENFORCED
(i.e., strip CLIENT_DEPRECATE_EOF only when both conditions are false) so
capability negotiation remains consistent between generate_pkt_initial_handshake
and PPHR_2.
| bool MySQL_Session::handler_again___verify_backend_session_track_variables() { | ||
| int mode = mysql_thread___session_track_variables; | ||
|
|
||
| // skip enabling session variable tracking in the following cases | ||
| if (mode == session_track_variables::DISABLED) { | ||
| return false; | ||
| } | ||
| if ((mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_SESSION_TRACKING) == 0 | ||
| || (mybe->server_myds->myconn->mysql->server_capabilities & CLIENT_DEPRECATE_EOF) == 0) { | ||
| return false; | ||
| } | ||
| if (!mysql_thread___enable_server_deprecate_eof && mode != session_track_variables::ENFORCED) { | ||
| return false; | ||
| } | ||
|
|
||
| // enable session tracking | ||
| if (mybe->server_myds->myconn->options.session_track_variables_sent == false) { | ||
| mybe->server_myds->myconn->options.session_track_variables_sent = true; | ||
| set_previous_status_mode3(); | ||
| NEXT_IMMEDIATE_NEW(SETTING_SESSION_TRACK_VARIABLES); | ||
| } | ||
| if (mybe->server_myds->myconn->options.session_track_state_sent == false) { | ||
| mybe->server_myds->myconn->options.session_track_state_sent = true; | ||
| set_previous_status_mode3(); | ||
| NEXT_IMMEDIATE_NEW(SETTING_SESSION_TRACK_STATE); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's search for all *_sent flag patterns in the relevant files
rg -n "options\.\w+_sent\s*=" lib/MySQL_Session.cpp lib/mysql_connection.cpp include/mysql_connection.h 2>/dev/null | head -50Repository: sysown/proxysql
Length of output: 1664
🏁 Script executed:
# Look at the ldap_user_variable_sent handling which has a reset pattern
sed -n '2065,2095p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 1616
🏁 Script executed:
# Look at the context around the flag resets in mysql_connection.cpp
sed -n '3090,3120p' lib/mysql_connection.cpp | cat -nRepository: sysown/proxysql
Length of output: 1191
🏁 Script executed:
# Find the function containing the flag reset code
sed -n '3050,3095p' lib/mysql_connection.cpp | cat -nRepository: sysown/proxysql
Length of output: 1749
🏁 Script executed:
# Find calls to reset() function
rg -n "\.reset\(\)" lib/MySQL_Session.cpp lib/mysql_connection.cpp | head -30Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Search for reset() with different patterns
rg -n "reset\(\)" lib/MySQL_Session.cpp lib/mysql_connection.cpp | head -30Repository: sysown/proxysql
Length of output: 494
🏁 Script executed:
# Check context around line 1668 and 3170 - likely error handling
echo "=== Context around line 1668 ===" && sed -n '1660,1675p' lib/MySQL_Session.cpp | cat -n && \
echo "=== Context around line 3170 ===" && sed -n '3160,3175p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 1756
🏁 Script executed:
# Look for error handling in the handler functions - search for error conditions after NEXT_IMMEDIATE_NEW
sed -n '1969,2000p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 1534
🏁 Script executed:
# Search for the handler function that processes SETTING_SESSION_TRACK_VARIABLES
rg -n "handler_again___status_SETTING_SESSION_TRACK_VARIABLES|SETTING_SESSION_TRACK_VARIABLES" lib/MySQL_Session.cpp | head -20Repository: sysown/proxysql
Length of output: 329
🏁 Script executed:
# View the handler function
sed -n '2798,2850p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 2545
🏁 Script executed:
# Find the handler_again___status_SETTING_GENERIC_VARIABLE function
rg -n "bool MySQL_Session::handler_again___status_SETTING_GENERIC_VARIABLE" lib/MySQL_Session.cppRepository: sysown/proxysql
Length of output: 225
🏁 Script executed:
# View the handler_again___status_SETTING_GENERIC_VARIABLE function
sed -n '2539,2650p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 5367
🏁 Script executed:
# Continue viewing the error handling
sed -n '2650,2700p' lib/MySQL_Session.cpp | cat -nRepository: sysown/proxysql
Length of output: 2431
Session track variable flags should be reset on soft SQL errors to allow retry.
The session_track_variables_sent and session_track_state_sent flags are set to true before the SET commands execute. In the error handler (handler_again___status_SETTING_GENERIC_VARIABLE), soft SQL errors (like 1193 - variable not supported) allow retry by popping back to the previous status, but these flags are never reset. This means the next time handler_again___verify_backend_session_track_variables() is called, the SET command is skipped because the flag check if (session_track_variables_sent == false) will be false, preventing retry.
The ldap_user_variable_sent flag in the same codebase demonstrates that flags can be reset on specific conditions (line 2082), showing this pattern is feasible. Consider resetting the *_sent flags when soft SQL errors occur and retry is triggered, or alternatively reset them only after the handler returns success.
🤖 Prompt for AI Agents
In @lib/MySQL_Session.cpp around lines 1969 - 1997, The flags
session_track_variables_sent and session_track_state_sent set in
handler_again___verify_backend_session_track_variables() must be reset when a
soft SQL error triggers a retry (or only set after the SET succeeds); update the
error path in handler_again___status_SETTING_GENERIC_VARIABLE to clear
mybe->server_myds->myconn->options.session_track_variables_sent and
session_track_state_sent when the handler pops back to the previous status (use
the ldap_user_variable_sent reset logic as a guide), or alternatively move
flag-setting to after the SET handlers return success so retries will re-send
the SETs.
| if (!admin) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
Same NULL pointer issue with mysql_error() on failed connection.
As noted in other test files, calling mysql_error(admin) when admin is NULL is undefined behavior.
🤖 Prompt for AI Agents
In @test/tap/tests/mysql-session_track_variables_ff_enforced-t.cpp around lines
121 - 124, The check for a failed connection uses mysql_error(admin) even when
admin is NULL; avoid calling mysql_error with a NULL pointer. Update the error
branch around the admin variable so you only call mysql_error(admin) when admin
is non-NULL; if admin is NULL, print a safe fallback (e.g., a generic "failed to
initialize connection" message or errno/strerror output) instead of passing NULL
to mysql_error.
| if (!admin) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
Same NULL pointer issue with mysql_error() on failed connection.
As noted in other test files, calling mysql_error(admin) when admin is NULL is undefined behavior.
🤖 Prompt for AI Agents
In @test/tap/tests/mysql-session_track_variables_ff_optional-t.cpp around lines
121 - 124, The code calls mysql_error(admin) even when admin is NULL; change it
to avoid passing a NULL pointer to mysql_error by checking admin first and using
the original MYSQL handle used to initiate the connection (e.g., call
mysql_error(&mysql) or mysql_error(mysql_init_handle) when admin is NULL), or
otherwise only call mysql_error(admin) when admin is non-NULL; update the
fprintf accordingly and keep the return exit_status() behavior.
| if (!admin) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
Same NULL pointer issue as in enforced test.
The mysql_error() call on a potentially NULL admin pointer (and similarly for proxy at line 127) should be fixed as noted in the enforced test review.
🤖 Prompt for AI Agents
In @test/tap/tests/mysql-session_track_variables_optional-t.cpp around lines 116
- 119, Avoid calling mysql_error() with a NULL pointer: update the
error-handling for the `admin` (and similarly `proxy`) initialization so you
only call mysql_error(admin) when `admin` is non-NULL; if `admin` is NULL log a
clear fallback message like "mysql_init() returned NULL" (or include
strerror(errno)) instead. Locate the checks around the `admin` and `proxy`
variables in this test and replace the unconditional mysql_error(...) call with
a conditional that prints mysql_error(admin) when admin != NULL, otherwise
prints the fallback message; do the same pattern for `proxy`.
| 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"); | ||
|
|
||
| MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); | ||
|
|
There was a problem hiding this comment.
Don’t “fix” regressions by disabling session tracking in this test; adjust the test to tolerate/measure the new init traffic instead.
reg_test_4264 relies on exact query/connection counters, so turning off mysql-session_track_variables (Line 1133) likely just sidesteps new backend SET/OK behavior introduced by this PR. That makes the test less meaningful and can hide real regressions in transaction routing.
Suggested direction: establish the baseline after any connection-init/session-track setup (or explicitly subtract/ignore the internal setup queries), so the test can run with tracking enabled and still assert routing correctly.
🤖 Prompt for AI Agents
In @test/tap/tests/reg_test_4264-commit_rollback-t.cpp around lines 1131 - 1136,
The test disables session tracking (the MYSQL_QUERY_T setting
mysql-session_track_variables=0) which masks new connection-init SET/OK traffic;
instead re-enable session tracking and adjust reg_test_4264 to measure or ignore
init traffic by establishing the baseline counters after connection
initialization (e.g., perform the connection(s) and session-track setup, then
call a helper that reads current query/connection counters and uses those as the
starting point) or explicitly subtract the known internal setup queries from
assertions; update the places using MYSQL_QUERY_T and the LOAD MYSQL VARIABLES
TO RUNTIME step so the test asserts delta values relative to the post-init
baseline rather than absolute counts.
Signed-off-by: René Cannaò <rene@proxysql.com>
|
retest this please |
Signed-off-by: Miro Stauder <miro@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
retest this please |
Signed-off-by: René Cannaò <rene@proxysql.com>
|
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.
| // Enforce session tracking; connection fails if backend does | ||
| // not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING | ||
| ENFORCED |
There was a problem hiding this comment.
ENFORCED comment overstates failure — the backend is backoff'd, not hard-failed.
// Enforce session tracking; connection fails if backend does
// not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING
The actual behaviour (see handle_session_track_capabilities, lib/MySQL_Session.cpp lines 9590–9596) is that the specific connection is returned to the pool or destroyed and the server receives a 30-second backoff (server_backoff_time). No error is propagated to the client; the session will retry on a compatible server. "connection fails" implies a hard client-visible error.
Proposed fix
- // Enforce session tracking; connection fails if backend does
- // not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING
+ // Enforce session tracking; connections to backends that lack
+ // CLIENT_DEPRECATE_EOF or CLIENT_SESSION_TRACKING are rejected and
+ // those servers are put in a temporary backoff.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Enforce session tracking; connection fails if backend does | |
| // not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING | |
| ENFORCED | |
| // Enforce session tracking; connections to backends that lack | |
| // CLIENT_DEPRECATE_EOF or CLIENT_SESSION_TRACKING are rejected and | |
| // those servers are put in a temporary backoff. | |
| ENFORCED |
🤖 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, 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".
| // override 'mysql-enable_server_deprecate_eof' behavior if 'session_track_variables' is set to 'ENFORCED' | ||
| if (mysql_thread___session_track_variables == session_track_variables::ENFORCED) { | ||
| mysql->options.client_flag |= CLIENT_DEPRECATE_EOF; | ||
| } |
There was a problem hiding this comment.
ENFORCED mode is silently overridden for fast-forward connections.
The new ENFORCED block (lines 901–903) sets CLIENT_DEPRECATE_EOF in mysql->options.client_flag, but the existing fast-forward block immediately below it (line 911) unconditionally clears that flag:
mysql->options.client_flag &= ~(CLIENT_DEPRECATE_EOF); // we disable it by defaultIt then only conditionally re-enables it based on the client's capabilities. For any session_fast_forward connection, ENFORCED mode has zero effect — the override is silent with no warning or error logged. At minimum, a proxy_warning should be emitted when ENFORCED is configured but cannot be honoured for a fast-forward session. Ideally, the ENFORCED guard should be placed after (or merged into) the fast-forward block to apply the correct enforcement semantics.
🔧 Suggested placement for the ENFORCED guard
if (myds != NULL) {
if (myds->sess != NULL) {
if (myds->sess->session_fast_forward) {
...
mysql->options.client_flag &= ~(CLIENT_DEPRECATE_EOF); // we disable it by default
if (c->options.client_flag & CLIENT_DEPRECATE_EOF) {
if (c->options.server_capabilities & CLIENT_DEPRECATE_EOF) {
mysql->options.client_flag |= CLIENT_DEPRECATE_EOF;
}
}
+ // ENFORCED mode cannot override fast_forward capability matching;
+ // log a warning if ENFORCED is configured but the requirement cannot be met.
+ if (mysql_thread___session_track_variables == session_track_variables::ENFORCED) {
+ if (!(mysql->options.client_flag & CLIENT_DEPRECATE_EOF)) {
+ proxy_warning("session_track_variables=ENFORCED but CLIENT_DEPRECATE_EOF "
+ "could not be set 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 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.
| if (mysql_session_track_get_first(mysql, SESSION_TRACK_SYSTEM_VARIABLES, &data, &length) == 0) { | ||
| string var_name(data, length); | ||
| string val; | ||
|
|
||
| // get_first() returns a variable_name | ||
| // get_next() will return the value | ||
| bool expect_value = true; | ||
|
|
||
| while (mysql_session_track_get_next(mysql, SESSION_TRACK_SYSTEM_VARIABLES, &data, &length) == 0) { | ||
| if (expect_value) { | ||
| val = string(data, length); | ||
| variables[var_name] = val; | ||
| // got a value in this iteration | ||
| // in the next iteration, we have to expect a variable_name | ||
| expect_value = false; | ||
| } else { | ||
| var_name = string(data, length); | ||
| // got a variable_name in this iteration | ||
| // in the next iteration, we have to expect the value of this variable | ||
| expect_value = true; | ||
| } | ||
| } | ||
|
|
||
| ret = true; | ||
| } |
There was a problem hiding this comment.
ret = true is set even when no name-value pairs are stored.
ret is set unconditionally after get_first() succeeds (line 3183), regardless of how many pairs the while loop actually inserted into variables. Two concrete problems:
- Empty map with
ret = true: Ifget_next()fails on the very first call (malformed data containing only a variable name with no value), the function returnstruewith an emptyvariablesmap, misleading callers. - Trailing unpaired name silently dropped: If
get_next()returns a variable name (flippingexpect_value = true) but the loop exits before the matching value call (odd-count malformed data), that lastvar_nameis never inserted into the map.
Moving ret = true to be conditional on at least one completed pair insertion would fix both:
🔧 Suggested fix
while (mysql_session_track_get_next(mysql, SESSION_TRACK_SYSTEM_VARIABLES, &data, &length) == 0) {
if (expect_value) {
val = string(data, length);
variables[var_name] = val;
+ ret = true;
expect_value = false;
} else {
var_name = string(data, length);
expect_value = true;
}
}
-
- 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 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.
4 participants
session_track_system_variablesandsession_track_state_changein all backend connections.MySQL_Set_Stmt_ParserSummary by CodeRabbit
New Features
Tests