Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/MySQL_HostGroups_Manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,15 @@ class MySrvC { // MySQL Server Container
bool shunned_and_kill_all_connections; // if a serious failure is detected, this will cause all connections to die even if the server is just shunned
int32_t use_ssl;
char *comment;

// '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;

Comment on lines +231 to +238
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 100

Repository: sysown/proxysql

Length of output: 10049


🏁 Script executed:

# Check the MySrvC class definition and surrounding code
fd "MySQL_HostGroups_Manager.h" --type f

Repository: sysown/proxysql

Length of output: 93


🏁 Script executed:

# Check context around the write in MySQL_Session.cpp
sed -n '8580,8605p' lib/MySQL_Session.cpp

Repository: sysown/proxysql

Length of output: 1180


🏁 Script executed:

# Check context around the read in MyHGC.cpp
sed -n '35,50p' lib/MyHGC.cpp

Repository: sysown/proxysql

Length of output: 985


🏁 Script executed:

# Check context around the read in MySQL_Thread.cpp
sed -n '5740,5755p' lib/MySQL_Thread.cpp

Repository: 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 -40

Repository: 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 -60

Repository: 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 };

MySrvConnList *ConnectionsUsed;
MySrvConnList *ConnectionsFree;
/**
Expand Down
82 changes: 82 additions & 0 deletions include/MySQL_Session.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,65 @@ class MySQL_Session: public Base_Session<MySQL_Session, MySQL_Data_Stream, MySQL
bool handler_again___verify_ldap_user_variable();
bool handler_again___verify_backend_autocommit();
bool handler_again___verify_backend_session_track_gtids();
/**
* @brief Verify and configure session variable tracking on backend connections.
*
* === PR 5166: Backend Configuration Entry Point ===
*
* This function is the main orchestrator for setting up session tracking on backend
* connections. It's called during connection initialization to ensure tracking is
* properly configured before query processing begins.
*
* CONFIGURATION LOGIC:
* 1. Check if global mysql-session_track_variables is enabled
* 2. For each tracking flag that's not yet set on the connection:
* - Mark the flag as sent (prevents duplicate configuration)
* - Transition session to appropriate configuration state
* - Return true to indicate state machine needs re-processing
*
* STATE MACHINE INTEGRATION:
* - SETTING_SESSION_TRACK_VARIABLES: Sets session_track_system_variables="*"
* - SETTING_SESSION_TRACK_STATE: Sets session_track_state_change=ON
* - Returns true to continue state machine processing until both are configured
*
* WHY THIS APPROACH:
* - Ensures tracking is configured exactly once per backend connection
* - Integrates cleanly with existing ProxySQL session state machine
* - Handles both tracking capabilities independently for flexibility
* - Prevents redundant SET commands on already configured connections
*
* @return true if session state needs to be re-processed (configuration pending), false otherwise
*/
bool handler_again___verify_backend_session_track_variables();
bool handler_again___verify_backend_multi_statement();
bool handler_again___verify_backend_user_schema();
bool handler_again___verify_multiple_variables(MySQL_Connection *);
bool handler_again___status_SETTING_INIT_CONNECT(int *);
bool handler_again___status_SETTING_LDAP_USER_VARIABLE(int *);
bool handler_again___status_SETTING_SQL_MODE(int *);
bool handler_again___status_SETTING_SESSION_TRACK_GTIDS(int *);
/**
* @brief Handle the SETTING_SESSION_TRACK_VARIABLES state.
*
* This method executes the SET command to configure session_track_system_variables="*"
* on the backend connection, enabling the server to track changes to all system
* variables and report them back to ProxySQL.
*
* @param _rc Pointer to return code that will be set with the operation result
* @return true if session state needs to be re-processed, false otherwise
*/
bool handler_again___status_SETTING_SESSION_TRACK_VARIABLES(int *);
/**
* @brief Handle the SETTING_SESSION_TRACK_STATE state.
*
* This method executes the SET command to configure session_track_state_change=ON
* on the backend connection, enabling the server to report when session state
* changes occur (including system variable changes).
*
* @param _rc Pointer to return code that will be set with the operation result
* @return true if session state needs to be re-processed, false otherwise
*/
bool handler_again___status_SETTING_SESSION_TRACK_STATE(int *);
bool handler_again___status_CHANGING_CHARSET(int *_rc);
bool handler_again___status_CHANGING_SCHEMA(int *);
bool handler_again___status_CONNECTING_SERVER(int *);
Expand Down Expand Up @@ -296,6 +348,35 @@ class MySQL_Session: public Base_Session<MySQL_Session, MySQL_Data_Stream, MySQL
int RunQuery(MySQL_Data_Stream *myds, MySQL_Connection *myconn);
void handler___status_WAITING_CLIENT_DATA();
void handler_rc0_Process_GTID(MySQL_Connection *myconn);
/**
* @brief Process session variable changes from backend connection response.
*
* === PR 5166: Variable Processing Workflow ===
*
* This function is the core of the variable tracking system and is called after
* every successful query execution when SERVER_SESSION_STATE_CHANGED flag is set.
*
* DETAILED WORKFLOW:
* 1. Extract variable changes from MySQL's session tracking data via get_variables()
* 2. Iterate through all tracked variables in mysql_tracked_variables array
* 3. For each variable that changed in the backend:
* - Update both client and server variable maps for state consistency
* - Handle character set variables specially (convert names to internal IDs)
* 4. This ensures ProxySQL's internal state matches the actual backend state
*
* WHY THIS IS NEEDED:
* - SQL statement parsing cannot detect all variable changes (e.g., stored procedures)
* - Some variables are changed implicitly by MySQL server operations
* - Without this tracking, client and backend states can diverge
*
* PERFORMANCE CONSIDERATIONS:
* - Only called when SERVER_SESSION_STATE_CHANGED flag is set
* - Processes all tracked variables but only updates those that actually changed
* - Character set conversions are done only for relevant variables
*
* @param myconn Pointer to the MySQL connection from which to extract variable changes
*/
void handler_rc0_Process_Variables(MySQL_Connection *myconn);
void handler_rc0_RefreshActiveTransactions(MySQL_Connection* myconn);
void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_INIT_DB_replace_CLICKHOUSE(PtrSize_t& pkt);
void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY___not_mysql(PtrSize_t& pkt);
Expand Down Expand Up @@ -489,6 +570,7 @@ class MySQL_Session: public Base_Session<MySQL_Session, MySQL_Data_Stream, MySQL
void reset_warning_hostgroup_flag_and_release_connection();
void set_previous_status_mode3(bool allow_execute=true);
char* get_current_query(int max_length = -1);
bool handle_session_track_capabilities();
/**
* @brief Attempts to get the server version string from a backend connection in the specified hostgroup.
* @details This function iterates through servers in the hostgroup and checks for any available
Expand Down
53 changes: 53 additions & 0 deletions include/MySQL_Thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,49 @@ struct th_metrics_map_idx {
};
};

/**
* @brief Configuration structure for session variable tracking functionality.
*
* === PR 5166: Session Variable Tracking Architecture ===
*
* This PR introduces comprehensive tracking of session-specific system variables
* by leveraging MySQL's native session tracking capabilities. The overall workflow
* consists of three main phases:
*
* 1. CONFIGURATION PHASE:
* - Global enable/disable via mysql-session_track_variables variable
* - Per-connection tracking state managed via flags in MySQL_Connection
* - New session states handle configuration of tracking on backends
*
* 2. BACKEND SETUP PHASE:
* - When enabled, configure session_track_system_variables="*"
* - Configure session_track_state_change=ON
* - MySQL server then automatically tracks all system variable changes
*
* 3. PROCESSING PHASE:
* - After each query, check SERVER_SESSION_STATE_CHANGED flag
* - Extract variable changes via MySQL's session tracking protocol
* - Update both client and server variable maps for state consistency
* - Special handling for character set variables (name → ID conversion)
*
* BENEFITS:
* - Captures changes that SQL parsing alone cannot detect (stored procedures, etc.)
* - Redundant tracking ensures accurate client/backend state synchronization
* - Leverages MySQL server's native capabilities for reliability
* - Performance optimized: only processes when session state actually changes
*/
struct session_track_variables {
enum mode {
// Disabled; default mode
DISABLED = 0,
// Enable session tracking if backend supports it
OPTIONAL,
// Enforce session tracking; connection fails if backend does
// not support CLIENT_DEPRECATE_EOF and CLIENT_SESSION_TRACKING
ENFORCED
};
};

