mirror of https://github.com/sysown/proxysql
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.
ProtocolX
Rene Cannao
1 month ago
parent
cd48c5613e
commit
ab9d5a1036
Loading…
Reference in new issue