From 4d251c80ec0d9026175d2525e6ffce9d7f1cd52d Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Fri, 17 Apr 2026 16:23:27 +0000 Subject: [PATCH] Revert 'enable_client_session_tracking' variable; keep greeting fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #5621 mis-diagnosed "CLIENT_SESSION_TRACKING missing from the greeting" as a long-standing configuration gap and proposed a new 'mysql-enable_client_session_tracking' variable analogous to 'mysql-enable_client_deprecate_eof'. On closer inspection this was in fact a regression introduced by commit 8c6a6444d (zstd refactor), which dropped the hard-coded upper-word bits from the greeting. The right fix is simply to restore those hard-coded bits — commits 536c9d916 and 6b311b701 already do that via the 'extended_capabilities' local in 'generate_pkt_initial_handshake'. The new variable adds no value on top of that: the hard-coded bits cannot be disabled through 'mysql-server_capabilities' (which is OR-combined with the greeting), and there is no protocol hazard motivating a per-instance off-switch for CLIENT_SESSION_TRACKING, unlike CLIENT_DEPRECATE_EOF. Revert the variable (MySQL_Thread.h / proxysql_structs.h / MySQL_Thread.cpp / MySQL_Protocol.cpp) and restore CLIENT_SESSION_TRACKING to the 'extended_capabilities' literal, matching v3.0.6 exactly. Keep the test's doxygen / diag / baseline-check rewrite from the investigation — it's strictly better coverage and will catch any future regression of the hard-coded bits early. Test plan reduced to 2 scenarios (deprecate_eof on/off) since session_tracking is now unconditionally advertised. --- include/MySQL_Thread.h | 1 - include/proxysql_structs.h | 2 - lib/MySQL_Protocol.cpp | 44 +++-- lib/MySQL_Thread.cpp | 4 - .../tests/test_greeting_capabilities-t.cpp | 160 ++++++++---------- 5 files changed, 89 insertions(+), 122 deletions(-) diff --git a/include/MySQL_Thread.h b/include/MySQL_Thread.h index bbae7ea09..6725ff730 100644 --- a/include/MySQL_Thread.h +++ b/include/MySQL_Thread.h @@ -584,7 +584,6 @@ class MySQL_Threads_Handler bool kill_backend_connection_when_disconnect; bool client_session_track_gtid; bool enable_client_deprecate_eof; - bool enable_client_session_tracking; bool enable_server_deprecate_eof; bool enable_load_data_local_infile; bool log_mysql_warnings_enabled; diff --git a/include/proxysql_structs.h b/include/proxysql_structs.h index 4036c6b1d..6f466ec14 100644 --- a/include/proxysql_structs.h +++ b/include/proxysql_structs.h @@ -1325,7 +1325,6 @@ __thread char * mysql_thread___default_variables[SQL_NAME_LAST_LOW_WM]; __thread int mysql_thread___query_digests_grouping_limit; __thread int mysql_thread___query_digests_groups_grouping_limit; __thread bool mysql_thread___enable_client_deprecate_eof; -__thread bool mysql_thread___enable_client_session_tracking; __thread bool mysql_thread___enable_server_deprecate_eof; __thread bool mysql_thread___log_mysql_warnings_enabled; __thread bool mysql_thread___enable_load_data_local_infile; @@ -1658,7 +1657,6 @@ extern __thread char * mysql_thread___default_variables[SQL_NAME_LAST_LOW_WM]; extern __thread int mysql_thread___query_digests_grouping_limit; extern __thread int mysql_thread___query_digests_groups_grouping_limit; extern __thread bool mysql_thread___enable_client_deprecate_eof; -extern __thread bool mysql_thread___enable_client_session_tracking; extern __thread bool mysql_thread___enable_server_deprecate_eof; extern __thread bool mysql_thread___log_mysql_warnings_enabled; extern __thread bool mysql_thread___enable_load_data_local_infile; diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index c8747fe38..67994c52c 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1093,11 +1093,6 @@ bool MySQL_Protocol::generate_pkt_initial_handshake(bool send, void **ptr, unsig } else { mysql_thread___server_capabilities &= ~CLIENT_DEPRECATE_EOF; } - if (mysql_thread___enable_client_session_tracking) { - mysql_thread___server_capabilities |= CLIENT_SESSION_TRACKING; - } else { - mysql_thread___server_capabilities &= ~CLIENT_SESSION_TRACKING; - } uint32_t server_capabilities = mysql_thread___server_capabilities; if (deprecate_eof_active && mysql_thread___enable_client_deprecate_eof) { server_capabilities |= CLIENT_DEPRECATE_EOF; @@ -1119,18 +1114,18 @@ bool MySQL_Protocol::generate_pkt_initial_handshake(bool send, void **ptr, unsig memcpy(_ptr+l,&uint8_charset, sizeof(uint8_charset)); l+=sizeof(uint8_charset); memcpy(_ptr+l,&server_status, sizeof(server_status)); l+=sizeof(server_status); // Upper-word ('capability_flags_2') capabilities advertised in the greeting. - // Baseline bits match what real MySQL servers advertise (see issue #4023); - // they were accidentally dropped during the zstd refactor in 8c6a6444d. - // The remaining upper-word bits (CLIENT_DEPRECATE_EOF, CLIENT_SESSION_TRACKING, - // CLIENT_ZSTD_COMPRESSION, ...) are folded in from 'server_capabilities' so - // the greeting stays in sync with the per-session / per-toggle state. - // 'extended_capabilities' is intentionally a local: it must NOT leak into - // '(*myds)->myconn->options.server_capabilities' nor into the low-word - // memcpy above, which record the per-connection state rather than the full - // greeting. + // These match what real MySQL servers advertise (see issue #4023) and were + // accidentally dropped during the zstd refactor in 8c6a6444d; this local + // restores the baseline. 'extended_capabilities' is intentionally a local: + // it must NOT leak into '(*myds)->myconn->options.server_capabilities' nor + // into the low-word memcpy above, which record per-connection state rather + // than the full greeting. Per-session/per-toggle upper-word bits (e.g. + // CLIENT_DEPRECATE_EOF when 'deprecate_eof_active', CLIENT_ZSTD_COMPRESSION + // when 'have_compress') are folded in from 'server_capabilities' so the + // greeting stays in sync with their runtime state. uint32_t extended_capabilities = CLIENT_MULTI_RESULTS | CLIENT_MULTI_STATEMENTS | CLIENT_PS_MULTI_RESULTS | - CLIENT_PLUGIN_AUTH | CLIENT_REMEMBER_OPTIONS; + CLIENT_PLUGIN_AUTH | CLIENT_SESSION_TRACKING | CLIENT_REMEMBER_OPTIONS; extended_capabilities |= server_capabilities & 0xFFFF0000u; uint16_t upper_word = static_cast(extended_capabilities >> 16); memcpy(_ptr+l, static_cast(&upper_word), sizeof(upper_word)); l += sizeof(upper_word); @@ -1607,19 +1602,18 @@ bool MySQL_Protocol::PPHR_2(unsigned char *pkt, unsigned int len, bool& ret, MyP if (vars1.capabilities & CLIENT_MULTI_STATEMENTS) { vars1.capabilities |= CLIENT_MULTI_RESULTS; } - // Enforce disabling 'CLIENT_DEPRECATE_EOF' and 'CLIENT_SESSION_TRACKING' from the - // supported capabilities when explicitly disabled via - // 'mysql_thread___enable_client_deprecate_eof' / 'mysql_thread___enable_client_session_tracking'. - // Downstream logic that treats the connection as supporting either capability relies on - // the 'client_flag' field of 'MySQL_Connection::options'. This is the second step of a - // two-step enforcement: the first step omits these bits from 'server_capabilities' - // during the initial handshake (see 'generate_pkt_initial_handshake'). + // we enforce disabling 'CLIENT_DEPRECATE_EOF' from the supported capabilities + // in case it's explicitly disabled by global variable 'mysql_thread___enable_client_deprecate_eof'. + // This is because further checks to actually threat the connection as a connection + // supporting 'CLIENT_DEPRECATE_EOF' rely in 'client_flag' field from + // 'MySQL_Connection::options'. + // This is the second step for ensuring that the connection is being handling + // in both ProxySQL and client side as a connection without 'CLIENT_DEPRECATE_EOF' support. + // First step is replying to client during initial handshake (in 'generate_pkt_initial_handshake') + // specifying no 'CLIENT_DEPRECATE_EOF' support in 'server_capabilities'. if (!mysql_thread___enable_client_deprecate_eof) { vars1.capabilities &= ~CLIENT_DEPRECATE_EOF; } - if (!mysql_thread___enable_client_session_tracking) { - vars1.capabilities &= ~CLIENT_SESSION_TRACKING; - } (*myds)->myconn->options.client_flag = vars1.capabilities; pkt += sizeof(uint32_t); vars1.max_pkt = CPY4(pkt); diff --git a/lib/MySQL_Thread.cpp b/lib/MySQL_Thread.cpp index 151806f46..079f108dc 100644 --- a/lib/MySQL_Thread.cpp +++ b/lib/MySQL_Thread.cpp @@ -358,7 +358,6 @@ static char * mysql_thread_variables_names[]= { (char *)"connect_timeout_server", (char *)"connect_timeout_server_max", (char *)"enable_client_deprecate_eof", - (char *)"enable_client_session_tracking", (char *)"enable_server_deprecate_eof", (char *)"enable_load_data_local_infile", (char *)"eventslog_filename", @@ -1442,7 +1441,6 @@ MySQL_Threads_Handler::MySQL_Threads_Handler() { variables.query_digests_grouping_limit = 3; variables.query_digests_groups_grouping_limit= 10; // changed in 2.6.0 , was 0 variables.enable_client_deprecate_eof=true; - variables.enable_client_session_tracking=true; variables.enable_server_deprecate_eof=true; variables.enable_load_data_local_infile=false; variables.log_mysql_warnings_enabled=false; @@ -2507,7 +2505,6 @@ char ** MySQL_Threads_Handler::get_variables_list() { VariablesPointers_bool["connection_warming"] = make_tuple(&variables.connection_warming, false); VariablesPointers_bool["default_reconnect"] = make_tuple(&variables.default_reconnect, false); VariablesPointers_bool["enable_client_deprecate_eof"] = make_tuple(&variables.enable_client_deprecate_eof, false); - VariablesPointers_bool["enable_client_session_tracking"] = make_tuple(&variables.enable_client_session_tracking, false); VariablesPointers_bool["enable_server_deprecate_eof"] = make_tuple(&variables.enable_server_deprecate_eof, false); #ifdef PROXYSQLFFTO VariablesPointers_bool["ffto_enabled"] = make_tuple(&variables.ffto_enabled, false); @@ -4746,7 +4743,6 @@ void MySQL_Thread::refresh_variables() { REFRESH_VARIABLE_BOOL(servers_stats); REFRESH_VARIABLE_BOOL(default_reconnect); REFRESH_VARIABLE_BOOL(enable_client_deprecate_eof); - REFRESH_VARIABLE_BOOL(enable_client_session_tracking); REFRESH_VARIABLE_BOOL(enable_server_deprecate_eof); REFRESH_VARIABLE_BOOL(enable_load_data_local_infile); REFRESH_VARIABLE_BOOL(log_mysql_warnings_enabled); diff --git a/test/tap/tests/test_greeting_capabilities-t.cpp b/test/tap/tests/test_greeting_capabilities-t.cpp index d0bf4b408..43b1805b5 100644 --- a/test/tap/tests/test_greeting_capabilities-t.cpp +++ b/test/tap/tests/test_greeting_capabilities-t.cpp @@ -11,32 +11,37 @@ * 32-bit capability bitmask that tells the client which protocol features the * server supports (see MySQL Client/Server Protocol documentation). * - * A subset of those capability bits are controlled by runtime ProxySQL - * variables and can therefore be toggled at will: + * Most of those bits are hard-coded to always-on values that match what a + * real MySQL server advertises — see @ref BASELINE_PRESENT_CAPS. One bit, + * `CLIENT_DEPRECATE_EOF`, is controlled at runtime by + * `mysql-enable_client_deprecate_eof`, because enabling it changes the wire + * protocol (no EOF packets) and not every client is ready for that. * - * - CLIENT_DEPRECATE_EOF -> `mysql-enable_client_deprecate_eof` - * - CLIENT_SESSION_TRACKING -> `mysql-enable_client_session_tracking` - * - * This test exercises all four combinations of those two toggles and checks - * the resulting capability bitmask returned by ProxySQL. + * Regression protection + * --------------------- + * The hard-coded bits were accidentally dropped during the zstd support + * refactor (commit `8c6a6444d`, shipped in v3.0.7). This regression manifested + * as `java.lang.ArrayIndexOutOfBoundsException` on JDBC clients relying on + * `CLIENT_SESSION_TRACKING` (see issues #4023 and #5621). The baseline check + * in @ref verify_caps is specifically designed to catch any future removal of + * those bits early. * * Test strategy * ------------- - * For each combination `{deprecate_eof, session_tracking}` in - * `{off,off} / {on,on} / {on,off} / {off,on}`: + * The test runs two scenarios covering the `mysql-enable_client_deprecate_eof` + * toggle (`on` / `off`). Each scenario: * - * 1. Set the two variables on the ProxySQL admin interface. - * 2. Apply the variables to runtime (`LOAD MYSQL VARIABLES TO RUNTIME`). - * 3. Open a fresh client connection against ProxySQL (mysql_real_connect). - * 4. Read `mysql->server_capabilities` (populated by libmariadb from the + * 1. Sets the variable on the ProxySQL admin interface. + * 2. Applies it to runtime (`LOAD MYSQL VARIABLES TO RUNTIME`). + * 3. Opens a fresh client connection against ProxySQL (`mysql_real_connect`). + * 4. Reads `mysql->server_capabilities` (populated by libmariadb from the * greeting packet). - * 5. Check that every capability in the expectation's `present_caps` is set - * and every capability in `absent_caps` is cleared. - * 6. Emit a single TAP `ok()` with a verbose message and `diag()` traces for - * every intermediate value so a failure can be debugged without attaching - * a protocol analyzer. + * 5. Checks that every bit in @ref BASELINE_PRESENT_CAPS is set, plus the + * scenario-specific expectation on `CLIENT_DEPRECATE_EOF`. + * 6. Emits a TAP `ok()` plus `diag()` traces for every intermediate value so + * a failure can be debugged without attaching a protocol analyzer. * - * The suite keeps running after a failure so that a single run surfaces every + * The suite keeps running after a failure so a single run surfaces every * broken scenario at once. */ @@ -151,21 +156,30 @@ static string caps_to_string(uint64_t caps) { } /** - * @brief Capability bits ProxySQL MUST advertise in every greeting, regardless - * of any runtime toggle. + * @brief Capability bits ProxySQL MUST advertise in every greeting. + * + * These are hard-coded into the `extended_capabilities` local in + * `MySQL_Protocol::generate_pkt_initial_handshake` and match what a real + * MySQL 8.x server advertises. Several clients rely on them — in particular + * JDBC needs `CLIENT_SESSION_TRACKING` to parse GTID data from OK packets, + * and PS_MULTI_RESULTS is required for stored procedures returning multiple + * result sets. * - * These match what a real MySQL 8.x server advertises in its Server Greeting - * and are relied upon by several clients (JDBC in particular — see issue - * #4023). Regressing any of them typically manifests as - * `java.lang.ArrayIndexOutOfBoundsException` on the client side. + * These bits are intentionally NOT configurable through + * `mysql-server_capabilities`: that admin variable is OR-combined with the + * greeting, so it can only *add* bits, never remove them — precisely so a + * misconfigured runtime cannot strip protocol-critical capabilities. * - * This list is checked against every scenario via @ref verify_caps. + * Regressing any of these bits typically manifests as + * `java.lang.ArrayIndexOutOfBoundsException` on JDBC clients (issue #4023) + * or as clients silently losing session-tracking data (issue #5621). */ static const vector BASELINE_PRESENT_CAPS = { CLIENT_MULTI_STATEMENTS, CLIENT_MULTI_RESULTS, CLIENT_PS_MULTI_RESULTS, CLIENT_PLUGIN_AUTH, + CLIENT_SESSION_TRACKING, CLIENT_REMEMBER_OPTIONS, }; @@ -175,8 +189,8 @@ static const vector BASELINE_PRESENT_CAPS = { * * A test scenario supplies one `caps_expectation_t` describing ONLY the bits * it cares to toggle. Bits in @ref present_caps MUST be set in - * `mysql->server_capabilities`; bits in @ref absent_caps MUST be cleared. Bits - * appearing in neither list are ignored here because ProxySQL's baseline + * `mysql->server_capabilities`; bits in @ref absent_caps MUST be cleared. + * Bits appearing in neither list are ignored here because ProxySQL's baseline * greeting also flips bits like `CLIENT_COMPRESS`, `CLIENT_SSL`, etc. whose * values depend on global configuration unrelated to the scenario. * @@ -234,20 +248,18 @@ static bool verify_caps(uint64_t actual_caps, const caps_expectation_t& exp) { } /** - * @brief Apply the two runtime toggles under test and reload them. + * @brief Apply the `mysql-enable_client_deprecate_eof` toggle and reload. * - * Runs three admin statements: + * Runs two admin statements: * - `SET mysql-enable_client_deprecate_eof = <0|1>` - * - `SET mysql-enable_client_session_tracking = <0|1>` * - `LOAD MYSQL VARIABLES TO RUNTIME` * - * @param admin Already-connected admin MYSQL* handle. - * @param deprecate_eof Desired value for `mysql-enable_client_deprecate_eof`. - * @param session_tracking Desired value for `mysql-enable_client_session_tracking`. - * @return `true` on success, `false` if any statement failed - * (the error is printed via `diag()` before returning). + * @param admin Already-connected admin MYSQL* handle. + * @param deprecate_eof Desired value for `mysql-enable_client_deprecate_eof`. + * @return `true` on success, `false` if any statement failed + * (the error is printed via `diag()` before returning). */ -static bool apply_runtime_toggles(MYSQL* admin, bool deprecate_eof, bool session_tracking) { +static bool apply_deprecate_eof(MYSQL* admin, bool deprecate_eof) { char stmt[256]; snprintf(stmt, sizeof(stmt), @@ -258,14 +270,6 @@ static bool apply_runtime_toggles(MYSQL* admin, bool deprecate_eof, bool session return false; } - snprintf(stmt, sizeof(stmt), - "SET mysql-enable_client_session_tracking=%d", session_tracking ? 1 : 0); - diag(" admin> %s", stmt); - if (mysql_query(admin, stmt)) { - diag(" admin query failed: %s", mysql_error(admin)); - return false; - } - const char* reload = "LOAD MYSQL VARIABLES TO RUNTIME"; diag(" admin> %s", reload); if (mysql_query(admin, reload)) { @@ -314,16 +318,14 @@ static MYSQL* open_client(const CommandLine& cl, uint64_t client_flag) { /** * @struct scenario_t - * @brief One permutation of the runtime toggles plus its expectation. + * @brief One permutation of the `deprecate_eof` toggle plus its expectation. * - * Keeping the four scenarios in a table makes the main loop small and the - * list of cases easy to extend: adding a fifth scenario only requires a new - * row here, not more code. + * Keeping the scenarios in a table makes the main loop small and the list of + * cases easy to extend: adding a new row here is all that's needed. */ struct scenario_t { const char* name; ///< Short label used in TAP + diag. bool deprecate_eof; ///< Value of the runtime toggle. - bool session_tracking; ///< Value of the runtime toggle. uint64_t client_flag; ///< Extra `client_flag` the client advertises before connect. caps_expectation_t expectation; ///< Bits required/forbidden in the greeting. }; @@ -332,7 +334,7 @@ struct scenario_t { * @brief Execute every scenario once, emitting one TAP assertion per run. * * The function: - * - calls @ref apply_runtime_toggles to push the variables, + * - calls @ref apply_deprecate_eof to push the variable, * - calls @ref open_client to connect, * - reads `proxy->server_capabilities`, * - prints a full before/after trace via `diag()`, @@ -346,43 +348,21 @@ struct scenario_t { static int run_scenarios(const CommandLine& cl, MYSQL* admin) { const scenario_t scenarios[] = { { - "both_off", - /* deprecate_eof */ false, - /* session_tracking */ false, - /* client_flag */ 0, - /* expectation */ { + "deprecate_eof_off", + /* deprecate_eof */ false, + /* client_flag */ 0, + /* expectation */ { /* present */ {}, - /* absent */ { CLIENT_DEPRECATE_EOF, CLIENT_SESSION_TRACKING }, - }, - }, - { - "both_on", - /* deprecate_eof */ true, - /* session_tracking */ true, - /* client_flag */ CLIENT_DEPRECATE_EOF, - /* expectation */ { - /* present */ { CLIENT_DEPRECATE_EOF, CLIENT_SESSION_TRACKING }, - /* absent */ {}, + /* absent */ { CLIENT_DEPRECATE_EOF }, }, }, { - "eof_on_tracking_off", - /* deprecate_eof */ true, - /* session_tracking */ false, - /* client_flag */ CLIENT_DEPRECATE_EOF, - /* expectation */ { + "deprecate_eof_on", + /* deprecate_eof */ true, + /* client_flag */ CLIENT_DEPRECATE_EOF, + /* expectation */ { /* present */ { CLIENT_DEPRECATE_EOF }, - /* absent */ { CLIENT_SESSION_TRACKING }, - }, - }, - { - "eof_off_tracking_on", - /* deprecate_eof */ false, - /* session_tracking */ true, - /* client_flag */ 0, - /* expectation */ { - /* present */ { CLIENT_SESSION_TRACKING }, - /* absent */ { CLIENT_DEPRECATE_EOF }, + /* absent */ {}, }, }, }; @@ -391,14 +371,14 @@ static int run_scenarios(const CommandLine& cl, MYSQL* admin) { for (const scenario_t& s : scenarios) { ++scenario_idx; diag("================================================================"); - diag("Scenario %d/%zu: %s (deprecate_eof=%d, session_tracking=%d)", + diag("Scenario %d/%zu: %s (deprecate_eof=%d)", scenario_idx, sizeof(scenarios) / sizeof(scenarios[0]), - s.name, s.deprecate_eof ? 1 : 0, s.session_tracking ? 1 : 0); + s.name, s.deprecate_eof ? 1 : 0); diag("================================================================"); - if (!apply_runtime_toggles(admin, s.deprecate_eof, s.session_tracking)) { - ok(false, "scenario '%s': failed to apply runtime toggles", s.name); + if (!apply_deprecate_eof(admin, s.deprecate_eof)) { + ok(false, "scenario '%s': failed to apply runtime toggle", s.name); continue; } @@ -417,8 +397,8 @@ static int run_scenarios(const CommandLine& cl, MYSQL* admin) { const bool verdict = verify_caps(caps, s.expectation); ok(verdict, - "scenario '%s' (deprecate_eof=%d, session_tracking=%d): greeting capabilities match expectation", - s.name, s.deprecate_eof ? 1 : 0, s.session_tracking ? 1 : 0); + "scenario '%s' (deprecate_eof=%d): greeting capabilities match expectation", + s.name, s.deprecate_eof ? 1 : 0); mysql_close(proxy); } @@ -438,7 +418,7 @@ int main(int argc, char** argv) { CommandLine cl; - plan(4); + plan(2); if (cl.getEnv()) { diag("Failed to get the required environmental variables.");