/**
* @brief Structure holding the data for a Client_Host_Cache entry.
*/
Expand Down Expand Up @@ -591,6 +634,16 @@ class MySQL_Threads_Handler
int processlist_max_query_length;

bool ignore_min_gtid_annotations;
/**
* @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;
Comment on lines +637 to 647
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
/**
* @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.

struct {
unsigned int mirror_sessions_current;
Expand Down
34 changes: 33 additions & 1 deletion include/mysql_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ class MySQL_Connection {
char * session_track_gtids;
char *ldap_user_variable;
char *ldap_user_variable_value;
bool session_track_gtids_sent;
bool session_track_gtids_sent; ///< Flag indicating if GTID session tracking has been configured on this connection
bool session_track_variables_sent; ///< Flag indicating if session_track_system_variables has been configured on this connection
bool session_track_state_sent; ///< Flag indicating if session_track_state_change has been configured on this connection
bool ldap_user_variable_sent;
uint8_t protocol_version;
int8_t last_set_autocommit;
Expand Down Expand Up @@ -262,6 +264,36 @@ class MySQL_Connection {
void reset();

bool get_gtid(char *buff, uint64_t *trx_id);
/**
* @brief Extract session variable changes from MySQL's session tracking system.
*
* === PR 5166: Backend Variable Extraction ===
*
* This method is the interface to MySQL's native session tracking protocol and
* extracts system variable changes that occurred during the last query execution.
*
* TECHNICAL DETAILS:
* - Uses mysql_session_track_get_first/next() to iterate through SESSION_TRACK_SYSTEM_VARIABLES
* - Only processes when SERVER_SESSION_STATE_CHANGED flag is set and no errors occurred
* - Handles the variable/value pairing protocol where variables and values are returned alternately
* - Populates a map with variable names as keys and their new values as values
*
* PROTOCOL FLOW:
* 1. Check if session state changed and no errors occurred
* 2. Get first tracked system variable (name)
* 3. Iterate to get corresponding value, next variable name, etc.
* 4. Build variable_name → value mappings in the provided map
*
* INTEGRATION POINT:
* This is called by handler_rc0_Process_Variables() which then processes the
* extracted changes to update ProxySQL's internal state. The separation allows
* for testing and potential future extensions of the tracking protocol.
*
* @param variables Reference to unordered_map that will be populated with
* variable names as keys and their new values as values
* @return true if session variable changes were found and extracted, false otherwise
*/
bool get_variables(std::unordered_map<std::string, std::string>&);
void reduce_auto_increment_delay_token() { if (auto_increment_delay_token) auto_increment_delay_token--; };

