feat: MySQL X Protocol plugin (dynamically loaded)#5593
Open
renecannao wants to merge 98 commits intov3.0from
Open
feat: MySQL X Protocol plugin (dynamically loaded)#5593renecannao wants to merge 98 commits intov3.0from
renecannao wants to merge 98 commits intov3.0from
Conversation
Plugin start now opens TCP listeners for each active route in runtime_mysqlx_routes. An accept thread dispatches incoming connections to worker threads round-robin. Phase 1 workers accept-then-close; Tasks 7-8 will add X Protocol handshake and backend relay.
Compile mysqlx*.proto files from MySQL Connector-C 8.4.0 into C++ sources. The plugin .so now links -lprotobuf for X Protocol frame/message encoding and decoding needed by Tasks 7-8.
Add mysqlx_protocol.h/cpp with frame encode/decode, MYSQL41 scramble auth using EVP SHA1, and protobuf message helpers for X Protocol Error/Ok/Capabilities/Auth frames. Add mysqlx_frontend_session.h/cpp implementing the full handshake state machine: CapabilitiesGet→Capabilities, CapabilitiesSet→Ok, AuthenticateStart→challenge→AuthenticateContinue→AuthenticateOk. Supports MYSQL41 and PLAIN auth methods. Enforces x_enabled flag, rejects pass_through backend auth mode. Workers now run the frontend session instead of immediately closing connections.
Add MysqlxBackendSession with TCP connect, X Protocol MYSQL41 auth to backend, and bidirectional byte-level relay via select(). Implement round_robin and round_robin_with_fallback endpoint selection strategies in MysqlxConfigStore::pick_endpoint(). Workers now connect to the backend after frontend auth and relay traffic until close. Phase 1 is one frontend = one backend, no pooling.
Add MysqlxStatsStore with atomic route counters (conn_ok, conn_err, bytes_sent, bytes_recv) and flush_to_sqlite() for stats_mysqlx_routes. Register stats_mysqlx_routes and stats_mysqlx_processlist tables in the plugin admin schema. Wire stats recording into the worker: record_conn_ok on successful backend connect, record_conn_err on failure. Add topology generation capture at session bind time as the Phase 2 seam for GR notification-driven route invalidation.
1. SQL injection in stats flush: escape route names with sqlite_escape() 2. Timing attack in PLAIN auth: use CRYPTO_memcmp for constant-time password comparison 3. OOM via oversized frame: add MYSQLX_MAX_PAYLOAD_SIZE (16MB) check in mysqlx_read_frame before allocating payload buffer 4. Thread safety: add shared_mutex to MysqlxConfigStore, shared_lock for readers (resolve_identity, pick_endpoint), unique_lock for writer (load_from_runtime), separate rr_mutex_ for round-robin counter mutation
1. Replace select() with poll() in accept loop and relay to avoid FD_SETSIZE undefined behavior with high file descriptors 2. Make g_rr_index atomic for safe concurrent access 3. Add SO_RCVTIMEO/SO_SNDTIMEO (10s backend, 30s frontend) to prevent indefinite blocking from slow peers 4. Reject TLS CapabilitiesSet requests explicitly in Phase 1 5. Enforce require_tls per user — reject if set since Phase 1 does not implement TLS negotiation 6. Enforce allowed_auth_methods per user — check the comma-separated list before proceeding with auth
Wires TLS into the session state machine following ProxySQL MySQL protocol pattern. When a client sends CapabilitiesSet with tls=true capability, the session initiates SSL_accept on the client data stream. Changes to Mysqlx_Thread: - Added get_ssl_ctx() that returns GloVars.get_SSL_ctx() - Updated rebuild_poll_set() to check has_ssl_pending_write() for POLLOUT on both client and backend data streams Changes to MysqlxSession::handler_capabilities_set(): - Parses CapabilitiesSet protobuf to detect tls capability - If TLS requested and SSL_CTX available: sends Ok, transitions to X_TLS_ACCEPT_INIT state - If SSL_CTX not configured: sends error 3150 and closes session - Without TLS: existing behavior unchanged Changes to MysqlxSession::handler_tls_accept_init(): - Replaced stub with real implementation - Gets SSL_CTX from Mysqlx_Thread - Calls client_ds_.init_ssl(ctx) to create per-session SSL object - Calls do_ssl_handshake() on each handler invocation - When handshake completes, transitions to CONNECTING_CLIENT Changes to MysqlxSession::send_capabilities(): - Advertises tls capability (V_BOOL true) when SSL_CTX is configured - Existing auth capability advertisement unchanged All 10 test suites pass with no regressions.
Adds a configurable connect timeout (default 10 seconds) for backend X Protocol connections. If the TCP connection does not complete within the timeout, the connection attempt fails with error 2003. Changes to MysqlxConnection: - Added connect_timeout_ms_ (default 10000ms) and connect_start_time_ private members - start_connect() records connect_start_time_ using steady_clock - check_connect() measures elapsed time since start_connect() and returns -1 if timeout exceeded, setting state to ERROR_STATE - Added set_connect_timeout()/get_connect_timeout() accessors Changes to MysqlxSession: - handler_connecting_server() sets connect timeout to 10s before calling start_connect() on new backend connections The timeout is checked on every check_connect() invocation from the event loop, ensuring non-blocking timeout detection without additional timers.
Server NOTICE frames are now explicitly handled in the response forwarding loop in handler_waiting_server_msg(). Notices are forwarded immediately to the client with a continue statement, skipping terminal frame detection. Previously, NOTICE frames fell through to the is_terminal_server_frame() check which correctly returned false for them. The explicit handling makes the intent clear and ensures notices are never accidentally included in terminal frame logic: - Warnings, session state changes, and other server-side notifications are always delivered to the client - NOTICE forwarding does not affect response state or terminal detection - NOTICE frames are still forwarded even while waiting for a terminal response (e.g., during a long-running query result set)
Replace generic error code 5001 with the standard MySQL X Protocol error code ER_X_CAPABILITY_COMPRESSION_INVALID_ALGORITHM (5008) when rejecting compression capability requests. Error 5008 is the standard MySQL X Protocol error for invalid compression algorithm negotiation, matching the error code used by MySQL Router and the MySQL X Plugin.
SESS_RESET is now properly forwarded to the backend with dedicated
response tracking via the new X_SESSION_RESET_WAITING state:
1. Client sends SESS_RESET -> forward_to_backend() sends to server
2. Session transitions to X_SESSION_RESET_WAITING
3. handler_session_reset_waiting() reads backend response:
- On NOTICE: forward to client immediately, keep waiting
- On OK: clear prepared statement tracking, clear transaction
flag, return backend to pool, resume WAITING_CLIENT_XMSG
- On ERROR: forward error to client, return backend to pool,
resume WAITING_CLIENT_XMSG
- On backend disconnect: return to WAITING_CLIENT_XMSG
This properly resets session state on both sides, clearing
prepared statement and transaction flags so the backend
connection can be safely reused from the pool.
Previously, SESS_RESET was forwarded but the response was not
tracked, potentially leaving the session in an inconsistent state.
Adds SSL handshake failure detection and descriptive error messages
for both client-side and backend TLS failures.
Changes to MysqlxDataStream:
- Added ssl_failed_ flag to distinguish WANT_IO (in progress) from
actual SSL failure
- ssl_handshake_failed() accessor for session to check failure state
- ssl_failed_ is set to true in do_ssl_handshake() when
get_ssl_status() returns MYSQLX_SSL_FAIL
- ssl_failed_ is initialized to false in constructor, init_ssl(),
and init_ssl_connect()
Changes to MysqlxSession handler_tls_accept_init():
- After do_ssl_handshake() returns false, checks ssl_handshake_failed()
- On failure: sends error 3151 ("TLS handshake failed") to client
with OpenSSL error details retrieved via ERR_get_error()/
ERR_error_string_n(), then closes session
- On WANT_IO: continues waiting (existing behavior)
- Error 3150: TLS not configured on server (unchanged)
- Error 3151: TLS handshake failed (new)
- Error 3152: Reserved for backend TLS failures (Task 3)
…detection Replace the single is_terminal_server_frame() catch-all with per-message-type response tracking. Each client message type now has its own terminal frame set, matching MySQL Router's approach. New MysqlxResponseState enum: - RESP_IDLE: no pending response - RESP_WAITING_STMT_EXECUTE: SQL_STMT_EXECUTE_OK, ERROR, FETCH_DONE - RESP_WAITING_CRUD: OK, ERROR, FETCH_DONE, FETCH_SUSPENDED - RESP_WAITING_PREPARE: OK, ERROR - RESP_WAITING_CURSOR: FETCH_DONE, FETCH_SUSPENDED, ERROR - RESP_WAITING_EXPECT: OK, ERROR - RESP_WAITING_SESS_RESET: OK, ERROR Changes: - dispatch_client_message() sets response_state_ before forward_to_backend() based on message type - handler_waiting_server_msg() uses is_terminal_for_state() instead of is_terminal_server_frame() for terminal detection - is_terminal_for_state() checks the response state to determine which frames are terminal - response_state_ resets to RESP_IDLE when terminal frame received - Generic fallback preserved via is_terminal_server_frame_generic() for unknown response states This is more robust than the single catch-all list and correctly handles cases where OK is terminal for Prepare but not for StmtExecute (which needs SQL_STMT_EXECUTE_OK).
Adds TLS support for backend X Protocol connections. When the client session is encrypted, the backend connection also negotiates TLS with the MySQL X Protocol server. Backend auth state machine changes: - New BACKEND_AUTH_TLS_HANDSHAKE state between CapSet and AuthStart - CapabilitiesSet now includes tls=true when backend_tls_required_ and an SSL_CTX is available - After CapSet Ok, if TLS required: init_ssl_connect() → handshake → send_authenticate_start() → continue normal auth - send_authenticate_start() extracted as a helper method to avoid goto-based flow control MysqlxConnection new members: - backend_tls_required_ flag (set by session when client is on TLS) - backend_ssl_ctx_ pointer (shared global SSL_CTX) - set_backend_tls_required() / is_backend_tls_required() accessors - set_ssl_ctx() to provide the SSL context TLS handshake in step_auth(): - BACKEND_AUTH_TLS_HANDSHAKE state calls backend_ds_.read_from_net() to feed encrypted bytes into the BIO pair - do_ssl_handshake() performs SSL_connect via memory BIOs - flush_ssl_write_buf() sends pending encrypted output - On failure: ssl_handshake_failed() check, returns BACKEND_AUTH_ERROR Session wiring in handler_connecting_server(): - When client_ds_.is_encrypted(), sets backend_tls_required_=true and provides SSL_CTX from Mysqlx_Thread - Backend auth failure detection distinguishes TLS failures (error 3152) from authentication failures (error 1045)
Adds MysqlxTlsMode enum and passthrough mode support: - TLS_OFF: No TLS (default, existing behavior) - TLS_TERMINATE: ProxySQL terminates TLS, decrypts, re-encrypts to backend (existing behavior when TLS is negotiated) - TLS_PASSTHROUGH: Raw encrypted bytes forwarded between client and backend without TLS termination Passthrough behavior: - handler_tls_accept_init(): skips SSL init and handshake entirely, returns to CONNECTING_CLIENT state immediately - handler_connecting_server(): skips backend TLS setup when in passthrough mode (backend_tls_required_ is not set) - X Protocol frames are still parsed at the framing layer since the proxy needs to route and multiplex connections Use cases for passthrough mode: - TLS terminated at an external load balancer - ProxySQL should not have access to TLS certificates - Compliance requirements that forbid TLS inspection set_tls_mode() / get_tls_mode() accessors on MysqlxSession allow runtime configuration of the TLS mode per session.
Add prometheus-cpp, libconfig, jemalloc, mariadb, and re2 include paths to the plugin build. Without these, mysqlx_thread.cpp fails to compile because it includes proxysql_structs.h which pulls in the full ProxySQL header chain. This was masked previously because the plugin was only built during full releases. Integration testing now requires building the .so independently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
doc/mysqlx/TESTING.md (1)
294-299: Avoid piping remote scripts directly to shell in docs.The install example executes remote content immediately. Please switch to a download + checksum/signature verification flow before running the script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/mysqlx/TESTING.md` around lines 294 - 299, The docs currently pipe a remote installer directly to the shell (curl ... | bash); change the example to first download the installer (curl -o dbdeployer-install.sh https://.../dbdeployer-install.sh), fetch or include the corresponding checksum/signature, verify the downloaded file (e.g. sha256sum and compare against a provided checksum or use gpg --verify if a signature is available), and only then run it (bash dbdeployer-install.sh). Also show the analogous safe steps for the subsequent dbdeployer commands (download/unpack/deploy) and include an explicit verification step or checksum for any downloaded tarball (mysql-8.4.*.tar.xz) before running dbdeployer unpack/deploy.plugins/mysqlx/include/mysqlx_connection.h (1)
51-57: Add null checks toconst char*setters for defensive safety.These setters assign
const char*directly tostd::string, which is undefined behavior ifnullptris passed. While current usage is safe (all calls use.c_str()on existingstd::stringobjects), adding defensive checks prevents potential future misuse.Proposed fix
- void set_user(const char* u) { user_ = u; } + void set_user(const char* u) { user_ = (u ? u : ""); } - void set_schema(const char* s) { schema_ = s; } + void set_schema(const char* s) { schema_ = (s ? s : ""); } - void set_address(const char* a) { address_ = a; } + void set_address(const char* a) { address_ = (a ? a : ""); } - void set_backend_user(const char* u) { backend_user_ = u; } + void set_backend_user(const char* u) { backend_user_ = (u ? u : ""); } - void set_backend_password(const char* p) { backend_password_ = p; } + void set_backend_password(const char* p) { backend_password_ = (p ? p : ""); } - void set_backend_schema(const char* s) { backend_schema_ = s; } + void set_backend_schema(const char* s) { backend_schema_ = (s ? s : ""); }Also applies to: lines 77-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mysqlx/include/mysqlx_connection.h` around lines 51 - 57, The setters set_user, set_schema, and set_address (and the analogous setters around lines 77-79) should defensively check incoming const char* parameters for nullptr before assigning to the underlying std::string members; update each setter to do something like if (param) member_ = param; else member_.clear() (or assign an empty string) to avoid undefined behavior when a null pointer is passed, ensuring all const char* -> std::string assignments are guarded.plugins/mysqlx/include/mysqlx_config_store.h (1)
15-19: Align enum constant naming with repository constant style.Consider renaming enum values to UPPER_SNAKE_CASE to match project convention for constants.
As per coding guidelines: "Use UPPER_SNAKE_CASE for constants and macros".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mysqlx/include/mysqlx_config_store.h` around lines 15 - 19, The enum MysqlxBackendAuthMode uses lower_snake_case values; rename the constants to UPPER_SNAKE_CASE (e.g., MAPPED, SERVICE_ACCOUNT, PASS_THROUGH) in the enum declaration and update all usages of MysqlxBackendAuthMode::mapped, ::service_account, and ::pass_through to the new names across the codebase (search for MysqlxBackendAuthMode and those identifiers so callers, switches, and serialization/deserialization code are updated accordingly) to comply with the UPPER_SNAKE_CASE constant style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/mysqlx/ARCHITECTURE.md`:
- Around line 346-368: The fenced ASCII diagram block is missing a language tag
which triggers MD040; update the Markdown block surrounding the flow diagram
(the block that shows recv(), feed_bytes(data, N), read_buf_, try_parse_frame(),
decode payload_size, complete_frames_.push(), advance read_offset_, etc.) by
adding a language identifier (e.g., change the opening ``` to ```text) so the
fence is properly tagged and MD040 is satisfied.
- Around line 165-166: The doc currently uses mysqlx_endpoints while other
schema/docs use mysqlx_backend_endpoints; pick the canonical table name (use
mysqlx_backend_endpoints) and update this section to match: change the table
name in the ARCHITECTURE.md text and ensure corresponding DDL/definitions and
LOAD/SAVE command handlers that reference mysqlx_endpoints are renamed to
mysqlx_backend_endpoints, and update any cross-references in the mysqlx
schema/docs so all mentions (mysqlx_endpoints, mysqlx_backend_endpoints) are
normalized to the chosen canonical symbol.
- Around line 104-105: The header claims "22 states" but the state table lists
21; reconcile by either adding the missing state to the list or updating the
count to the actual number and propagate that change to the other occurrences
referenced around the same section (including the repeated summary at 302-327).
Inspect the state identifiers in the table (e.g., CONNECTING_CLIENT,
X_CAPABILITIES_GET, X_CAPABILITIES_SET, X_TLS_ACCEPT_*, X_AUTH_START,
X_AUTH_CHALLENGE_SENT, X_AUTH_OK_SENT, WAITING_CLIENT_XMSG, CONNECTING_SERVER,
WAITING_SERVER_XMSG, X_FAST_FORWARD, X_SESSION_CLOSING, etc.), ensure every
unique state is present exactly once (or explicitly document why a wildcard like
X_TLS_ACCEPT_* represents multiple states), then either add the missing state
identifier to the list or change the "22 states" text to the correct count and
update all other occurrences of that count in the document.
In `@doc/mysqlx/MYSQL_ROUTER_PARITY.md`:
- Around line 14-17: Update the stale parity doc entries for TLS and timeout
behavior to reflect the current implementation: change the rows mentioning "TLS
modes", "TLS termination", "TLS passthrough", and "Asymmetric TLS" to indicate
that client and backend TLS handshakes are implemented (not "Not
implemented"/"No"), and add a row or note for "backend connect timeout"
explaining that backend connect timeout controls are exposed; also include any
remaining gaps (e.g., unimplemented passthrough edge-cases or configuration
limits) so the table and accompanying text accurately state what's implemented
versus what is still missing.
In `@doc/mysqlx/README.md`:
- Line 195: Update the PLAIN auth method description (the table entry headed
"**PLAIN**") to use a complete sentence instead of a fragment; replace "Password
sent in cleartext. Should only be used over TLS." with a single, grammatical
sentence such as "Password is sent in cleartext and should only be used over
TLS." so the README (doc/mysqlx/README.md) reads clearly.
- Around line 7-20: The README's top-line claim "Version 2.0.0 introduces ...
TLS support" is inconsistent with the Feature Matrix rows "TLS (frontend)" and
"TLS (backend)" that mark TLS as stubs; update the header and any other
duplicate occurrences (e.g., other summary lines) to state that TLS is
experimental/partial or stubbed (e.g., "experimental TLS support / TLS is
currently stubbed (frontend/backend modes pending)") so the "Version 2.0.0"
summary, the "TLS (frontend)" and "TLS (backend)" feature matrix rows, and any
repeated mentions all consistently indicate incomplete/experimental TLS rather
than full shipped support.
- Around line 93-105: The README table for mysqlx admin schema is out of sync
with the implementation in plugins/mysqlx/src/mysqlx_admin_schema.cpp (missing
columns like comment and incorrect/defaults); update the documentation to
exactly match the DDL in mysqlx_admin_schema.cpp: verify each column name, type,
CHECKs, and default values (e.g., ensure presence of comment column and
remove/add any defaults not present in the C++ DDL) for the sections around the
users table (username, active, require_tls, allowed_auth_methods, default_route,
policy_profile, backend_auth_mode, backend_username, backend_password,
attributes) and the other referenced blocks (lines ~114-123, 132-139) so the
README mirrors the source-of-truth DDL.
In `@docs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.md`:
- Around line 213-229: The markdown contains multiple fenced code blocks missing
a language tag (violating markdownlint MD040); update each fenced block in this
document to include an appropriate language identifier (e.g., use ```text for
commit/message or prose blocks and ```diff or ```patch where showing diffs) —
look for blocks around the described TLS feature notes such as the
commit/summary starting with "feat(mysqlx): add TLS infrastructure to
MysqlxDataStream" and the sections that mention symbols like sslstatus enum,
init_ssl(), do_ssl_handshake(), read_from_net(), write_to_net(), ssl_write_buf,
and has_ssl_pending_write() and add the language tokens consistently to the
fenced delimiters mentioned in the review (including the ranges called out).
- Around line 369-379: The TLS passthrough section is inconsistent: when
tls_mode is PASSTHROUGH the proxy forwards raw encrypted bytes and must not
inspect decrypted X Protocol frames; update the text around read_from_net() and
write_to_net() to state that in PASSTHROUGH mode raw TLS records are forwarded
without TLS termination and X Protocol frame parsing is disabled, and adjust the
commit message/description (the feat(mysqlx) commit snippet) to explicitly say
frame parsing only occurs when the proxy terminates TLS (i.e., not in
PASSTHROUGH).
In `@plugins/mysqlx/include/mysqlx_config_store.h`:
- Line 73: get_tls_mode() currently returns a const std::string& which allows
callers to hold a reference into mutable shared state and get invalidated on
reload; change the API to return std::string by value (update the declaration
from const std::string& get_tls_mode() const to std::string get_tls_mode()
const) and implement it to acquire a std::shared_lock<std::shared_mutex>
(mutex_) and return a copy of tls_mode_; apply the same change (signature +
shared_lock + return copy) to any other getters that currently return const
std::string& so callers never keep dangling references.
In `@plugins/mysqlx/include/mysqlx_thread.h`:
- Around line 32-33: Add a dedicated mutex to protect the listener collections
and lock it on every access: introduce a member std::mutex (e.g.
listener_mutex_) in the class declared in mysqlx_thread.h, then acquire a
std::lock_guard<std::mutex> (or std::unique_lock) inside add_listener(const
char* bind_addr, int port) and remove_listeners() when mutating listener_fds_,
and also take the same lock at the start of rebuild_poll_set() (and any other
readers that iterate listener_fds_) to prevent concurrent modification; ensure
all read/write paths that reference listener_fds_ use this mutex.
In `@plugins/mysqlx/Makefile`:
- Line 3: The current PROXYSQL_PATH assignment uses a shell while loop that can
hang if src/proxysql_global.cpp is missing; update the Makefile to compute
PROXYSQL_PATH starting from the directory of this Makefile (use $(dir $(lastword
$(MAKEFILE_LIST))) or dirname/realpath on that file) and then walk up to the
filesystem root with a bounded loop (stop when you reach / or after N
iterations) or fail-fast with a clear error; replace the existing PROXYSQL_PATH
:= $(shell while ... ) line with a safer shell snippet or Make expression that
breaks at root or after a max depth and emits an error if the file is not found.
---
Nitpick comments:
In `@doc/mysqlx/TESTING.md`:
- Around line 294-299: The docs currently pipe a remote installer directly to
the shell (curl ... | bash); change the example to first download the installer
(curl -o dbdeployer-install.sh https://.../dbdeployer-install.sh), fetch or
include the corresponding checksum/signature, verify the downloaded file (e.g.
sha256sum and compare against a provided checksum or use gpg --verify if a
signature is available), and only then run it (bash dbdeployer-install.sh). Also
show the analogous safe steps for the subsequent dbdeployer commands
(download/unpack/deploy) and include an explicit verification step or checksum
for any downloaded tarball (mysql-8.4.*.tar.xz) before running dbdeployer
unpack/deploy.
In `@plugins/mysqlx/include/mysqlx_config_store.h`:
- Around line 15-19: The enum MysqlxBackendAuthMode uses lower_snake_case
values; rename the constants to UPPER_SNAKE_CASE (e.g., MAPPED, SERVICE_ACCOUNT,
PASS_THROUGH) in the enum declaration and update all usages of
MysqlxBackendAuthMode::mapped, ::service_account, and ::pass_through to the new
names across the codebase (search for MysqlxBackendAuthMode and those
identifiers so callers, switches, and serialization/deserialization code are
updated accordingly) to comply with the UPPER_SNAKE_CASE constant style.
In `@plugins/mysqlx/include/mysqlx_connection.h`:
- Around line 51-57: The setters set_user, set_schema, and set_address (and the
analogous setters around lines 77-79) should defensively check incoming const
char* parameters for nullptr before assigning to the underlying std::string
members; update each setter to do something like if (param) member_ = param;
else member_.clear() (or assign an empty string) to avoid undefined behavior
when a null pointer is passed, ensuring all const char* -> std::string
assignments are guarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f908413d-4008-4b64-99de-6f84fa9de6cb
📒 Files selected for processing (32)
doc/mysqlx/ARCHITECTURE.mddoc/mysqlx/MYSQL_ROUTER_PARITY.mddoc/mysqlx/README.mddoc/mysqlx/TESTING.mddocs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.mdplugins/mysqlx/Makefileplugins/mysqlx/include/mysqlx_config_store.hplugins/mysqlx/include/mysqlx_connection.hplugins/mysqlx/include/mysqlx_data_stream.hplugins/mysqlx/include/mysqlx_plugin.hplugins/mysqlx/include/mysqlx_protocol.hplugins/mysqlx/include/mysqlx_session.hplugins/mysqlx/include/mysqlx_thread.hplugins/mysqlx/src/mysqlx_admin_schema.cppplugins/mysqlx/src/mysqlx_config_store.cppplugins/mysqlx/src/mysqlx_connection.cppplugins/mysqlx/src/mysqlx_data_stream.cppplugins/mysqlx/src/mysqlx_plugin.cppplugins/mysqlx/src/mysqlx_protocol.cppplugins/mysqlx/src/mysqlx_session.cppplugins/mysqlx/src/mysqlx_thread.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/mysqlx_backend_auth_unit-t.cpptest/tap/tests/unit/mysqlx_concurrent_unit-t.cpptest/tap/tests/unit/mysqlx_connection_unit-t.cpptest/tap/tests/unit/mysqlx_credential_verify_unit-t.cpptest/tap/tests/unit/mysqlx_data_stream_unit-t.cpptest/tap/tests/unit/mysqlx_message_dispatch_unit-t.cpptest/tap/tests/unit/mysqlx_robustness_unit-t.cpptest/tap/tests/unit/mysqlx_session_unit-t.cpptest/tap/tests/unit/mysqlx_thread_unit-t.cpptest/tap/tests/unit/mysqlx_tls_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/mysqlx/include/mysqlx_plugin.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use UPPER_SNAKE_CASE for constants and macros
Files:
plugins/mysqlx/include/mysqlx_protocol.hplugins/mysqlx/include/mysqlx_thread.hplugins/mysqlx/include/mysqlx_data_stream.hplugins/mysqlx/include/mysqlx_config_store.hplugins/mysqlx/include/mysqlx_session.hplugins/mysqlx/include/mysqlx_connection.h
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: See `doc/agents/project-conventions.md` for ProxySQL-specific rules including directories, build, test harness, and git workflow
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, and similar feature flags for feature tiers
Applied to files:
plugins/mysqlx/include/mysqlx_protocol.hplugins/mysqlx/Makefile
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: See `doc/agents/project-conventions.md` for ProxySQL-specific rules including directories, build, test harness, and git workflow
Applied to files:
doc/mysqlx/ARCHITECTURE.mddoc/mysqlx/README.mdplugins/mysqlx/Makefile
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
doc/mysqlx/TESTING.mdplugins/mysqlx/Makefile
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
doc/mysqlx/TESTING.mdplugins/mysqlx/Makefile
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
doc/mysqlx/TESTING.md
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Applied to files:
doc/mysqlx/TESTING.md
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
docs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.md
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Write class names in PascalCase with protocol prefixes (`MySQL_`, `PgSQL_`, `ProxySQL_`)
Applied to files:
plugins/mysqlx/Makefile
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Follow the three-stage GNU Make build pipeline: `deps` → `lib` → `src`
Applied to files:
plugins/mysqlx/Makefile
🪛 checkmake (0.2.2)
plugins/mysqlx/Makefile
[warning] 62-62: Missing required phony target "test"
(minphony)
🪛 Clang (14.0.6)
plugins/mysqlx/include/mysqlx_protocol.h
[error] 4-4: 'cstdint' file not found
(clang-diagnostic-error)
plugins/mysqlx/include/mysqlx_thread.h
[error] 4-4: 'cstdint' file not found
(clang-diagnostic-error)
plugins/mysqlx/include/mysqlx_data_stream.h
[error] 4-4: 'cstdint' file not found
(clang-diagnostic-error)
plugins/mysqlx/include/mysqlx_config_store.h
[error] 4-4: 'atomic' file not found
(clang-diagnostic-error)
🪛 LanguageTool
doc/mysqlx/README.md
[style] ~195-~195: To form a complete sentence, be sure to include a subject.
Context: ...PLAIN | Password sent in cleartext. Should only be used over TLS. | ### 6.2. Auth...
(MISSING_IT_THERE)
doc/mysqlx/TESTING.md
[style] ~130-~130: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...l E2E tests print a diagnostic and exit with success (TAP skip). E2E tests are also runnabl...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🪛 markdownlint-cli2 (0.22.0)
doc/mysqlx/ARCHITECTURE.md
[warning] 347-347: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
doc/mysqlx/MYSQL_ROUTER_PARITY.md
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.md
[warning] 213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 289-289: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 341-341: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 373-373: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 431-431: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 479-479: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 518-518: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 553-553: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 578-578: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 618-618: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 630-630: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
plugins/mysqlx/include/mysqlx_protocol.h (1)
16-20: Good defensive boundary for frame sizes.The explicit 16MB payload cap and clear header-size constant are solid safeguards for frame parsing and allocation paths.
plugins/mysqlx/include/mysqlx_session.h (1)
83-83: No action needed—credential_lookup_invocations are already guarded.All three invocation sites (lines 283, 349, 732 in
mysqlx_session.cpp) checkif (credential_lookup_)before calling the function, preventingstd::bad_function_callexceptions. The guard pattern is correct and comprehensive.plugins/mysqlx/include/mysqlx_data_stream.h (2)
89-89: No action needed—poll_fds_idxis already initialized defensively.The member variable
poll_fds_idxis initialized to-1in the constructor's member initializer list and is also reset to-1in theinit()method before first use. Defensive initialization is already in place.> Likely an incorrect or invalid review comment.
1-2: This include guard style is the established convention in the ProxySQL codebase.The
__IDENTIFIER_Hpattern for include guards is explicitly prescribed in the project's coding guidelines (CLAUDE.md). All mysqlx headers consistently use this style (__MYSQLX_SESSION_H,__MYSQLX_THREAD_H,__MYSQLX_CONNECTION_H, etc.). While C++ reserves__prefixed identifiers, ProxySQL has deliberately adopted this as the standard for include guards across the codebase; no change is needed.> Likely an incorrect or invalid review comment.
Comment on lines
+346
to
+368
| ``` | ||
| recv() returns N bytes (may be less than a full frame) | ||
| │ | ||
| ▼ | ||
| feed_bytes(data, N) | ||
| │ | ||
| ▼ | ||
| read_buf_ += data | ||
| │ | ||
| ▼ | ||
| try_parse_frame() | ||
| │ | ||
| ├── available < 5 bytes? → return false (need header) | ||
| │ | ||
| ├── decode payload_size from header | ||
| │ payload_size < 1 or > 16MB? → parse error | ||
| │ | ||
| ├── available < frame_total? → return false (need body) | ||
| │ | ||
| └── copy frame → complete_frames_.push() | ||
| advance read_offset_ | ||
| try again (may be more frames in buffer) | ||
| ``` |
There was a problem hiding this comment.
Add fenced-block language to satisfy MD040.
The diagram/code fence in this section is missing a language tag (text is fine for ASCII flow blocks).
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 347-347: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/mysqlx/ARCHITECTURE.md` around lines 346 - 368, The fenced ASCII diagram
block is missing a language tag which triggers MD040; update the Markdown block
surrounding the flow diagram (the block that shows recv(), feed_bytes(data, N),
read_buf_, try_parse_frame(), decode payload_size, complete_frames_.push(),
advance read_offset_, etc.) by adding a language identifier (e.g., change the
opening ``` to ```text) so the fence is properly tagged and MD040 is satisfied.
Comment on lines
+213
to
+229
| ``` | ||
| feat(mysqlx): add TLS infrastructure to MysqlxDataStream | ||
|
|
||
| Adds SSL support following ProxySQL's BIO-based pattern: | ||
| - sslstatus enum (OK/WANT_IO/FAIL) mapping OpenSSL errors | ||
| - init_ssl() creates per-session SSL object from shared SSL_CTX | ||
| - Memory BIO pairs (rbio_ssl/wbio_ssl) for encrypted I/O | ||
| - do_ssl_handshake() with WANT_IO handling | ||
| - TLS-aware read_from_net(): recv→BIO_write→SSL_read→feed_bytes | ||
| - TLS-aware write_to_net(): SSL_write→BIO_read→send | ||
| - ssl_write_buf for pending encrypted output | ||
| - has_ssl_pending_write() for poll integration | ||
| - Non-TLS paths unchanged when ssl_ is null | ||
|
|
||
| Tests: init with null ctx, encrypted read/write with self-signed cert, | ||
| pending write tracking, non-TLS passthrough | ||
| ``` |
There was a problem hiding this comment.
Add fence languages for markdownlint MD040 compliance.
Several fenced blocks are missing language identifiers. Please annotate them (for example text for commit message templates and dependency-order blocks).
Example pattern
-```
+```text
feat(mysqlx): add backend connect timeout
...
-```
+```Also applies to: 289-304, 341-352, 373-382, 431-448, 479-492, 518-528, 553-559, 578-590, 618-624, 630-638
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.md` around lines 213 -
229, The markdown contains multiple fenced code blocks missing a language tag
(violating markdownlint MD040); update each fenced block in this document to
include an appropriate language identifier (e.g., use ```text for commit/message
or prose blocks and ```diff or ```patch where showing diffs) — look for blocks
around the described TLS feature notes such as the commit/summary starting with
"feat(mysqlx): add TLS infrastructure to MysqlxDataStream" and the sections that
mention symbols like sslstatus enum, init_ssl(), do_ssl_handshake(),
read_from_net(), write_to_net(), ssl_write_buf, and has_ssl_pending_write() and
add the language tokens consistently to the fenced delimiters mentioned in the
review (including the ranges called out).
Comment on lines
+369
to
+379
| When passthrough is active, read_from_net() and write_to_net() operate on raw bytes without SSL processing. The X Protocol frames are still parsed since X Protocol runs inside TLS. | ||
|
|
||
| - [ ] **Step 3: Commit** | ||
|
|
||
| ``` | ||
| feat(mysqlx): add TLS passthrough mode | ||
|
|
||
| When tls_mode is PASSTHROUGH, raw encrypted bytes are forwarded | ||
| between client and backend without TLS termination. X Protocol | ||
| frame parsing operates on the decrypted stream visible to each side. | ||
|
|
There was a problem hiding this comment.
TLS passthrough section is logically inconsistent with frame parsing.
Line 369 says passthrough forwards raw TLS records, but Line 379 still claims X Protocol frame parsing on decrypted streams. In passthrough mode, the proxy should not inspect decrypted X frames.
Suggested doc fix
-When passthrough is active, read_from_net() and write_to_net() operate on raw bytes without SSL processing. The X Protocol frames are still parsed since X Protocol runs inside TLS.
+When passthrough is active, read_from_net() and write_to_net() operate on raw TLS bytes without SSL termination in ProxySQL. No X Protocol frame parsing or inspection is performed in this mode.🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 373-373: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-10-mysqlx-feature-gaps.md` around lines 369 -
379, The TLS passthrough section is inconsistent: when tls_mode is PASSTHROUGH
the proxy forwards raw encrypted bytes and must not inspect decrypted X Protocol
frames; update the text around read_from_net() and write_to_net() to state that
in PASSTHROUGH mode raw TLS records are forwarded without TLS termination and X
Protocol frame parsing is disabled, and adjust the commit message/description
(the feat(mysqlx) commit snippet) to explicitly say frame parsing only occurs
when the proxy terminates TLS (i.e., not in PASSTHROUGH).
| @@ -0,0 +1,64 @@ | |||
| #!/bin/make -f | |||
|
|
|||
| PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd) | |||
There was a problem hiding this comment.
Root-path discovery can loop forever.
The while loop has no fail-fast condition at /; if src/proxysql_global.cpp is not found, make may hang indefinitely. Prefer deriving the root relative to $(lastword $(MAKEFILE_LIST)) or add an explicit max-depth/root break.
Proposed fix
-PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd)
+PROXYSQL_PATH := $(abspath $(dir $(lastword $(MAKEFILE_LIST)))/../..)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mysqlx/Makefile` at line 3, The current PROXYSQL_PATH assignment uses
a shell while loop that can hang if src/proxysql_global.cpp is missing; update
the Makefile to compute PROXYSQL_PATH starting from the directory of this
Makefile (use $(dir $(lastword $(MAKEFILE_LIST))) or dirname/realpath on that
file) and then walk up to the filesystem root with a bounded loop (stop when you
reach / or after N iterations) or fail-fast with a clear error; replace the
existing PROXYSQL_PATH := $(shell while ... ) line with a safer shell snippet or
Make expression that breaks at root or after a max depth and emits an error if
the file is not found.
The MYSQLX plugin admin commands (LOAD/SAVE MYSQLX USERS/ROUTES/ BACKEND_ENDPOINTS/VARIABLES TO RUNTIME/FROM MEMORY) were returning 'syntax error' because the MYSQLX dispatch block in Admin_Handler.cpp was placed AFTER the generic LOAD/SAVE catch-all at line 3883. This meant all LOAD/SAVE commands were intercepted by admin_handler_command_load_or_save() which knows nothing about MYSQLX. Changes: - Move MYSQLX dispatch block before the generic LOAD/SAVE handler so MYSQLX commands are checked first (Admin_Handler.cpp lines ~3883-3900) - Add LOAD_MYSQLX_VARIABLES_FROM_MEMORY and SAVE_MYSQLX_VARIABLES_TO_MEMORY alias vectors for MYSQLX VARIABLES commands - Add ProxySQL_Admin::materialize_plugin_tables() in Admin_Bootstrap.cpp that merges plugin table definitions into admin/config/stats table lists and creates the tables in SQLite - Call materialize_plugin_tables() in main.cpp after LoadConfiguredPlugins() but before StartConfiguredPlugins() so plugin tables exist before the plugin starts - Declare materialize_plugin_tables() in proxysql_admin.h This ensures that when a user issues e.g. 'LOAD MYSQLX USERS TO RUNTIME', the command reaches dispatch_plugin_admin_command() which forwards it to the MYSQLX plugin's registered admin command handlers.
The MYSQLX admin command dispatch was sending the raw alias query text
(e.g., 'LOAD MYSQLX USERS TO RUN') to dispatch_plugin_admin_command(),
but the plugin only registered the canonical forms (e.g., 'LOAD MYSQLX
USERS TO RUNTIME'). This caused all alias variants to fail with 'MYSQLX
plugin is not loaded' even when the plugin was loaded and the canonical
command worked.
Add resolve_admin_alias_to_canonical() helper that matches an alias
against a vector and returns the specified canonical command string.
Update the MYSQLX dispatch block to resolve each alias to its canonical
form before dispatching to the plugin manager.
Tested all 20 variants (8 command groups x 2-4 aliases each):
- LOAD/SAVE MYSQLX USERS/ROUTES/BACKEND_ENDPOINTS/VARIABLES
- TO RUNTIME, FROM MEMORY, TO RUN, FROM MEM, TO MEMORY, FROM RUNTIME,
TO MEM, FROM RUN aliases all work correctly.
ProxySQL's plugin system was previously undocumented except for the header file (include/ProxySQL_Plugin.h) and the mysqlx reference implementation. A developer wanting to write a plugin had to reverse-engineer the contract from those sources. Add doc/PLUGIN_API.md covering: - How plugins are loaded (plugins= directive in proxysql.cnf) - Startup/shutdown lifecycle sequence - The ProxySQL_PluginDescriptor contract (name, abi_version, init/start/stop/status_json) - The proxysql_plugin_descriptor_v1 entry point - ProxySQL_PluginServices callbacks (register_table, register_command, log_message, get_admindb/configdb/statsdb, snapshots) - ProxySQL_PluginCommandContext and ProxySQL_PluginCommandResult types - Complete minimal plugin example - Build requirements and compiler compatibility notes - Admin integration patterns (three-tier config model, table registration) - Current limitations (no hot-loading, no dependency resolution, compiler coupling) - Pointer to the mysqlx reference implementation
ProxySQL uses a three-tier configuration model: DISK ↔ MEMORY ↔ RUNTIME. The MYSQLX plugin previously only supported the MEMORY ↔ RUNTIME tier (LOAD MYSQLX USERS TO RUNTIME, SAVE MYSQLX USERS TO MEMORY, etc.). Configuration was lost on restart because there was no way to persist it to disk. Add the missing DISK ↔ MEMORY tier commands for all four MYSQLX object types: LOAD MYSQLX USERS FROM DISK (disk → memory) SAVE MYSQLX USERS TO DISK (memory → disk) LOAD MYSQLX ROUTES FROM DISK SAVE MYSQLX ROUTES TO DISK LOAD MYSQLX BACKEND ENDPOINTS FROM DISK SAVE MYSQLX BACKEND ENDPOINTS TO DISK LOAD MYSQLX VARIABLES FROM DISK SAVE MYSQLX VARIABLES TO DISK Implementation: - Add disk_to_memory() and memory_to_disk() helpers in mysqlx_admin_schema.cpp that use qualified SQLite table names (main.<table> and disk.<table>) to copy between the in-memory admin database and the on-disk config database - Register 8 new command handlers in mysqlx_register_admin_schema() - Add alias vectors for all disk commands in Admin_Handler.cpp - Add resolve_admin_alias_to_canonical() calls for disk commands in the MYSQLX dispatch block Tested: Insert → SAVE TO DISK → DELETE → LOAD FROM DISK round-trip confirms data persistence works for all four object types.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/PLUGIN_API.md`:
- Around line 21-25: Add language identifiers to the fenced code blocks in
PLUGIN_API examples to satisfy markdownlint MD040 and improve rendering: update
the plugin list blocks that contain the plugins = ( "/path/to/my_plugin.so" )
and the multi-plugin example ("/path/to/protocol_a.so",
"/path/to/protocol_b.so") to use an appropriate language tag such as ini, and
change the final example block that contains "DISK (on-disk SQLite) ↔ MEMORY
(in-memory admin tables) ↔ RUNTIME (live state)" to use text (or console) as
the fence language; locate the snippets by the unique example contents (plugins
= (...) and the DISK ↔ MEMORY ↔ RUNTIME line) and add the language tokens to the
opening triple-backtick lines.
- Around line 421-430: Update the MySQL X Protocol plugin description to remove
the unsupported TLS claim: locate the "MySQL X Protocol plugin
(`plugins/mysqlx/`)" feature list and delete or edit the bullet "TLS integration
via ProxySQL's global SSL context" so the doc no longer advertises TLS support
for the mysqlx plugin; leave the other bullets (admin tables, admin commands,
plugin threads, connection pooling, etc.) unchanged.
In `@lib/Admin_Bootstrap.cpp`:
- Around line 1349-1366: The merge_new lambda currently silently skips duplicate
table names while materialize_plugin_tables() does not — make the bootstrap
behavior consistent with ProxySQL_Admin::init by detecting duplicate table names
(across target, existing builtin defs and
plugin_manager->tables(ProxySQL_PluginDBKind::...)) and failing loudly: when
merge_new finds a name collision using strcasecmp with existing->table_name and
def.table_name, call the same failure path used in ProxySQL_Admin::init (return
an error/throw or log and abort bootstrap) instead of simply skipping, or
alternatively make ProxySQL_Admin::init accept the same silent-skip policy;
update merge_new and any callers
(materialize_plugin_tables/insert_into_tables_defs) to use the chosen behavior
so duplicate plugin/builtin table name conflicts are consistently surfaced.
- Around line 1372-1373: In materialize_plugin_tables(), avoid blindly
re-attaching aliases by either checking for existing attachments or detaching
first: before calling __attach_db(admindb, configdb, "disk") and
__attach_db(admindb, statsdb, "stats"), detect whether those aliases are already
attached (or call the existing detach helper used elsewhere, e.g., __detach_db
or the pattern used at lines 1224/3090) and only perform __attach_db if not
attached (or after successful detach); update materialize_plugin_tables() to
follow the established detach-then-attach pattern to prevent duplicate ATTACH
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1f6d414-236d-41f8-9db9-ab4d591f10f1
📒 Files selected for processing (6)
doc/PLUGIN_API.mdinclude/proxysql_admin.hlib/Admin_Bootstrap.cpplib/Admin_Handler.cppplugins/mysqlx/src/mysqlx_admin_schema.cppsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/proxysql_admin.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.{cpp,h,hpp}: Write class names in PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Use snake_case for member variables
Require C++17 for all code
Use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, and similar feature flags for feature tiers
Use RAII for resource management
Use jemalloc for memory allocation
Use Pthread mutexes for synchronization
Usestd::atomic<>for counters
Files:
lib/Admin_Handler.cpplib/Admin_Bootstrap.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use UPPER_SNAKE_CASE for constants and macros
Files:
lib/Admin_Handler.cpplib/Admin_Bootstrap.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in the library
Files:
lib/Admin_Handler.cpplib/Admin_Bootstrap.cpp
🧠 Learnings (10)
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, and similar feature flags for feature tiers
Applied to files:
lib/Admin_Bootstrap.cppdoc/PLUGIN_API.md
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/Admin_Bootstrap.cppdoc/PLUGIN_API.md
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Write class names in PascalCase with protocol prefixes (`MySQL_`, `PgSQL_`, `ProxySQL_`)
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: See `doc/agents/project-conventions.md` for ProxySQL-specific rules including directories, build, test harness, and git workflow
Applied to files:
doc/PLUGIN_API.md
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
doc/PLUGIN_API.md
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/PLUGIN_API.md
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
doc/PLUGIN_API.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
doc/PLUGIN_API.md
🪛 LanguageTool
doc/PLUGIN_API.md
[style] ~373-~373: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...dmin_db` for the live runtime state. 3. Register admin commands for each tier transition...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.0)
doc/PLUGIN_API.md
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 364-364: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
lib/Admin_Handler.cpp (2)
527-538: Nice extraction of the alias canonicalizer.Pulling the exact-length, case-insensitive match into a helper makes the new plugin dispatch path easier to reason about.
318-378: No issues found. All MYSQLX command families are consistently canonicalized and registered.The code correctly defines and dispatches only four command families (USERS, ROUTES, BACKEND ENDPOINTS, VARIABLES). No
LOAD/SAVE MYSQLX LISTENERScommands are registered anywhere. Listeners in the codebase are internal route-based infrastructure inmysqlx_worker.cpp, not exposed as admin commands. All 16 canonical command vectors match their dispatch entries.doc/PLUGIN_API.md (1)
19-40: No changes needed—the doc correctly usespluginsas the configuration directive name.The parser, tests, and implementation all confirm the config directive is
plugins(notplugin_modules). The internal variable that stores the parsed list is calledplugin_modules, which is a separate naming concern. The examples in doc/PLUGIN_API.md lines 19–40 are correct.> Likely an incorrect or invalid review comment.lib/Admin_Bootstrap.cpp (3)
41-41: Looks good: plugin manager include is correctly wired in.This include is necessary for the plugin table merge path introduced later in
ProxySQL_Admin::init.
931-939: Feature-flag gating remains correct.The MYSQLX-adjacent MCP stats tables stay guarded by
#ifdef PROXYSQLGENAI, which keeps tiered feature compilation intact. As per coding guidelines: “Use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, and similar feature flags for feature tiers”.
942-957: Remove null/empty pointer checks—already enforced upstream.Plugin table definitions processed by
merge_plugin_tablesare sourced fromplugin_manager->tables(kind), which returns only successfully registered tables. The registration gateway (ProxySQL_PluginManager::register_table()) already validates thattable_nameandtable_defare non-null and non-empty before storing them, so additional checks inmerge_plugin_tablesare redundant.> Likely an incorrect or invalid review comment.
Comment on lines
+21
to
+25
| ``` | ||
| plugins = ( | ||
| "/path/to/my_plugin.so" | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Add languages to the fenced code blocks.
These examples already trip markdownlint MD040 and render less clearly in GitHub viewers.
Suggested doc fix
-```
+```ini
plugins = (
"/path/to/my_plugin.so"
)- +ini
plugins = (
"/path/to/protocol_a.so",
"/path/to/protocol_b.so"
)
-```
+```text
DISK (on-disk SQLite) ↔ MEMORY (in-memory admin tables) ↔ RUNTIME (live state)
</details>
Also applies to: 29-33, 364-366
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @doc/PLUGIN_API.md around lines 21 - 25, Add language identifiers to the
fenced code blocks in PLUGIN_API examples to satisfy markdownlint MD040 and
improve rendering: update the plugin list blocks that contain the plugins = (
"/path/to/my_plugin.so" ) and the multi-plugin example
("/path/to/protocol_a.so", "/path/to/protocol_b.so") to use an appropriate
language tag such as ini, and change the final example block that contains "DISK
(on-disk SQLite) ↔ MEMORY (in-memory admin tables) ↔ RUNTIME (live state)"
to use text (or console) as the fence language; locate the snippets by the
unique example contents (plugins = (...) and the DISK ↔ MEMORY ↔ RUNTIME line)
and add the language tokens to the opening triple-backtick lines.
</details>
<!-- fingerprinting:phantom:medusa:grasshopper:08818a6d-c2de-43ef-a1e2-78f596390fa3 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+421
to
+430
| The MySQL X Protocol plugin (`plugins/mysqlx/`) is the reference implementation | ||
| of a full ProxySQL plugin. It demonstrates: | ||
|
|
||
| - Multi-file plugin structure with separate headers/sources | ||
| - Custom `Makefile` within the ProxySQL build tree | ||
| - Admin table registration (config + runtime + stats tables) | ||
| - Admin command handlers with the three-tier model | ||
| - Plugin-owned threads with listener sockets | ||
| - TLS integration via ProxySQL's global SSL context | ||
| - Connection pooling for backend connections |
There was a problem hiding this comment.
Remove the TLS claim from the mysqlx feature list.
The PR scope explicitly says Phase 2 excludes TLS/SSL, so this bullet advertises a capability users cannot rely on yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/PLUGIN_API.md` around lines 421 - 430, Update the MySQL X Protocol plugin
description to remove the unsupported TLS claim: locate the "MySQL X Protocol
plugin (`plugins/mysqlx/`)" feature list and delete or edit the bullet "TLS
integration via ProxySQL's global SSL context" so the doc no longer advertises
TLS support for the mysqlx plugin; leave the other bullets (admin tables, admin
commands, plugin threads, connection pooling, etc.) unchanged.
Comment on lines
+1349
to
+1366
| auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs) { | ||
| for (const auto& def : defs) { | ||
| bool exists = false; | ||
| for (const auto* existing : *target) { | ||
| if (strcasecmp(existing->table_name, def.table_name) == 0) { | ||
| exists = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!exists) { | ||
| insert_into_tables_defs(target, def.table_name, def.table_def); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)); | ||
| merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db)); | ||
| merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)); |
There was a problem hiding this comment.
Conflict behavior is inconsistent with bootstrap merge logic.
materialize_plugin_tables() silently skips duplicate names, while ProxySQL_Admin::init fails on duplicate plugin/builtin names. This inconsistency can mask schema conflicts and leave plugins running against unexpected table definitions.
♻️ Suggested consistency fix
- auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs) {
+ auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs, const char* db_name) -> bool {
for (const auto& def : defs) {
bool exists = false;
for (const auto* existing : *target) {
if (strcasecmp(existing->table_name, def.table_name) == 0) {
exists = true;
break;
}
}
- if (!exists) {
- insert_into_tables_defs(target, def.table_name, def.table_def);
- }
+ if (exists) {
+ proxy_error("Plugin table %s for %s conflicts with an existing table name\n", def.table_name, db_name);
+ return false;
+ }
+ insert_into_tables_defs(target, def.table_name, def.table_def);
}
+ return true;
};
- merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db));
- merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db));
- merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db));
+ if (!merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db), "admin") ||
+ !merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db), "config") ||
+ !merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db), "stats")) {
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Admin_Bootstrap.cpp` around lines 1349 - 1366, The merge_new lambda
currently silently skips duplicate table names while materialize_plugin_tables()
does not — make the bootstrap behavior consistent with ProxySQL_Admin::init by
detecting duplicate table names (across target, existing builtin defs and
plugin_manager->tables(ProxySQL_PluginDBKind::...)) and failing loudly: when
merge_new finds a name collision using strcasecmp with existing->table_name and
def.table_name, call the same failure path used in ProxySQL_Admin::init (return
an error/throw or log and abort bootstrap) instead of simply skipping, or
alternatively make ProxySQL_Admin::init accept the same silent-skip policy;
update merge_new and any callers
(materialize_plugin_tables/insert_into_tables_defs) to use the chosen behavior
so duplicate plugin/builtin table name conflicts are consistently surfaced.
Comment on lines
+1372
to
+1373
| __attach_db(admindb, configdb, (char *)"disk"); | ||
| __attach_db(admindb, statsdb, (char *)"stats"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify idempotency/behavior of __attach_db and current call flow.
rg -n -C5 '\b__attach_db\s*\(' --type=cpp --type=h
rg -n -C5 '\bmaterialize_plugin_tables\s*\(' --type=cpp --type=h
rg -n -C5 'ATTACH DATABASE|database_list' --type=cpp --type=hRepository: sysown/proxysql
Length of output: 13462
🏁 Script executed:
# Get the complete materialize_plugin_tables function
sed -n '1345,1374p' lib/Admin_Bootstrap.cppRepository: sysown/proxysql
Length of output: 1189
🏁 Script executed:
# Check if execute() has error handling for ATTACH errors
rg -n -A10 'SQLite3DB.*execute' include/SQLite3_Server.h | head -50Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Look for error handling in SQLite3DB execute method
fd -e hpp -e h | xargs grep -l 'SQLite3DB' | head -5Repository: sysown/proxysql
Length of output: 204
🏁 Script executed:
# Find SQLite3DB header file
find . -name "SQLite3_Server.h" -o -name "*SQLite3*" -type f | grep -E '\.(h|hpp)$'Repository: sysown/proxysql
Length of output: 85
🏁 Script executed:
# Look for SQLite3DB execute method implementation
rg -n 'int.*execute\s*\(' lib/SQLite3_Server.cpp | head -20Repository: sysown/proxysql
Length of output: 121
🏁 Script executed:
# Check error handling when ATTACH DATABASE is called on existing alias
rg -n -B5 -A10 'DETACH DATABASE' lib/ProxySQL_Admin.cpp | head -60Repository: sysown/proxysql
Length of output: 1643
🏁 Script executed:
# Read SQLite3_Server.h to understand execute method
cat -n include/SQLite3_Server.h | grep -A 20 'execute'Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Look at the complete initialization and materialize_plugin_tables sequence
rg -n -B5 -A10 'ProxySQL_Main_init_Admin|materialize_plugin_tables' src/main.cppRepository: sysown/proxysql
Length of output: 1258
🏁 Script executed:
# Check if there's error suppression in execute() when handling ATTACH
rg -n 'execute\s*\(' include/SQLite3_Server.h -A 5 | head -40Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Search for SQLite3DB class and execute method implementation
rg -n 'class SQLite3DB' --type=h --type=cppRepository: sysown/proxysql
Length of output: 829
🏁 Script executed:
# Find execute() method definition
rg -n 'int\s+execute\(' --type=cpp --type=h | grep -i sqlite | head -10Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Look for any error checking or logging around ATTACH failures
rg -n 'database.*is already in use|database.*alias|ATTACH.*error' --type=cppRepository: sysown/proxysql
Length of output: 44233
🏁 Script executed:
# Read the SQLite3DB header file to find execute method
head -250 include/sqlite3db.h | tail -100Repository: sysown/proxysql
Length of output: 3068
🏁 Script executed:
# Search for execute method in sqlite3db.h
rg -n 'int.*execute.*{' include/sqlite3db.h -A 10 | head -50Repository: sysown/proxysql
Length of output: 152
🏁 Script executed:
# Check if there's any error handling/wrapping around ATTACH in existing code
rg -n '__attach_db' lib/Admin_Bootstrap.cpp -B2 -A2Repository: sysown/proxysql
Length of output: 793
🏁 Script executed:
# Find the execute method implementation to see error handling
rg -n 'bool SQLite3DB::execute' lib/ -A 15 | head -40Repository: sysown/proxysql
Length of output: 2179
🏁 Script executed:
# Check what happens if execute fails - look for assert_on_error usage
rg -n 'assert_on_error' lib/ -B2 -A2 | head -60Repository: sysown/proxysql
Length of output: 1122
🏁 Script executed:
# Verify the exact sequence: init() calls materialize_plugin_tables() after init?
sed -n '1539,1545p' src/main.cppRepository: sysown/proxysql
Length of output: 230
Add guards to prevent re-attaching already-attached database aliases in materialize_plugin_tables().
Lines 1372–1373 unconditionally call __attach_db() to attach disk and stats. Since init() already attaches these aliases at lines 993–994 and materialize_plugin_tables() is invoked immediately after initialization (main.cpp:1541), duplicate ATTACH attempts occur. While SQLite logs the error, the failure is silent and subsequent operations on these aliases may fail. The established pattern in the codebase (see lines 1224 and 3090) detaches before re-attaching. Either guard the calls with existence checks or detach before re-attaching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Admin_Bootstrap.cpp` around lines 1372 - 1373, In
materialize_plugin_tables(), avoid blindly re-attaching aliases by either
checking for existing attachments or detaching first: before calling
__attach_db(admindb, configdb, "disk") and __attach_db(admindb, statsdb,
"stats"), detect whether those aliases are already attached (or call the
existing detach helper used elsewhere, e.g., __detach_db or the pattern used at
lines 1224/3090) and only perform __attach_db if not attached (or after
successful detach); update materialize_plugin_tables() to follow the established
detach-then-attach pattern to prevent duplicate ATTACH errors.
The mysqlx_admin_schema_unit-t.cpp test was asserting exactly 6 registered commands, but the addition of 8 disk commands brought the total to 16. This caused the test to fail. Updates to mysqlx_admin_schema_unit-t.cpp: - Update command count assertion from 6 to 16 - Add assertions for all 8 new disk commands - Add assertions for mysqlx_variables and runtime_mysqlx_variables tables - Increase plan from 15 to 25 - Add minimal MysqlxPluginContext stub for mysqlx_context() to avoid pulling in the full plugin dependency chain New test: mysqlx_admin_commands_unit-t.cpp (27 assertions) - Tests all 8 memory↔runtime LOAD/SAVE commands with real SQLite databases - Verifies data round-trips: insert → LOAD TO RUNTIME → verify runtime table → DELETE memory → SAVE TO MEMORY → verify memory table restored - Tests that re-LOAD replaces (not appends) runtime data - Tests error handling when admindb is null Both tests pass: 25/25 + 27/27 = 52 assertions, 0 failures.
…M DISK Add mysqlx_admin_disk_commands_unit-t with 32 assertions covering the disk persistence tier of the MYSQLX admin interface. Tests verify full round-trip data integrity for all four MYSQLX object types: - mysqlx_users: SAVE to disk, clear memory, LOAD from disk, verify rows and individual column values (username, active flag) are preserved. - mysqlx_routes: round-trip with destination_hostgroup value check. - mysqlx_backend_endpoints: round-trip with mysqlx_port value check. - mysqlx_variables: round-trip with per-variable value verification for both thread_pool_size and max_cached_connections. Also tests error handling: - SAVE/LOAD with null admindb returns non-zero error code. Uses shared-cache in-memory SQLite (file: URI with cache=shared) so that the admin DB can ATTACH the config DB and disk commands work correctly without filesystem artifacts. Update Makefile to include the new test in UNIT_TESTS list with proper build rule linking mysqlx_admin_schema.cpp and mysqlx_config_store.cpp.
…_to_canonical Add mysqlx_admin_alias_resolution_unit-t with 23 assertions testing the resolve_admin_alias_to_canonical() function that maps admin command aliases (e.g. "TO RUN", "FROM MEM") to their canonical plugin form (e.g. "TO RUNTIME", "FROM MEMORY"). Test coverage: - All 4 alias variants for memory↔runtime commands resolve correctly - Case-insensitive matching (lowercase, mixed case) - Resolved pointer identity equals the canonical string pointer - Negative cases: unrelated queries, empty query, partial query - Empty alias vector returns nullptr - Single-element alias vector: match and non-match - Backend endpoints aliases (4-variant with FROM MEM / TO RUN) - Disk aliases (single canonical form, no short aliases) - Variables aliases across all 4 memory variants - Cross-tier rejection: FROM DISK does not match memory aliases The test links directly against libproxysql.a where the function lives, using extern declaration to access the non-static function.
Add sync_disk_to_memory() and copy_to_runtime() to mysqlx_start() so the MYSQLX plugin automatically restores its configuration from the persisted config DB when ProxySQL restarts. Startup sequence in mysqlx_start(): 1. sync_disk_to_memory() — copies rows from disk.mysqlx_* tables (config DB, ATTACHed as 'disk') into the admin DB's mysqlx_* tables, but only if the disk tables have data. 2. copy_to_runtime() — copies admin DB mysqlx_* tables into runtime_mysqlx_* tables, mirroring what LOAD MYSQLX * TO RUNTIME does manually. 3. load_from_runtime() — existing code reads runtime tables into the in-memory config store. This ensures routes, users, backend endpoints, and variables survive ProxySQL restarts without requiring manual LOAD commands after reboot. Note: dynamic listener addition (starting X Protocol listeners after the thread event loop is running) is not yet implemented. The listener is only created during start() based on the loaded config. A future change will add hot-reload capability for routes.
Two critical fixes for MYSQLX plugin config persistence: 1. materialize_plugin_tables() no longer calls check_and_build_standard_tables() for plugin tables. The previous implementation added plugin tables to the standard table defs vector, then check_and_build_standard_tables() would DROP TABLE IF EXISTS + CREATE TABLE for each — destroying persisted data in the config DB on every restart. Now plugin tables are created with a plain execute(table_def) which uses CREATE TABLE (fails harmlessly if the table already exists, preserving its data). 2. sync_disk_to_memory() and copy_to_runtime() in mysqlx_start() automatically restore MYSQLX config from the persisted config DB: - sync_disk_to_memory: copies disk.mysqlx_* → main.mysqlx_* (admin DB) - copy_to_runtime: copies main.mysqlx_* → main.runtime_mysqlx_* This means routes, users, backend endpoints, and variables configured via SAVE MYSQLX * TO DISK are automatically available at startup, allowing the X Protocol listener to bind the configured ports immediately. Verified: ProxySQL starts with port 33060 listening when MYSQLX routes were previously saved to disk.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/Admin_Bootstrap.cpp (2)
1349-1366:⚠️ Potential issue | 🟠 MajorConflict behavior is inconsistent with
init()merge logic.
merge_newsilently skips duplicate table names (lines 1358-1360), whilemerge_plugin_tablesininit()(lines 951-954) logs an error and returnsfalse. This inconsistency can mask schema conflicts for plugins loaded duringLoadConfiguredPlugins().Consider applying the same fail-fast behavior as
init()for consistent conflict handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Admin_Bootstrap.cpp` around lines 1349 - 1366, merge_new currently ignores duplicate table names when merging plugin table defs into tables_defs_admin/config/stats, which differs from init()'s merge_plugin_tables behavior that logs an error and returns false; change merge_new to perform the same fail-fast conflict handling: when strcasecmp(existing->table_name, def.table_name) == 0 detect the conflict, log an error (same message style as merge_plugin_tables) and propagate failure to the caller (return bool) instead of silently skipping, update callers (where merge_new is invoked for plugin_manager->tables(ProxySQL_PluginDBKind::admin_db/config_db/stats_db)) to check the boolean and abort LoadConfiguredPlugins()/init() flow on false, and retain use of insert_into_tables_defs only when no conflict is found.
1378-1379:⚠️ Potential issue | 🟠 MajorRedundant
__attach_dbcalls for already-attached aliases.
init()already attachesdiskandstatsat lines 993-994. Sincematerialize_plugin_tables()is called afterinit()completes (per main.cpp:1541), these aliases are already attached. SQLite will log errors for duplicate ATTACH attempts.Either guard with existence checks or remove these calls if the attachment from
init()is sufficient for the use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Admin_Bootstrap.cpp` around lines 1378 - 1379, The two __attach_db(admindb, configdb, "disk") and __attach_db(admindb, statsdb, "stats") calls in materialize_plugin_tables() are redundant because init() already attaches "disk" and "stats" (see init()) and cause duplicate ATTACH errors; remove these duplicate calls from materialize_plugin_tables() or wrap them with an existence check (e.g., query PRAGMA database_list or track attached aliases) before calling __attach_db so you only attach admindb->configdb and admindb->statsdb when those aliases are not already present.
🧹 Nitpick comments (1)
lib/Admin_Bootstrap.cpp (1)
931-939: Minor formatting inconsistency within#ifdef PROXYSQLGENAIblock.Lines 932-933 and 937-938 have extra tab indentation while lines 935-936 do not, creating inconsistent formatting within the same preprocessor block. This is a minor nitpick and doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Admin_Bootstrap.cpp` around lines 931 - 939, Normalize indentation inside the `#ifdef` PROXYSQLGENAI block so all calls to insert_into_tables_defs use the same indentation level; locate the block guarded by PROXYSQLGENAI and adjust the lines that call insert_into_tables_defs (references: insert_into_tables_defs, tables_defs_stats, STATS_SQLITE_TABLE_MCP_QUERY_TOOLS_COUNTERS, STATS_SQLITE_TABLE_MCP_QUERY_TOOLS_COUNTERS_RESET, STATS_SQLITE_TABLE_MCP_QUERY_DIGEST, STATS_SQLITE_TABLE_MCP_QUERY_DIGEST_RESET, STATS_SQLITE_TABLE_MCP_QUERY_RULES) to match the surrounding lines' indentation for consistent formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/Admin_Bootstrap.cpp`:
- Around line 1349-1366: merge_new currently ignores duplicate table names when
merging plugin table defs into tables_defs_admin/config/stats, which differs
from init()'s merge_plugin_tables behavior that logs an error and returns false;
change merge_new to perform the same fail-fast conflict handling: when
strcasecmp(existing->table_name, def.table_name) == 0 detect the conflict, log
an error (same message style as merge_plugin_tables) and propagate failure to
the caller (return bool) instead of silently skipping, update callers (where
merge_new is invoked for
plugin_manager->tables(ProxySQL_PluginDBKind::admin_db/config_db/stats_db)) to
check the boolean and abort LoadConfiguredPlugins()/init() flow on false, and
retain use of insert_into_tables_defs only when no conflict is found.
- Around line 1378-1379: The two __attach_db(admindb, configdb, "disk") and
__attach_db(admindb, statsdb, "stats") calls in materialize_plugin_tables() are
redundant because init() already attaches "disk" and "stats" (see init()) and
cause duplicate ATTACH errors; remove these duplicate calls from
materialize_plugin_tables() or wrap them with an existence check (e.g., query
PRAGMA database_list or track attached aliases) before calling __attach_db so
you only attach admindb->configdb and admindb->statsdb when those aliases are
not already present.
---
Nitpick comments:
In `@lib/Admin_Bootstrap.cpp`:
- Around line 931-939: Normalize indentation inside the `#ifdef` PROXYSQLGENAI
block so all calls to insert_into_tables_defs use the same indentation level;
locate the block guarded by PROXYSQLGENAI and adjust the lines that call
insert_into_tables_defs (references: insert_into_tables_defs, tables_defs_stats,
STATS_SQLITE_TABLE_MCP_QUERY_TOOLS_COUNTERS,
STATS_SQLITE_TABLE_MCP_QUERY_TOOLS_COUNTERS_RESET,
STATS_SQLITE_TABLE_MCP_QUERY_DIGEST, STATS_SQLITE_TABLE_MCP_QUERY_DIGEST_RESET,
STATS_SQLITE_TABLE_MCP_QUERY_RULES) to match the surrounding lines' indentation
for consistent formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 863db097-17fb-441f-9c7e-5e84dea25359
📒 Files selected for processing (7)
lib/Admin_Bootstrap.cppplugins/mysqlx/src/mysqlx_plugin.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/mysqlx_admin_alias_resolution_unit-t.cpptest/tap/tests/unit/mysqlx_admin_commands_unit-t.cpptest/tap/tests/unit/mysqlx_admin_disk_commands_unit-t.cpptest/tap/tests/unit/mysqlx_admin_schema_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.{cpp,h,hpp}: Write class names in PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Use snake_case for member variables
Require C++17 for all code
Use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, and similar feature flags for feature tiers
Use RAII for resource management
Use jemalloc for memory allocation
Use Pthread mutexes for synchronization
Usestd::atomic<>for counters
Files:
lib/Admin_Bootstrap.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use UPPER_SNAKE_CASE for constants and macros
Files:
lib/Admin_Bootstrap.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in the library
Files:
lib/Admin_Bootstrap.cpp
🧠 Learnings (5)
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, and similar feature flags for feature tiers
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Write class names in PascalCase with protocol prefixes (`MySQL_`, `PgSQL_`, `ProxySQL_`)
Applied to files:
lib/Admin_Bootstrap.cpp
🔇 Additional comments (2)
lib/Admin_Bootstrap.cpp (2)
41-41: LGTM!The include for
ProxySQL_PluginManager.his correctly added to support the new plugin table integration.
941-966: LGTM!The plugin table merging logic in
init()correctly implements fail-fast behavior on name conflicts:
- Uses case-insensitive comparison (
strcasecmp) consistent withProxySQL_PluginManager::register_table()- Logs descriptive error and returns
falseto abort initialization on conflict- Properly short-circuits if any merge step fails
- PluginManager: don't mark plugin stopped when stop() fails - PluginManager: add mutex to proxysql_get_plugin_manager() access - PluginManager: reject duplicate plugin paths in load() - mysqlx_thread: add listener_mutex_ for listener_fds_ synchronization - mysqlx_config_store: return tls_mode by value, not by reference - Makefile: propagate PROXYSQLGENAI/31/FFTO/TSDB flags to mysqlx builds
Merge origin/v3.0 into ProtocolX. One conflict in test/tap/tests/unit/Makefile resolved by keeping both mysqlx unit tests (ProtocolX) and pgsql_servers_ssl_params unit test + GENAI vec.o link (v3.0).
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
lib/ProxySQL_PluginManager.cpp (2)
168-176:⚠️ Potential issue | 🔴 CriticalSkip
dlclose()for plugins that did not stop cleanly.
stop_all()already leavesit->stopped == falsewhendescriptor->stop()fails, but the destructor still unloads every handle unconditionally. If a plugin still has live threads or callbacks after a failed stop, unloading its DSO here can crash the process on the next wake-up. Only unload plugins that either never started or were stopped successfully.🐛 Minimal fix
for (auto it = plugins_.rbegin(); it != plugins_.rend(); ++it) { - if (it->handle != nullptr) { - dlclose(it->handle); - it->handle = nullptr; - } + if (it->handle == nullptr) { + continue; + } + if (it->started && !it->stopped) { + proxy_warning("Skipping dlclose for plugin that did not stop cleanly: %s\n", + plugin_name(it->descriptor).c_str()); + continue; + } + dlclose(it->handle); + it->handle = nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_PluginManager.cpp` around lines 168 - 176, Destructor ProxySQL_PluginManager::~ProxySQL_PluginManager unconditionally calls dlclose on every plugin handle even if stop_all() left some plugins with it->stopped == false; change the loop over plugins_ to only call dlclose on entries that are safe to unload (e.g., where it->handle != nullptr AND (it->stopped == true OR the plugin never started)), so check the plugin's stopped/started status (the same flags set by stop_all()/descriptor->stop()) before calling dlclose to avoid unloading DSOs that failed to stop cleanly.
440-442:⚠️ Potential issue | 🔴 CriticalReturning a raw manager pointer is still a use-after-free race.
The lock only protects the read of
g_active_plugin_manager. Callers still receive a non-owning pointer that can be destroyed byproxysql_stop_configured_plugins()immediately after this function returns, so dereferencing the result outside the mutex is still unsafe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_PluginManager.cpp` around lines 440 - 442, The function proxysql_get_plugin_manager currently returns a raw pointer to g_active_plugin_manager which allows a use-after-free race when proxysql_stop_configured_plugins() can destroy the manager after the mutex is released; change the design so proxysql_get_plugin_manager returns an owning std::shared_ptr<ProxySQL_PluginManager> (and make g_active_plugin_manager a std::shared_ptr too) so callers hold a reference that prevents destruction; update proxysql_stop_configured_plugins to reset the shared_ptr (or swap with nullptr) under the same g_active_plugin_manager_mutex and update all callers of proxysql_get_plugin_manager to accept a std::shared_ptr and use it instead of assuming lifetime beyond the call.
🧹 Nitpick comments (3)
plugins/mysqlx/include/mysqlx_config_store.h (1)
23-56: Align public type names with required protocol prefixes.
MysqlxResolvedIdentity,MysqlxRoute,MysqlxBackendEndpoint, andMysqlxConfigStoredo not follow the required class-prefix convention (MySQL_,PgSQL_,ProxySQL_). Consider renaming for consistency with the codebase standard.As per coding guidelines: "Class names use PascalCase with protocol prefixes:
MySQL_,PgSQL_, orProxySQL_".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mysqlx/include/mysqlx_config_store.h` around lines 23 - 56, The public types MysqlxResolvedIdentity, MysqlxRoute, MysqlxBackendEndpoint, and MysqlxConfigStore violate the project naming convention that requires protocol prefixes; rename these symbols to use the MySQL_ prefix (e.g., MysqlxResolvedIdentity -> MySQL_ResolvedIdentity, MysqlxRoute -> MySQL_Route, MysqlxBackendEndpoint -> MySQL_BackendEndpoint, MysqlxConfigStore -> MySQL_ConfigStore) and update all references/usages, forward declarations, constructors/destructors, and any serialization/deserialization, factory functions, or typedefs that mention these names so the codebase consistently follows the PascalCase protocol-prefixed convention.lib/Admin_Handler.cpp (1)
4047-4073: Consider table-driving the MySQLX dispatch.The 16-step
if (!mysqlx_canonical)ladder is easy to desynchronize from the alias vectors above. A small{aliases, canonical}table plus one loop would make future additions much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Admin_Handler.cpp` around lines 4047 - 4073, Replace the repeated 16-step if-ladder that calls resolve_admin_alias_to_canonical with a small table/array of {alias_constant, canonical_string} entries and a single loop that iterates the table, calling resolve_admin_alias_to_canonical for each entry until mysqlx_canonical is set; then keep the existing dispatch logic using ProxySQL_Admin::dispatch_plugin_admin_command and send_error_msg_to_client unchanged. Locate the ladder around the mysqlx_canonical variable in Admin_Handler.cpp and replace it with a loop over a static const array (or std::initializer_list) of pairs containing the symbols like LOAD_MYSQLX_USERS_FROM_MEMORY, SAVE_MYSQLX_USERS_TO_MEMORY, etc., and their corresponding canonical strings to ensure future additions only require updating the table.lib/ProxySQL_PluginManager.cpp (1)
19-19: Use the repo's pthread mutex primitive for the global lifecycle lock.This introduces
std::mutexinto a codepath that the project standardizes on pthread mutexes for. Switching it now will keep the plugin lifecycle synchronization consistent with the rest of the C++ codebase.As per coding guidelines "Use pthread mutexes for synchronization and
std::atomic<>for counters".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_PluginManager.cpp` at line 19, The global lifecycle lock currently declares std::mutex g_active_plugin_manager_mutex which violates the project's guideline to use the repo pthread mutex primitive; replace that declaration with the repository's pthread-based mutex (e.g., a pthread_mutex_t or the project's PMUTEX/PTHREAD wrapper) and initialize it with the appropriate initializer (PTHREAD_MUTEX_INITIALIZER or the repo's init routine), update any includes to <pthread.h> or the repo mutex header, and ensure existing lock/unlock usage around g_active_plugin_manager_mutex uses pthread_mutex_lock/pthread_mutex_unlock (or the repo wrapper functions) so the global plugin lifecycle synchronization matches the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Admin_Bootstrap.cpp`:
- Around line 1350-1377: merge_new currently only prevents duplicate entries in
tables_defs_* but does not tell which defs were newly inserted, so
admindb->execute/configdb->execute/statsdb->execute replay every plugin schema;
change merge_new (the lambda) to collect and return the list of
ProxySQL_PluginTableDef (or their table_def strings) that were actually inserted
into tables_defs_admin/tables_defs_config/tables_defs_stats via
insert_into_tables_defs, then replace the three full plugin_manager->tables(...)
execute loops with loops over only the returned "newly_inserted" defs and call
admindb->execute/configdb->execute/statsdb->execute for those; keep
insert_into_tables_defs usage and comparisons (strcasecmp on table_name) the
same so dedup logic is preserved.
In `@lib/Admin_Handler.cpp`:
- Around line 363-378: The vectors registering disk-only aliases
(LOAD_MYSQLX_USERS_FROM_DISK, SAVE_MYSQLX_USERS_TO_DISK,
LOAD_MYSQLX_ROUTES_FROM_DISK, SAVE_MYSQLX_ROUTES_TO_DISK,
LOAD_MYSQLX_BACKEND_ENDPOINTS_FROM_DISK, SAVE_MYSQLX_BACKEND_ENDPOINTS_TO_DISK,
LOAD_MYSQLX_VARIABLES_FROM_DISK, SAVE_MYSQLX_VARIABLES_TO_DISK) need to also
include the canonical DISK↔MEMORY aliases so the canonicalizer will match;
update each vector to include the corresponding "TO MEMORY" or "FROM MEMORY"
form (e.g., add "LOAD MYSQLX USERS TO MEMORY" alongside "LOAD MYSQLX USERS FROM
DISK", and "SAVE MYSQLX USERS FROM MEMORY" alongside "SAVE MYSQLX USERS TO
DISK") for every LOAD/SAVE constant above.
In `@lib/ProxySQL_Admin.cpp`:
- Around line 6485-6497: The plugin result fields must be validated before
building protocol packets: inside dispatch_plugin_admin_command, sanitize
ProxySQL_PluginCommandResult by replacing a nullptr/empty message with an empty
string for safe c_str() use, clamp/map result.rows_affected (uint64_t) into a
safe signed int range before passing to send_ok_msg_to_client, and clamp/map
result.error_code into the 0..UINT16_MAX range (and treat any non-zero
out-of-range as a generic error code) before casting to uint16_t for
send_error_msg_to_client; locate and update the handling around
ProxySQL_PluginCommandContext/ProxySQL_PluginCommandResult and the calls to
send_ok_msg_to_client and send_error_msg_to_client in
dispatch_plugin_admin_command.
In `@lib/ProxySQL_GloVars.cpp`:
- Around line 88-104: proxysql_load_plugin_modules_from_config currently
silently ignores non-string entries under the "plugins" setting; change it to
detect malformed children and surface them (log or throw) so bad dlopen paths
are visible: inside proxysql_load_plugin_modules_from_config, iterate the
Setting plugins and when plugins[i] is not a string record the offending element
(e.g., plugins[i].getPath()/toString()/type or index) and emit an explicit
warning/error via the existing logging mechanism or return an error code/throw
an exception instead of silently skipping; keep adding valid strings to
plugin_modules as before but ensure any non-string entry triggers the new
diagnostic so startup/reload failures are easier to debug.
In `@lib/ProxySQL_PluginManager.cpp`:
- Around line 450-455: Under g_active_plugin_manager_mutex, copy or move
g_active_plugin_manager into a local owning smart pointer/reference (e.g.,
shared_ptr or raw pointer variable) then release the lock and call
g_active_plugin_manager->dispatch_admin_command(ctx, sql, result) outside the
mutex; specifically, in ProxySQL_PluginManager.cpp replace the direct call while
holding g_active_plugin_manager_mutex with code that acquires the mutex, checks
g_active_plugin_manager != nullptr, stores it into a local (e.g., auto mgr =
g_active_plugin_manager), unlocks, and only then invokes dispatch_admin_command
(which may call command.cb) to avoid holding the mutex across plugin callbacks
and prevent deadlocks/reentrancy issues.
In `@plugins/mysqlx/include/mysqlx_config_store.h`:
- Around line 79-84: Replace C++ mutex types with pthread primitives to conform
to project standards: change mutable std::shared_mutex mutex_ to mutable
pthread_rwlock_t mutex_; change mutable std::mutex rr_mutex_ to mutable
pthread_mutex_t rr_mutex_; change mutable std::unordered_map<int, uint32_t>
rr_counters_ to mutable std::unordered_map<int, std::atomic<uint32_t>>
rr_counters_; leave identities_, routes_, and hostgroup_endpoints_ as-is but
update all code using mutex_, rr_mutex_, and rr_counters_ (lock/unlock calls
around accesses) to use
pthread_rwlock_rdlock/pthread_rwlock_wrlock/pthread_rwlock_unlock for mutex_ and
pthread_mutex_lock/pthread_mutex_unlock for rr_mutex_, and use atomic operations
(fetch_add/load) on rr_counters_ instead of non-atomic increments/reads; ensure
proper initialization and destruction of pthread_rwlock_t and pthread_mutex_t in
the class constructor/destructor.
In `@plugins/mysqlx/include/mysqlx_thread.h`:
- Around line 38-40: The setters and getters (set_max_cached_connections,
set_max_sessions, get_max_sessions — and the similar functions at lines ~66-67)
mutate/read plain size_t shared state without synchronization, causing data
races; change the underlying fields to std::atomic<size_t> and update the
accessors to use atomic operations (include <atomic>), i.e. have set_* perform
atomic.store(...) and get_* perform atomic.load(..., std::memory_order_relaxed
or appropriate ordering) so runtime limit updates are thread-safe.
- Around line 59-65: Replace the three std::mutex members (listener_mutex_,
sessions_mutex_, conn_cache_mutex_) with pthread_mutex_t types; update the class
(in mysqlx_thread.h where these members are declared) to initialize each
pthread_mutex_t (either with PTHREAD_MUTEX_INITIALIZER or via pthread_mutex_init
in the class constructor) and destroy them with pthread_mutex_destroy in the
destructor, and then replace any usages relying on
std::lock_guard/std::unique_lock with corresponding
pthread_mutex_lock/pthread_mutex_unlock calls (or write small RAII wrappers if
needed) for the member variables listener_mutex_, sessions_mutex_, and
conn_cache_mutex_.
- Around line 28-30: get_session_count(), get_listener_count(), and
get_cached_connection_count() are reading container sizes without
synchronization and max_cached_/max_sessions_ accessors/mutators are
unsynchronized; fix by acquiring the appropriate mutexes before accessing these
members (use sessions_mutex_ when reading sessions_.size() in
get_session_count(), listener_mutex_ for listener_fds_.size() in
get_listener_count(), conn_cache_mutex_ for conn_cache_.size() in
get_cached_connection_count()), and protect set_max_cached_connections(),
set_max_sessions(), and get_max_sessions() with a mutex (or make those counters
atomic) so all reads/writes to max_cached_ and max_sessions_ are synchronized;
use std::lock_guard or std::scoped_lock in the respective functions (refer to
function names get_session_count, get_listener_count,
get_cached_connection_count, set_max_cached_connections, set_max_sessions,
get_max_sessions).
---
Duplicate comments:
In `@lib/ProxySQL_PluginManager.cpp`:
- Around line 168-176: Destructor
ProxySQL_PluginManager::~ProxySQL_PluginManager unconditionally calls dlclose on
every plugin handle even if stop_all() left some plugins with it->stopped ==
false; change the loop over plugins_ to only call dlclose on entries that are
safe to unload (e.g., where it->handle != nullptr AND (it->stopped == true OR
the plugin never started)), so check the plugin's stopped/started status (the
same flags set by stop_all()/descriptor->stop()) before calling dlclose to avoid
unloading DSOs that failed to stop cleanly.
- Around line 440-442: The function proxysql_get_plugin_manager currently
returns a raw pointer to g_active_plugin_manager which allows a use-after-free
race when proxysql_stop_configured_plugins() can destroy the manager after the
mutex is released; change the design so proxysql_get_plugin_manager returns an
owning std::shared_ptr<ProxySQL_PluginManager> (and make g_active_plugin_manager
a std::shared_ptr too) so callers hold a reference that prevents destruction;
update proxysql_stop_configured_plugins to reset the shared_ptr (or swap with
nullptr) under the same g_active_plugin_manager_mutex and update all callers of
proxysql_get_plugin_manager to accept a std::shared_ptr and use it instead of
assuming lifetime beyond the call.
---
Nitpick comments:
In `@lib/Admin_Handler.cpp`:
- Around line 4047-4073: Replace the repeated 16-step if-ladder that calls
resolve_admin_alias_to_canonical with a small table/array of {alias_constant,
canonical_string} entries and a single loop that iterates the table, calling
resolve_admin_alias_to_canonical for each entry until mysqlx_canonical is set;
then keep the existing dispatch logic using
ProxySQL_Admin::dispatch_plugin_admin_command and send_error_msg_to_client
unchanged. Locate the ladder around the mysqlx_canonical variable in
Admin_Handler.cpp and replace it with a loop over a static const array (or
std::initializer_list) of pairs containing the symbols like
LOAD_MYSQLX_USERS_FROM_MEMORY, SAVE_MYSQLX_USERS_TO_MEMORY, etc., and their
corresponding canonical strings to ensure future additions only require updating
the table.
In `@lib/ProxySQL_PluginManager.cpp`:
- Line 19: The global lifecycle lock currently declares std::mutex
g_active_plugin_manager_mutex which violates the project's guideline to use the
repo pthread mutex primitive; replace that declaration with the repository's
pthread-based mutex (e.g., a pthread_mutex_t or the project's PMUTEX/PTHREAD
wrapper) and initialize it with the appropriate initializer
(PTHREAD_MUTEX_INITIALIZER or the repo's init routine), update any includes to
<pthread.h> or the repo mutex header, and ensure existing lock/unlock usage
around g_active_plugin_manager_mutex uses
pthread_mutex_lock/pthread_mutex_unlock (or the repo wrapper functions) so the
global plugin lifecycle synchronization matches the rest of the codebase.
In `@plugins/mysqlx/include/mysqlx_config_store.h`:
- Around line 23-56: The public types MysqlxResolvedIdentity, MysqlxRoute,
MysqlxBackendEndpoint, and MysqlxConfigStore violate the project naming
convention that requires protocol prefixes; rename these symbols to use the
MySQL_ prefix (e.g., MysqlxResolvedIdentity -> MySQL_ResolvedIdentity,
MysqlxRoute -> MySQL_Route, MysqlxBackendEndpoint -> MySQL_BackendEndpoint,
MysqlxConfigStore -> MySQL_ConfigStore) and update all references/usages,
forward declarations, constructors/destructors, and any
serialization/deserialization, factory functions, or typedefs that mention these
names so the codebase consistently follows the PascalCase protocol-prefixed
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c23f673f-5433-4eb0-9d3a-9c5f26216a56
📒 Files selected for processing (18)
.gitignoreMakefileinclude/ProxySQL_PluginManager.hinclude/proxysql_admin.hinclude/proxysql_glovars.hpplib/Admin_Bootstrap.cpplib/Admin_Handler.cpplib/Makefilelib/ProxySQL_Admin.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_PluginManager.cppplugins/mysqlx/include/mysqlx_config_store.hplugins/mysqlx/include/mysqlx_thread.hplugins/mysqlx/src/mysqlx_config_store.cppplugins/mysqlx/src/mysqlx_thread.cpptest/tap/groups/groups.jsontest/tap/tests/Makefiletest/tap/tests/unit/Makefile
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- lib/Makefile
🚧 Files skipped from review as they are similar to previous changes (4)
- include/proxysql_admin.h
- Makefile
- include/proxysql_glovars.hpp
- include/ProxySQL_PluginManager.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
lib/Admin_Bootstrap.cpplib/Admin_Handler.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_Admin.cpplib/ProxySQL_PluginManager.cppplugins/mysqlx/include/mysqlx_config_store.hplugins/mysqlx/include/mysqlx_thread.h
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention in lib/ directory
Files:
lib/Admin_Bootstrap.cpplib/Admin_Handler.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_Admin.cpplib/ProxySQL_PluginManager.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
Applied to files:
lib/Admin_Bootstrap.cpplib/Admin_Handler.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_Admin.cpplib/ProxySQL_PluginManager.cppplugins/mysqlx/include/mysqlx_config_store.h
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
lib/Admin_Bootstrap.cpplib/ProxySQL_GloVars.cpplib/ProxySQL_PluginManager.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Dual-protocol design uses parallel class hierarchies for MySQL and PostgreSQL with same architecture but protocol-specific implementations (e.g., `MySQL_Protocol`/`PgSQL_Protocol`, `MySQL_Session`/`PgSQL_Session`, `MySQL_Thread`/`PgSQL_Thread`)
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to include/**/*.{h,hpp} : Include guards use the pattern `#ifndef __CLASS_*_H` (e.g., `#ifndef __ADMIN_H__`, `#ifndef __PROXYSQL_PROTOCOL_H__`)
Applied to files:
lib/Admin_Bootstrap.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
lib/ProxySQL_Admin.cpp
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
lib/ProxySQL_PluginManager.cpp
🪛 Clang (14.0.6)
plugins/mysqlx/include/mysqlx_config_store.h
[error] 4-4: 'atomic' file not found
(clang-diagnostic-error)
plugins/mysqlx/include/mysqlx_thread.h
[error] 4-4: 'cstdint' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
lib/ProxySQL_PluginManager.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 33-33: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (6)
plugins/mysqlx/include/mysqlx_config_store.h (1)
73-73: Good fix:get_tls_mode()now returns by value.Line 73 avoids exposing references to mutable shared state and removes lifetime hazards for callers.
lib/Admin_Handler.cpp (1)
527-538: Nice helper extraction.This keeps alias matching side-effect free and avoids building temporary strings in the admin hot path.
lib/ProxySQL_GloVars.cpp (1)
118-118: Explicit state reset here is fine.Making the new
plugin_modulescache visibly empty at construction and teardown keeps its lifecycle obvious.Also applies to: 252-252
lib/Admin_Bootstrap.cpp (1)
942-967: Good fail-fast handling for plugin table name collisions.Returning
falsebeforecheck_and_build_standard_tables()keeps bootstrap deterministic when a plugin tries to reuse an existing table name.lib/ProxySQL_Admin.cpp (1)
342-352: No changes needed — these functions are not directly exposed as a C ABI surface.The plugin interface passes DB accessors to plugins via the
ProxySQL_PluginServicesstruct (function pointers set at initialization time), not via direct symbol resolution. Plugins never directly resolveproxysql_plugin_get_admindb(),proxysql_plugin_get_configdb(), orproxysql_plugin_get_statsdb()— they call through the wrapper functions (get_admindb_service(), etc.) via function pointers populated at init. Since the wrappers are resolved once during plugin initialization within the same process, C linkage protection is not applicable here.plugins/mysqlx/include/mysqlx_thread.h (1)
57-60: Nice fix: listener state now has explicit synchronization.Adding
listener_mutex_alongside listener vectors is the right direction to prevent concurrent mutation/read races in poll-set rebuild paths.
Comment on lines
+1350
to
+1377
| auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs) { | ||
| for (const auto& def : defs) { | ||
| bool exists = false; | ||
| for (const auto* existing : *target) { | ||
| if (strcasecmp(existing->table_name, def.table_name) == 0) { | ||
| exists = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!exists) { | ||
| insert_into_tables_defs(target, def.table_name, def.table_def); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)); | ||
| merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db)); | ||
| merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)); | ||
|
|
||
| for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)) { | ||
| admindb->execute(def.table_def); | ||
| } | ||
| for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::config_db)) { | ||
| configdb->execute(def.table_def); | ||
| } | ||
| for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)) { | ||
| statsdb->execute(def.table_def); | ||
| } |
There was a problem hiding this comment.
Only execute DDL for newly materialized plugin tables.
merge_new() deduplicates tables_defs_*, but the three execute(def.table_def) loops still replay every registered plugin schema on each call. If this helper runs again after another plugin load, already-materialized tables can start failing here and block later plugin tables from being created.
♻️ Proposed fix
- auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs) {
+ auto materialize_new = [this](SQLite3DB* db, std::vector<table_def_t *>* target,
+ const std::vector<ProxySQL_PluginTableDef>& defs) {
for (const auto& def : defs) {
bool exists = false;
for (const auto* existing : *target) {
if (strcasecmp(existing->table_name, def.table_name) == 0) {
exists = true;
break;
}
}
- if (!exists) {
- insert_into_tables_defs(target, def.table_name, def.table_def);
- }
+ if (exists) {
+ continue;
+ }
+ insert_into_tables_defs(target, def.table_name, def.table_def);
+ db->execute(def.table_def);
}
};
- merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db));
- merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db));
- merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db));
-
- for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)) {
- admindb->execute(def.table_def);
- }
- for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::config_db)) {
- configdb->execute(def.table_def);
- }
- for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)) {
- statsdb->execute(def.table_def);
- }
+ materialize_new(admindb, tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db));
+ materialize_new(configdb, tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db));
+ materialize_new(statsdb, tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Admin_Bootstrap.cpp` around lines 1350 - 1377, merge_new currently only
prevents duplicate entries in tables_defs_* but does not tell which defs were
newly inserted, so admindb->execute/configdb->execute/statsdb->execute replay
every plugin schema; change merge_new (the lambda) to collect and return the
list of ProxySQL_PluginTableDef (or their table_def strings) that were actually
inserted into tables_defs_admin/tables_defs_config/tables_defs_stats via
insert_into_tables_defs, then replace the three full plugin_manager->tables(...)
execute loops with loops over only the returned "newly_inserted" defs and call
admindb->execute/configdb->execute/statsdb->execute for those; keep
insert_into_tables_defs usage and comparisons (strcasecmp on table_name) the
same so dedup logic is preserved.
Comment on lines
+363
to
+378
| const std::vector<std::string> LOAD_MYSQLX_USERS_FROM_DISK = { | ||
| "LOAD MYSQLX USERS FROM DISK" }; | ||
| const std::vector<std::string> SAVE_MYSQLX_USERS_TO_DISK = { | ||
| "SAVE MYSQLX USERS TO DISK" }; | ||
| const std::vector<std::string> LOAD_MYSQLX_ROUTES_FROM_DISK = { | ||
| "LOAD MYSQLX ROUTES FROM DISK" }; | ||
| const std::vector<std::string> SAVE_MYSQLX_ROUTES_TO_DISK = { | ||
| "SAVE MYSQLX ROUTES TO DISK" }; | ||
| const std::vector<std::string> LOAD_MYSQLX_BACKEND_ENDPOINTS_FROM_DISK = { | ||
| "LOAD MYSQLX BACKEND ENDPOINTS FROM DISK" }; | ||
| const std::vector<std::string> SAVE_MYSQLX_BACKEND_ENDPOINTS_TO_DISK = { | ||
| "SAVE MYSQLX BACKEND ENDPOINTS TO DISK" }; | ||
| const std::vector<std::string> LOAD_MYSQLX_VARIABLES_FROM_DISK = { | ||
| "LOAD MYSQLX VARIABLES FROM DISK" }; | ||
| const std::vector<std::string> SAVE_MYSQLX_VARIABLES_TO_DISK = { | ||
| "SAVE MYSQLX VARIABLES TO DISK" }; |
There was a problem hiding this comment.
Add the standard DISK↔MEMORY aliases for MySQLX commands.
Line 363 onward only registers FROM DISK / TO DISK. The usual LOAD MYSQLX ... TO MEMORY and SAVE MYSQLX ... FROM MEMORY forms will never match the canonicalization block at Lines 4048-4063, so they fall through to raw SQL execution and fail even though the other LOAD/SAVE families accept those aliases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Admin_Handler.cpp` around lines 363 - 378, The vectors registering
disk-only aliases (LOAD_MYSQLX_USERS_FROM_DISK, SAVE_MYSQLX_USERS_TO_DISK,
LOAD_MYSQLX_ROUTES_FROM_DISK, SAVE_MYSQLX_ROUTES_TO_DISK,
LOAD_MYSQLX_BACKEND_ENDPOINTS_FROM_DISK, SAVE_MYSQLX_BACKEND_ENDPOINTS_TO_DISK,
LOAD_MYSQLX_VARIABLES_FROM_DISK, SAVE_MYSQLX_VARIABLES_TO_DISK) need to also
include the canonical DISK↔MEMORY aliases so the canonicalizer will match;
update each vector to include the corresponding "TO MEMORY" or "FROM MEMORY"
form (e.g., add "LOAD MYSQLX USERS TO MEMORY" alongside "LOAD MYSQLX USERS FROM
DISK", and "SAVE MYSQLX USERS FROM MEMORY" alongside "SAVE MYSQLX USERS TO
DISK") for every LOAD/SAVE constant above.
Comment on lines
+6485
to
+6497
| bool ProxySQL_Admin::dispatch_plugin_admin_command(S* sess, const char* sql) { | ||
| ProxySQL_PluginCommandContext ctx { admindb, configdb, statsdb }; | ||
| ProxySQL_PluginCommandResult result { 0, 0, "" }; | ||
| if (!proxysql_dispatch_configured_plugin_admin_command(ctx, sql, result)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (result.error_code == 0) { | ||
| send_ok_msg_to_client(sess, result.message.empty() ? NULL : result.message.c_str(), static_cast<int>(result.rows_affected), sql); | ||
| } else { | ||
| send_error_msg_to_client(sess, result.message.c_str(), static_cast<uint16_t>(result.error_code)); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Harden plugin results before building protocol packets.
Line 6493 passes NULL for a successful command with an empty message, and Lines 6493-6495 narrow uint64_t/int plugin fields into protocol integers without validation. Since ProxySQL_PluginCommandResult is plugin-controlled, one empty success message or out-of-range code can become a crash or a bogus wire response. Normalize to "" and clamp/map the numeric fields here.
Proposed hardening
+#include <limits>
+
template <typename S>
bool ProxySQL_Admin::dispatch_plugin_admin_command(S* sess, const char* sql) {
ProxySQL_PluginCommandContext ctx { admindb, configdb, statsdb };
ProxySQL_PluginCommandResult result { 0, 0, "" };
if (!proxysql_dispatch_configured_plugin_admin_command(ctx, sql, result)) {
return false;
}
if (result.error_code == 0) {
- send_ok_msg_to_client(sess, result.message.empty() ? NULL : result.message.c_str(), static_cast<int>(result.rows_affected), sql);
+ const int rows_affected =
+ result.rows_affected > static_cast<uint64_t>(std::numeric_limits<int>::max())
+ ? std::numeric_limits<int>::max()
+ : static_cast<int>(result.rows_affected);
+ send_ok_msg_to_client(sess, result.message.c_str(), rows_affected, sql);
} else {
- send_error_msg_to_client(sess, result.message.c_str(), static_cast<uint16_t>(result.error_code));
+ const uint16_t error_code =
+ result.error_code < 0
+ ? 1
+ : (result.error_code > std::numeric_limits<uint16_t>::max()
+ ? std::numeric_limits<uint16_t>::max()
+ : static_cast<uint16_t>(result.error_code));
+ send_error_msg_to_client(sess, result.message.c_str(), error_code);
}
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ProxySQL_Admin.cpp` around lines 6485 - 6497, The plugin result fields
must be validated before building protocol packets: inside
dispatch_plugin_admin_command, sanitize ProxySQL_PluginCommandResult by
replacing a nullptr/empty message with an empty string for safe c_str() use,
clamp/map result.rows_affected (uint64_t) into a safe signed int range before
passing to send_ok_msg_to_client, and clamp/map result.error_code into the
0..UINT16_MAX range (and treat any non-zero out-of-range as a generic error
code) before casting to uint16_t for send_error_msg_to_client; locate and update
the handling around ProxySQL_PluginCommandContext/ProxySQL_PluginCommandResult
and the calls to send_ok_msg_to_client and send_error_msg_to_client in
dispatch_plugin_admin_command.
Comment on lines
+88
to
+104
| void proxysql_load_plugin_modules_from_config( | ||
| const Setting& root, | ||
| std::vector<std::string>& plugin_modules | ||
| ) { | ||
| plugin_modules.clear(); | ||
|
|
||
| if (root.exists("plugins") == false) { | ||
| return; | ||
| } | ||
|
|
||
| const Setting& plugins = root["plugins"]; | ||
| for (int i = 0; i < plugins.getLength(); ++i) { | ||
| if (plugins[i].isString()) { | ||
| plugin_modules.emplace_back(plugins[i].c_str()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Surface malformed plugins entries instead of silently skipping them.
Line 100 drops non-string children with no warning or error. For a dlopen path list, that turns a bad config into a silent “plugin not configured” outcome, which is painful to diagnose during startup or reload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ProxySQL_GloVars.cpp` around lines 88 - 104,
proxysql_load_plugin_modules_from_config currently silently ignores non-string
entries under the "plugins" setting; change it to detect malformed children and
surface them (log or throw) so bad dlopen paths are visible: inside
proxysql_load_plugin_modules_from_config, iterate the Setting plugins and when
plugins[i] is not a string record the offending element (e.g.,
plugins[i].getPath()/toString()/type or index) and emit an explicit
warning/error via the existing logging mechanism or return an error code/throw
an exception instead of silently skipping; keep adding valid strings to
plugin_modules as before but ensure any non-string entry triggers the new
diagnostic so startup/reload failures are easier to debug.
Comment on lines
+450
to
+455
| std::lock_guard<std::mutex> lock(g_active_plugin_manager_mutex); | ||
| if (g_active_plugin_manager == nullptr) { | ||
| return false; | ||
| } | ||
|
|
||
| return g_active_plugin_manager->dispatch_admin_command(ctx, sql, result); |
There was a problem hiding this comment.
Don't hold the global manager mutex while invoking plugin callbacks.
dispatch_admin_command() can reach arbitrary plugin code via command.cb(...). Keeping g_active_plugin_manager_mutex locked across that call means a slow or re-entrant admin command can block load/stop, and it can deadlock if the callback reaches back into manager APIs that take the same mutex. Grab a safe owning reference under the lock, release it, then dispatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ProxySQL_PluginManager.cpp` around lines 450 - 455, Under
g_active_plugin_manager_mutex, copy or move g_active_plugin_manager into a local
owning smart pointer/reference (e.g., shared_ptr or raw pointer variable) then
release the lock and call g_active_plugin_manager->dispatch_admin_command(ctx,
sql, result) outside the mutex; specifically, in ProxySQL_PluginManager.cpp
replace the direct call while holding g_active_plugin_manager_mutex with code
that acquires the mutex, checks g_active_plugin_manager != nullptr, stores it
into a local (e.g., auto mgr = g_active_plugin_manager), unlocks, and only then
invokes dispatch_admin_command (which may call command.cb) to avoid holding the
mutex across plugin callbacks and prevent deadlocks/reentrancy issues.
Comment on lines
+79
to
+84
| mutable std::shared_mutex mutex_ {}; | ||
| std::unordered_map<std::string, MysqlxResolvedIdentity> identities_ {}; | ||
| std::unordered_map<std::string, MysqlxRoute> routes_ {}; | ||
| std::unordered_map<int, std::vector<MysqlxBackendEndpoint>> hostgroup_endpoints_ {}; | ||
| mutable std::mutex rr_mutex_ {}; | ||
| mutable std::unordered_map<int, uint32_t> rr_counters_ {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use pthread mutex primitives to match project synchronization standard.
Lines 79-84 use std::shared_mutex/std::mutex, but this repository’s rule requires pthread mutexes for synchronization. Please align these fields (and corresponding lock usage in implementation) with pthread-based primitives.
As per coding guidelines: "Use pthread mutexes for synchronization and std::atomic<> for counters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mysqlx/include/mysqlx_config_store.h` around lines 79 - 84, Replace
C++ mutex types with pthread primitives to conform to project standards: change
mutable std::shared_mutex mutex_ to mutable pthread_rwlock_t mutex_; change
mutable std::mutex rr_mutex_ to mutable pthread_mutex_t rr_mutex_; change
mutable std::unordered_map<int, uint32_t> rr_counters_ to mutable
std::unordered_map<int, std::atomic<uint32_t>> rr_counters_; leave identities_,
routes_, and hostgroup_endpoints_ as-is but update all code using mutex_,
rr_mutex_, and rr_counters_ (lock/unlock calls around accesses) to use
pthread_rwlock_rdlock/pthread_rwlock_wrlock/pthread_rwlock_unlock for mutex_ and
pthread_mutex_lock/pthread_mutex_unlock for rr_mutex_, and use atomic operations
(fetch_add/load) on rr_counters_ instead of non-atomic increments/reads; ensure
proper initialization and destruction of pthread_rwlock_t and pthread_mutex_t in
the class constructor/destructor.
Comment on lines
+28
to
+30
| size_t get_session_count() const; | ||
| bool is_running() const { return running_.load(); } | ||
| int get_listener_count() const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating mysqlx_thread implementation files..."
fd -i 'mysqlx_thread.*\.(cc|cpp|cxx)$'
echo
echo "Inspecting getter/setter implementations and nearby synchronization..."
rg -n -C4 --type=cpp 'Mysqlx_Thread::(get_session_count|get_listener_count|get_cached_connection_count|set_max_cached_connections|set_max_sessions)\s*\('
echo
echo "Inspecting lock usage and shared fields..."
rg -n -C3 --type=cpp '(listener_mutex_|sessions_mutex_|conn_cache_mutex_|max_cached_|max_sessions_)'Repository: sysown/proxysql
Length of output: 23943
Fix thread-unsafe const getter implementations.
Lines 28, 30, and 37 implement getters that read vector sizes without synchronization, while other methods (e.g., process_all_sessions, accept_new_connections, return_connection_to_cache) hold corresponding mutexes when modifying those vectors. This creates data races:
get_session_count()readssessions_.size()without holdingsessions_mutex_get_listener_count()readslistener_fds_.size()without holdinglistener_mutex_get_cached_connection_count()readsconn_cache_.size()without holdingconn_cache_mutex_
Additionally, set_max_cached_connections(), set_max_sessions(), and get_max_sessions() (lines 38–40) access max_cached_ and max_sessions_ without any synchronization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mysqlx/include/mysqlx_thread.h` around lines 28 - 30,
get_session_count(), get_listener_count(), and get_cached_connection_count() are
reading container sizes without synchronization and max_cached_/max_sessions_
accessors/mutators are unsynchronized; fix by acquiring the appropriate mutexes
before accessing these members (use sessions_mutex_ when reading
sessions_.size() in get_session_count(), listener_mutex_ for
listener_fds_.size() in get_listener_count(), conn_cache_mutex_ for
conn_cache_.size() in get_cached_connection_count()), and protect
set_max_cached_connections(), set_max_sessions(), and get_max_sessions() with a
mutex (or make those counters atomic) so all reads/writes to max_cached_ and
max_sessions_ are synchronized; use std::lock_guard or std::scoped_lock in the
respective functions (refer to function names get_session_count,
get_listener_count, get_cached_connection_count, set_max_cached_connections,
set_max_sessions, get_max_sessions).
Comment on lines
+38
to
+40
| void set_max_cached_connections(size_t max) { max_cached_ = max; } | ||
| void set_max_sessions(size_t max) { max_sessions_ = max; } | ||
| size_t get_max_sessions() const { return max_sessions_; } |
There was a problem hiding this comment.
Protect runtime limit updates from data races.
Line 38 and Line 39 write shared state as plain size_t. If admin/config updates happen while worker threads read these fields, this is undefined behavior.
Suggested fix
- void set_max_cached_connections(size_t max) { max_cached_ = max; }
- void set_max_sessions(size_t max) { max_sessions_ = max; }
- size_t get_max_sessions() const { return max_sessions_; }
+ void set_max_cached_connections(size_t max) { max_cached_.store(max, std::memory_order_relaxed); }
+ void set_max_sessions(size_t max) { max_sessions_.store(max, std::memory_order_relaxed); }
+ size_t get_max_sessions() const { return max_sessions_.load(std::memory_order_relaxed); }
...
- size_t max_cached_;
- size_t max_sessions_;
+ std::atomic<size_t> max_cached_{0};
+ std::atomic<size_t> max_sessions_{0};Also applies to: 66-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mysqlx/include/mysqlx_thread.h` around lines 38 - 40, The setters and
getters (set_max_cached_connections, set_max_sessions, get_max_sessions — and
the similar functions at lines ~66-67) mutate/read plain size_t shared state
without synchronization, causing data races; change the underlying fields to
std::atomic<size_t> and update the accessors to use atomic operations (include
<atomic>), i.e. have set_* perform atomic.store(...) and get_* perform
atomic.load(..., std::memory_order_relaxed or appropriate ordering) so runtime
limit updates are thread-safe.
Comment on lines
+59
to
+65
| std::mutex listener_mutex_; | ||
|
|
||
| std::vector<MysqlxSession*> sessions_; | ||
| std::mutex sessions_mutex_; | ||
|
|
||
| std::vector<MysqlxConnection*> conn_cache_; | ||
| std::mutex conn_cache_mutex_; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Compare mutex primitive usage in mysqlx plugin and nearby core code:"
rg -n -C2 --type=cpp '(std::mutex|pthread_mutex_t|pthread_mutex_lock|pthread_mutex_unlock)' plugins/mysqlx include srcRepository: sysown/proxysql
Length of output: 48204
Replace std::mutex with pthread_mutex_t to align with project synchronization standard.
The file uses std::mutex for three member variables (listener_mutex_, sessions_mutex_, and conn_cache_mutex_ at lines 59, 62, 65), while the coding guidelines require pthread mutexes for synchronization. Update these members to use pthread_mutex_t instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mysqlx/include/mysqlx_thread.h` around lines 59 - 65, Replace the
three std::mutex members (listener_mutex_, sessions_mutex_, conn_cache_mutex_)
with pthread_mutex_t types; update the class (in mysqlx_thread.h where these
members are declared) to initialize each pthread_mutex_t (either with
PTHREAD_MUTEX_INITIALIZER or via pthread_mutex_init in the class constructor)
and destroy them with pthread_mutex_destroy in the destructor, and then replace
any usages relying on std::lock_guard/std::unique_lock with corresponding
pthread_mutex_lock/pthread_mutex_unlock calls (or write small RAII wrappers if
needed) for the member variables listener_mutex_, sessions_mutex_, and
conn_cache_mutex_.
Update all four doc/mysqlx/ files to reflect current implementation: - TLS: change from "stub" to "supported" (OpenSSL Memory BIO) - Fix table name: mysqlx_endpoints → mysqlx_backend_endpoints - Add missing X_SESSION_RESET_WAITING state - Add section 5.3 documenting 8 DISK commands - Fix allowed_auth_methods default (empty, not 'MYSQL41,PLAIN') - Add missing comment column to all table schemas - Update test counts: 24 files, 600+ assertions - Update MYSQL_ROUTER_PARITY with accurate TLS/connect-timeout info
The groups.json linter requires keys sorted alphabetically. Reorder mysqlx_* entries to satisfy the CI lint check.
- Replace void* thread_ptr_ with typed Mysqlx_Thread* for type safety - Use RAII (unique_ptr) for SQLite3 result and error cleanup in plugin - Extract handle_auth_mysql41/handle_auth_plain from handler_auth_start to reduce cognitive complexity (37 -> ~10 per method) - Extract step_auth sub-states into individual methods to reduce cognitive complexity in mysqlx_connection.cpp (55 -> ~8 per method) - Add forward_frame_to_client helper to eliminate repeated frame forwarding code - Replace hardcoded password strings in credential verify test with named test constants - Add NOSONAR annotations for const_cast (required by SQLite3DB API) and SSL_VERIFY_NONE (test-only self-signed certs) - Fix Makefile whitespace issue with mysqlx unit test entries
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a MySQL X Protocol plugin to ProxySQL, enabling it to accept, authenticate, and route connections using MySQL's X Protocol (port 33060). The plugin is built as a dynamically loaded
.so— no core engine modifications required for the plugin itself.This is the first protocol plugin for ProxySQL, establishing the plugin infrastructure (
ProxySQL_PluginManager) that future plugins can reuse.61 commits, 80 files changed, ~34,500 lines added.
What's Included
Plugin Infrastructure (Core Changes)
ProxySQL_PluginManager— dlopen/dlsym-based plugin loader with lifecycle management (load → start → stop)ProxySQL_PluginC ABI — stable interface header for third-party plugin developmentdispatch_plugin_admin_commandroutes plugin-specific SQL commandsplugin_modulesfield in ProxySQL config for declaring pluginsmain.cpp— LoadConfiguredPlugins / StartConfiguredPlugins / StopConfiguredPluginsMySQL X Protocol Plugin (
plugins/mysqlx/).pb.cc/.pb.hfrom MySQL X Protocol.protodefinitions (committed, not generated at build time)MYSQL41andPLAINauthenticationmysqlx_users,mysqlx_routes,mysqlx_listeners,mysqlx_variables,runtime_mysqlx_*,stats_mysqlx_*)Admin Commands
Standard ProxySQL syntax, completely separate from MySQL commands:
Testing (292+ assertions across 16 test files)
fake_plugin.cpp+test_globals.h+test_init.hfor unit testing plugin codeCI
.github/workflows/CI-mysqlx.yml— GitHub Actions workflow with unit-tests and e2e-tests jobstest/tap/groups/mysqlx-e2e/Documentation
doc/mysqlx/README.md— Reference manual (tables, commands, auth, routing, quick start)doc/mysqlx/ARCHITECTURE.md— Architecture with ASCII diagrams, data flows, thread model, ABI contractdoc/mysqlx/TESTING.md— Testing documentation with coverage mapdocs/superpowers/specs/— Design spec + canonical commands designSecurity Hardening (from Code Review)
CRYPTO_memcmpgetaddrinfo(nogethostbyname)TCP_NODELAYon all X Protocol socketsNot in Scope (Phase 2)
mysqlx_*admin tablesHow to Test Locally
Summary by CodeRabbit
New Features
New Admin Commands
Documentation
Infrastructure
Chores