Serve the last eight PostgreSQL collectors - #2632
Conversation
Every PostgreSQL collector except pg_plan_capture_readiness now has an
MCP tool and a web panel. The ratchet drops from nine to one, and that
one is the case it was built to tolerate: a single row of configuration
state is a panel, not a question anyone asks an agent.
The store readers all existed already - they were built for the Viewer.
What was missing was the tool, the dispatch entry, the grid and the
pins, which is why nine of these accumulated: each one's panel WAS the
deliverable and the read was always the next PR.
Placed by the question each answers, not by convenience:
Activity predicate selectivity and column statistics, beside top
queries - a predicate row and a query row are two views of
one execution; and sampled lock activity, because
PostgreSQL has no Blocking tab and get_pg_blocking lives
here too.
Storage measured index bloat, last of the three, because it is the
only one of them that is not an estimate and a reader who
has just been warned about estimates should meet the real
measurement immediately after.
I/O buffer residency and checkpoint/WAL - what the I/O counters
already there are reading into, and what writes it back.
Overview the extension inventory, because it is the answer to most
of the empty panels elsewhere on the page: 'available'
means the files are there and one CREATE EXTENSION fills a
grid that currently reads as a permanent absence.
Replication connected replicas beside the slots. The dangerous case
is the DISAGREEMENT - a slot that outlives the replica
retains WAL forever - and neither panel alone shows it.
Each tool carries the caveat its data cannot be read safely without.
Predicate counts are SAMPLED and are not scaled up here, because
multiplying by 100 and presenting the product would launder an estimate
into a fact. An index bloat row with a skipped_reason was NOT measured,
which must never read as healthy. Lock stats are a sample of pg_locks,
so an absence is not proof nothing was locked. n_distinct is a ratio
when negative and a count when positive.
And one defect found in my own work before it shipped: get_pg_extensions
reported "installed: 10" at a 50-row limit for a server with 13,
because 50 rows was all it had looked at. A summary counted over a
capped result describes the PAGE and reads as a fact about the SERVER.
Those totals are now withheld when the result is truncated, with
truncated:true and a sentence saying why - the same shape as the
partial-failure note, and the same lesson: a number is only as wide as
what it counted.
Closes #2629.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| + "on a perfectly healthy server — widen it before concluding anything."); | ||
| } | ||
|
|
||
| var truncated = rows.Count >= limit; |
There was a problem hiding this comment.
Truncation false-positive: rows.Count >= limit reports truncated: true whenever the server happens to have exactly limit indexes and no more — since the reader binds LIMIT $4 directly to limit (see DarlingPgIndexBloatReader.cs:104), there's no over-fetch to distinguish "exactly full" from "more exists."
This codebase already hit and fixed this exact bug in DarlingMcpQueryHeatmapTools.cs:78-89, which over-fetches by one (limit + 1) and compares with rows.Count > limit, with a comment explaining why:
Comparing the row count to the cap reports truncation for a server that happens to have exactly
limitcells and nothing more, which is a false positive in the one field whose whole reason for existing is that the cap should not have to be inferred.
As written, a server with exactly 25 measured indexes gets truncated: true and measured_count withheld even though the result is complete. Same pattern repeats at DarlingMcpPgServerStateTools.cs:138 (get_pg_extensions) and :215 (get_pg_lock_stats).
| tool reported "installed: 10" for a server with more than that, because 50 was all it had | ||
| looked at. Suppressed rather than renamed — "installed_in_this_page" is a number nobody | ||
| wants. */ | ||
| var truncated = rows.Count >= limit; |
There was a problem hiding this comment.
Same false-positive as DarlingMcpPgIndexTools.cs:72 — rows.Count >= limit (reader binds LIMIT $4 directly to limit, DarlingPgExtensionAvailabilityReader.cs:86) reports truncated: true for a server whose extension inventory happens to total exactly limit (default 50) rows, wrongly withholding installed/available_not_installed even though the count is complete.
This is the exact bug this PR's own commit message calls out fixing for get_pg_extensions ("a summary counted over a capped result describes the page and reads as a fact about the server") — except the fix itself is now vulnerable to the boundary case, because it can't tell "capped" from "exactly full." The codebase's established fix for this (DarlingMcpQueryHeatmapTools.cs:78-89) over-fetches by one (limit + 1) and compares with rows.Count > limit so the boundary is unambiguous. Same issue repeats at line 215 (get_pg_lock_stats) and in DarlingMcpPgIndexTools.cs:72 (get_pg_index_bloat).
| + "that nothing was ever locked."); | ||
| } | ||
|
|
||
| var truncated = rows.Count >= limit; |
There was a problem hiding this comment.
Same truncation false-positive as line 138 and DarlingMcpPgIndexTools.cs:72: rows.Count >= limit (reader binds LIMIT $4 to limit directly, DarlingPgLockStatsReader.cs:97) will withhold ungranted_count for a server whose sampled lock rows happen to total exactly limit (default 25), even though the result is complete. See the fix pattern already established in DarlingMcpQueryHeatmapTools.cs:78-89 (over-fetch limit + 1, compare rows.Count > limit).
| ## Tool Reference | ||
|
|
||
| This server exposes 118 tools. 86 are the same names Performance Monitor Lite exposes, spanning diagnostic analysis, plan analysis, data reads at core and diagnostic depth, resource contention + jobs, trends, system-health parse-on-read, alerts + health overview, and the Default Trace. The remaining 32 are unique to Darling: fifteen are the PostgreSQL reads (Aurora/PostgreSQL targets only Darling's central store can hold), eight are the Custom Views tools (seven manage the saved views — the one view-authoring write surface — and `describe_custom_view_catalog` returns the read-only compose vocabulary those authoring tools draw from), three are alert-tuning write tools (`update_alert_settings` tunes the alert engine's thresholds; `create_mute_rule` / `delete_mute_rule` manage the mute rules) that write only the shared alert configuration in the monitoring store, two are server-onboarding write tools (`add_servers` bulk-adds monitored servers; `remove_server` removes one) that add or remove rows in the monitoring store's monitored-server registry, `get_fleet_overview` and `get_ag_health` are the two cross-server reads only a central store can answer, `get_store_metrics` reads the monitoring store's OWN hourly size/compression/growth series for capacity forecasting, and `get_blocking` is Darling's name for the blocked-process-report read that Lite exposes as `get_blocked_process_reports` — a naming difference, not a capability gap. Every data-read tool reads the data the collectors already captured into the store — a stored read, never a live query against the monitored server. | ||
| This server exposes 127 tools. 86 are the same names Performance Monitor Lite exposes, spanning diagnostic analysis, plan analysis, data reads at core and diagnostic depth, resource contention + jobs, trends, system-health parse-on-read, alerts + health overview, and the Default Trace. The remaining 41 are unique to Darling: twenty-four are the PostgreSQL reads (Aurora/PostgreSQL targets only Darling's central store can hold), eight are the Custom Views tools (seven manage the saved views — the one view-authoring write surface — and `describe_custom_view_catalog` returns the read-only compose vocabulary those authoring tools draw from), three are alert-tuning write tools (`update_alert_settings` tunes the alert engine's thresholds; `create_mute_rule` / `delete_mute_rule` manage the mute rules) that write only the shared alert configuration in the monitoring store, two are server-onboarding write tools (`add_servers` bulk-adds monitored servers; `remove_server` removes one) that add or remove rows in the monitoring store's monitored-server registry, `get_fleet_overview` and `get_ag_health` are the two cross-server reads only a central store can answer, `get_store_metrics` reads the monitoring store's OWN hourly size/compression/growth series for capacity forecasting, and `get_blocking` is Darling's name for the blocked-process-report read that Lite exposes as `get_blocked_process_reports` — a naming difference, not a capability gap. Every data-read tool reads the data the collectors already captured into the store — a stored read, never a live query against the monitored server. |
There was a problem hiding this comment.
The updated counts appear to be off by one. Counting [McpServerTool(Name = "...")] registrations under Darling/PerformanceMonitor.Darling.Service/:
- Total unique Darling tool names: 126, not 127.
- Names shared with Lite (
Lite/Mcp/*.cs): 86 (matches). - Darling-only tools: 126 − 86 = 40, not 41.
- Of those, PostgreSQL reads (
get_pg_*): 23, not "twenty-four" (custom views 8 + alert-tuning 3 + onboarding 2 + cross-server reads 2 + store_metrics 1 +get_blocking1 + pg reads 23 = 40).
This is the server's own instructions text handed to MCP clients describing its tool catalog, so a wrong count here is a small but real self-description bug (previously accurate: 118 total / 32 unique / fifteen PostgreSQL, and this PR adds exactly 8 PostgreSQL tools, so 118+8=126 and 15+8=23 is what falls out).
|
Reviewed. This PR adds MCP tools + web panels for the last eight PostgreSQL collectors (no T-SQL/.sql files touched, so the T-SQL style rules in CONTRIBUTING.md don't apply here). Overall the work is careful — the truncation-safety pattern (withhold summary counts over a capped page) is the right instinct, and Two issues found, posted inline:
No security, SQL injection, or performance concerns found — all new reads go through parameterized queries in pre-existing reader classes, and the new code follows the established |
Both registries have tabs called 'io' and 'activity', and my insertion anchored on the first match, so buffer usage, checkpoint/WAL, predicate selectivity and column statistics landed on the SQL Server page - where they can never have content and where the PostgreSQL page they belong to still showed nothing. EveryPostgresRead_IsReachableFromThePostgresRegistry_AndFromNoOtherTab caught it by name. That guard exists because this is invisible on inspection: the code reads correctly, the tab id is right, and only the registry it sits in is wrong. Census corrected too: eight tools, not nine - 126 and twenty-three PostgreSQL reads, held to the scanned inventory by its own pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| ["get_pg_predicate_stats"] = "pg_predicate_stats", | ||
| ["get_pg_index_bloat"] = "pg_index_bloat", | ||
| ["get_pg_column_stats"] = "pg_column_stats", | ||
| ["get_pg_buffer_usage"] = "pg_buffer_usage", | ||
| ["get_pg_extensions"] = "pg_extension_availability", | ||
| ["get_pg_lock_stats"] = "pg_lock_stats", | ||
| ["get_pg_write_stats"] = "pg_write_stats", | ||
| ["get_pg_replication_stats"] = "pg_replication_stats", |
There was a problem hiding this comment.
Nit: these 8 new entries are indented 8 spaces instead of the 12 spaces used by every other entry in this dictionary initializer (lines 75-77, 86+). Purely cosmetic, but worth a quick reflow for consistency.
|
|
||
| using System; | ||
| using System.ComponentModel; | ||
| using System.Globalization; |
There was a problem hiding this comment.
using System.Globalization; is unused in this file (no CultureInfo reference here, unlike DarlingMcpPgPredicateTools.cs which does use it for queryid.ToString(CultureInfo.InvariantCulture)). Minor, but CONTRIBUTING.md's PR checklist calls for "C# code compiles without warnings."
|
Reviewed the diff (base Correctness
Lite/Darling parity
Security / performance
Two minor nits posted inline (indentation drift in a test dictionary, one unused |
1 participant
Closes #2629.
Every PostgreSQL collector except
pg_plan_capture_readinessnow has an MCP tool and a web panel. The ratchet drops from nine to one, and that one is the case it was built to tolerate — a single row of configuration state is a panel, not a question anyone asks an agent.get_pg_predicate_stats,get_pg_index_bloat,get_pg_column_stats,get_pg_buffer_usage,get_pg_extensions,get_pg_lock_stats,get_pg_write_stats,get_pg_replication_stats.The store readers all existed already — built for the Viewer. What was missing was the tool, the dispatch entry, the grid and the pins, which is exactly why nine accumulated: each panel was the deliverable and the read was always the next PR.
Placed by the question, not by convenience
get_pg_blockinglives here tooEach tool carries the caveat its data cannot be read safely without
skipped_reasonwas not measured. That must never read as healthy.pg_locks, so an absence is not proof nothing was locked.n_distinctis a ratio when negative and a count when positive.worst_*travels beside every latest value.A defect in my own work, caught before it shipped
get_pg_extensionsat a 50-row limit:{ "extension_count": 50, "installed": 10, "available_not_installed": 38 }The server has 13 installed. 50 rows was all it had looked at. A summary counted over a capped result describes the page and reads as a fact about the server.
Now:
{ "extension_count": 50, "truncated": true, "installed": null, "available_not_installed": null, "note": "… TRUNCATED at the row limit, so the state totals are WITHHELD …" } { "extension_count": 100, "truncated": false, "installed": 13, "available_not_installed": 85 }Withheld rather than renamed —
installed_in_this_pageis a number nobody wants. Same treatment for the measured-index count and the ungranted-lock count. Same shape as the partial-failure note from #2623, and the same lesson: a number is only as wide as what it counted.Verified
All eight exercised against the self-hosted rig. Six return data;
get_pg_lock_statsandget_pg_replication_statsreturn an honestemptywith the sentence explaining why (nothing contended, no replicas attached), which is the correct answer for that target.