bool match_ff_req_options(const MySQL_Connection *c);
Expand Down
4 changes: 4 additions & 0 deletions include/proxysql_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,8 @@ enum session_status {
SETTING_NEXT_TRANSACTION_READ,
PROCESSING_EXTENDED_QUERY_SYNC,
RESYNCHRONIZING_CONNECTION,
SETTING_SESSION_TRACK_VARIABLES,
SETTING_SESSION_TRACK_STATE,
session_status___NONE // special marker
};

Expand Down Expand Up @@ -1305,6 +1307,7 @@ __thread int mysql_thread___client_host_error_counts;
__thread int mysql_thread___handle_warnings;
__thread int mysql_thread___evaluate_replication_lag_on_servers_load;
__thread bool mysql_thread___ignore_min_gtid_annotations;
__thread int mysql_thread___session_track_variables;

/* variables used for Query Cache */
__thread int mysql_thread___query_cache_size_MB;
Expand Down Expand Up @@ -1609,6 +1612,7 @@ extern __thread int mysql_thread___client_host_error_counts;
extern __thread int mysql_thread___handle_warnings;
extern __thread int mysql_thread___evaluate_replication_lag_on_servers_load;
extern __thread bool mysql_thread___ignore_min_gtid_annotations;
extern __thread int mysql_thread___session_track_variables;

