Collect PostgreSQL configuration and what changed in it (#2658), and register ten MCP tools that shipped unreachable (#2659) - #2660
Conversation
…register ten MCP tools that shipped unreachable (#2659) pg_settings was never collected. SQL Server answers this three ways and PostgreSQL had no answer at all, so neither "what is work_mem set to here" nor "what changed last Tuesday" could be answered -- and the second is the kind that cannot be recovered afterwards at any price, because a configuration history nobody recorded is not sitting on the server waiting to be read. V102 stores the snapshot hourly, 365 days, and stores every column without filtering. source is what separates the server's configuration from the collector's own session -- pg_settings is a per-backend view -- and dropping a session-scoped row at collection time would make that evidence unrecoverable, so both reads filter instead, where it can be explained. Reporting one as a CHANGE would be worse than showing it: the collector reconnects, application_name moves, and the read would announce a change nobody made. get_pg_server_config reports what somebody actually chose, non-default first. Defaultness comes from PostgreSQL's own `source`, NOT from comparing setting to boot_val -- the string comparison invents non-defaults on a server nobody configured, measured on a live 17.11: data_directory_mode reads 0700 against a boot_val of 448, the same value in octal and decimal; archive_command reads (disabled) against an empty default; commit_timestamp_buffers reads 32 against 0 because 0 means auto-tune. That defect was in the first version of this read and only running it found it. get_pg_server_config_changes reports value changes between snapshots. A setting APPEARING is deliberately not a change: LAG returns NULL for the first snapshot of every setting, so without the guard the first collection would manufacture several hundred changes, and would again for every extension whose GUCs appear when its library loads. Both name pending_restart loudly -- the file edited and reloaded while the running server is still on the old value, disagreeing with no symptom until a restart months later changes behaviour during someone else's incident. #2659, found while wiring the above: six [McpServerToolType] classes -- ten shipped PostgreSQL reads -- were never registered with the MCP host. They were implemented, documented, dispatched by the web dashboard and counted in the census, and tools/list answered 116 where the census claimed 126. Nothing failed: the inventory pin checks NAMES, which exist either way, and the tab pin exists to stop a read shipping reachable only through MCP, which is the exact inverse of this. Registering them here rather than separately because two of the ten are this change's own tools, and shipping a feature into a class agents cannot reach is shipping nothing. A reflection-derived pin now asserts every tool class is registered, so it fails when a class is added rather than when an agent next reaches for the tool. Verified end to end against the rig through the real service: store at v102, tools/list 116 -> 128 with 25 PostgreSQL reads matching the census exactly, and after ALTER SYSTEM on the target the changes read reported exactly one change (work_mem 4096 -> 8192) out of 415 settings while shared_buffers correctly showed pending_restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var shown = include_defaults ? rows : rows.Where(r => !r.IsDefault).ToList(); | ||
| var pendingRestart = rows.Where(r => r.PendingRestart).Select(r => r.Name).ToList(); | ||
|
|
||
| return JsonSerializer.Serialize(new | ||
| { | ||
| server = resolved.ServerName, | ||
| status = "server_config", | ||
| /* Both counts, because they answer different questions and one without the other invites | ||
| the wrong conclusion: a small returned count is reassuring only if you know it was | ||
| filtered rather than truncated. */ | ||
| settings_returned = shown.Count, | ||
| non_default_count = rows.Count(r => !r.IsDefault), | ||
| truncated = rows.Count >= limit, |
There was a problem hiding this comment.
non_default_count doesn't mean what the comment above it says.
rows here has already had the SQL LIMIT $2 (= limit) applied inside GetCurrentConfigAsync, ordered pending_restart DESC, is_default ASC, name. So when a server has more non-default settings than limit (very plausible — Timescale/Citus/pg_partman/pg_cron etc. each add a batch of GUCs, and the doc comment for PgServerConfigCollector notes 28 non-default out of 415 was just one measurement), rows.Count(r => !r.IsDefault) isn't the true count of non-default settings on the server — it's the count of non-default settings within the already-truncated top-limit rows, which will just equal shown.Count (since ordering puts non-default rows first).
The comment right above this ("Both counts... a small returned count is reassuring only if you know it was filtered rather than truncated") implies non_default_count is meant to be the true total so a caller can tell filtering from truncation. It can't do that job while it's computed from the same truncated rows list — truncated will be true but non_default_count will silently look like a complete answer.
Two ways to fix: compute non_default_count with a separate COUNT(*) WHERE NOT is_default query (untruncated), or drop the claim that this field represents the total and document it as "non-default count among the returned rows."
| /// majors, and an unknown one is far more likely to be a real server source than a new kind of | ||
| /// session state, so an unrecognised value should show up rather than vanish. | ||
| /// </summary> | ||
| public const string SessionScopedSources = "'client', 'session', 'override'"; |
There was a problem hiding this comment.
Worth double-checking 'override' belongs in this list. The doc comment above frames SessionScopedSources as "values that describe THIS connection rather than the server," which is accurate for client and session — but PostgreSQL's PGC_S_OVERRIDE source isn't primarily about the connecting session. It's used for a handful of GUCs the backend forces regardless of any configured value (things like in_hot_standby, server_version, lc_collate/lc_ctype, wal_segment_size) — genuine, stable server-level facts, not the monitoring connection's own state (that's is_superuser/session_authorization, which do vary by connecting role).
As written, both CurrentConfigSql and ConfigChangesSql silently drop every override-sourced setting from get_pg_server_config and get_pg_server_config_changes. If any of those turn out to be facts worth surfacing (e.g. in_hot_standby flipping is exactly the kind of thing "what changed" should catch), they're unreachable through either read. Worth confirming against SELECT name, source FROM pg_settings WHERE source = 'override' on a real target before shipping — if it's intentional, a short note on why override rows are being treated as session noise (same as client/session) would help the next reader.
|
Reviewed the diff for both #2658 ( #2659 registration fix — verified correct. Diffed the full set of Lite/Darling parity — no drift. This feature is Darling-only by design (PostgreSQL/Aurora central-store data), and Schema/migration — V102 follows the established pattern (schema-qualified Two things worth a look, left as inline comments:
Everything else — the |
Eight failures, every one a pin doing its job, and together they are the checklist for adding a PostgreSQL collector. The viewer half was the real work. EveryPostgresCollector_IsShownOnExactlyOnePostgresTab and EveryCollectorTable_HasAViewerReader_OrIsAllowListed both refused a collector the WPF viewer never shows -- the allow-list is a shrink-only ratchet, so the answer is a panel, not an entry. The settings grid sits under the extension axis on the Overview tab because it is the same kind of fact one layer in: extensions say what this server CAN do, settings say what it was told to do. It is the one panel on that tab NOT scoped to the toolbar window, because a configuration is the state now and an hours filter returns nothing for a server whose hourly collector last ran just outside it -- which reads as "no configuration" rather than "widen the window". The rest were counts a new collector legitimately moves: the pg rung list, the catalog count, the PostgreSQL tab count (7 -> 8, prose included), and the CapturePathByCollector noun phrase without which the capability message falls back to generic phrasing. The web tab used format: "datetime", which the renderer does not know -- an unknown format falls through to raw text, so the column still renders and the defect is invisible on inspection. It is "time". CiClusterWorkerSizingTests corrected a factual error in V102's own doc comment rather than just a number. I had written that pg_server_config is not a hypertable, arguing from its shape -- a snapshot of something a person changes, not a series of measurements. TimescaleSupport.HypertableTables is CollectorCatalog.All, so membership follows from being a collector and is not a per-table choice, and CI sizes its cluster's workers off that count. The doc now says so, and both workflows move to 70/81. Every pin re-validated locally against the product assemblies before pushing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| var shown = include_defaults ? rows : rows.Where(r => !r.IsDefault).ToList(); | ||
| var pendingRestart = rows.Where(r => r.PendingRestart).Select(r => r.Name).ToList(); | ||
|
|
||
| return JsonSerializer.Serialize(new | ||
| { | ||
| server = resolved.ServerName, | ||
| status = "server_config", | ||
| /* Both counts, because they answer different questions and one without the other invites | ||
| the wrong conclusion: a small returned count is reassuring only if you know it was | ||
| filtered rather than truncated. */ | ||
| settings_returned = shown.Count, | ||
| non_default_count = rows.Count(r => !r.IsDefault), | ||
| truncated = rows.Count >= limit, |
There was a problem hiding this comment.
GetCurrentConfigAsync applies LIMIT $2 (the limit param, default 100) in SQL, and only after that fixed-size result comes back does this method filter out defaults (rows.Where(r => !r.IsDefault)). Because the query's ORDER BY puts every non-default row ahead of every default row, this is safe only while the target's true non-default-setting count is ≤ limit. On a target where it exceeds limit — plausible on Aurora/RDS, where parameter groups routinely diverge on 100+ GUCs, exactly the kind of target this PR is aimed at — the SQL LIMIT silently cuts off some genuinely non-default settings before the C# filter ever sees them (an arbitrary, alphabetically-truncated subset, not "least important last").
Worse, non_default_count (line 408) is computed from that same already-truncated rows, so it quietly undercounts the server's real non-default-setting total in this scenario — there's no signal in the field itself that it's a partial count; only cross-referencing the generically-named truncated flag would reveal it, and an LLM/dashboard consumer has no reason to make that connection.
Consider filtering on defaultness in the SQL (behind include_defaults) so LIMIT operates on the same population that's being counted and returned, rather than filtering client-side after an unrelated cap.
|
Reviewed the PostgreSQL server-config feature (#2658) and the MCP tool-registration fix (#2659). Overall this is well-executed and unusually well self-verified:
One correctness edge case flagged inline on No SQL injection, secrets, or missing-index-DMV concerns found. Didn't spot anything else worth raising. |
Both statements carried a SESSION_SCOPED token substituted at call time, which meant the constants were not SQL. DarlingPgReadSqlParsesLiveTests runs parse analysis on every shipped PostgreSQL read against a real server and both failed with 42703 column "session_scoped" does not exist. The deduplication was not worth it. A read whose text only becomes valid after a string replacement cannot be checked by anything -- not parse analysis, not a reader, not a person. The excluded sources are now spelled out inline in both statements, and PgServerConfigTests asserts each contains NOT IN ( plus the named constant, so the two cannot drift now that they are written twice. Verified by running PREPARE for both statements against a real PostgreSQL with their actual parameter signatures -- the same thing CI does -- rather than by rebuilding and hoping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| FROM pg_server_config AS c | ||
| WHERE c.server_id = $1 | ||
| AND c.collection_time >= $2 | ||
| AND c.collection_time <= $3 | ||
| AND coalesce(c.source, '') NOT IN ('client', 'session', 'override') |
There was a problem hiding this comment.
Correctness: the time-range filter runs before LAG, so a genuine change at the start of the window is silently dropped.
LAG(...) OVER (PARTITION BY c.name ORDER BY c.collection_time) only sees rows that survived the WHERE c.collection_time >= $2 ... <= $3 filter in this same CTE. For each setting, the earliest row inside the window therefore always has prev_time IS NULL regardless of whether the value actually changed going into the window — its true predecessor (the last snapshot before $2) was filtered out before the window function ran. The prev_time IS NOT NULL guard then discards that row, so a real transition that happened right at (or just before) the window boundary is reported as "no change" instead of a change.
This isn't just a rare edge case: the collector runs hourly (CollectorScheduleDefaults["pg_server_config"] = new(60, 365)), and hours_back can legally be as low as 1 (McpHelpers.ValidateHoursBack). With a 1-hour window there is typically only one snapshot inside [$2, $3], so get_pg_server_config_changes will almost always report zero changes for short/recent windows — exactly the "what changed just now / this afternoon" query the tool exists to answer — even when a change genuinely occurred in that period.
Suggest computing LAG over the setting's full series (unfiltered by time) and applying the collection_time >= $2 AND <= $3 bounds in the outer SELECT ... FROM ordered WHERE ... instead of inside the CTE, so the row just before the window is still available to compare against. (If scanning the full retention window per query is a cost concern, padding the CTE's lower bound backward by one collection interval would also fix the boundary case without a full-history scan.)
Note: DarlingPgDatabaseReader/DarlingPgStatementReader/DarlingPgIoReader have the same filter-before-window shape, but there it just makes one interval's delta unmeasurable (GREATEST(raw, 0) → NULL, dropped from a SUM) — a minor undercount. Here the same shape drops a whole discrete change event from a change-history read, which is a materially different (and much more visible) failure mode for the specific promise this tool makes.
Review summaryScope: adds PostgreSQL Correctness bug found (left as an inline comment on Everything else checked out:
|
1 participant
Closes #2658. Closes #2659.
#2658 — configuration
pg_settingswas never collected. SQL Server answers this three ways; PostgreSQL had no answer at all. Thesecond question is the one that matters most and it is the one that cannot be recovered later at any price:
a configuration history nobody recorded is not sitting on the server waiting to be read.
it; 365 days because this gets asked long after the fact ("it got slow sometime last quarter").
get_pg_server_config— what somebody actually chose, non-default first, withsource,context(restart / reload / nothing) and the compiled-in default beside each value.
get_pg_server_config_changes— value changes between snapshots, old beside new.incident is asking what moved, not what the server is.
Two decisions that were wrong first and got fixed by running it
Defaultness comes from PostgreSQL's own
source, not from comparingsettingtoboot_val. The stringcomparison looks equivalent and invents non-defaults on a server nobody configured. Measured on a live
17.11:
settingboot_valsourcedata_directory_mode0700448defaultarchive_command(disabled)defaultcommit_timestamp_buffers320defaultOctal against decimal, a display convention, and an auto-tuned value the server resolved at startup. All
three say
source = 'default', which is PostgreSQL stating plainly that nobody set them.boot_valisstill stored and shown — it is useful to see the default — it just does not get to decide.
A setting appearing is not a change.
LAGreturns NULL for the first snapshot of every setting, sowithout the guard the first collection reports several hundred fabricated changes, and does it again for
every extension whose GUCs appear when its library loads. On the rig the first snapshot carried 415 settings
and the read correctly reported one change.
Session rows are stored and filtered at the read, not dropped at collection
pg_settingsis a per-BACKEND view, so aclient-source row describes the monitoring connection. Droppingit at collection would make the evidence unrecoverable and still leave every read guessing. Showing one as
the server's configuration would be wrong; reporting one as a change would be worse — the collector
reconnects,
application_namemoves, and the read announces a change nobody made.#2659 — ten tools nobody could call
Found while wiring the above. Six
[McpServerToolType]classes were never registered with the MCP host:get_pg_write_stats,get_pg_buffer_usage,get_pg_extensions,get_pg_lock_stats,get_pg_index_bloat,get_pg_column_stats,get_pg_kernel_stats,get_pg_predicate_stats,get_pg_replication_stats,get_pg_wait_sampling— implemented, documented, dispatched by the web dashboard, counted in the census,and unreachable by any agent.
Nothing failed, because neither existing guard looks at this: the inventory pin checks tool names, which
exist whether or not the class is registered, and the
POSTGRES_TABSpin exists (its header says so) to stopa read shipping reachable only through MCP — this is the exact inverse. A reflection-derived pin now
asserts every tool class is registered, so it fails when someone adds a class rather than when an agent next
reaches for the tool.
Fixed here rather than separately because two of the ten are this change's own tools, and shipping a feature
into a class agents cannot reach is shipping nothing.
Verification
End to end against the rig through the real service:
tools/list116 → 128, with 25 PostgreSQL reads — matching the updated census exactlyALTER SYSTEM SET work_mem = '8MB'then a second snapshot: the changes read reported exactly onechange out of 415 settings,
work_mem 4096 → 8192ALTER SYSTEM SET shared_buffers = '256MB'+ reload: correctly reported as pending_restart, valueunchanged — the file and the running server disagreeing, which is the state the read exists to surface
All 22 assertions in the new tests were validated locally against the product assemblies before pushing,
including that the V102 rung's DDL is byte-identical to the generated schema — the check that cost a CI
round on the previous rung.