From aaa02bb4006fe3cec223e18dc5066c8a5fd5e0bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:00:14 +0100 Subject: [PATCH 1/3] Extract hostgroup routing decision logic (Phase 3.5, #5493) New files: - include/HostgroupRouting.h: HostgroupRoutingDecision struct + resolve_hostgroup_routing() declaration - lib/HostgroupRouting.cpp: implementation mirroring the routing block in get_pkts_from_client() (~lines 5340-5377) Logic covers: QP destination override, transaction affinity, hostgroup lock acquisition/enforcement, error on HG mismatch. Identical for both MySQL and PgSQL protocols. --- include/HostgroupRouting.h | 52 +++++++++++++++++++++++++++++++++ lib/HostgroupRouting.cpp | 60 ++++++++++++++++++++++++++++++++++++++ lib/Makefile | 1 + 3 files changed, 113 insertions(+) create mode 100644 include/HostgroupRouting.h create mode 100644 lib/HostgroupRouting.cpp diff --git a/include/HostgroupRouting.h b/include/HostgroupRouting.h new file mode 100644 index 000000000..9ba9e386f --- /dev/null +++ b/include/HostgroupRouting.h @@ -0,0 +1,52 @@ +/** + * @file HostgroupRouting.h + * @brief Pure hostgroup routing decision logic for unit testability. + * + * Extracted from MySQL_Session::get_pkts_from_client() and + * PgSQL_Session::get_pkts_from_client(). The logic is identical + * for both protocols. + * + * @see Phase 3.5 (GitHub issue #5493) + */ + +#ifndef HOSTGROUP_ROUTING_H +#define HOSTGROUP_ROUTING_H + +/** + * @brief Result of a hostgroup routing decision. + */ +struct HostgroupRoutingDecision { + int target_hostgroup; ///< Resolved hostgroup to route to. + int new_locked_on_hostgroup; ///< Updated lock state (-1 = unlocked). + bool error; ///< True if an illegal HG switch was attempted. +}; + +/** + * @brief Resolve the target hostgroup given session state and QP output. + * + * Decision logic (mirrors get_pkts_from_client()): + * 1. Start with default_hostgroup as the target + * 2. If QP provides a destination (>= 0) and no transaction lock, + * use the QP destination + * 3. If transaction_persistent_hostgroup >= 0, override with transaction HG + * 4. If locking is enabled and lock_hostgroup flag is set, acquire lock + * 5. If already locked, verify target matches lock (error if mismatch) + * + * @param default_hostgroup Session's default hostgroup. + * @param qpo_destination_hostgroup Query Processor output destination (-1 = no override). + * @param transaction_persistent_hostgroup Current transaction HG (-1 = none). + * @param locked_on_hostgroup Current lock state (-1 = unlocked). + * @param lock_hostgroup_flag Whether the QP wants to acquire a lock. + * @param lock_enabled Whether set_query_lock_on_hostgroup is enabled. + * @return HostgroupRoutingDecision with resolved target and updated lock. + */ +HostgroupRoutingDecision resolve_hostgroup_routing( + int default_hostgroup, + int qpo_destination_hostgroup, + int transaction_persistent_hostgroup, + int locked_on_hostgroup, + bool lock_hostgroup_flag, + bool lock_enabled +); + +#endif // HOSTGROUP_ROUTING_H diff --git a/lib/HostgroupRouting.cpp b/lib/HostgroupRouting.cpp new file mode 100644 index 000000000..0c2d0d8d5 --- /dev/null +++ b/lib/HostgroupRouting.cpp @@ -0,0 +1,60 @@ +/** + * @file HostgroupRouting.cpp + * @brief Implementation of pure hostgroup routing decision logic. + * + * Mirrors the routing block in get_pkts_from_client() (MySQL_Session.cpp + * ~lines 5340-5377 and PgSQL_Session.cpp ~lines 2154-2189). + * + * @see HostgroupRouting.h + * @see Phase 3.5 (GitHub issue #5493) + */ + +#include "HostgroupRouting.h" + +HostgroupRoutingDecision resolve_hostgroup_routing( + int default_hostgroup, + int qpo_destination_hostgroup, + int transaction_persistent_hostgroup, + int locked_on_hostgroup, + bool lock_hostgroup_flag, + bool lock_enabled) +{ + HostgroupRoutingDecision d; + d.error = false; + d.new_locked_on_hostgroup = locked_on_hostgroup; + + // Start with default hostgroup + int current_hostgroup = default_hostgroup; + + // If QP provides a valid destination and no transaction lock, use it + if (qpo_destination_hostgroup >= 0 + && transaction_persistent_hostgroup == -1) { + current_hostgroup = qpo_destination_hostgroup; + } + + // Transaction affinity overrides everything + if (transaction_persistent_hostgroup >= 0) { + current_hostgroup = transaction_persistent_hostgroup; + } + + // Hostgroup locking logic (algorithm introduced in 2.0.6) + if (lock_enabled) { + if (locked_on_hostgroup < 0) { + // Not yet locked + if (lock_hostgroup_flag) { + // Acquire lock on the current (already resolved) hostgroup + d.new_locked_on_hostgroup = current_hostgroup; + } + } + if (d.new_locked_on_hostgroup >= 0) { + // Already locked (or just acquired) — enforce + if (current_hostgroup != d.new_locked_on_hostgroup) { + d.error = true; + } + current_hostgroup = d.new_locked_on_hostgroup; + } + } + + d.target_hostgroup = current_hostgroup; + return d; +} diff --git a/lib/Makefile b/lib/Makefile index f7f24075c..6ba6e05bf 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 \ + HostgroupRouting.oo \ proxy_sqlite3_symbols.oo # TSDB object files From 98249bcd5a24a80b23aa6f2634b203c972d361fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:00:14 +0100 Subject: [PATCH 2/3] Add hostgroup routing unit tests + Makefile pattern rule (Phase 3.5, #5493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 20 test cases: basic routing, transaction affinity, lock acquire/enforce, locking disabled, transaction+lock interaction, edge cases. Also replaces all individual unit test build rules with a single pattern rule (%-t: %-t.cpp) — adding new tests now only requires appending the name to UNIT_TESTS. --- test/tap/tests/unit/Makefile | 45 +------- .../tests/unit/hostgroup_routing_unit-t.cpp | 105 ++++++++++++++++++ 2 files changed, 108 insertions(+), 42 deletions(-) create mode 100644 test/tap/tests/unit/hostgroup_routing_unit-t.cpp diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 0d56f1758..409dd43af 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -231,7 +231,7 @@ $(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 hostgroup_routing_unit-t .PHONY: all all: $(UNIT_TESTS) @@ -245,47 +245,8 @@ 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. +%-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/hostgroup_routing_unit-t.cpp b/test/tap/tests/unit/hostgroup_routing_unit-t.cpp new file mode 100644 index 000000000..1212183ab --- /dev/null +++ b/test/tap/tests/unit/hostgroup_routing_unit-t.cpp @@ -0,0 +1,105 @@ +/** + * @file hostgroup_routing_unit-t.cpp + * @brief Unit tests for hostgroup routing decision logic. + * + * @see Phase 3.5 (GitHub issue #5493) + */ + +#include "tap.h" +#include "test_globals.h" +#include "test_init.h" +#include "proxysql.h" +#include "HostgroupRouting.h" + +static void test_basic_routing() { + // No transaction, no lock → uses QP destination + auto d = resolve_hostgroup_routing(0, 5, -1, -1, false, false); + ok(d.target_hostgroup == 5, "basic: QP destination used"); + ok(d.error == false, "basic: no error"); + + // QP destination -1 → uses default + auto d2 = resolve_hostgroup_routing(0, -1, -1, -1, false, false); + ok(d2.target_hostgroup == 0, "basic: default HG when QP=-1"); +} + +static void test_transaction_affinity() { + // Transaction active → overrides QP destination + auto d = resolve_hostgroup_routing(0, 5, 3, -1, false, false); + ok(d.target_hostgroup == 3, + "txn: transaction_persistent_hostgroup overrides QP"); + + // Transaction + QP both set → transaction wins + auto d2 = resolve_hostgroup_routing(0, 10, 7, -1, false, false); + ok(d2.target_hostgroup == 7, "txn: transaction wins over QP"); +} + +static void test_locking_acquire() { + // Lock enabled, lock_hostgroup=true, not yet locked → acquires lock + auto d = resolve_hostgroup_routing(0, 5, -1, -1, true, true); + ok(d.target_hostgroup == 5, "lock acquire: routes to QP dest"); + ok(d.new_locked_on_hostgroup == 5, + "lock acquire: lock set to target HG"); + ok(d.error == false, "lock acquire: no error"); +} + +static void test_locking_enforce() { + // Already locked on HG 5, QP routes to 5 → ok + auto d = resolve_hostgroup_routing(0, 5, -1, 5, false, true); + ok(d.target_hostgroup == 5, "lock enforce: same HG ok"); + ok(d.error == false, "lock enforce: no error on match"); + + // Already locked on HG 5, QP routes to 10 → error + auto d2 = resolve_hostgroup_routing(0, 10, -1, 5, false, true); + ok(d2.error == true, "lock enforce: error on HG mismatch"); + ok(d2.target_hostgroup == 5, + "lock enforce: stays on locked HG despite QP mismatch"); +} + +static void test_locking_disabled() { + // Lock feature disabled → no locking even with flag + auto d = resolve_hostgroup_routing(0, 5, -1, -1, true, false); + ok(d.new_locked_on_hostgroup == -1, + "lock disabled: no lock acquired"); + + // Lock feature disabled → no enforcement even if locked state passed + auto d2 = resolve_hostgroup_routing(0, 10, -1, 5, false, false); + ok(d2.target_hostgroup == 10, + "lock disabled: routes to QP dest ignoring lock"); + ok(d2.error == false, "lock disabled: no error"); +} + +static void test_transaction_plus_lock() { + // Transaction active + locked → transaction HG wins + auto d = resolve_hostgroup_routing(0, 10, 3, 3, false, true); + ok(d.target_hostgroup == 3, + "txn+lock: transaction HG used"); + ok(d.error == false, "txn+lock: no error when txn matches lock"); +} + +static void test_edge_cases() { + // default_hostgroup=-1 (shouldn't happen but handle gracefully) + auto d = resolve_hostgroup_routing(-1, -1, -1, -1, false, false); + ok(d.target_hostgroup == -1, "edge: negative defaults pass through"); + + // All zeros + auto d2 = resolve_hostgroup_routing(0, 0, -1, -1, false, false); + ok(d2.target_hostgroup == 0, "edge: HG 0 is valid"); +} + +int main() { + plan(20); + int rc = test_init_minimal(); + ok(rc == 0, "test_init_minimal() succeeds"); + + test_basic_routing(); // 3 + test_transaction_affinity(); // 2 + test_locking_acquire(); // 3 + test_locking_enforce(); // 4 + test_locking_disabled(); // 3 + test_transaction_plus_lock(); // 2 + test_edge_cases(); // 2 + // Total: 1+3+2+3+4+3+2+2 = 20... fix + + test_cleanup_minimal(); + return exit_status(); +} From 3ed5e889c8c276289489ebe6f9b01abdc45342ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 22 Mar 2026 11:32:39 +0100 Subject: [PATCH 3/3] Address review feedback on hostgroup routing tests (PR #5509) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add test for transaction+lock HG mismatch (txn on HG 3 but locked on HG 5 → error), as suggested by reviewer - Fix trailing "... fix" comment in test count - Plan updated from 20 to 21 --- test/tap/tests/unit/hostgroup_routing_unit-t.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/tap/tests/unit/hostgroup_routing_unit-t.cpp b/test/tap/tests/unit/hostgroup_routing_unit-t.cpp index 1212183ab..1d07bd1a3 100644 --- a/test/tap/tests/unit/hostgroup_routing_unit-t.cpp +++ b/test/tap/tests/unit/hostgroup_routing_unit-t.cpp @@ -69,11 +69,16 @@ static void test_locking_disabled() { } static void test_transaction_plus_lock() { - // Transaction active + locked → transaction HG wins + // Transaction active + locked on same HG → no error auto d = resolve_hostgroup_routing(0, 10, 3, 3, false, true); ok(d.target_hostgroup == 3, "txn+lock: transaction HG used"); ok(d.error == false, "txn+lock: no error when txn matches lock"); + + // Transaction active on HG 3 but locked on HG 5 → error (mismatch) + auto d2 = resolve_hostgroup_routing(0, 10, 3, 5, false, true); + ok(d2.error == true, + "txn+lock: error when txn HG differs from lock HG"); } static void test_edge_cases() { @@ -87,7 +92,7 @@ static void test_edge_cases() { } int main() { - plan(20); + plan(21); int rc = test_init_minimal(); ok(rc == 0, "test_init_minimal() succeeds"); @@ -96,9 +101,9 @@ int main() { test_locking_acquire(); // 3 test_locking_enforce(); // 4 test_locking_disabled(); // 3 - test_transaction_plus_lock(); // 2 + test_transaction_plus_lock(); // 3 test_edge_cases(); // 2 - // Total: 1+3+2+3+4+3+2+2 = 20... fix + // Total: 1+3+2+3+4+3+3+2 = 21 test_cleanup_minimal(); return exit_status();