From 90e888e1d0ebe4dd9b95ad86dfe8a4bbd377799d Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Thu, 30 Apr 2026 21:04:12 +0000 Subject: [PATCH] fix(chassis): runtime-view dispatch fires unconditionally on admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doc-accuracy review found that proxysql_refresh_configured_plugin_ runtime_views was unreachable for plugin-registered tables. The call was placed inside if (refresh==true) { // ProxySQL_Admin.cpp:1637 ... if (admin) { // existing core runtime_* refreshes proxysql_refresh_configured_plugin_runtime_views(...) } } `refresh` only gets set to true by hardcoded substring matches against core's own table names (lines 1358-1634: runtime_mysql_users, runtime_mysql_servers, runtime_mysql_query_rules, etc.). None of those substrings match runtime_mysqlx_* (or any other plugin- registered view), so on a bare SELECT * FROM runtime_mysqlx_users the gate was false, my dispatch was skipped, the table was empty (or stale from a previous query that DID match a core substring), and the SELECT returned wrong data. The whole "on-demand projection" mechanism the previous commits documented was broken for the entry case. Issue #5687 / PR #5688. The fix is one-line structurally: hoist the dispatch out of the `if (refresh==true)` block and place it right after the substring- detection section, gated only on `if (admin)`. The chassis dispatcher itself (refresh_runtime_views_for_query in ProxySQL_PluginManager.cpp) already does its own per-view substring match against query_no_space, so a query that touches no registered view is a cheap no-op (one shared lock + N substring scans, N == registered-view count). Calling unconditionally on every admin query is therefore both correct and cheap. Test: new plugin_runtime_views_unit-t (20 ok asserts) drives ProxySQL_PluginManager::register_runtime_view + refresh_runtime_views_for_query directly. Covers: - register_runtime_view rejects null callback / empty name / case-insensitive duplicate. - Per-query dispatch fan-out: only matching callbacks fire, join queries fire all referenced views, unrelated queries fire nothing, case-insensitive match works, backtick-quoted identifiers match. - Whole-identifier boundary: longer-suffix overlap (runtime_ mysqlx_users_extra), left-prefix overlap (stats_runtime_ mysqlx_users), embedded-in-string-literal — none falsely match. Boundary cases (start of string, end of string) do match. This is the test the PR-#5688 review pass identified as the chassis- hook coverage gap. Builds standalone (no fake-plugin loader needed) since it drives the manager directly. --- lib/ProxySQL_Admin.cpp | 32 ++-- test/tap/tests/unit/Makefile | 5 + .../unit/plugin_runtime_views_unit-t.cpp | 170 ++++++++++++++++++ 3 files changed, 196 insertions(+), 11 deletions(-) create mode 100644 test/tap/tests/unit/plugin_runtime_views_unit-t.cpp diff --git a/lib/ProxySQL_Admin.cpp b/lib/ProxySQL_Admin.cpp index 2daacc516..c66701db2 100644 --- a/lib/ProxySQL_Admin.cpp +++ b/lib/ProxySQL_Admin.cpp @@ -1633,6 +1633,27 @@ bool ProxySQL_Admin::GenericRefreshStatistics(const char *query_no_space, unsign if (strstr(query_no_space,"mysql_server_aws_aurora_check_status")) { monitor_mysql_server_aws_aurora_check_status=true; refresh=true; } +#ifdef PROXYSQL40 + // Plugin-registered runtime views: if the query references any chassis- + // registered runtime view (e.g. runtime_mysqlx_users), refresh it on + // the admin path BEFORE the SELECT runs against admindb. We always + // invoke the dispatcher when the session is on the admin port; the + // chassis itself decides whether to fire any plugin's refresh + // callback by per-view substring match against query_no_space, so a + // query that touches no registered view is a cheap no-op (one shared + // lock + N substring scans, N == registered-view count). + // + // Note: we do NOT toggle the existing `refresh` flag here. That flag + // gates a block of core-only refreshes (stats_mysql_processlist, + // runtime_mysql_users, etc.) and is unrelated to plugin views. We + // also place this OUTSIDE the `if (refresh==true)` block so a SELECT + // that touches only a plugin view (e.g. SELECT * FROM runtime_mysqlx_ + // users with no other runtime_* mention) still gets its projection + // fired. + if (admin) { + proxysql_refresh_configured_plugin_runtime_views(query_no_space, admindb); + } +#endif /* PROXYSQL40 */ // if (stats_mysql_processlist || stats_mysql_connection_pool || stats_mysql_query_digest || stats_mysql_query_digest_reset) { if (refresh==true) { //pthread_mutex_lock(&admin_mutex); @@ -1872,17 +1893,6 @@ bool ProxySQL_Admin::GenericRefreshStatistics(const char *query_no_space, unsign } #endif /* PROXYSQLCLICKHOUSE */ -#ifdef PROXYSQL40 - // Plugin-registered runtime views (canonical pattern, mirrors - // the runtime_mysql_users refresh above): each plugin declares - // its admin-side view of module state via - // services.register_runtime_view(); the chassis dispatcher - // invokes the registered refresh callback for any view whose - // table name is referenced in this query, before the query - // runs against admindb. - proxysql_refresh_configured_plugin_runtime_views(query_no_space, admindb); -#endif /* PROXYSQL40 */ - } if (monitor_mysql_server_group_replication_log) { if (GloMyMon) { diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 23379e0d7..8eaa7bd3b 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -452,6 +452,11 @@ plugin_dispatch_unit-t: plugin_dispatch_unit-t.cpp $(FAKE_PLUGIN_SO) $(FAKE_PLUG $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ $(MYLIBS) -ldl $(ALLOW_MULTI_DEF) -o $@ +plugin_runtime_views_unit-t: plugin_runtime_views_unit-t.cpp $(ODIR)/tap.o $(ODIR)/test_globals.o $(ODIR)/test_init.o $(LIBPROXYSQLAR) + $(CXX) $< $(ODIR)/tap.o $(ODIR)/test_globals.o $(ODIR)/test_init.o \ + $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ + $(MYLIBS) -ldl $(ALLOW_MULTI_DEF) -o $@ + test_mysqlx_plugin_load-t: ../test_mysqlx_plugin_load-t.cpp $(ODIR)/tap.o $(ODIR)/test_globals.o $(ODIR)/test_init.o $(LIBPROXYSQLAR) mysqlx_plugin_build $(CXX) ../test_mysqlx_plugin_load-t.cpp $(ODIR)/tap.o $(ODIR)/test_globals.o $(ODIR)/test_init.o \ -DPROXYSQL_MYSQLX_PLUGIN_PATH=\"$(MYSQLX_PLUGIN_SO)\" \ diff --git a/test/tap/tests/unit/plugin_runtime_views_unit-t.cpp b/test/tap/tests/unit/plugin_runtime_views_unit-t.cpp new file mode 100644 index 000000000..032ecc331 --- /dev/null +++ b/test/tap/tests/unit/plugin_runtime_views_unit-t.cpp @@ -0,0 +1,170 @@ +// Unit tests for the chassis runtime-view registration + dispatch added in +// PR #5688: ProxySQL_PluginManager::register_runtime_view and +// refresh_runtime_views_for_query. +// +// This is the only test that drives the chassis surface directly without +// the plugin loader, so it can exercise: +// - registration rejection (null cb, empty name, duplicate) +// - case-insensitive whole-identifier substring match +// - per-query dispatch fan-out (only matching callbacks fire) +// - no-op for queries that reference no registered view +// +// Without these tests a regression that, say, replaced the careful +// whole-identifier match with a plain strstr() would silently start firing +// the wrong projection callback on substring-overlap table names. + +#include "tap.h" +#include "test_globals.h" +#include "test_init.h" +#include "ProxySQL_PluginManager.h" + +#include +#include +#include +#include + +namespace { + +struct CallbackProbe { + const char* tag; + std::atomic* counter; +}; + +void probe_refresh_cb(SQLite3DB* /*admindb*/, void* opaque) { + auto* probe = static_cast(opaque); + if (probe && probe->counter) probe->counter->fetch_add(1); +} + +void noop_cb(SQLite3DB*, void*) {} + +} // namespace + +int main() { + setvbuf(stdout, nullptr, _IOLBF, 0); + plan(20); + diag("=== plugin_runtime_views_unit-t starting ==="); + + // ---- Registration validation ---- + + { + diag(">>> register_runtime_view rejects null callback / empty name / duplicate"); + ProxySQL_PluginManager mgr; + + ok(mgr.register_runtime_view({nullptr, &noop_cb, nullptr}) == false, + "register_runtime_view rejects null table_name"); + ok(mgr.register_runtime_view({"", &noop_cb, nullptr}) == false, + "register_runtime_view rejects empty table_name"); + ok(mgr.register_runtime_view({"runtime_x", nullptr, nullptr}) == false, + "register_runtime_view rejects null refresh callback"); + + ok(mgr.register_runtime_view({"runtime_x", &noop_cb, nullptr}) == true, + "first registration of runtime_x succeeds"); + ok(mgr.register_runtime_view({"runtime_x", &noop_cb, nullptr}) == false, + "duplicate registration of runtime_x is rejected"); + ok(mgr.register_runtime_view({"RUNTIME_X", &noop_cb, nullptr}) == false, + "duplicate registration with different case is also rejected"); + } + + // ---- Dispatch: matching queries fire the callback ---- + + { + diag(">>> refresh_runtime_views_for_query fires only matching callbacks"); + + std::atomic users_fires{0}; + std::atomic routes_fires{0}; + CallbackProbe users_probe{"users", &users_fires}; + CallbackProbe routes_probe{"routes", &routes_fires}; + + ProxySQL_PluginManager mgr; + ok(mgr.register_runtime_view({"runtime_mysqlx_users", &probe_refresh_cb, &users_probe}) == true, + "registered runtime_mysqlx_users callback"); + ok(mgr.register_runtime_view({"runtime_mysqlx_routes", &probe_refresh_cb, &routes_probe}) == true, + "registered runtime_mysqlx_routes callback"); + + // Query references only one of the registered views. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM runtime_mysqlx_users WHERE active=1", nullptr); + ok(users_fires.load() == 1 && routes_fires.load() == 0, + "users-only query fires users (got %d) and not routes (got %d)", + users_fires.load(), routes_fires.load()); + + // Query references both. + mgr.refresh_runtime_views_for_query( + "SELECT u.username FROM runtime_mysqlx_users u JOIN runtime_mysqlx_routes r ON u.default_route=r.name", + nullptr); + ok(users_fires.load() == 2 && routes_fires.load() == 1, + "join query fires both (users=%d, routes=%d)", + users_fires.load(), routes_fires.load()); + + // Query references neither. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM mysql_users", nullptr); + ok(users_fires.load() == 2 && routes_fires.load() == 1, + "unrelated query fires nothing (users=%d, routes=%d)", + users_fires.load(), routes_fires.load()); + + // Case-insensitive match on the table name. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM RUNTIME_MYSQLX_USERS", nullptr); + ok(users_fires.load() == 3, + "uppercase table name still matches (users=%d)", users_fires.load()); + + // Backtick-quoted identifier — backtick is not an identifier char so + // the whole-identifier match still succeeds. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM `runtime_mysqlx_users`", nullptr); + ok(users_fires.load() == 4, + "backtick-quoted identifier still matches (users=%d)", users_fires.load()); + } + + // ---- Whole-identifier match: prefix and suffix overlaps must NOT match ---- + + { + diag(">>> sql_references_table_ci respects identifier boundaries"); + + std::atomic fires{0}; + CallbackProbe probe{"x", &fires}; + + ProxySQL_PluginManager mgr; + ok(mgr.register_runtime_view({"runtime_mysqlx_users", &probe_refresh_cb, &probe}) == true, + "registered runtime_mysqlx_users callback"); + + // Substring overlap that's part of a longer identifier — must NOT match. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM runtime_mysqlx_users_extra", nullptr); + ok(fires.load() == 0, + "longer-identifier overlap does not match (fires=%d)", fires.load()); + + // Suffix overlap. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM stats_runtime_mysqlx_users", nullptr); + ok(fires.load() == 0, + "left-side identifier prefix does not match (fires=%d)", fires.load()); + + // Embedded inside a longer word, no boundary on either side. + mgr.refresh_runtime_views_for_query( + "SELECT 'xruntime_mysqlx_usersy' FROM dual", nullptr); + ok(fires.load() == 0, + "embedded-in-string-literal does not match (fires=%d)", fires.load()); + + // Real reference: should match. + mgr.refresh_runtime_views_for_query( + "SELECT * FROM runtime_mysqlx_users", nullptr); + ok(fires.load() == 1, + "exact identifier match fires the callback (fires=%d)", fires.load()); + + // Identifier at end of string (no trailing whitespace). + mgr.refresh_runtime_views_for_query( + "DESC runtime_mysqlx_users", nullptr); + ok(fires.load() == 2, + "identifier at end-of-string still matches (fires=%d)", fires.load()); + + // Identifier at start of string. + mgr.refresh_runtime_views_for_query( + "runtime_mysqlx_users", nullptr); + ok(fires.load() == 3, + "identifier at start-of-string still matches (fires=%d)", fires.load()); + } + + return exit_status(); +}