get_pg_logging_audit judges a PostgreSQL target's logging settings facet by facet from the stored config, so a target with everything off stops looking identical to an instrumented one (#3607)

[erikdarlingdata]

The lie

A PostgreSQL target with log_lock_waits, log_temp_files, log_autovacuum_min_duration, log_checkpoints, log_connections, log_disconnections and log_min_duration_statement all off looks identical to a fully instrumented one from every read this product has. The counters are the same, the sampled views are the same, get_pg_server_config lists 400 settings non-default-first and nobody scrolls to the logging block. The operator finds out at incident time, when the log they reach for holds nothing — which is exactly the silence plan-capture readiness (#2564/#3070) fixed for auto_explain, over a different setting list.

Mechanism

get_pg_logging_audit (Darling-only; Lite has no PostgreSQL target) judges the seven settings from the newest stored pg_server_config snapshot (its collection_time is the response's captured_at) and answers per setting: verdict, the value/source/unit/default_value verbatim, what the lines it writes unlock and what this product has instead today, the Darling consumer that would read the lines, the recommended value, its cost_note, and the remedy in the hosting flavour's syntax. The summary counts every facet (there is no page — the list is fixed) and names the off_settings and unknown_settings, because those are the actionable ones.

Three pieces:

• Darling/PerformanceMonitor.Darling.Storage/DarlingPgLoggingAuditReader.cs — the SQL: newest collection_time per server, every non-session row at it. Named DarlingPg*Reader deliberately so DarlingPgReadSqlParsesLiveTests parse-checks it with the rest (it did, on this lane's rig).
• Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPgLoggingAudit.cs — the judgment, a pure function over the rows. Tests hit it with no store.
• Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPgLoggingAuditTools.cs — the wire, with BuildAuditJson split out the way BuildReadinessJson is.

Where this copies readiness, and where it differs — and why

Copies: a facet per setting rather than one verdict; every facet reported whether satisfied or not (a list of only the failures cannot show that a target is instrumented); the remedy per row worded for the flavour; the readiness collector's exact phrasing for managed hosting ("parameter-group change that applies WITHOUT a reboot"); the captured_at = snapshot stamp dialect #3541 A10 (#3637) established for every latest-snapshot read, selected on the row statement, with LATEST IS A TIME in the description; the not_collected → empty miss ladder get_pg_server_config uses, with the empty message saying plainly it is the absence of evidence, not a verdict.

Differs — it is a read, not a collector, and takes no rung. The readiness collector persists because judging its facets means probing the target: is the library in shared_preload_libraries, does an auto_explain.* GUC exist at all. Every setting here is a plain core GUC PgServerConfigCollector (#2658) already stores hourly with value, source, unit and context, so a second collector would write the same rows under a second name and the two would drift. Settings change rarely; the config collector's hourly snapshot is the history. (The storage ladder is also spoken for tonight, which is a reason to prefer this shape but not the reason it is right.)

Differs — four verdicts instead of a boolean, and partial is not a lesser instrumented. instrumented writes every line the setting can; off writes none; unknown is not in the snapshot and nothing is inferred — not from the default, not from the major; partial means a threshold is filtering and the row says what falls under it. For log_min_duration_statement the threshold is the recommended posture: 0 logs every statement the server runs, and #2565 measured the capture-everything shape of that mechanism at 31% of throughput and 772 MB of log in 20 seconds. So a 1000 ms row's remedy is "no change needed", its cost_note calls it the recommendation, and the 0 row is instrumented with the measured cost and a remedy that moves it to a threshold. Sending an agent to "fix" a sane threshold to 0 is the worst outcome this read could produce, and the shape is built to refuse it.

Differs — hosting flavour is decided from evidence in the same rows. The registry's engine token cannot do it: MonitoredEngineKind.Postgres is both self-hosted and RDS for PostgreSQL, and they need opposite instructions (ALTER SYSTEM SET …; SELECT pg_reload_conf(); on one; a parameter group on the other, where ALTER SYSTEM is refused). Any rds.* GUC in the snapshot — RDS and Aurora both carry them — flips every remedy, and hosting_evidence says how many there were or that there were none. The reload-vs-restart clause comes from each setting's own context column, so superuser-backend settings say "applies to connections opened after it" and a postmaster one would say restart — no table of contexts here to rot.

Differs — the cross-reference. Plan capture's own settings (shared_preload_libraries, auto_explain.log_min_duration, log_line_prefix, lc_messages) appear under judged_by_readiness as observed values with the readiness facet that owns each, never as a second verdict — get_pg_plan_capture_readiness holds their traps (loaded-at-−1, placeholder GUC, translated catalogue) and a second judgment here is a second place for those to drift. lc_messages is in the list because it governs whether any of the seven settings' lines are written in the English the parsers match.

Honesty about consumers

The issue's sequencing note says this audit earns its keep once #3601's log pipeline and its first parser families (#3602 temp files, #3603 autovacuum) consume the lines, and none of those ships. Every facet's consumer therefore begins PLANNED, names the issue, and names the counter read that exists today with what it cannot see (get_pg_top_queries averages a shape, never says one execution took 40 s at 03:07; get_pg_blocking samples and misses a wait that starts and ends between samples; get_pg_autovacuum_health says whether, never what it cost). A test pins that every consumer says PLANNED and is written to go red when one ships — so the prose an agent plans against becomes a deliberate edit rather than stale. The setting is still worth turning on first: the log it fills is the one somebody opens at incident time, whichever tool reads it.

Per-setting facts, verified on the lane's PostgreSQL 18.4 rig

• pg_settings.setting renders integer GUCs in the base unit with no suffix (600000 / ms, -1 / kB), so the threshold parse is a plain long.TryParse; anything else is unknown.
• log_connections is a boolean through 17 and a string list in 18 (receipt,authentication,authorization,setup_durations,all; on/true/1 still mean all; empty is off). Measured: 18.4 stores on, true, 1, all, receipt,authentication each verbatim as written. The audit reads any non-empty non-false value as producing lines and shows the value.
• log_autovacuum_min_duration's default moved from −1 to 600000 in 15; a server with source = 'default' at a positive threshold gets the "PostgreSQL's own default since 15 — sees only the outlier runs" wording, a deliberate threshold (any other source — including an explicit 600000 written into the file) gets neutral wording. PostgreSQL's own verdict, not a text comparison against boot_val, for the reason CurrentConfigSql's comment gives (review round 1).
• log_checkpoints default moved to on in 15; log_lock_waits fires at deadlock_timeout, whose value is quoted from the same snapshot (or "not in the snapshot" — never the compiled-in 1 s as if it were this server's).
• Contexts: log_lock_waits/log_temp_files/log_min_duration_statement are superuser, log_autovacuum_min_duration/log_checkpoints are sighup, log_connections/log_disconnections are superuser-backend. All dynamic — every self-hosted remedy is a reload.
• pending_restart travels: a judged row whose file value already differs from the running one carries pending_restart = true and a restart_note (the running value is what was judged; pg_settings cannot say which way the file moves it; read the file or get_pg_server_config's pending_restart_settings before acting on the remedy), and the summary names pending_restart_settings the way get_pg_server_config does. Review round 2 caught this dropped on the floor between the reader and the Facet — a real gap on exactly the row whose remedy needs qualifying.
• A client-sourced row (SET log_min_duration_statement = 0 in the monitoring session) is excluded at the SQL, spelled inline as the config reader spells it so the constant stays parse-checkable; a user/database-sourced row is judged but carries a scope_note saying the server-wide value may differ — the limit the readiness collector states for lc_messages, applied to every GUC here.

What this does NOT do

• It does not read the live server, and says so on the wire (source) and in the description. Its catch re-checks the engine gate before answering error, as the plan tools do (review round 2).
• It does not judge logging_collector, log_destination, log_min_messages or log_statement — the transport's and the pipeline's concerns, not "what can this target tell you".
• It does not take hours_back or as_of parameters — the Stamped latest-read shape (MCP payload-contract campaign: the agent-facing API tells the truth (~30 defects + an 11-rule contract) #3541 A10) for a state, not a window; AsOfWindowAnchorTests' derived pins hold the catalog to that, and McpLatestSnapshotStampTests holds the stamp. dev moved under the lane (Every latest-snapshot MCP read says when it was captured, and no tool accepts a window it does not read (#3541 A10) #3637 landed the stamp roster while this was open): the roster is a roster of pairs — every entry names its Lite twin's file — so a Darling-only tool cannot sit in it; it gets a DarlingOnlyStamped allowance held to the same three Stamped assertions (stamp on the payload, LATEST IS A TIME, neither knob), with the set-equality control the other allowances have, and the new reader's SQL joins the stamp-column theory. TsqlConventionGuardTests flagged Threshold()'s expression body (a { } property pattern reads as a member body to its scan); it became a block body rather than a KnownTruncatedRanges entry.
• It does not add a Viewer (WPF) panel; the web tab gains one under Plan Capture Readiness on the Activity tab, where EveryPostgresRead_IsReachableFromThePostgresRegistry requires every get_pg_* read to land.
• It does not close the sequencing question the issue raises — it delivers the audit the issue asks for, with the consumers labelled as they truly are.

Surface

Registered beside the plan tools in DarlingMcpHostService (McpToolTypeRegistrationTests is derived and now counts it); dispatched at /api/read/get_pg_logging_audit with a server-only catalog entry; censused in DarlingMcpInstructions (152 / 86 / 66, thirty-four PostgreSQL reads, the onboarding pair named), README.md (both sites), llms.txt (87–152), and docs/postgres-first-target-runbook.md (step 8 table row, "34 get_pg_* tools", the §12 plan-capture bullet gains the sibling); ratcheted in CrossAppMcpToolInventoryPinTests.KnownLiteMissingMcpTools with its near-twin (get_server_config, a listing not a judgment) named; mapped in ServerPageTabsTests.CollectorForRead to pg_server_config.

Tests

Darling/Darling.Tests/DarlingMcpPgLoggingAuditToolsTests.cs, executed on this Mac via the mactest harness against a timescale/timescaledb:2.28.1-pg18 rig (316 tests in the harness green, including McpLatestSnapshotStampTests, TsqlConventionGuardTests, ServerPageTabsTests, AsOfWindowAnchorTests, EngineCapabilityMissTests, DarlingWebEndpointsTests, DarlingCustomViewsTests, DarlingPeerDisclosureTests and the live DarlingPgReadSqlParsesLiveTests census that now includes the new reader; McpToolTypeRegistrationTests cannot run out-of-tree and was replayed in Python — missing ∅, stale ∅):

• the issue's scenario: everything off → seven off, all named, every remedy the ALTER SYSTEM … pg_reload_conf() form;
• the three threshold settings at −1 / 0 / N (theory), the partial row quoting its threshold;
• the statement threshold being the recommendation and 0 carrying the Choose the PostgreSQL plan capture mechanism (auto_explain vs on-demand EXPLAIN vs extension) #2565 figure;
• the ten-minute autovacuum default named as the default, a deliberate threshold not;
• log_connections across the 17/18 boundary in the spellings 18.4 stores;
• absent → unknown ("not in the stored snapshot"), kept out of off_settings; unparseable → unknown with a message that says the row IS in the snapshot and quotes the value — the two unknowns told apart (review round 1);
• pending_restart on the row, restart_note, and pending_restart_settings in the summary; plain rows carry false/null (review round 2);
• fully instrumented → nothing off, every remedy "no change needed";
• one rds.* row → every remedy the parameter-group form, no pg_reload_conf, rds.* rows are evidence not facets;
• the change clause following the setting's own context (per-backend, forced postmaster, plain; both flavours);
• role/database override → scope_note, server setting → null;
• deadlock_timeout quoted from the snapshot or declared missing;
• the readiness cross-reference: four settings, four facets, no verdict here, null for an unloaded auto_explain;
• the wire: captured_at (and no as_of key), counts summing to total, named off/unknown, every prose column non-empty, no hours_back;
• every consumer PLANNED with its issue and today's read named (the pin meant to go red);
• description pins (all seven names, STORED / never live, LATEST IS A TIME + captured_at, readiness pointer, partial defined, PostgreSQL-only) and the no-window/no-anchor signature;
• dispatch + server-only catalog entry;
• live, end to end: two servers modelling the two flavours, an older snapshot that must lose to the newest, a client-sourced 0 that must not become the server's setting, a per-role override with its scope_note, an unseeded deadlock_timeout declared missing, and a third server with no snapshot → empty with the not-a-verdict wording.

Built Darling.Storage, Darling.Service, Darling.Tests, Lite.Tests -c Release -p:EnableWindowsTargeting=true — 0 warnings.

Closes #3607

[claude]

Non-blocking: atDefault decides "PostgreSQL's own default since 15" wording by comparing row.Setting to row.BootValue as text, rather than checking row.Source == "default".

This is exactly the anti-pattern DarlingPgServerConfigReader.CurrentConfigSql documents and deliberately avoids a few files over (lines 88-97 there): "PostgreSQL's OWN verdict, not a text comparison against boot_val. Comparing the strings looks equivalent and is not... source = 'default' is PostgreSQL saying plainly that nobody set them."

Concretely: an admin who writes log_autovacuum_min_duration = 600000 explicitly in postgresql.conf (coincidentally matching the PG15+ boot value) would get source = 'configuration file', but this code would still say "This is PostgreSQL's own default since 15" — attributing a deliberate choice to inaction. Using row.Source == "default" instead would match the established, documented convention and avoid the false positive.

[erikdarlingdata]

Fixed in 2b48da1 — atDefault now reads source = 'default', PostgreSQL's own verdict, with a comment citing CurrentConfigSql's reasoning. Test TheTenMinuteAutovacuumDefault_IsNamedAsTheDefault_AndADeliberateThresholdIsNot gained your exact case: an explicit 600000 from configuration file is NOT called the default.

[claude]

Non-blocking: Threshold/Bool return null for both "row is missing" and "row is present but the value doesn't parse" (e.g. log_temp_files = "lots"). Both collapse to the Unknown verdict, and every caller's _ => NotInSnapshot / verdict == Unknown ? NotInSnapshot : ... branch (e.g. lines 250, 299-300, 336, 398, 439, 500) then emits "This setting is not in the stored snapshot, so nothing is claimed about it" — which is false when the row exists but is unparseable. Remedy (line ~623) has the same conflation: "No remedy is offered for a setting the snapshot does not hold".

This contradicts the tool's own documented contract ("unknown means the setting is not in the snapshot") and is tested only for the verdict (AnUnparseableValue_IsUnknown), not the message. In practice this is close to unreachable since PostgreSQL only ever renders well-formed values for boolean/integer GUCs, so I'd call it low-severity — but worth distinguishing "absent" from "present but unparseable" in the prose if the collector or a future setting ever produces the latter.

[erikdarlingdata]

Fixed in 2b48da1 — a shared UnknownNote(row) tells the two apart: a null row says "not in the stored snapshot", a present-but-unreadable row says "IS in the stored snapshot but its value '…' is not one this audit can read", and Remedy's unknown arm splits the same way. AnUnparseableValue_IsUnknown_AndSaysItIsInTheSnapshot pins the message, not just the verdict — you are right that nothing else would ever exercise it.

[claude]

LGTM — reviewed the new get_pg_logging_audit read (reader SQL, pure-function judgment, MCP wire, JS panel, docs/instructions/README counts, and the Lite/Darling parity pin). The reader correctly excludes session-scoped rows via the same inline list convention as DarlingPgServerConfigReader, the parameterized query has no injection surface, the four-verdict logic (instrumented/partial/off/unknown) and hosting-flavour detection from rds.* evidence check out against the extensive test suite (including the live-Postgres end-to-end test), and the Lite exclusion-list pin plus doc/tool counts (151→152, 86→86 shared, PostgreSQL reads 33→34) are internally consistent. Left two non-blocking inline notes: (1) the autovacuum-default wording compares setting text to boot_val text instead of checking source == 'default', which is the exact anti-pattern the sibling DarlingPgServerConfigReader documents avoiding; (2) the unknown verdict's prose always says "not in the stored snapshot," which is inaccurate for the (currently unreachable in practice) case of a present-but-unparseable value. Neither affects verdict correctness or produces an unsafe remedy.

[claude]

Facet (and every builder below it) drops PgLoggingSettingRow.PendingRestart on the floor — the reader fetches it and documents it as "The file and the running server disagree about this one" (DarlingPgLoggingAuditReader.cs:63), but it's never read anywhere in this file, never appears on Facet, and never reaches the JSON in DarlingMcpPgLoggingAuditTools.BuildAuditJson.

That's a real gap given this tool's own stated bar for honest disclosure (the ScopeNote machinery exists for exactly this kind of "the value shown isn't the whole truth" caveat). The sibling get_pg_server_config tool treats this field as important enough to report "loudly" (DarlingMcpPgServerStateTools.cs:595): if postgresql.conf was edited and reloaded but the running server hasn't restarted yet, the file value differs from what pg_settings.setting currently reports. For a judged setting sitting on pending_restart = true, this audit will confidently report today's verdict/remedy with no indication that the setting is about to flip on the next restart — which is exactly the kind of silent staleness this PR's design otherwise goes out of its way to avoid.

Worth surfacing pending_restart per facet (or at least noting it in scope_note) the same way the config reader does. Also worth noting: no test in DarlingMcpPgLoggingAuditToolsTests.cs ever sets PendingRestart = true (the Row() helper hardcodes false), so this path is untested as well as unused.

[erikdarlingdata]

Fixed in 2b48da1 — real gap, thank you. Facet carries PendingRestart + a RestartNote (the running value is what was judged, the file already holds another, pg_settings cannot say which way it moves, so read the file / get_pg_server_config's pending_restart_settings before acting on the remedy). On the wire per row as pending_restart / restart_note, named at the top as pending_restart_count / pending_restart_settings the way get_pg_server_config does it, and the response note points at it. APendingRestart_IsCarriedOnTheRow_AndNamedInTheSummary covers the judgment and the wire; the live test now seeds one pending_restart = true row on the managed server and asserts it lands in both places.

[claude]

This catch block doesn't check DarlingEngineCapability.NotCollectedStatusAsync the way its siblings do. DarlingMcpPgPlanTools.GetPgPlans and GetPgPlanCaptureReadiness both re-check the engine gate inside their catch (not just on the empty-rows path) so a read that throws for an engine-mismatch or schema reason still comes back as a friendly not_collected status rather than a raw error with ex.Message. Here any exception from GetNewestSnapshotAsync (e.g. hitting an un-migrated store, a transient timeout, etc.) just surfaces as McpHelpers.Status("error", ...), which is a step down in message quality from the pattern this file otherwise mirrors ("BuildAuditJson split out the way BuildReadinessJson is"). Minor compared to the PendingRestart gap, but worth aligning for consistency.

[erikdarlingdata]

Fixed in 2b48da1 — the catch asks NotCollectedStatusAsync("pg_server_config") before answering error, mirroring GetPgPlanCaptureReadiness, with a comment saying why the gate is the more specific fact.

[claude]

Reviewed the diff against origin/dev. The Darling-only scope is well justified (Lite has no PostgreSQL target — CrossAppMcpToolInventoryPinTests, McpLatestSnapshotStampTests's new DarlingOnlyStamped roster, and ServerPageTabsTests are all updated consistently), tool counts in README/llms.txt/docs match the registration change, and the T-SQL/PostgreSQL-store style conventions (schema-qualified reads, naive-UTC collection_time, session-scoped source exclusion spelled inline and identically to DarlingPgServerConfigReader) are followed correctly.

Two findings from the review, left as inline comments:

1. Correctness/completeness: DarlingPgLoggingAuditReader.PgLoggingSettingRow.PendingRestart is read from the store and documented as significant ("the file and the running server disagree about this one"), but DarlingPgLoggingAudit's Facet never carries it and it never reaches the JSON response in DarlingMcpPgLoggingAuditTools.BuildAuditJson. The sibling get_pg_server_config tool reports this "loudly" for exactly the scenario where it matters: a judged setting sitting on pending_restart = true means the value this audit reports (and the remedy it offers) may be about to change on the next restart with no signal to the reader. No test exercises PendingRestart = true either.

2. Minor consistency: the catch block in GetPgLoggingAudit doesn't re-check the engine gate (DarlingEngineCapability.NotCollectedStatusAsync) the way DarlingMcpPgPlanTools.GetPgPlans/GetPgPlanCaptureReadiness do in their catch blocks, so an exception during the read surfaces as a raw error instead of the established not_collected message.

Neither is a blocker on architecture or parity — the PostgreSQL/RDS/Aurora remedy branching, the threshold judgment logic, the log_connections 17/18 boundary handling, and the readiness cross-reference all check out against the described design. Requesting changes mainly for finding #1, which weakens the audit's core promise of full, honest disclosure.

Both findings remediated in 2b48da1 (pending_restart on the row and in the summary with a live-seeded case; catch re-checks the engine gate) plus the two non-blocking notes from the first pass (source='default' verdict; unknown-vs-unparseable wording). Replied inline to each.

[claude]

LGTM — reviewed the audit judgment logic (DarlingPgLoggingAudit.cs), the parameterized Postgres reader SQL, the MCP wire/JSON shape, and the doc/registration/parity updates (README, llms.txt, MCP instructions, runbook, CrossAppMcpToolInventoryPinTests, McpLatestSnapshotStampTests). No correctness bugs, injection risks, or Lite/Darling parity gaps found; Lite's lack of a PostgreSQL target is explicitly and correctly documented as the reason there's no counterpart.