Conversation
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the Observe tool with a new Stats tool and wiring (endpoint, auth var, handler), adds a full in-process Stats_Tool_Handler implementation and header, SQLite→JSON helper, SQL/util helpers, GenAI guards/flag consolidation, three large documentation files, and related config/test/build updates. No public API signatures were removed; many new handler methods were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP_Server as MCP_Server
participant Stats_Handler as Stats_Handler
participant Admin_DB as Admin_DB
participant Stats_DB as Stats_DB
participant JSON_Helper as JSON_Helper
Client->>MCP_Server: JSON-RPC execute_tool(tool, args)
MCP_Server->>Stats_Handler: execute_tool(tool, args)
activate Stats_Handler
Stats_Handler->>Stats_Handler: lock mutex
alt live data tool
Stats_Handler->>Admin_DB: execute_admin_query(sql)
Admin_DB-->>Stats_Handler: SQLite3_result
else historical data tool
Stats_Handler->>Stats_DB: execute_statsdb_disk_query(sql)
Stats_DB-->>Stats_Handler: SQLite3_result
end
Stats_Handler->>JSON_Helper: resultset_to_json(resultset, cols)
JSON_Helper-->>Stats_Handler: json array
Stats_Handler->>Stats_Handler: unlock mutex
deactivate Stats_Handler
Stats_Handler-->>MCP_Server: JSON-RPC result
MCP_Server-->>Client: response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @wazir-ahmed, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances ProxySQL's monitoring and diagnostic capabilities by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
wazir-ahmed
changed the title
statsstats
wazir-ahmed
changed the title
statsstats
wazir-ahmed
changed the title
statsstats
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new /mcp/stats endpoint, replacing the old /mcp/observe endpoint. This is a substantial feature addition, providing a wide range of tools for monitoring and diagnostics. The implementation is extensive, adding a new Stats_Tool_Handler with 17 distinct tools. My review focuses on the security, correctness, and maintainability of this new implementation. I've identified a critical SQL injection vulnerability in the get_stats tool that needs to be addressed. Additionally, there are a few other issues related to incorrect identifier escaping, robustness, and code structure that I've commented on. Overall, this is a great feature, but the security concerns should be prioritized before merging.
…entation Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@doc/MCP/MCP_Stats_Implementation_Plan.md`:
- Around line 878-895: The pseudocode for calculate_percentile is missing the
safety bounds check and empty-bucket fallback used in the real implementation;
update the pseudocode (or add a note) to mirror
calculate_percentile_from_histogram by checking i < thresholds.size() when
indexing bucket_thresholds and by handling the empty/bucket-less case before
returning bucket_thresholds.back() (e.g., verify thresholds is non-empty and
return a sensible default or error if it is empty). Ensure references to
calculate_percentile, calculate_percentile_from_histogram, bucket_thresholds,
and thresholds.size() are used so readers can locate the corresponding real-code
checks.
In `@doc/Stats_Tables.md`:
- Line 269: The link to [Source References](`#source-references`) will not resolve
because the target uses a bold line with an explicit anchor syntax; change the
target from the bold line "**Source References** {`#source-references`}" to a real
Markdown heading (e.g., "#### Source References") so the anchor
"#source-references" is generated consistently and the in-page link resolves
across renderers; ensure the heading text matches the link fragment or update
the link fragment to match the new heading.
In `@lib/proxysql_utils.cpp`:
- Around line 760-781: In calculate_percentile_from_histogram change the
accumulator types to a 64-bit integer (e.g., int64_t or long long) to avoid
overflow: make `total` and `cumulative` 64-bit, compute `target` as a 64-bit
value derived from total and `percentile` (cast/round appropriately), and use
64-bit arithmetic when adding bucket values and comparing cumulative >= target;
keep the function return type as int and return thresholds[i] as before. This
fixes potential overflow in the loops that use `total`, `cumulative`, `target`,
and `buckets`.
In `@lib/Stats_Tool_Handler.cpp`:
- Around line 1388-1399: The issue is that total_queries is incremented with
count_star before applying the min_time_us filter, causing a mismatch between
total_queries and the summed queries returned; move the accumulation of
total_queries so it only happens after the min_time_us check (i.e., after
computing avg_time and after the "if (min_time_us > 0 && avg_time < min_time_us)
continue;" line) or alternatively add a separate filtered_total variable and
increment that after the filter while keeping total_queries as the unfiltered
sum — adjust uses of total_queries accordingly (refer to total_queries,
count_star, min_time_us, avg_time, and the queries aggregation logic).
- Around line 808-811: The handler currently ignores the severity_threshold
parameter declared in the schema: update Stats_Tool_Handler::handle_get_health
to read severity_threshold from arguments (e.g., int or string using
arguments.value("severity_threshold", <default>)), validate/normalize it, and
apply it when assembling or filtering the alert/health results (where alerts are
collected in this function) so only alerts at or above the threshold are
returned; ensure the parameter name matches the schema and preserve existing
database and include_backend behavior in the same function.
- Around line 1061-1067: The backend filter currently takes port as a raw
substring and concatenates it into the SQL (see backend_filter handling and sql
construction using srv_port), which allows SQL injection; update the code that
extracts port (the block that creates host and port from backend_filter) to
validate that port contains only digits (and optionally check range 1-65535),
and if not numeric reject the filter or skip adding the srv_port clause;
alternatively use a parameterized query or cast the validated integer into the
SQL rather than direct string concatenation; apply the same numeric
validation/fix in handle_show_connections where the same pattern is used.
- Around line 1688-1691: The code is converting PostgreSQL sqlstate into an int
via std::stoi (error[errno_col] = ... std::stoi(...)) which will throw or
truncate; change the handler to treat sqlstate as a string for pgsql — assign
the raw string to error[errno_col] (or a new error["sqlstate"] key) instead of
calling std::stoi, and update any filtering logic that compares the errno
parameter to be type-aware so that for PostgreSQL it performs string comparison
against sqlstate (rather than integer comparison); also consider adding a
separate sqlstate string parameter to the tool description and use that
parameter in the filter branch that currently applies the errno filter.
- Around line 964-992: The current branch that builds the SQL table name chooses
only one pool (stats_pgsql_connection_pool vs stats_mysql_connection_pool) based
on database == "pgsql", so database == "all" returns only MySQL; change the
logic in the include_backend block to query both pools when database == "all"
(e.g., build a list of table names or run two queries), call execute_admin_query
for each table (using the existing resultset, cols, sql, err variables), loop
over each successful resultset to populate the backends array (same parsing code
for fields and computing utilization_pct), push results into
health_data["backend_servers"], and delete/free each resultset after processing;
keep the single-query behavior when database is "mysql" or "pgsql".
- Around line 2048-2076: The code concatenates untrusted inputs into SQL via
columns_str (built with sql_escape), arguments["where"], and
arguments["order_by"] when building `sql` (SELECT ... FROM ...), which allows
SQL injection; fix by validating and whitelisting identifiers and restricting
allowed syntax: (1) For columns_str / arguments["columns"] (in the loop that
builds columns_str), validate each entry is a simple identifier or a permitted
wildcard (e.g., match /^[A-Za-z_][A-Za-z0-9_]*$/ or a single '*'), reject or map
anything else to an error; (2) For arguments["where"] and arguments["order_by"],
do not concatenate raw strings—either build these clauses from validated
structured input (e.g., column/operator/value triples that bind values with
prepared statements) or at minimum reject dangerous characters/patterns
(semicolon, --, /* */, unbalanced quotes) and only allow a strict grammar
(column names from the same whitelist and a limited set of operators); and (3)
use parameterized queries / prepared statements for any values instead of
embedding literals; update the code paths that read arguments["where"],
arguments["order_by"], and the columns_str construction to perform these
validations and to return an error when input is not strictly allowed.
🧹 Nitpick comments (8)
lib/MCP_Tool_Handler.cpp (3)
1-6: Redundant includes and macros already provided by the header.
json.hpp,using json, and#define PROXYJSONare already included/defined inMCP_Tool_Handler.h(lines 7–9). While self-contained.cppfiles are fine in principle, the duplication of#define PROXYJSON(a non-idempotent macro) could mask ordering issues if the header landscape changes.
15-43: Consider guarding against nullrowor nullcolumn_definitionentries.Line 18 dereferences
resultset->column_definition[i]->nameand line 19 dereferencesrow->fields[i]without null-checking the intermediate pointers. If acolumn_definitionentry or arowpointer is ever null (e.g., due to a partial/corrupt result set), this will segfault.Given that this is defensive utility code consumed by many tools, a brief null guard would be cheap insurance:
🛡️ Suggested defensive guards
for (const auto& row : resultset->rows) { + if (!row) continue; json obj = json::object(); for (int i = 0; i < cols && i < (int)resultset->column_definition.size(); i++) { - const char* col_name = resultset->column_definition[i]->name; + if (!resultset->column_definition[i]) continue; + const char* col_name = resultset->column_definition[i]->name; + if (!col_name) continue; const char* val = row->fields[i];
29-41:strtodwill parse"NaN","inf", and"-inf"as numeric doubles.If any stats value happens to contain these strings (or e.g.
"Infinity"), they will be stored as JSON doubles (NaN/Inf), which are not valid JSON values and will cause serialization issues withnlohmann::json(it replaces them withnullby default). This is unlikely in normal ProxySQL stats but worth noting.If you want to be safe, add a
std::isfinite(d)check:Proposed guard
double d = strtod(val, &end); - if (end != val && *end == '\0') { + if (end != val && *end == '\0' && std::isfinite(d)) { obj[col_name] = d;doc/MCP/MCP_Stats_Tools_Spec.md (1)
71-84: Section numbering: "2.1" is a subsection of "2" but uses a different heading format.Section "2.1 Time Field Conventions" reads as a subsection of "2. Response Format Convention" but is not listed in the Table of Contents. This is minor — consider adding it to the TOC or renumbering as a standalone section for discoverability.
doc/Stats_Tables.md (1)
1729-1737: Missing language specifier on fenced code block.This data flow diagram uses a fenced code block without a language identifier, which triggers markdownlint MD040. Add
textas the language identifier for plain-text diagrams.Proposed fix
-``` +```text Query executes ↓ MySQL_Logger::log_request() adds event to circular buffer (MyLogCB)doc/MCP/MCP_Stats_Implementation_Plan.md (1)
149-178: Missing language specifier on ASCII art flow diagrams.Three fenced code blocks (lines 149, 214, 271) containing flow diagrams lack a language identifier, triggering markdownlint MD040. Add
textas the language specifier.Also applies to: 214-240, 271-313
lib/ProxySQL_MCP_Server.cpp (1)
125-129: Missing cleanup on init failure, inconsistent with other handlers.Config (Line 83), Query (Line 105), and AI (Line 135) handlers all delete and NULL the handler on
init()failure. Stats follows the Admin/Cache pattern of silently ignoring failure, which could leave a partially-initialized handler in play.Proposed fix
// 5. Stats Tool Handler handler->stats_tool_handler = new Stats_Tool_Handler(handler); if (handler->stats_tool_handler->init() == 0) { proxy_info("Stats Tool Handler initialized\n"); + } else { + proxy_error("Failed to initialize Stats Tool Handler\n"); + delete handler->stats_tool_handler; + handler->stats_tool_handler = NULL; }lib/Stats_Tool_Handler.cpp (1)
2677-2687: Integer division inavgaggregation truncates the result.Line 2680:
agg_result = sum / (long long)values.size()performs integer division. For example, averaging values[3, 4]yields3instead of3.5. Sinceagg_resultislong long, the fractional part is always lost.Consider either returning the average as a
doublein the JSON, or documenting this as integer-truncated average.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MCP_Endpoint.cpp (1)
319-324:⚠️ Potential issue | 🟡 MinorTypo in server name:
"proxysql-mcp-mcp-mysql-tools"contains a duplicatedmcp-prefix.This string is returned to MCP clients during the
initializehandshake. It likely should be"proxysql-mcp-mysql-tools"or"proxysql-mcp-stats"(or similar). The current value may confuse clients and monitoring tools.Proposed fix
result["serverInfo"] = { - {"name", "proxysql-mcp-mcp-mysql-tools"}, + {"name", "proxysql-mcp-server"}, {"version", MCP_THREAD_VERSION} };
🤖 Fix all issues with AI agents
In `@doc/MCP/MCP_Stats_Tools_Spec.md`:
- Around line 28-31: The docs list mutating tools flush_query_log and
flush_queries under the /stats monitoring endpoint which is misleading; update
the spec so these write-capable tools are either moved to the admin surface
(/mcp/admin) or annotated with explicit write warnings and auth requirements.
Edit the MCP_Stats_Tools_Spec to: 1) remove or relocate entries for
flush_query_log and flush_queries from the /stats section and add them under
/mcp/admin (or add a clear “write-capable” subsection), 2) add text describing
their destructive effects (drains circular buffer, resets in-memory counters)
and required elevated auth, and 3) update any cross-references to point to the
new location or the authorization note so readers won’t assume /stats is
read-only.
- Around line 70-72: The section header "## 2.1 Time Field Conventions" is
numbered as a subsection but is placed between top-level sections and missing
from the TOC; fix by either promoting it to a top-level header (change "## 2.1
Time Field Conventions" to "## 3. Time Field Conventions" or the appropriate
top-level numbering and renumber subsequent sections) or by adding an entry for
"2.1 Time Field Conventions" to the document's Table of Contents; update any
cross-references and the TOC entries so numbering and placement are consistent
with the surrounding headers ("2. Response Format Convention" and "3. Tool
Categories").
In `@include/MCP_Tool_Handler.h`:
- Around line 185-197: MCP_Tool_Handler.h uses the type SQLite3_result* (in the
declaration of resultset_to_json) but never forward-declares or includes it,
creating a fragile include-order dependency; to fix, add a forward declaration
"class SQLite3_result;" near the top of MCP_Tool_Handler.h (after the existing
includes) so the declaration of resultset_to_json and any other uses of
SQLite3_result* compile even if sqlite3db.h isn’t included first.
In `@include/proxysql_glovars.hpp`:
- Around line 123-125: The field global.genai is uninitialized unless --genai is
passed; update the struct/class constructor that initializes other flags (same
place where gdbg, nostart, foreground are set) to explicitly set global.genai =
false so it has a defined default value before process_opts_pre() and any reads
in main.cpp; touch the constructor in include/proxysql_glovars.hpp where other
booleans are initialized and add the initialization for genai.
In `@include/Stats_Tool_Handler.h`:
- Around line 11-41: The Doxygen block in Stats_Tool_Handler.h lists a different
set of tools (e.g., show_status, show_prepared_statements, flush_query_log,
show_query_log, flush_queries) than the PR description (e.g., get_health,
show_metrics, list_stats, get_stats, aggregate_metrics); reconcile them by
choosing the authoritative toolset, then update the Doxygen comment in
Stats_Tool_Handler.h to exactly match the implemented/advertised MCP endpoints
and tool names (or vice versa update the implementation/PR text to match the
docs). Ensure every tool listed (show_status, show_processlist, show_queries,
show_commands, show_connections, show_errors, show_users, show_client_cache,
show_query_rules, show_prepared_statements, show_gtid, show_cluster,
show_system_history, show_query_cache_history, show_connection_history,
show_query_history, flush_query_log, show_query_log, flush_queries or the PR
equivalents get_health, show_metrics, list_stats, get_stats, aggregate_metrics)
corresponds to an implemented handler function or is removed, and keep the
Doxygen summary, counts (Live/Historical/Utility) and names consistent.
In `@lib/ProxySQL_GloVars.cpp`:
- Around line 434-438: The constructor for the global GloVars instance is
missing explicit initialization for the boolean global.genai; update the GloVars
constructor (the same place where global.gdbg, global.nostart,
global.foreground, global.sqlite3_server, global.clickhouse_server, etc. are
set) to explicitly set global.genai = false (or the desired default) alongside
those members — this ensures consistency and avoids relying on implicit
zero-initialization; locate the init block around the existing boolean
assignments (near the sqlite3_server initialization) and add global.genai
initialization there.
🧹 Nitpick comments (7)
include/proxysql_utils.h (1)
376-380: Consider usingint64_tfor bucket counts to avoid overflow on busy instances.In
calculate_percentile_from_histogram, bothbucketsand the internaltotalaccumulator areint. On a high-traffic ProxySQL instance, command counters (e.g.,Total_cnt) can exceedINT_MAX(~2.1B). Thetotal * percentilemultiplication would also overflow. If this is expected to only handle per-command histogram buckets (which are typically smaller), this may be acceptable — but worth a note.doc/MCP/MCP_Stats_Implementation_Plan.md (1)
175-204: Add language identifiers to ASCII-art fenced code blocks.The diagram blocks at lines 175, 245, and 310 lack language specifiers, triggering markdownlint MD040. Use
```textfor plain-text diagrams.lib/MCP_Tool_Handler.cpp (1)
28-43: Consider checkingerrnoforstrtoll/strtodoverflow.If a numeric string overflows
long long,strtollreturnsLLONG_MAX/LLONG_MINand setserrno = ERANGE, but the code doesn't check for this. Similarly forstrtodwith extreme values. For ProxySQL stats data this is unlikely to matter, but for correctness:Suggested improvement
char* end = nullptr; + errno = 0; long long ll = strtoll(val, &end, 10); - if (end != val && *end == '\0') { + if (end != val && *end == '\0' && errno != ERANGE) { obj[col_name] = ll; } else { // Not a plain integer; try floating-point + errno = 0; double d = strtod(val, &end); - if (end != val && *end == '\0') { + if (end != val && *end == '\0' && errno != ERANGE) { obj[col_name] = d; } else { obj[col_name] = std::string(val);src/main.cpp (1)
1671-1676: Misleading indentation:GloAdmin->init_mcp_variables()appears to be outside theif (GloMCPH)guard.Line 1675 is the body of the
if (GloMCPH)on line 1674, but its indentation makes it appear to be at the same level as theif. This is a readability trap that could lead to future maintenance mistakes. Adding braces would clarify intent.Proposed fix
if (GloVars.global.genai) { - if (GloGATH) + if (GloGATH) { GloAdmin->init_genai_variables(); - if (GloMCPH) - GloAdmin->init_mcp_variables(); + } + if (GloMCPH) { + GloAdmin->init_mcp_variables(); + } }lib/ProxySQL_MCP_Server.cpp (1)
127-131: Missing error handling for Stats Tool Handler initialization failure.If
init()returns non-zero, the handler is left allocated but potentially in an invalid state. Compare with Config/Query handlers (lines 84-91, 107-113) which log an error, delete the handler, and set it to NULL on failure.This is consistent with the Admin and Cache handlers (which also lack error handling), but it would be safer to handle failures uniformly.
Add error handling consistent with Config/Query handlers
// 5. Stats Tool Handler handler->stats_tool_handler = new Stats_Tool_Handler(handler); if (handler->stats_tool_handler->init() == 0) { proxy_info("Stats Tool Handler initialized\n"); + } else { + proxy_error("Failed to initialize Stats Tool Handler\n"); + delete handler->stats_tool_handler; + handler->stats_tool_handler = NULL; }include/Stats_Tool_Handler.h (2)
6-9: Missing direct includes for<string>and<vector>.The class declarations use
std::string,std::vector<int>, andjsonbut only<map>is directly included. The others are resolved transitively throughMCP_Tool_Handler.h/MCP_Thread.h, which is fragile — if those headers change their includes, this file breaks.Proposed fix
`#include` <map> +#include <string> +#include <vector> `#include` "MCP_Tool_Handler.h" `#include` "MCP_Thread.h"
177-193: Consider documenting or enforcing RAII forSQLite3_result**output parameters.Both
execute_admin_queryandexecute_statsdb_disk_queryrequire the caller todeletethe resultset. This is a common leak vector, especially if early returns or exceptions occur between allocation and cleanup. If ProxySQL's codebase doesn't already have a RAII wrapper forSQLite3_result, a smallunique_ptrdeleter alias would prevent leaks in all 19+ tool handlers.
| - [6. Utility Tools](#6-utility-tools) | ||
| - [6.1 flush_query_log](#61-flush_query_log) | ||
| - [6.2 show_query_log](#62-show_query_log) | ||
| - [6.3 flush_queries](#63-flush_queries) |
There was a problem hiding this comment.
Mutating tools (flush_query_log, flush_queries) on a monitoring/stats endpoint may violate principle of least surprise.
The /stats endpoint is semantically a read-only monitoring endpoint. Including tools that flush data (flush_query_log drains the circular buffer, flush_queries resets in-memory counters) means a monitoring-only auth token could inadvertently allow data-destructive operations. Consider either:
- Moving these to the
/mcp/adminendpoint, or - Clearly documenting the write-capable nature and ensuring the auth model accounts for it.
🤖 Prompt for AI Agents
In `@doc/MCP/MCP_Stats_Tools_Spec.md` around lines 28 - 31, The docs list mutating
tools flush_query_log and flush_queries under the /stats monitoring endpoint
which is misleading; update the spec so these write-capable tools are either
moved to the admin surface (/mcp/admin) or annotated with explicit write
warnings and auth requirements. Edit the MCP_Stats_Tools_Spec to: 1) remove or
relocate entries for flush_query_log and flush_queries from the /stats section
and add them under /mcp/admin (or add a clear “write-capable” subsection), 2)
add text describing their destructive effects (drains circular buffer, resets
in-memory counters) and required elevated auth, and 3) update any
cross-references to point to the new location or the authorization note so
readers won’t assume /stats is read-only.
statsstats
|
retest this please |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doc/Stats_Tables.md`:
- Around line 1728-1736: The fenced code block showing the data flow lacks a
language identifier; update the block to include "text" (e.g., ```text) so
linters render it consistently. Locate the diagram block that mentions
MySQL_Logger::log_request(), MyLogCB, mysql_eventslog_table_memory_size and the
Manual DUMP command and change the opening fence to ```text while keeping the
content unchanged.
|
@gemini-code-assist full review |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new /mcp/stats endpoint, replacing /mcp/observe and adding a comprehensive set of 19 tools for monitoring and diagnostics. The changes are extensive, including new documentation, a new Stats_Tool_Handler, and updates across the MCP framework to support the new endpoint. The implementation is well-structured, and the documentation is excellent. I've found one potential data corruption bug in the JSON serialization logic and a minor fragility in the implementation plan documentation. Overall, this is a great enhancement to ProxySQL's observability capabilities.
| char* end = nullptr; | ||
| long long ll = strtoll(val, &end, 10); | ||
| if (end != val && *end == '\0') { | ||
| obj[col_name] = ll; | ||
| } else { | ||
| // Not a plain integer; try floating-point | ||
| double d = strtod(val, &end); | ||
| if (end != val && *end == '\0') { | ||
| obj[col_name] = d; | ||
| } else { | ||
| obj[col_name] = std::string(val); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current numeric parsing logic using strtoll can incorrectly handle large unsigned 64-bit integers, which are common for statistics counters. When strtoll is given a value larger than LLONG_MAX, it returns LLONG_MAX and sets errno to ERANGE. Since errno is not checked, this leads to silent data corruption, where large counters are reported as LLONG_MAX.
A more robust approach is to first attempt parsing as unsigned long long for non-negative values, and then fall back to other types, while also checking errno for range errors.
char* end = nullptr;
bool parsed = false;
// Try unsigned long long for non-negative numbers, which is common for stats counters.
if (val[0] != '-') {
errno = 0;
unsigned long long ull = strtoull(val, &end, 10);
if (end != val && *end == '\0' && errno != ERANGE) {
obj[col_name] = ull;
parsed = true;
}
}
// Try long long if not parsed yet (e.g., for negative numbers).
if (!parsed) {
errno = 0;
end = nullptr;
long long ll = strtoll(val, &end, 10);
if (end != val && *end == '\0' && errno != ERANGE) {
obj[col_name] = ll;
parsed = true;
}
}
// Fallback to double if it's not an integer.
if (!parsed) {
end = nullptr;
double d = strtod(val, &end);
if (end != val && *end == '\0') {
obj[col_name] = d;
parsed = true;
}
}
// Finally, treat as a string if no numeric conversion worked.
if (!parsed) {
obj[col_name] = std::string(val);
}| atoi(row->fields[3]), // cnt_100us | ||
| atoi(row->fields[4]), // cnt_500us | ||
| atoi(row->fields[5]), // cnt_1ms | ||
| atoi(row->fields[6]), // cnt_5ms | ||
| atoi(row->fields[7]), // cnt_10ms | ||
| atoi(row->fields[8]), // cnt_50ms | ||
| atoi(row->fields[9]), // cnt_100ms | ||
| atoi(row->fields[10]), // cnt_500ms | ||
| atoi(row->fields[11]), // cnt_1s | ||
| atoi(row->fields[12]), // cnt_5s | ||
| atoi(row->fields[13]), // cnt_10s | ||
| atoi(row->fields[14]) // cnt_INFs |
There was a problem hiding this comment.
Accessing result set fields by hardcoded numeric indices (row->fields[3], row->fields[4], etc.) is fragile. If the column order in the SELECT query for stats_mysql_commands_counters changes in the future, this code will lead to incorrect data or crashes. It would be more robust to look up column indices by their names from the SQLite3_result->column_definition at the beginning of the result processing loop. This would make the implementation more maintainable and resilient to schema changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@doc/Stats_Tables.md`:
- Around line 57-74: The CREATE TABLE for stats_mysql_commands_counters is
missing a type (and NOT NULL) for the cnt_INFs column; update the column
definition for cnt_INFs to match the other counters (e.g., "INT NOT NULL") in
the stats_mysql_commands_counters table and apply the same fix to the PostgreSQL
variant where cnt_INFs is likewise missing its type to keep the schema
consistent.
In `@lib/ProxySQL_MCP_Server.cpp`:
- Around line 127-131: The Stats_Tool_Handler allocation/initialization lacks
failure handling: when handler->stats_tool_handler->init() != 0 you must mirror
the pattern used by the Config and Query handlers by deleting the
partially-constructed handler, setting handler->stats_tool_handler = nullptr,
and not registering the /mcp/stats endpoint; update the code around
Stats_Tool_Handler, and also review Admin/Cache handler initializations to
ensure they similarly clean up on init failure and guard their endpoint
registrations so endpoints are only registered when init() succeeds.
🧹 Nitpick comments (2)
doc/MCP/MCP_Stats_Implementation_Plan.md (1)
412-420: Note:build_time_range_queryuses string concatenation for SQL construction.The pseudocode builds SQL via string concatenation. While
std::to_string(start)andstd::to_string(now)are safe here (integer timestamps, not user input), the actual implementation should prefer parameterized queries for consistency with the rest of the codebase, where parameterized patterns (? OR ? IS NULL) are already demonstrated (e.g., lines 502–506).src/main.cpp (1)
1671-1676: Inconsistent indentation on Line 1675 —GloAdmin->init_mcp_variables()appears at the same level as itsifguard.While functionally correct (braceless
ifin C++ takes the next statement), the indentation doesn't match the pattern on Line 1673. This is a pre-existing line, but the newif (GloMCPH)guard on Line 1674 makes the visual inconsistency more noticeable.Suggested fix
if (GloMCPH) - GloAdmin->init_mcp_variables(); + GloAdmin->init_mcp_variables();
| int port = std::stoi(port_str); | ||
| sql += " AND srv_host = '" + sql_escape(host) + "' AND srv_port = " + std::to_string(port); | ||
| } catch (...) { | ||
| // Invalid port - skip server filter |
There was a problem hiding this comment.
We need to log this.
It should never happen, I am tempted to put an assert() here
| int port = std::stoi(port_str); | ||
| sql += " AND srv_host = '" + sql_escape(host) + "' AND srv_port = " + std::to_string(port); | ||
| } catch (...) { | ||
| // Invalid port - skip server filter |
There was a problem hiding this comment.
As before, we need to log this.
It should never happen, I am tempted to put an assert() here
| SQLite3_result* count_rs = NULL; | ||
| int count_cols = 0; | ||
| int total_sessions = 0; | ||
| execute_admin_query(count_sql.c_str(), &count_rs, &count_cols); |
There was a problem hiding this comment.
We need to log if any error happens.
Although syntax errors shouldn't happen, some tables may exist only if certain modules are loaded. Unless we filter certain tablename based on what module is not loaded, we should log an error AND return an error to the client.
I see there are multiple cases like this in the code, like at line 1190 , 1617 , etc
| conditions.push_back("Variable_Name LIKE '" + sql_escape(variable_name) + "'"); | ||
| } else if (!category.empty()) { | ||
| auto it = CATEGORY_PREFIXES.find(category); | ||
| if (it != CATEGORY_PREFIXES.end()) { |
There was a problem hiding this comment.
What happens when a category is not found?
|
Added some comments. |
Signed-off-by: René Cannaò <rene@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MCP_Endpoint.cpp (1)
37-52:⚠️ Potential issue | 🟠 MajorThe
"ai"endpoint is missing from the authentication chain and is permanently inaccessible.The
"ai"endpoint is registered inProxySQL_MCP_Server.cpp(lines 171–177) but has no case in theif-elsechain atlib/MCP_Endpoint.cpplines 37–48. Every POST to/mcp/aihits theelsebranch, which logs an error and returnsfalse, causing HTTP 401 for all requests.Add the missing `"ai"` case
} else if (endpoint_name == "rag") { expected_token = handler->variables.mcp_rag_endpoint_auth; + } else if (endpoint_name == "ai") { + expected_token = handler->variables.mcp_ai_endpoint_auth; } else { proxy_error("MCP authentication on %s: unknown endpoint\n", endpoint_name.c_str());This requires creating a corresponding
mcp_ai_endpoint_authvariable inMCP_Threads_Handler::variables(matching the pattern of other auth vars), or setexpected_token = nullptrif unauthenticated access is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MCP_Endpoint.cpp` around lines 37 - 52, The auth branch is missing a case for endpoint_name == "ai", causing /mcp/ai requests to fall into the unknown endpoint path; add an else-if branch similar to the other endpoints that sets expected_token = handler->variables.mcp_ai_endpoint_auth (or expected_token = nullptr if the AI endpoint should be unauthenticated), and ensure MCP_Threads_Handler::variables defines mcp_ai_endpoint_auth with the same type/pattern as the other mcp_*_endpoint_auth members so the symbol exists when referenced.
🧹 Nitpick comments (2)
src/main.cpp (1)
1668-1673: Missing braces onif (GloMCPH)body makes intent unclear.Line 1671 guards
init_mcp_variables()without braces, and the indentation makes it look likeinit_mcp_variables()is unguarded. Add braces to make the guard explicit.🔧 Proposed fix
if (GloVars.global.genai) { if (GloGATH) GloAdmin->init_genai_variables(); - if (GloMCPH) - GloAdmin->init_mcp_variables(); + if (GloMCPH) { + GloAdmin->init_mcp_variables(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 1668 - 1673, The if-statement guarding init_mcp_variables() lacks braces making the intent ambiguous; update the conditional that checks GloMCPH so its body is wrapped in braces—i.e., change the block containing GloMCPH and the call to GloAdmin->init_mcp_variables() to use { } so the call is clearly executed only when GloMCPH is true and aligns with the existing GloGATH branch that calls GloAdmin->init_genai_variables().lib/ProxySQL_MCP_Server.cpp (1)
129-129: Duplicateextern AI_Features_Manager *GloAI;declaration in the same constructor.The same
externdeclaration appears again at line 180. The first declaration already bringsGloAIinto scope for the entire function body; the second is dead code.🔧 Remove the duplicate extern
- // 7. RAG endpoint (for Retrieval-Augmented Generation) - extern AI_Features_Manager *GloAI; if (GloAI) {The
extern AI_Features_Manager *GloAI;on line 180 can be removed sinceGloAIis already in scope from line 129.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_MCP_Server.cpp` at line 129, Remove the redundant extern declaration for the global pointer by deleting the second "extern AI_Features_Manager *GloAI;" occurrence inside the same constructor/function; keep the first declaration (the one already at the top of the function) so GloAI remains in scope and remove the duplicate near line 180 to avoid dead/redundant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/proxysql_utils.h`:
- Around line 369-381: Validate inputs and fix the P0 edge case in
calculate_percentile_from_histogram: first assert or clamp percentile to
[0.0,1.0] and verify buckets.size() == thresholds.size() and non-empty
buckets/thresholds; if percentile == 0.0, return the threshold of the first
bucket with a non-zero count (and return 0 if all counts are zero) instead of
using the cumulative >= 0 check; otherwise compute the target rank from
percentile and total count and iterate cumulative counts to find the bucket
whose cumulative meets or exceeds that target (return thresholds.back() if
target exceeds total).
- Around line 362-367: The sql_escape function currently doubles backslashes in
addition to doubling single quotes, which is incorrect for SQLite and causes
mismatches (e.g., "C:\Users\test" -> "C:\\Users\\test"); modify the
implementation of sql_escape to stop escaping/backslash-doubling and only
replace single-quote characters (') with two single quotes ('') per SQL/SQLite
rules, update the function's documentation comment to reflect this
SQLite-correct behavior, and run/verify the callers (notably usages in
Stats_Tool_Handler.cpp that query stats.* tables) still behave as expected.
---
Outside diff comments:
In `@lib/MCP_Endpoint.cpp`:
- Around line 37-52: The auth branch is missing a case for endpoint_name ==
"ai", causing /mcp/ai requests to fall into the unknown endpoint path; add an
else-if branch similar to the other endpoints that sets expected_token =
handler->variables.mcp_ai_endpoint_auth (or expected_token = nullptr if the AI
endpoint should be unauthenticated), and ensure MCP_Threads_Handler::variables
defines mcp_ai_endpoint_auth with the same type/pattern as the other
mcp_*_endpoint_auth members so the symbol exists when referenced.
---
Duplicate comments:
In `@lib/ProxySQL_MCP_Server.cpp`:
- Around line 122-126: The Stats_Tool_Handler initialization lacks failure
handling: after constructing handler->stats_tool_handler and calling
handler->stats_tool_handler->init(), check for a non-zero return and on failure
log an error (e.g., proxy_err or proxy_info with clear message including "Stats
Tool Handler init failed"), delete or free handler->stats_tool_handler and set
it to nullptr, and propagate the error (return failure or set an appropriate
status) so the caller knows initialization failed; update the block around
Stats_Tool_Handler, its init() call, and any cleanup paths to handle this case.
---
Nitpick comments:
In `@lib/ProxySQL_MCP_Server.cpp`:
- Line 129: Remove the redundant extern declaration for the global pointer by
deleting the second "extern AI_Features_Manager *GloAI;" occurrence inside the
same constructor/function; keep the first declaration (the one already at the
top of the function) so GloAI remains in scope and remove the duplicate near
line 180 to avoid dead/redundant code.
In `@src/main.cpp`:
- Around line 1668-1673: The if-statement guarding init_mcp_variables() lacks
braces making the intent ambiguous; update the conditional that checks GloMCPH
so its body is wrapped in braces—i.e., change the block containing GloMCPH and
the call to GloAdmin->init_mcp_variables() to use { } so the call is clearly
executed only when GloMCPH is true and aligns with the existing GloGATH branch
that calls GloAdmin->init_genai_variables().
| /** | ||
| * @brief Calculate an approximate percentile value from histogram bucket counts. | ||
| * @param buckets Vector of counts per histogram bucket. | ||
| * @param thresholds Vector of upper-bound threshold values for each bucket (same length as buckets). | ||
| * @param percentile The percentile to calculate, in the range [0.0, 1.0]. | ||
| * @return The threshold value of the bucket in which the target percentile falls, | ||
| * or 0 if the buckets are empty. | ||
| */ | ||
| int calculate_percentile_from_histogram( | ||
| const std::vector<int>& buckets, | ||
| const std::vector<int>& thresholds, | ||
| double percentile | ||
| ); |
There was a problem hiding this comment.
calculate_percentile_from_histogram: missing input validation and P0 edge-case quirk.
Two minor concerns:
-
No range check on
percentile: Values outside[0.0, 1.0]silently producethresholds.back()(target exceeds total). Adding an assert or clamp would make misuse visible early. -
P0 always returns
thresholds[0]: Whenpercentile == 0.0,target = 0; after the firstcumulative += buckets[0], the conditioncumulative >= 0is immediately satisfied regardless of whether that bucket contains any counts. For sparse histograms (e.g., first several buckets are zero) this returns the wrong threshold.
💡 Suggested guard and doc note
int calculate_percentile_from_histogram(
const std::vector<int>& buckets,
const std::vector<int>& thresholds,
double percentile
) {
+ assert(percentile >= 0.0 && percentile <= 1.0);
long long total = 0;
for (int b : buckets) total += b;
if (total == 0) return 0;
long long target = (long long)(total * percentile);
+ if (target == 0) target = 1; // ensure at least one count is crossed for P0
long long cumulative = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/proxysql_utils.h` around lines 369 - 381, Validate inputs and fix the
P0 edge case in calculate_percentile_from_histogram: first assert or clamp
percentile to [0.0,1.0] and verify buckets.size() == thresholds.size() and
non-empty buckets/thresholds; if percentile == 0.0, return the threshold of the
first bucket with a non-zero count (and return 0 if all counts are zero) instead
of using the cumulative >= 0 check; otherwise compute the target rank from
percentile and total count and iterate cumulative counts to find the bucket
whose cumulative meets or exceeds that target (return thresholds.back() if
target exceeds total).
There was a problem hiding this comment.
🧹 Nitpick comments (4)
include/Stats_Tool_Handler.h (1)
213-225:calculate_percentilereturn type may truncate; vector parameters should beconst&.Two related issues with this signature:
Return type: The function returns
int(signed 32-bit, capped at ~2.1 billion µs ≈ 35 minutes). ProxySQL's histogram has an open-ended "INF" bucket; an interpolated value within it, or any extreme latency, would silently truncate. Usingint64_teliminates the risk with no ABI cost at this internal scope.Parameter passing:
bucketsandthresholdsare passed by value (heap-allocated vector copies) for read-only inputs. They should beconst std::vector<int>&.♻️ Proposed fix
- int calculate_percentile(const std::vector<int>& buckets, const std::vector<int>& thresholds, double percentile); + int64_t calculate_percentile(const std::vector<int64_t>& buckets, const std::vector<int64_t>& thresholds, double percentile);(bucket counts are already described as 64-bit in the doc-comment; aligning the threshold vector element type avoids a mixed-width comparison in the implementation.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Stats_Tool_Handler.h` around lines 213 - 225, Change the signature of calculate_percentile to return int64_t and accept the vectors by const reference to avoid truncation and unnecessary copies: update calculate_percentile(const std::vector<int>& buckets, const std::vector<int>& thresholds, double percentile) to use int64_t for the return and make buckets and thresholds const std::vector<int64_t>& (or at least const std::vector<int64_t>& for buckets and thresholds to match 64-bit accumulation referenced in the doc and avoid mixed-width comparisons), and propagate that type change through any callers and implementations of calculate_percentile.lib/ProxySQL_MCP_Server.cpp (1)
176-203: AI and RAG endpoints bypass the newregister_endpointlambda.These two endpoints duplicate the registration pattern (construct
unique_ptr, callregister_resource, push to_endpoints) rather than reusing the lambda introduced at line 158. Consistency would also close the gap where the RAG handler init failure (proxy_error/delete/ null) isn't mirrored for the successful-init path's endpoint registration.♻️ Suggested refactoring for AI/RAG endpoints to use the lambda
- if (handler->ai_tool_handler) { - std::unique_ptr<httpserver::http_resource> ai_resource = - std::unique_ptr<httpserver::http_resource>(new MCP_JSONRPC_Resource(handler, handler->ai_tool_handler, "ai")); - ws->register_resource("/mcp/ai", ai_resource.get(), true); - _endpoints.push_back({"/mcp/ai", std::move(ai_resource)}); - } + register_endpoint("/mcp/ai", handler->ai_tool_handler, "ai");And after the RAG handler init block:
- std::unique_ptr<httpserver::http_resource> rag_resource = - std::unique_ptr<httpserver::http_resource>(new MCP_JSONRPC_Resource(handler, handler->rag_tool_handler, "rag")); - ws->register_resource("/mcp/rag", rag_resource.get(), true); - _endpoints.push_back({"/mcp/rag", std::move(rag_resource)}); - proxy_info("RAG Tool Handler initialized\n"); + proxy_info("RAG Tool Handler initialized\n"); + register_endpoint("/mcp/rag", handler->rag_tool_handler, "rag");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_MCP_Server.cpp` around lines 176 - 203, The AI and RAG endpoint blocks duplicate manual resource creation/registration instead of using the existing register_endpoint lambda (the helper that constructs a unique_ptr, calls ws->register_resource and pushes into _endpoints); change both AI and RAG branches to call that lambda with MCP_JSONRPC_Resource(...) and the path ("/mcp/ai" and "/mcp/rag") so registration is consistent and centralized, and ensure the RAG success/failure symmetry: only create/register the resource after rag_tool_handler->init() returns success, and on failure call proxy_error, delete handler->rag_tool_handler and set it to NULL exactly as the failure branch already does. Reference symbols: MCP_JSONRPC_Resource, handler->ai_tool_handler, handler->rag_tool_handler, RAG_Tool_Handler, GloAI, _endpoints, ws->register_resource, and the register_endpoint lambda.lib/MCP_Tool_Handler.cpp (2)
1-12: Add direct include ofMCP_Tool_Handler.hfor explicit self-inclusion.
MCP_Tool_Handler.h(which declaresMCP_Tool_Handler::resultset_to_json) is not directly#included in this translation unit. While compilation succeeds due to the transitive path (cpp.h→MCP_Tool_Handler.h), explicit self-inclusion is the recommended practice and provides protection against future header refactors.Add
#include "MCP_Tool_Handler.h"after the existing includes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MCP_Tool_Handler.cpp` around lines 1 - 12, Add a direct include for the header that declares this unit’s API: insert `#include` "MCP_Tool_Handler.h" among the existing includes (after the other project headers like "cpp.h" or "sqlite3db.h") so the translation unit explicitly includes the declaration for MCP_Tool_Handler::resultset_to_json and related symbols instead of relying on transitive includes.
20-24: Addrow->cntguard before accessingrow->fields[i].The inner loop bounds check uses
min(cols, column_definition.size())but does not validate the actual field count of the row. Thecntfield tracks the number of fields in each row instance and can be less than expected columns if rows are malformed or truncated. Other code paths in the codebase (e.g.,MCP_Thread.cppline 420 andQuery_Tool_Handler.cpp) consistently guard field access withrow->cntbefore indexing intofields. Without this guard, accessingrow->fields[i]fori >= row->cntis out-of-bounds.Suggested defensive guard
for (int i = 0; i < cols && i < (int)resultset->column_definition.size(); i++) { + if (i >= row->cnt) break; const char* col_name = resultset->column_definition[i]->name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MCP_Tool_Handler.cpp` around lines 20 - 24, The loop iterating resultset->rows must check each row's actual field count before indexing into row->fields; modify the inner loop (the loop that currently uses cols and resultset->column_definition.size()) to also ensure i < row->cnt (i.e., guard with row->cnt) before accessing row->fields[i], and skip or set null for missing fields so you never read out-of-bounds from row->fields; update the code around resultset, row, cols, column_definition, and fields to use this defensive check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/MCP/MCP_Stats_Tools_Spec.md`:
- Around line 29-32: The docs list write-capable tools under the MCP
stats/monitoring endpoint (flush_query_log, flush_queries); move these tool
entries and their sections from the stats endpoint into the MCP admin area
(e.g., /mcp/admin) and update the TOC/anchors (sections for flush_query_log and
flush_queries) accordingly, or alternatively explicitly document that these
endpoints require elevated auth by adding an authentication/authorization
subsection for flush_query_log and flush_queries (specifying admin role, scopes,
and CSRF/intent checks) and update all example paths and links to reflect the
chosen location/auth model; ensure references to flush_query_log and
flush_queries in the document and TOC are consistent.
- Around line 72-74: The section header "## 2.1 Time Field Conventions" is
mis-numbered and missing from the Table of Contents; either promote it to a
top-level section (rename the header to "## 3. Time Field Conventions" and
update any subsequent section numbering) or add an entry for "2.1 Time Field
Conventions" into the existing ToC (lines referenced as 7–32) so the document
and ToC stay consistent; update any cross-references accordingly (look for the
header string "## 2.1 Time Field Conventions" to locate the section and the ToC
block to insert or adjust entries).
---
Nitpick comments:
In `@include/Stats_Tool_Handler.h`:
- Around line 213-225: Change the signature of calculate_percentile to return
int64_t and accept the vectors by const reference to avoid truncation and
unnecessary copies: update calculate_percentile(const std::vector<int>& buckets,
const std::vector<int>& thresholds, double percentile) to use int64_t for the
return and make buckets and thresholds const std::vector<int64_t>& (or at least
const std::vector<int64_t>& for buckets and thresholds to match 64-bit
accumulation referenced in the doc and avoid mixed-width comparisons), and
propagate that type change through any callers and implementations of
calculate_percentile.
In `@lib/MCP_Tool_Handler.cpp`:
- Around line 1-12: Add a direct include for the header that declares this
unit’s API: insert `#include` "MCP_Tool_Handler.h" among the existing includes
(after the other project headers like "cpp.h" or "sqlite3db.h") so the
translation unit explicitly includes the declaration for
MCP_Tool_Handler::resultset_to_json and related symbols instead of relying on
transitive includes.
- Around line 20-24: The loop iterating resultset->rows must check each row's
actual field count before indexing into row->fields; modify the inner loop (the
loop that currently uses cols and resultset->column_definition.size()) to also
ensure i < row->cnt (i.e., guard with row->cnt) before accessing row->fields[i],
and skip or set null for missing fields so you never read out-of-bounds from
row->fields; update the code around resultset, row, cols, column_definition, and
fields to use this defensive check.
In `@lib/ProxySQL_MCP_Server.cpp`:
- Around line 176-203: The AI and RAG endpoint blocks duplicate manual resource
creation/registration instead of using the existing register_endpoint lambda
(the helper that constructs a unique_ptr, calls ws->register_resource and pushes
into _endpoints); change both AI and RAG branches to call that lambda with
MCP_JSONRPC_Resource(...) and the path ("/mcp/ai" and "/mcp/rag") so
registration is consistent and centralized, and ensure the RAG success/failure
symmetry: only create/register the resource after rag_tool_handler->init()
returns success, and on failure call proxy_error, delete
handler->rag_tool_handler and set it to NULL exactly as the failure branch
already does. Reference symbols: MCP_JSONRPC_Resource, handler->ai_tool_handler,
handler->rag_tool_handler, RAG_Tool_Handler, GloAI, _endpoints,
ws->register_resource, and the register_endpoint lambda.
This reverts commit 33fce1c.
|
|
Putting this PR on hold. |
2 participants
/mcp/statsMCP endpoint (replacing the previous/mcp/observeendpoint) that provides 19 tools for monitoring, diagnostics, and performance analysis of ProxySQL through the MCP protocol.Live Data Tools
show_status- Global status variablesshow_processlist- Active sessionsshow_queries- Query digest statsshow_commands- Command execution statsshow_connections- Backend connection pool metricsshow_errors- Error tracking statsshow_users- Per-user connection statsshow_client_cache- Client host cacheshow_query_rules- Query rule hit statsshow_prepared_statements- Prepared statement infoshow_gtid- GTID replication infoshow_cluster- Cluster node healthHistorical Data Tools
show_system_history- CPU/memory usage trendsshow_query_cache_history- Query cache metricsshow_connection_history- Historical connection metricsshow_query_history- Historical query digestsUtility Tools
flush_query_log- Flush query events to tablesshow_query_log- Individual query eventsflush_queries- Save digests to diskSummary by CodeRabbit
New Features
Configuration
Utilities
Documentation
Tests