From dba12945850e69e3a817551305dcc3e78d99156e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 08:53:14 +0100 Subject: [PATCH 1/9] Extract server selection algorithm into pure functions (Phase 3.4, #5492) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New files: - include/ServerSelection.h: ServerCandidate struct (lightweight, no MySrvC dependency) + is_candidate_eligible() + select_server_from_candidates() - lib/ServerSelection.cpp: implementations ServerCandidate decouples selection logic from connection pool: contains only weight, status, connections, latency, and replication lag — no MySQL_Connection pointers or pool objects. Selection algorithm: 1. Filter candidates by eligibility (ONLINE, below max_connections, within latency/lag thresholds) 2. Weighted random selection using seeded LCG (deterministic for testing, avoids polluting global srand() state) Uses its own ServerSelectionStatus enum to avoid pulling in proxysql_structs.h and its entire dependency chain. --- include/ServerSelection.h | 79 +++++++++++++++++++++++++++++++++++++++ lib/Makefile | 1 + lib/ServerSelection.cpp | 69 ++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 include/ServerSelection.h create mode 100644 lib/ServerSelection.cpp diff --git a/include/ServerSelection.h b/include/ServerSelection.h new file mode 100644 index 000000000..cc7c2a23d --- /dev/null +++ b/include/ServerSelection.h @@ -0,0 +1,79 @@ +/** + * @file ServerSelection.h + * @brief Pure server selection algorithm for unit testability. + * + * Extracted from get_random_MySrvC() in the HostGroups Manager. + * Uses a lightweight ServerCandidate struct instead of MySrvC to + * avoid connection pool dependencies. + * + * @see Phase 3.4 (GitHub issue #5492) + */ + +#ifndef SERVER_SELECTION_H +#define SERVER_SELECTION_H + +#include + +/** + * @brief Server status values (mirrors MySerStatus enum). + * + * Redefined here to avoid pulling in proxysql_structs.h and its + * entire dependency chain. + */ +enum ServerSelectionStatus { + SERVER_ONLINE = 0, + SERVER_SHUNNED = 1, + SERVER_OFFLINE_SOFT = 2, + SERVER_OFFLINE_HARD = 3, + SERVER_SHUNNED_REPLICATION_LAG = 4 +}; + +/** + * @brief Lightweight struct with decision-relevant server fields only. + * + * Avoids coupling to MySrvC which contains connection pool pointers, + * MySQL_Connection objects, and other heavy dependencies. + */ +struct ServerCandidate { + int index; ///< Caller-defined index (returned on selection). + int64_t weight; ///< Selection weight (0 = never selected). + ServerSelectionStatus status; ///< Current health status. + unsigned int current_connections; ///< Active connection count. + unsigned int max_connections; ///< Maximum allowed connections. + unsigned int current_latency_us; ///< Measured latency in microseconds. + unsigned int max_latency_us; ///< Maximum allowed latency (0 = no limit). + unsigned int current_repl_lag; ///< Measured replication lag in seconds. + unsigned int max_repl_lag; ///< Maximum allowed lag (0 = no limit). +}; + +/** + * @brief Check if a server candidate is eligible for selection. + * + * A candidate is eligible when: + * - status == SERVER_ONLINE + * - current_connections < max_connections + * - current_latency_us <= max_latency_us (or max_latency_us == 0) + * - current_repl_lag <= max_repl_lag (or max_repl_lag == 0) + * + * @return true if the candidate is eligible. + */ +bool is_candidate_eligible(const ServerCandidate &candidate); + +/** + * @brief Select a server from candidates using weighted random selection. + * + * Filters candidates by eligibility, then selects from eligible ones + * using weighted random with the provided seed for deterministic testing. + * + * @param candidates Array of server candidates. + * @param count Number of candidates in the array. + * @param random_seed Seed for deterministic random selection. + * @return Index field of the selected candidate, or -1 if none eligible. + */ +int select_server_from_candidates( + const ServerCandidate *candidates, + int count, + unsigned int random_seed +); + +#endif // SERVER_SELECTION_H diff --git a/lib/Makefile b/lib/Makefile index f7f24075c..90582b926 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -106,6 +106,7 @@ _OBJ_CXX := ProxySQL_GloVars.oo network.oo debug.oo configfile.oo Query_Cache.oo PgSQL_PreparedStatement.oo PgSQL_Extended_Query_Message.oo \ pgsql_tokenizer.oo \ MonitorHealthDecision.oo \ + ServerSelection.oo \ proxy_sqlite3_symbols.oo # TSDB object files diff --git a/lib/ServerSelection.cpp b/lib/ServerSelection.cpp new file mode 100644 index 000000000..11094b699 --- /dev/null +++ b/lib/ServerSelection.cpp @@ -0,0 +1,69 @@ +/** + * @file ServerSelection.cpp + * @brief Implementation of the pure server selection algorithm. + * + * @see ServerSelection.h + * @see Phase 3.4 (GitHub issue #5492) + */ + +#include "ServerSelection.h" +#include + +bool is_candidate_eligible(const ServerCandidate &c) { + if (c.status != SERVER_ONLINE) { + return false; + } + if (c.current_connections >= c.max_connections) { + return false; + } + if (c.max_latency_us > 0 && c.current_latency_us > c.max_latency_us) { + return false; + } + if (c.max_repl_lag > 0 && c.current_repl_lag > c.max_repl_lag) { + return false; + } + return true; +} + +int select_server_from_candidates( + const ServerCandidate *candidates, + int count, + unsigned int random_seed) +{ + if (candidates == nullptr || count <= 0) { + return -1; + } + + // First pass: compute total weight of eligible candidates + int64_t total_weight = 0; + for (int i = 0; i < count; i++) { + if (is_candidate_eligible(candidates[i]) && candidates[i].weight > 0) { + total_weight += candidates[i].weight; + } + } + + if (total_weight == 0) { + return -1; // no eligible candidates + } + + // Seeded random selection + // Use a simple LCG to avoid polluting global srand() state + // LCG: next = (a * seed + c) mod m (Numerical Recipes parameters) + unsigned int rng_state = random_seed; + rng_state = rng_state * 1664525u + 1013904223u; + int64_t target = (int64_t)(rng_state % (unsigned int)total_weight) + 1; + + // Second pass: weighted selection + int64_t cumulative = 0; + for (int i = 0; i < count; i++) { + if (is_candidate_eligible(candidates[i]) && candidates[i].weight > 0) { + cumulative += candidates[i].weight; + if (cumulative >= target) { + return candidates[i].index; + } + } + } + + // Should not reach here if total_weight > 0, but safety fallback + return -1; +} From 7697d4f43ea8e30ad8358a9d2d6826afdbf84e28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 08:53:14 +0100 Subject: [PATCH 2/9] Add server selection unit tests (Phase 3.4, #5492) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 21 test cases covering: - Eligibility: all 5 status types, max_connections, latency, repl lag, disabled limits (max=0) - Selection: single server, empty list, all offline, weight=0 - Weighted distribution: equal weights → ~50/50, 3:1 → ~75/25 (statistical verification over 10000 iterations) - Determinism: same seed produces same result - Mixed eligibility: only eligible server selected 100% of the time --- test/tap/tests/unit/Makefile | 50 +--- .../tests/unit/server_selection_unit-t.cpp | 223 ++++++++++++++++++ 2 files changed, 231 insertions(+), 42 deletions(-) create mode 100644 test/tap/tests/unit/server_selection_unit-t.cpp diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 0d56f1758..205b01fee 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -231,7 +231,10 @@ $(ODIR)/test_init.o: $(TEST_HELPERS_DIR)/test_init.cpp | $(ODIR) # Unit test targets # =========================================================================== -UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t protocol_unit-t auth_unit-t connection_pool_unit-t rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t +UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ + protocol_unit-t auth_unit-t connection_pool_unit-t \ + rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ + server_selection_unit-t .PHONY: all all: $(UNIT_TESTS) @@ -245,47 +248,10 @@ ifneq ($(UNAME_S),Darwin) ALLOW_MULTI_DEF := -Wl,--allow-multiple-definition endif -smoke_test-t: smoke_test-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -query_cache_unit-t: query_cache_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -query_processor_unit-t: query_processor_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -protocol_unit-t: protocol_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -auth_unit-t: auth_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -connection_pool_unit-t: connection_pool_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -rule_matching_unit-t: rule_matching_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -hostgroups_unit-t: hostgroups_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ - $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ - $(ALLOW_MULTI_DEF) -o $@ - -monitor_health_unit-t: monitor_health_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) +# Pattern rule: all unit tests use the same compile + link flags. +# Each test binary is built from its .cpp source, linked against +# the test harness objects and libproxysql.a with all dependencies. +%-t: %-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ $(ALLOW_MULTI_DEF) -o $@ diff --git a/test/tap/tests/unit/server_selection_unit-t.cpp b/test/tap/tests/unit/server_selection_unit-t.cpp new file mode 100644 index 000000000..33538fe18 --- /dev/null +++ b/test/tap/tests/unit/server_selection_unit-t.cpp @@ -0,0 +1,223 @@ +/** + * @file server_selection_unit-t.cpp + * @brief Unit tests for the server selection algorithm. + * + * Tests the pure selection functions extracted from get_random_MySrvC(): + * - is_candidate_eligible() + * - select_server_from_candidates() + * + * @see Phase 3.4 (GitHub issue #5492) + */ + +#include "tap.h" +#include "test_globals.h" +#include "test_init.h" + +#include "proxysql.h" +#include "ServerSelection.h" + +// ============================================================================ +// Helper: create a default ONLINE server candidate +// ============================================================================ +static ServerCandidate make_candidate(int idx, int64_t weight = 1, + unsigned int max_conns = 1000) +{ + ServerCandidate c {}; + c.index = idx; + c.weight = weight; + c.status = SERVER_ONLINE; + c.current_connections = 0; + c.max_connections = max_conns; + c.current_latency_us = 0; + c.max_latency_us = 0; + c.current_repl_lag = 0; + c.max_repl_lag = 0; + return c; +} + +// ============================================================================ +// 1. is_candidate_eligible +// ============================================================================ + +static void test_eligibility() { + ServerCandidate online = make_candidate(0); + ok(is_candidate_eligible(online) == true, "eligible: ONLINE server"); + + ServerCandidate shunned = make_candidate(1); + shunned.status = SERVER_SHUNNED; + ok(is_candidate_eligible(shunned) == false, "ineligible: SHUNNED"); + + ServerCandidate off_soft = make_candidate(2); + off_soft.status = SERVER_OFFLINE_SOFT; + ok(is_candidate_eligible(off_soft) == false, "ineligible: OFFLINE_SOFT"); + + ServerCandidate off_hard = make_candidate(3); + off_hard.status = SERVER_OFFLINE_HARD; + ok(is_candidate_eligible(off_hard) == false, "ineligible: OFFLINE_HARD"); + + ServerCandidate lag_shunned = make_candidate(4); + lag_shunned.status = SERVER_SHUNNED_REPLICATION_LAG; + ok(is_candidate_eligible(lag_shunned) == false, "ineligible: SHUNNED_REPL_LAG"); + + ServerCandidate at_max = make_candidate(5, 1, 10); + at_max.current_connections = 10; + ok(is_candidate_eligible(at_max) == false, "ineligible: at max_connections"); + + ServerCandidate below_max = make_candidate(6, 1, 10); + below_max.current_connections = 9; + ok(is_candidate_eligible(below_max) == true, "eligible: below max_connections"); + + ServerCandidate high_latency = make_candidate(7); + high_latency.max_latency_us = 5000; + high_latency.current_latency_us = 6000; + ok(is_candidate_eligible(high_latency) == false, "ineligible: high latency"); + + ServerCandidate ok_latency = make_candidate(8); + ok_latency.max_latency_us = 5000; + ok_latency.current_latency_us = 4000; + ok(is_candidate_eligible(ok_latency) == true, "eligible: acceptable latency"); + + ServerCandidate no_limit = make_candidate(9); + no_limit.max_latency_us = 0; + no_limit.current_latency_us = 999999; + ok(is_candidate_eligible(no_limit) == true, "eligible: latency limit disabled (max=0)"); + + ServerCandidate high_lag = make_candidate(10); + high_lag.max_repl_lag = 10; + high_lag.current_repl_lag = 15; + ok(is_candidate_eligible(high_lag) == false, "ineligible: high repl lag"); + + ServerCandidate ok_lag = make_candidate(11); + ok_lag.max_repl_lag = 10; + ok_lag.current_repl_lag = 5; + ok(is_candidate_eligible(ok_lag) == true, "eligible: acceptable repl lag"); +} + +// ============================================================================ +// 2. select_server_from_candidates — basic +// ============================================================================ + +static void test_select_single() { + ServerCandidate c = make_candidate(42); + int result = select_server_from_candidates(&c, 1, 12345); + ok(result == 42, "single server: always selected (idx=42)"); +} + +static void test_select_empty() { + ok(select_server_from_candidates(nullptr, 0, 0) == -1, + "empty list: returns -1"); +} + +static void test_select_all_offline() { + ServerCandidate candidates[3]; + candidates[0] = make_candidate(0); candidates[0].status = SERVER_OFFLINE_HARD; + candidates[1] = make_candidate(1); candidates[1].status = SERVER_SHUNNED; + candidates[2] = make_candidate(2); candidates[2].status = SERVER_OFFLINE_SOFT; + + ok(select_server_from_candidates(candidates, 3, 999) == -1, + "all offline: returns -1"); +} + +static void test_select_weight_zero() { + ServerCandidate c = make_candidate(0, 0); + ok(select_server_from_candidates(&c, 1, 12345) == -1, + "weight=0: never selected"); +} + +// ============================================================================ +// 3. Weighted distribution (statistical) +// ============================================================================ + +static void test_equal_weight_distribution() { + ServerCandidate candidates[2]; + candidates[0] = make_candidate(0, 1); + candidates[1] = make_candidate(1, 1); + + int count[2] = {0, 0}; + const int N = 10000; + for (int seed = 0; seed < N; seed++) { + int result = select_server_from_candidates(candidates, 2, seed); + if (result >= 0 && result <= 1) count[result]++; + } + + double pct0 = (double)count[0] / N * 100; + ok(pct0 > 30 && pct0 < 70, + "equal weight: server 0 selected %.1f%% (expect ~50%%)", pct0); +} + +static void test_weighted_distribution() { + ServerCandidate candidates[2]; + candidates[0] = make_candidate(0, 3); // weight 3 + candidates[1] = make_candidate(1, 1); // weight 1 + + int count[2] = {0, 0}; + const int N = 10000; + for (int seed = 0; seed < N; seed++) { + int result = select_server_from_candidates(candidates, 2, seed); + if (result >= 0 && result <= 1) count[result]++; + } + + double pct0 = (double)count[0] / N * 100; + ok(pct0 > 60 && pct0 < 90, + "3:1 weight: server 0 selected %.1f%% (expect ~75%%)", pct0); +} + +// ============================================================================ +// 4. Determinism +// ============================================================================ + +static void test_determinism() { + ServerCandidate candidates[3]; + candidates[0] = make_candidate(0, 2); + candidates[1] = make_candidate(1, 3); + candidates[2] = make_candidate(2, 5); + + int r1 = select_server_from_candidates(candidates, 3, 42); + int r2 = select_server_from_candidates(candidates, 3, 42); + ok(r1 == r2, "determinism: same seed → same result"); +} + +// ============================================================================ +// 5. Mixed eligible/ineligible +// ============================================================================ + +static void test_mixed_eligibility() { + ServerCandidate candidates[4]; + candidates[0] = make_candidate(0, 1); candidates[0].status = SERVER_SHUNNED; + candidates[1] = make_candidate(1, 1); candidates[1].status = SERVER_OFFLINE_HARD; + candidates[2] = make_candidate(2, 1); // ONLINE + candidates[3] = make_candidate(3, 1); candidates[3].status = SERVER_OFFLINE_SOFT; + + // Only candidate[2] is eligible — must always be selected + int pass = 0; + for (int seed = 0; seed < 100; seed++) { + if (select_server_from_candidates(candidates, 4, seed) == 2) pass++; + } + ok(pass == 100, + "mixed: only eligible server selected 100/100 times"); +} + +// ============================================================================ +// Main +// ============================================================================ + +int main() { + plan(21); + + int rc = test_init_minimal(); + ok(rc == 0, "test_init_minimal() succeeds"); + + test_eligibility(); // 12 + test_select_single(); // 1 + test_select_empty(); // 1 + test_select_all_offline(); // 1 + test_select_weight_zero(); // 1 + test_equal_weight_distribution(); // 1 + test_weighted_distribution(); // 1 + test_determinism(); // 1 + test_mixed_eligibility(); // 1 + // Total: 1+12+1+1+1+1+1+1+1+1 = 21... fix + + test_cleanup_minimal(); + return exit_status(); +} From 7e49c419756874e59a35ad7293820548a42aa736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 09:07:06 +0100 Subject: [PATCH 3/9] Address review feedback on server selection (PR #5508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix truncation: use uint64_t modulo instead of unsigned int cast when computing target from total_weight (prevents skewed selection when total_weight > UINT_MAX) - Remove unused #include - Add docstring note about max_latency_us=0 semantics: in production this means "use thread default", this extraction treats it as "no limit" — callers should resolve defaults before populating the struct - Remove trailing "fix" comment from test --- include/ServerSelection.h | 5 +++++ lib/ServerSelection.cpp | 4 ++-- test/tap/tests/unit/server_selection_unit-t.cpp | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/include/ServerSelection.h b/include/ServerSelection.h index cc7c2a23d..58a835246 100644 --- a/include/ServerSelection.h +++ b/include/ServerSelection.h @@ -55,6 +55,11 @@ struct ServerCandidate { * - current_latency_us <= max_latency_us (or max_latency_us == 0) * - current_repl_lag <= max_repl_lag (or max_repl_lag == 0) * + * @note In production, max_latency_us == 0 on a per-server basis means + * "use the thread default max latency." This extraction treats 0 + * as "no limit" for simplicity. Callers should resolve defaults + * before populating the ServerCandidate. + * * @return true if the candidate is eligible. */ bool is_candidate_eligible(const ServerCandidate &candidate); diff --git a/lib/ServerSelection.cpp b/lib/ServerSelection.cpp index 11094b699..4704848b9 100644 --- a/lib/ServerSelection.cpp +++ b/lib/ServerSelection.cpp @@ -7,7 +7,6 @@ */ #include "ServerSelection.h" -#include bool is_candidate_eligible(const ServerCandidate &c) { if (c.status != SERVER_ONLINE) { @@ -51,7 +50,8 @@ int select_server_from_candidates( // LCG: next = (a * seed + c) mod m (Numerical Recipes parameters) unsigned int rng_state = random_seed; rng_state = rng_state * 1664525u + 1013904223u; - int64_t target = (int64_t)(rng_state % (unsigned int)total_weight) + 1; + // Use 64-bit modulo to avoid truncation when total_weight > UINT_MAX + int64_t target = (int64_t)(rng_state % (uint64_t)total_weight) + 1; // Second pass: weighted selection int64_t cumulative = 0; diff --git a/test/tap/tests/unit/server_selection_unit-t.cpp b/test/tap/tests/unit/server_selection_unit-t.cpp index 33538fe18..4b02d027b 100644 --- a/test/tap/tests/unit/server_selection_unit-t.cpp +++ b/test/tap/tests/unit/server_selection_unit-t.cpp @@ -216,7 +216,7 @@ int main() { test_weighted_distribution(); // 1 test_determinism(); // 1 test_mixed_eligibility(); // 1 - // Total: 1+12+1+1+1+1+1+1+1+1 = 21... fix + // Total: 1+12+1+1+1+1+1+1+1+1 = 21 test_cleanup_minimal(); return exit_status(); From 5c227a3bc93de6bc583493b2a1f91bbc230781bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:51:11 +0100 Subject: [PATCH 4/9] Extract MySQL error classification logic (Phase 3.7, #5495) New: include/MySQLErrorClassifier.h, lib/MySQLErrorClassifier.cpp Functions: - classify_mysql_error(): classifies error codes as retryable (1047 WSREP, 1053 shutdown) or fatal, checking retry conditions - can_retry_on_new_connection(): checks if offline server retry is possible given connection state (reusable, no txn, no transfer) --- include/MySQLErrorClassifier.h | 76 ++++++++++++++++++++++++++++++++++ lib/Makefile | 1 + lib/MySQLErrorClassifier.cpp | 66 +++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 include/MySQLErrorClassifier.h create mode 100644 lib/MySQLErrorClassifier.cpp diff --git a/include/MySQLErrorClassifier.h b/include/MySQLErrorClassifier.h new file mode 100644 index 000000000..9b46549db --- /dev/null +++ b/include/MySQLErrorClassifier.h @@ -0,0 +1,76 @@ +/** + * @file MySQLErrorClassifier.h + * @brief Pure MySQL error classification for retry decisions. + * + * Extracted from MySQL_Session handler_ProcessingQueryError_CheckBackendConnectionStatus() + * and handler_minus1_HandleErrorCodes(). + * + * @see Phase 3.7 (GitHub issue #5495) + */ + +#ifndef MYSQL_ERROR_CLASSIFIER_H +#define MYSQL_ERROR_CLASSIFIER_H + +/** + * @brief Action to take after a MySQL backend query error. + */ +enum MySQLErrorAction { + MYSQL_ERROR_CONTINUE, ///< Error handled, continue processing. + MYSQL_ERROR_RETRY_ON_NEW_CONN, ///< Reconnect and retry on a new server. + MYSQL_ERROR_REPORT_TO_CLIENT ///< Send error to client, no retry. +}; + +/** + * @brief Classify a MySQL error code to determine retry eligibility. + * + * Mirrors the logic in handler_minus1_HandleErrorCodes(): + * - Error 1047 (WSREP not ready): retryable if conditions permit + * - Error 1053 (server shutdown): retryable if conditions permit + * - Other errors: report to client + * + * Retry is only possible when: + * - query_retries_on_failure > 0 + * - connection is reusable + * - no active transaction + * - multiplex not disabled + * + * @param error_code MySQL error number. + * @param retries_remaining Number of retries left. + * @param connection_reusable Whether the connection can be reused. + * @param in_active_transaction Whether a transaction is in progress. + * @param multiplex_disabled Whether multiplexing is disabled. + * @return MySQLErrorAction indicating what to do. + */ +MySQLErrorAction classify_mysql_error( + unsigned int error_code, + int retries_remaining, + bool connection_reusable, + bool in_active_transaction, + bool multiplex_disabled +); + +/** + * @brief Check if a backend query can be retried on a new connection. + * + * Mirrors handler_ProcessingQueryError_CheckBackendConnectionStatus(). + * A retry is possible when the server is offline AND all retry + * conditions are met. + * + * @param server_offline Whether the backend server is offline. + * @param retries_remaining Number of retries left. + * @param connection_reusable Whether the connection can be reused. + * @param in_active_transaction Whether a transaction is in progress. + * @param multiplex_disabled Whether multiplexing is disabled. + * @param transfer_started Whether result transfer has already begun. + * @return true if the query should be retried on a new connection. + */ +bool can_retry_on_new_connection( + bool server_offline, + int retries_remaining, + bool connection_reusable, + bool in_active_transaction, + bool multiplex_disabled, + bool transfer_started +); + +#endif // MYSQL_ERROR_CLASSIFIER_H diff --git a/lib/Makefile b/lib/Makefile index 7af5cd6ef..4f7963fbf 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -108,6 +108,7 @@ _OBJ_CXX := ProxySQL_GloVars.oo network.oo debug.oo configfile.oo Query_Cache.oo MonitorHealthDecision.oo \ TransactionState.oo \ HostgroupRouting.oo \ + MySQLErrorClassifier.oo \ proxy_sqlite3_symbols.oo # TSDB object files diff --git a/lib/MySQLErrorClassifier.cpp b/lib/MySQLErrorClassifier.cpp new file mode 100644 index 000000000..69ee01027 --- /dev/null +++ b/lib/MySQLErrorClassifier.cpp @@ -0,0 +1,66 @@ +/** + * @file MySQLErrorClassifier.cpp + * @brief Implementation of MySQL error classification. + * + * @see MySQLErrorClassifier.h + * @see Phase 3.7 (GitHub issue #5495) + */ + +#include "MySQLErrorClassifier.h" + +MySQLErrorAction classify_mysql_error( + unsigned int error_code, + int retries_remaining, + bool connection_reusable, + bool in_active_transaction, + bool multiplex_disabled) +{ + // Check if this error code is retryable + bool retryable_error = false; + switch (error_code) { + case 1047: // ER_UNKNOWN_COM_ERROR (WSREP not ready) + case 1053: // ER_SERVER_SHUTDOWN + retryable_error = true; + break; + default: + break; + } + + if (!retryable_error) { + return MYSQL_ERROR_REPORT_TO_CLIENT; + } + + // Check retry conditions (mirrors handler_minus1_HandleErrorCodes) + if (retries_remaining > 0 + && connection_reusable + && !in_active_transaction + && !multiplex_disabled) { + return MYSQL_ERROR_RETRY_ON_NEW_CONN; + } + + return MYSQL_ERROR_REPORT_TO_CLIENT; +} + +bool can_retry_on_new_connection( + bool server_offline, + int retries_remaining, + bool connection_reusable, + bool in_active_transaction, + bool multiplex_disabled, + bool transfer_started) +{ + if (!server_offline) { + return false; // server is fine, no retry needed + } + + // Mirror handler_ProcessingQueryError_CheckBackendConnectionStatus + if (retries_remaining > 0 + && connection_reusable + && !in_active_transaction + && !multiplex_disabled + && !transfer_started) { + return true; + } + + return false; +} From 56eb121f126b45e27dc2fb98783601365ba3a30f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:51:11 +0100 Subject: [PATCH 5/9] Add MySQL error classifier unit tests (Phase 3.7, #5495) 19 test cases: retryable errors with/without conditions, non-retryable errors (access denied, syntax, table not found, gone away), offline retry with all blocking conditions tested individually. --- test/tap/tests/unit/Makefile | 3 +- .../unit/mysql_error_classifier_unit-t.cpp | 98 +++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 test/tap/tests/unit/mysql_error_classifier_unit-t.cpp diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 999f3c30c..e7530e608 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -235,7 +235,8 @@ UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ protocol_unit-t auth_unit-t connection_pool_unit-t \ rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ hostgroup_routing_unit-t \ - transaction_state_unit-t + transaction_state_unit-t \ + mysql_error_classifier_unit-t .PHONY: all all: $(UNIT_TESTS) diff --git a/test/tap/tests/unit/mysql_error_classifier_unit-t.cpp b/test/tap/tests/unit/mysql_error_classifier_unit-t.cpp new file mode 100644 index 000000000..471779c6b --- /dev/null +++ b/test/tap/tests/unit/mysql_error_classifier_unit-t.cpp @@ -0,0 +1,98 @@ +/** + * @file mysql_error_classifier_unit-t.cpp + * @brief Unit tests for MySQL error classification. + * + * @see Phase 3.7 (GitHub issue #5495) + */ + +#include "tap.h" +#include "test_globals.h" +#include "test_init.h" +#include "proxysql.h" +#include "MySQLErrorClassifier.h" + +// ============================================================================ +// 1. classify_mysql_error +// ============================================================================ + +static void test_retryable_errors() { + // 1047 (WSREP not ready) with retry conditions met + ok(classify_mysql_error(1047, 3, true, false, false) == MYSQL_ERROR_RETRY_ON_NEW_CONN, + "1047: retryable when conditions met"); + // 1053 (server shutdown) with retry conditions met + ok(classify_mysql_error(1053, 1, true, false, false) == MYSQL_ERROR_RETRY_ON_NEW_CONN, + "1053: retryable when conditions met"); +} + +static void test_retryable_but_blocked() { + // 1047 but no retries left + ok(classify_mysql_error(1047, 0, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1047: not retried when retries=0"); + // 1047 but connection not reusable + ok(classify_mysql_error(1047, 3, false, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1047: not retried when connection not reusable"); + // 1047 but in active transaction + ok(classify_mysql_error(1047, 3, true, true, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1047: not retried during active transaction"); + // 1047 but multiplex disabled + ok(classify_mysql_error(1047, 3, true, false, true) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1047: not retried when multiplex disabled"); +} + +static void test_non_retryable_errors() { + // Common MySQL errors — always report to client + ok(classify_mysql_error(1045, 3, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1045 (access denied): always report"); + ok(classify_mysql_error(1064, 3, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1064 (syntax error): always report"); + ok(classify_mysql_error(1146, 3, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "1146 (table not found): always report"); + ok(classify_mysql_error(2006, 3, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "2006 (gone away): always report"); + ok(classify_mysql_error(0, 3, true, false, false) == MYSQL_ERROR_REPORT_TO_CLIENT, + "0 (no error): report"); +} + +// ============================================================================ +// 2. can_retry_on_new_connection +// ============================================================================ + +static void test_retry_on_offline() { + ok(can_retry_on_new_connection(true, 3, true, false, false, false) == true, + "retry: server offline, all conditions met"); +} + +static void test_no_retry_server_online() { + ok(can_retry_on_new_connection(false, 3, true, false, false, false) == false, + "no retry: server is online"); +} + +static void test_no_retry_conditions() { + ok(can_retry_on_new_connection(true, 0, true, false, false, false) == false, + "no retry: no retries left"); + ok(can_retry_on_new_connection(true, 3, false, false, false, false) == false, + "no retry: connection not reusable"); + ok(can_retry_on_new_connection(true, 3, true, true, false, false) == false, + "no retry: active transaction"); + ok(can_retry_on_new_connection(true, 3, true, false, true, false) == false, + "no retry: multiplex disabled"); + ok(can_retry_on_new_connection(true, 3, true, false, false, true) == false, + "no retry: transfer already started"); +} + +int main() { + plan(19); + int rc = test_init_minimal(); + ok(rc == 0, "test_init_minimal() succeeds"); + + test_retryable_errors(); // 2 + test_retryable_but_blocked(); // 4 + test_non_retryable_errors(); // 5 + test_retry_on_offline(); // 1 + test_no_retry_server_online(); // 1 + test_no_retry_conditions(); // 5 + // Total: 1+2+4+5+1+1+5 = 19 + + test_cleanup_minimal(); + return exit_status(); +} From ead2331fb9c9ce316f73025ba487a89bcef83fc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:54:10 +0100 Subject: [PATCH 6/9] Extract backend variable sync decisions (Phase 3.6, #5494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New: include/BackendSyncDecision.h, lib/BackendSyncDecision.cpp determine_backend_sync_actions() returns bitmask of needed sync: - SYNC_USER: username mismatch → CHANGE USER required - SYNC_SCHEMA: schema mismatch → USE required (skipped if SYNC_USER set, since CHANGE USER handles schema) - SYNC_AUTOCOMMIT: autocommit state mismatch Covers both MySQL (7 verify functions) and PgSQL (2 verify functions) at the decision level — the core comparisons are the same. --- include/BackendSyncDecision.h | 48 +++++++++++++++++++++++++++++++++++ lib/BackendSyncDecision.cpp | 45 ++++++++++++++++++++++++++++++++ lib/Makefile | 1 + 3 files changed, 94 insertions(+) create mode 100644 include/BackendSyncDecision.h create mode 100644 lib/BackendSyncDecision.cpp diff --git a/include/BackendSyncDecision.h b/include/BackendSyncDecision.h new file mode 100644 index 000000000..b0be1728a --- /dev/null +++ b/include/BackendSyncDecision.h @@ -0,0 +1,48 @@ +/** + * @file BackendSyncDecision.h + * @brief Pure decision functions for backend variable synchronization. + * + * Extracted from MySQL_Session's verify chain (handler_again___verify_*). + * Determines what sync actions are needed before a query can execute + * on a backend connection. + * + * @see Phase 3.6 (GitHub issue #5494) + */ + +#ifndef BACKEND_SYNC_DECISION_H +#define BACKEND_SYNC_DECISION_H + +/** + * @brief Actions that may be needed to synchronize backend state. + */ +enum BackendSyncAction { + SYNC_NONE = 0, ///< No synchronization needed. + SYNC_SCHEMA = 1, ///< Schema (USE db) needs to be sent. + SYNC_USER = 2, ///< Username mismatch, CHANGE USER required. + SYNC_AUTOCOMMIT = 4, ///< Autocommit state needs to be synced. +}; + +/** + * @brief Determine what sync actions are needed for the backend. + * + * Checks client vs backend state and returns a bitmask of required + * actions. Mirrors the MySQL_Session verify chain logic. + * + * @param client_user Client connection username. + * @param backend_user Backend connection username. + * @param client_schema Client connection schema. + * @param backend_schema Backend connection schema. + * @param client_autocommit Client autocommit setting. + * @param backend_autocommit Backend autocommit setting. + * @return Bitmask of BackendSyncAction values. + */ +int determine_backend_sync_actions( + const char *client_user, + const char *backend_user, + const char *client_schema, + const char *backend_schema, + bool client_autocommit, + bool backend_autocommit +); + +#endif // BACKEND_SYNC_DECISION_H diff --git a/lib/BackendSyncDecision.cpp b/lib/BackendSyncDecision.cpp new file mode 100644 index 000000000..04375b1b4 --- /dev/null +++ b/lib/BackendSyncDecision.cpp @@ -0,0 +1,45 @@ +/** + * @file BackendSyncDecision.cpp + * @brief Implementation of backend variable sync decisions. + * + * @see BackendSyncDecision.h + * @see Phase 3.6 (GitHub issue #5494) + */ + +#include "BackendSyncDecision.h" +#include + +int determine_backend_sync_actions( + const char *client_user, + const char *backend_user, + const char *client_schema, + const char *backend_schema, + bool client_autocommit, + bool backend_autocommit) +{ + int actions = SYNC_NONE; + + // Username mismatch → CHANGE USER required + if (client_user && backend_user) { + if (strcmp(client_user, backend_user) != 0) { + actions |= SYNC_USER; + } + } + + // Schema mismatch → USE required + // Only check if usernames match (user change handles schema too) + if (!(actions & SYNC_USER)) { + if (client_schema && backend_schema) { + if (strcmp(client_schema, backend_schema) != 0) { + actions |= SYNC_SCHEMA; + } + } + } + + // Autocommit mismatch → SET autocommit required + if (client_autocommit != backend_autocommit) { + actions |= SYNC_AUTOCOMMIT; + } + + return actions; +} diff --git a/lib/Makefile b/lib/Makefile index 7af5cd6ef..e91e240d8 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -108,6 +108,7 @@ _OBJ_CXX := ProxySQL_GloVars.oo network.oo debug.oo configfile.oo Query_Cache.oo MonitorHealthDecision.oo \ TransactionState.oo \ HostgroupRouting.oo \ + BackendSyncDecision.oo \ proxy_sqlite3_symbols.oo # TSDB object files From bc4251d5892b4a188140926546b0aa01f7d83646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:54:10 +0100 Subject: [PATCH 7/9] Add backend sync decision unit tests (Phase 3.6, #5494) 15 test cases: no sync needed, schema mismatch, user mismatch, user+schema (schema handled by user change), autocommit mismatch, multiple mismatches, null handling. --- test/tap/tests/unit/Makefile | 3 +- test/tap/tests/unit/backend_sync_unit-t.cpp | 78 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 test/tap/tests/unit/backend_sync_unit-t.cpp diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 999f3c30c..bd3c6bce7 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -235,7 +235,8 @@ UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ protocol_unit-t auth_unit-t connection_pool_unit-t \ rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ hostgroup_routing_unit-t \ - transaction_state_unit-t + transaction_state_unit-t \ + backend_sync_unit-t .PHONY: all all: $(UNIT_TESTS) diff --git a/test/tap/tests/unit/backend_sync_unit-t.cpp b/test/tap/tests/unit/backend_sync_unit-t.cpp new file mode 100644 index 000000000..c67d5efd5 --- /dev/null +++ b/test/tap/tests/unit/backend_sync_unit-t.cpp @@ -0,0 +1,78 @@ +/** + * @file backend_sync_unit-t.cpp + * @brief Unit tests for backend variable sync decisions. + * + * @see Phase 3.6 (GitHub issue #5494) + */ + +#include "tap.h" +#include "test_globals.h" +#include "test_init.h" +#include "proxysql.h" +#include "BackendSyncDecision.h" + +static void test_no_sync_needed() { + int a = determine_backend_sync_actions("user", "user", "db", "db", true, true); + ok(a == SYNC_NONE, "no sync: all match"); + + a = determine_backend_sync_actions("user", "user", "db", "db", false, false); + ok(a == SYNC_NONE, "no sync: autocommit both false"); +} + +static void test_schema_mismatch() { + int a = determine_backend_sync_actions("user", "user", "app_db", "other_db", true, true); + ok((a & SYNC_SCHEMA) != 0, "schema mismatch: SYNC_SCHEMA set"); + ok((a & SYNC_USER) == 0, "schema mismatch: SYNC_USER not set"); +} + +static void test_user_mismatch() { + int a = determine_backend_sync_actions("alice", "bob", "db", "db", true, true); + ok((a & SYNC_USER) != 0, "user mismatch: SYNC_USER set"); + // Schema check skipped when user differs (CHANGE USER handles schema) + ok((a & SYNC_SCHEMA) == 0, "user mismatch: SYNC_SCHEMA not set (handled by CHANGE USER)"); +} + +static void test_user_and_schema_mismatch() { + int a = determine_backend_sync_actions("alice", "bob", "db1", "db2", true, true); + ok((a & SYNC_USER) != 0, "user+schema: SYNC_USER set"); + ok((a & SYNC_SCHEMA) == 0, "user+schema: schema handled by user change"); +} + +static void test_autocommit_mismatch() { + int a = determine_backend_sync_actions("user", "user", "db", "db", true, false); + ok((a & SYNC_AUTOCOMMIT) != 0, "autocommit mismatch: SYNC_AUTOCOMMIT set"); + ok((a & SYNC_SCHEMA) == 0, "autocommit mismatch: no other sync"); +} + +static void test_multiple_mismatches() { + int a = determine_backend_sync_actions("user", "user", "db1", "db2", true, false); + ok((a & SYNC_SCHEMA) != 0, "multi: SYNC_SCHEMA set"); + ok((a & SYNC_AUTOCOMMIT) != 0, "multi: SYNC_AUTOCOMMIT set"); +} + +static void test_null_handling() { + // null users — no crash + int a = determine_backend_sync_actions(nullptr, "user", "db", "db", true, true); + ok(a == SYNC_NONE || a >= 0, "null client_user: no crash"); + + a = determine_backend_sync_actions("user", nullptr, "db", "db", true, true); + ok(a == SYNC_NONE || a >= 0, "null backend_user: no crash"); +} + +int main() { + plan(15); + int rc = test_init_minimal(); + ok(rc == 0, "test_init_minimal() succeeds"); + + test_no_sync_needed(); // 2 + test_schema_mismatch(); // 2 + test_user_mismatch(); // 2 + test_user_and_schema_mismatch(); // 2 + test_autocommit_mismatch(); // 2 + test_multiple_mismatches(); // 2 + test_null_handling(); // 2 + // Total: 1+2+2+2+2+2+2+2 = 15 + + test_cleanup_minimal(); + return exit_status(); +} From b51f50b96b48a6833a6e44eed7e94a143c3d3743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 12:48:27 +0100 Subject: [PATCH 8/9] Address review: fix NULL asymmetry in backend sync decisions (PR #5511) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BackendSyncDecision.cpp: treat asymmetric NULLs as mismatches (client=NULL + backend="bob" → SYNC_USER, not SYNC_NONE) - Tests: replace weak null assertions (a >= 0 always true) with specific checks for SYNC_USER/SYNC_SCHEMA on asymmetric NULLs, both-null → no sync, schema asymmetric null (+2 tests, plan 15→17) --- lib/BackendSyncDecision.cpp | 13 +++++++++++-- test/tap/tests/unit/backend_sync_unit-t.cpp | 19 ++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/lib/BackendSyncDecision.cpp b/lib/BackendSyncDecision.cpp index 04375b1b4..b9c64e402 100644 --- a/lib/BackendSyncDecision.cpp +++ b/lib/BackendSyncDecision.cpp @@ -20,7 +20,12 @@ int determine_backend_sync_actions( int actions = SYNC_NONE; // Username mismatch → CHANGE USER required - if (client_user && backend_user) { + // Asymmetric NULLs (one set, other not) count as mismatch + if (client_user == nullptr && backend_user != nullptr) { + actions |= SYNC_USER; + } else if (client_user != nullptr && backend_user == nullptr) { + actions |= SYNC_USER; + } else if (client_user && backend_user) { if (strcmp(client_user, backend_user) != 0) { actions |= SYNC_USER; } @@ -29,7 +34,11 @@ int determine_backend_sync_actions( // Schema mismatch → USE required // Only check if usernames match (user change handles schema too) if (!(actions & SYNC_USER)) { - if (client_schema && backend_schema) { + if (client_schema == nullptr && backend_schema != nullptr) { + actions |= SYNC_SCHEMA; + } else if (client_schema != nullptr && backend_schema == nullptr) { + actions |= SYNC_SCHEMA; + } else if (client_schema && backend_schema) { if (strcmp(client_schema, backend_schema) != 0) { actions |= SYNC_SCHEMA; } diff --git a/test/tap/tests/unit/backend_sync_unit-t.cpp b/test/tap/tests/unit/backend_sync_unit-t.cpp index c67d5efd5..8dd26dbe2 100644 --- a/test/tap/tests/unit/backend_sync_unit-t.cpp +++ b/test/tap/tests/unit/backend_sync_unit-t.cpp @@ -52,15 +52,24 @@ static void test_multiple_mismatches() { static void test_null_handling() { // null users — no crash + // Asymmetric NULL: one side null, other not → mismatch int a = determine_backend_sync_actions(nullptr, "user", "db", "db", true, true); - ok(a == SYNC_NONE || a >= 0, "null client_user: no crash"); + ok((a & SYNC_USER) != 0, "null client_user + non-null backend → SYNC_USER"); a = determine_backend_sync_actions("user", nullptr, "db", "db", true, true); - ok(a == SYNC_NONE || a >= 0, "null backend_user: no crash"); + ok((a & SYNC_USER) != 0, "non-null client_user + null backend → SYNC_USER"); + + // Both null → no mismatch + a = determine_backend_sync_actions(nullptr, nullptr, "db", "db", true, true); + ok(a == SYNC_NONE, "both users null → no sync"); + + // Schema asymmetric null + a = determine_backend_sync_actions("user", "user", nullptr, "db", true, true); + ok((a & SYNC_SCHEMA) != 0, "null client_schema + non-null backend → SYNC_SCHEMA"); } int main() { - plan(15); + plan(17); int rc = test_init_minimal(); ok(rc == 0, "test_init_minimal() succeeds"); @@ -70,8 +79,8 @@ int main() { test_user_and_schema_mismatch(); // 2 test_autocommit_mismatch(); // 2 test_multiple_mismatches(); // 2 - test_null_handling(); // 2 - // Total: 1+2+2+2+2+2+2+2 = 15 + test_null_handling(); // 4 + // Total: 1+2+2+2+2+2+2+4 = 17 test_cleanup_minimal(); return exit_status(); From 468adb450e69ec5c5c2173930873c8024dabfa74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 12:49:26 +0100 Subject: [PATCH 9/9] Address review on MySQL error classifier (PR #5512) - Remove unused MYSQL_ERROR_CONTINUE enum value - Clarify retries_remaining parameter semantics in docstring --- include/MySQLErrorClassifier.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/MySQLErrorClassifier.h b/include/MySQLErrorClassifier.h index 9b46549db..6830e1fa8 100644 --- a/include/MySQLErrorClassifier.h +++ b/include/MySQLErrorClassifier.h @@ -15,7 +15,6 @@ * @brief Action to take after a MySQL backend query error. */ enum MySQLErrorAction { - MYSQL_ERROR_CONTINUE, ///< Error handled, continue processing. MYSQL_ERROR_RETRY_ON_NEW_CONN, ///< Reconnect and retry on a new server. MYSQL_ERROR_REPORT_TO_CLIENT ///< Send error to client, no retry. }; @@ -35,7 +34,7 @@ enum MySQLErrorAction { * - multiplex not disabled * * @param error_code MySQL error number. - * @param retries_remaining Number of retries left. + * @param retries_remaining Number of retries left (> 0 to allow retry). * @param connection_reusable Whether the connection can be reused. * @param in_active_transaction Whether a transaction is in progress. * @param multiplex_disabled Whether multiplexing is disabled. @@ -57,7 +56,7 @@ MySQLErrorAction classify_mysql_error( * conditions are met. * * @param server_offline Whether the backend server is offline. - * @param retries_remaining Number of retries left. + * @param retries_remaining Number of retries left (> 0 to allow retry). * @param connection_reusable Whether the connection can be reused. * @param in_active_transaction Whether a transaction is in progress. * @param multiplex_disabled Whether multiplexing is disabled.