3 Commits (feature/pgsql-native-backend-protocol)

Author	SHA1	Message	Date
Rene Cannao	ab9d5a1036	fix(plugin-chassis): address deep-review findings ... Follow-up to cd48c5613 (PROXYSQL40 gating commit). Multiple reviewers reported issues ranging from descriptor-layout UB to test false-positives; this commit resolves all of them. Showstoppers fixed: * plugins/mysqlx/Makefile: PROXYSQL40 (and all feature-tier flags) are now propagated from the top-level Makefile and converted to -DPROXYSQL40 in CXXFLAGS. Before this, PROXYSQL40=1 built a v4 core that dlopen'd a 6-field v3 mysqlx descriptor, and the loader then read past the end of the plugin's static struct. Top-level Makefile's four plugins/mysqlx recursive-make invocations now pass PROXYSQL40=$(PROXYSQL40); the test/tap/tests/unit mysqlx_plugin_build rule does the same. * ABI version gating (S2/S3): add PROXYSQL_PLUGIN_ABI_VERSION macro to ProxySQL_Plugin.h (1 without PROXYSQL40, 2 with). Plugins set descriptor.abi_version from this macro. Loader rejects abi_version < 1 or > PROXYSQL_PLUGIN_ABI_VERSION_MAX. The register_schemas field is only read when abi_version >= 2 -- a v1 plugin's 6-field descriptor is no longer dereferenced past its own layout even when loaded by a v4 core. ScopedRegistryTarget RAII guard replaces the inline g_registry_target = this / = nullptr pairs so exceptions from plugin callbacks can't leave the globals dirty. * Admin_Handler TOCTOU: proxysql_resolve_configured_plugin_admin_alias now returns std::string by value instead of const char*. The previous signature borrowed a c_str() from inside commands_ under the plugin manager lock; callers then released the lock before dispatch, and a concurrent reload between the two calls could dangle the pointer. Returning by value copies the canonical SQL out while the lock is still held. * test_phase_b_db_handles_are_null was a false-positive trap: it logged "null" whenever the getter pointer was null OR returned null, so a regression to get_admindb = nullptr would have silently passed. The fake plugin now emits three distinct markers (phase_b_getter_null vs phase_b_handles_null vs phase_b_handles_live); the test asserts the exact contract (non-null function pointer that returns nullptr). * Snapshot stubs contract: the header claimed Phase D services are "every field LIVE", but get_mysql_users_snapshot et al. point to a nullptr-returning stub in every phase. Header now explicitly documents that snapshot getters are not yet implemented and plugins MUST treat null return as "not available" across every phase. * stop() teardown symmetry: stop_all() previously skipped plugins that succeeded init() but failed start(). Resources allocated in init would leak. Now stop_all() runs stop() for every plugin where initialized=true, irrespective of started. proxysql_plugin_stop_cb's header docstring documents this: stop pairs with init, not start. * register_schemas partial-failure rollback: if a plugin's register_schemas registered some tables/commands and then returned false, the registry kept the partial registrations. A retry (same plugin, bug fixed) would then duplicate-fail. Both invoke_register_schemas_phase and init_all now snapshot the registration counts before invoking the plugin callback and truncate back on failure. Tightened / documented: * alias normalization delta (PROXYSQL40 collapses whitespace and trims trailing ';', !PROXYSQL40 is length-strict) is documented in canonicalize_plugin_command's docstring as an intentional UX improvement for the chassis tier, not a regression. * init-after-publish ordering invariant documented at both the publish site (proxysql_load_configured_plugins Phase-B branch) and at proxysql_init_configured_plugins. Phase D writes to commands_ / mysql_query_hook_ / pgsql_query_hook_ on the already-published manager; safe today because ProxySQL_Main_init_phase3___start_all runs strictly after Phase D, so no thread ever observes the manager during its Phase-D mutations. * Phase-D failure path documented: treated as fatal startup error (exit(EXIT_FAILURE) in main.cpp). Plugins that succeeded init() are still torn down via stop_all() during process teardown, now that stop pairs with init. Runtime reload is out of scope for this iteration. * Admin_Bootstrap.cpp materialize_plugin_tables(): removed duplicate ATTACH DATABASE calls for "disk" and "stats". init_sqlite3db() in the same startup flow attaches them first; a duplicate ATTACH errors out in SQLite. * test/tap/tests/unit/Makefile PROXYSQL31 autodetection: previously used the same probe symbol (MySQLFFTO) as PSQLFFTO, which collapsed PROXYSQL31 onto PROXYSQLFFTO. Now PROXYSQL31 is set when either PROXYSQLFFTO or PROXYSQLTSDB is detected (matching the top-level cascade's PROXYSQL31 => FFTO && TSDB). Tests added (all under PROXYSQL40 only): * plugin_lifecycle_unit-t: - test_phase_b_partial_failure_rolls_back: plugin registers a table then returns false; rollback verified by retrying the same plugin with the failure toggle cleared -- succeeds. - test_stop_runs_when_start_fails: exercises the init-success / start-failure path and asserts stop() marker in log. - test_bogus_abi_version_rejected: loads a descriptor with abi_version=99; loader rejects with "unsupported plugin ABI version" and init is not called. - plan(26), up from plan(14). * plugin_dispatch_unit-t: - test_cross_plugin_command_collision: loads both fake_plugin and fake_plugin2 as real .so's, both registering the same admin-command SQL; init fails with a message naming the collision. Complements the same-manager API-level collision test already present. - plan(52), up from plan(50). * plugin_query_hook_unit-t: - test_embedded_nul_in_query_text: dispatch with an embedded NUL in query_text asserts the full length_len byte range reaches the hook intact (not strlen-truncated). - test_reentrant_dispatch_across_protocols: mysql-hook callback re-enters dispatch_query_hook(pgsql). Asserts no deadlock and correct inner-hook invocation count. - plan(46), up from plan(41). Total plugin unit tests: 48 (config) + 52 (dispatch) + 26 (lifecycle) + 41 (prometheus) + 46 (query_hook) = 213 under PROXYSQL40; 47 + 32 = 79 under v3.1 non-chassis.	2 months ago
Rene Cannao	f13463d334	feat(plugin-abi): Step 2.2 — four-phase plugin lifecycle ... Adds the register_schemas() callback to the plugin descriptor and wires the loader + main.cpp so that a plugin's schema is declared BEFORE the admin module bootstraps, and the plugin's init() runs AFTER the schema has been materialized. Phase sequence (previously two-phase: load+init): A. load() — dlopen each .so, read the descriptor. B. register_schemas() — NEW, optional; plugin calls services->register_table with its admin/config/stats table defs. Services hand has register_table live but all DB-handle getters stubbed to nullptr. C. ProxySQL_Main_init_Admin_module + GloAdmin->materialize_plugin_tables — admin bootstrap drains the pending-tables list via merge_plugin_tables (first-boot == reload). D. init() — plugin's startup logic with full services (live DB handles pointing at a schema that already contains its own tables). E. start() — plugin launches threads / accept loops. ABI (include/ProxySQL_Plugin.h): * New callback typedef proxysql_plugin_register_schemas_cb. * New descriptor field register_schemas — appended at the end of ProxySQL_PluginDescriptor so existing 6-field initializers leave it null (aggregate init value-initializes the trailing slot). Plugins that left register_schemas null keep the pre-existing two-phase behavior: Phase B is skipped and their init() is responsible for both schema registration and startup work. * Phase-by-phase services availability matrix documented next to ProxySQL_PluginServices. Manager (lib/ProxySQL_PluginManager.cpp): * invoke_register_schemas_phase(err) — public method that runs Phase B for every plugin whose descriptor sets the callback. Uses a "services_phase_b_" variant where DB-handle getters are nullptr stubs; register_query_hook returns false with a warning (hooks cannot register this early). * proxysql_load_configured_plugins() — now stops at Phase B. On success, installs the built manager as the active one so that ProxySQL_Admin::materialize_plugin_tables (which reads from the active manager's pending-tables list) sees the registered tables. * proxysql_init_configured_plugins() — new entry point, calls init_all() for Phase D. Separated from load so admin bootstrap can run between the two. Main (src/main.cpp): * Reorder in ProxySQL_Main_init_phase2___not_started: LoadConfiguredPlugins(); // Phase A + B ProxySQL_Main_init_Admin_module(...); // Phase C GloAdmin->materialize_plugin_tables(); // Phase C (materialize) InitConfiguredPlugins(); // Phase D — NEW StartConfiguredPlugins(); // Phase E * Error messages updated to name the phase that failed. mysqlx plugin: * mysqlx_init split into: - mysqlx_register_schemas() — calls mysqlx_register_admin_schema (pure services->register_table use, no DB access); - mysqlx_init() — wires the plugin context (services, config store, started flag) and returns true. * Descriptor wires the new callback, exercising the full four- phase path end-to-end. Tests: * plugin_lifecycle_unit-t (new) — four cases: both callbacks fire in order; register_schemas-null skips Phase B; Phase-B DB handles return nullptr; Phase-B failure aborts init. * plugin_config_unit-t, plugin_dispatch_unit-t, plugin_query_hook_unit-t — existing tests updated to also call proxysql_init_configured_plugins after proxysql_load_configured_plugins where init side-effects are asserted. test_init_registration_failure_aborts_load reworked: the fake plugin's invalid-table registration happens in init (not register_schemas), so load now succeeds and the failure surfaces at init time. * test_mysqlx_admin_tables / test_mysqlx_listener_smoke — add an explicit invoke_register_schemas_phase() call before init_all() so the mysqlx plugin's schema gets registered under its new split. No behavior change for plugins that didn't opt into Phase B: they still see the same services_ layout and the same {load, init, start, stop} ordering. Opt-in plugins gain DB access from init() against a schema that already contains their tables, eliminating the chicken- and-egg that required ProtocolX's fd5f02947 workaround.	2 months ago
Rene Cannao	55556979e0	feat(plugin-abi): Step 2.1 — pre-execution query-hook ABI + dispatch ... First half of Step 2 from the GenAI plugin carve-out design (see docs/superpowers/specs/2026-04-16-genai-plugin-carveout-design.md). Adds the query-hook ABI surface and the manager-level dispatch, but NOT yet the hot-path call sites in MySQL_Session / PgSQL_Session -- those are intentionally split into the next commit so the hot-path diff stays small and reviewable on its own. ABI extension (include/ProxySQL_Plugin.h): - ProxySQL_PluginProtocol enum {mysql, pgsql} - ProxySQL_PluginQueryHookPayload {user, client_ip, schema, query_text, query_len} - ProxySQL_PluginQueryHookAction enum {allow, deny} - ProxySQL_PluginQueryHookResult {action, message} - proxysql_plugin_query_hook_cb typedef - proxysql_plugin_register_query_hook_cb typedef - new field on ProxySQL_PluginServices: register_query_hook (additive at the end of the struct -- older plugins that were built against the previous layout don't read past it; new plugins must check non-null before calling, same convention used elsewhere) Manager (lib/ProxySQL_PluginManager.cpp + .h): - One hook per protocol per manager (nullptr means "no hook"). - register_query_hook(proto, cb): rejects null cb; rejects duplicate registration for a protocol that already has a hook; surfaces registration errors through the same note_registration_failure / init-aborts path that register_table and register_command use. - dispatch_query_hook(proto, payload, result): synchronous, returns true if a hook fired and stored its result, false otherwise. - has_query_hook(proto): cheap predicate for the hot-path predicate. Global helpers (for the hot path, next commit): - proxysql_dispatch_configured_plugin_query_hook(proto, payload, result): takes the active-manager mutex, dispatches if there is one. - proxysql_has_configured_plugin_query_hook(proto): lock-free atomic read of the manager pointer + a pointer-sized field read. Designed to be the gate the hot path checks first; the dispatch call can be elided entirely on the no-plugin path. Documented as spurious-true-tolerant -- the dispatch helper re-checks under the lock. Test plugin extension (test/tap/test_helpers/fake_plugin.cpp): - New env vars PROXYSQL_FAKE_PLUGIN[2]_REGISTER_QUERY_HOOK, REGISTER_QUERY_HOOK_PROTO ("mysql"|"pgsql"), HOOK_DENY. - fake_query_hook echoes the SQL through the result message so tests can verify the payload reached the callback intact. New unit test: plugin_query_hook_unit-t (41 assertions) - empty manager: neither protocol has a hook, dispatch returns false, result struct untouched - register MySQL allow hook → dispatch returns ALLOW with empty msg - register MySQL deny hook → dispatch returns DENY with the message - null callback rejected; duplicate registration rejected; original hook still in effect after rejected duplicate - protocols are independent (mysql allow + pgsql deny coexist) - payload threaded through (echo hook reads user/ip/schema/sql) - global dispatcher: false when no active manager, untouched result - global dispatcher with active manager: routes to the fake plugin's hook, propagates ALLOW vs DENY (env-flipped between calls), and goes back to false after stop_configured_plugins All 59 unit-test binaries pass.	2 months ago