Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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