/* variables used for Query Cache */
extern __thread int mysql_thread___query_cache_size_MB;
Expand Down
4 changes: 4 additions & 0 deletions lib/MyHGC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ MySrvC *MyHGC::get_random_MySrvC(char * gtid_uuid, uint64_t gtid_trxid, int max_
for (j=0; j<l; j++) {
mysrvc=mysrvs->idx(j);
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;

Comment on lines 40 to +44
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files | head -20

Repository: sysown/proxysql

Length of output: 799


🏁 Script executed:

find . -name "*.h" -o -name "*.hpp" | grep -i "mysql\|hgc" | head -20

Repository: 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 -30

Repository: sysown/proxysql

Length of output: 309


🏁 Script executed:

cat -n lib/MyHGC.cpp | head -60

Repository: 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 -100

Repository: sysown/proxysql

Length of output: 2462


🏁 Script executed:

# Find all writes to server_backoff_time
rg "server_backoff_time\s*=" -B 3 -A 3

Repository: 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 -20

Repository: 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 -40

Repository: 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 2

Repository: sysown/proxysql

Length of output: 41


🏁 Script executed:

# Check MyHGC context - who calls get_random_MySrvC
rg "get_random_MySrvC" -B 5 | head -50

Repository: 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 -80

Repository: 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 -20

Repository: 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 -60

Repository: 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 -20

Repository: 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 -60

Repository: 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 5

Repository: 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 -80

Repository: 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 -50

Repository: 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 -80

Repository: 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 -80

Repository: sysown/proxysql

Length of output: 41


🏁 Script executed:

# Search more broadly for MySrvList
fd "MySQL_HostGroups_Manager" --type f

Repository: sysown/proxysql

Length of output: 126


🏁 Script executed:

# Try to find MySrvList definition
rg "class MySrvList" -A 50

Repository: 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 -100

Repository: 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 -40

Repository: 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 (mysrvc->myhgc->num_online_servers.load(std::memory_order_relaxed) <= mysrvc->myhgc->attributes.max_num_online_servers) { // number of online servers in HG is within configured range
if (mysrvc->ConnectionsUsed->conns_length() < mysrvc->max_connections) { // consider this server only if didn't reach max_connections
if (mysrvc->current_latency_us < (mysrvc->max_latency_us ? mysrvc->max_latency_us : mysql_thread___default_max_latency_ms*1000)) { // consider the host only if not too far
Expand Down
8 changes: 6 additions & 2 deletions lib/MySQL_Protocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1111,9 +1111,13 @@ bool MySQL_Protocol::generate_pkt_initial_handshake(bool send, void **ptr, unsig
// variable. This is the first step of ensuring that client connections doesn't
// enable 'CLIENT_DEPRECATE_EOF' unless explicitly stated by 'mysql-enable_client_deprecate_eof'.
// Second step occurs during client handshake response (process_pkt_handshake_response).
if (deprecate_eof_active && mysql_thread___enable_client_deprecate_eof) {
extended_capabilities |= CLIENT_DEPRECATE_EOF;
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;
}
}

Comment on lines +1114 to +1120
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "MySQL_Protocol.cpp" -type f

Repository: 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 -20

Repository: 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 -10

Repository: sysown/proxysql

Length of output: 309


🏁 Script executed:

# Find the full function context around line 1114-1120
sed -n '1100,1130p' lib/MySQL_Protocol.cpp

Repository: 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.cpp

Repository: 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

  1. MySQL OK_Packet documentation (explains OK used to indicate EOF and CLIENT_DEPRECATE_EOF flag). [dev.mysql.com]. [1]
  2. MySQL EOF_Packet / protocol notes. [dev.mysql.com]. [2]
  3. MySQL Session-State Tracking overview and session_track_* variables. [dev.mysql.com / Oracle MySQL manual]. [3][4]
  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.cpp

Repository: sysown/proxysql

Length of output: 1225


🏁 Script executed:

# Get more context to understand the full condition
sed -n '1580,1620p' lib/MySQL_Protocol.cpp

Repository: 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.

Suggested change
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.

// Copy the 'capability_flags_2'
uint16_t upper_word = static_cast<uint16_t>(extended_capabilities >> 16);
memcpy(_ptr+l, static_cast<void*>(&upper_word), sizeof(upper_word)); l += sizeof(upper_word);
Expand Down
Loading