Re-derive managed-store Postgres sizing on a hardware change (Fixes #2845)

[erikdarlingdata]

Fixes #2845.

The defect

Conf heal blocks v1-v7 each ask "is this marker absent?" — a question answered once in a cluster's life. So every one of them re-derives on a formula change and none on a hardware change. All three monitoring boxes were resized 16 GB -> 31.5 GB and kept effective_cache_size at 11.86 GB, which is 75% of the RAM they no longer had.

The fix

A v8 block keyed on a fingerprint of the derivation inputs (RAM + hypertable count) rather than a version. The two triggers are orthogonal and compose — a version marker still heals "we changed our mind about the formula", this heals "the machine changed underneath it".

Precedence is unambiguous even though both write the same file: v8 calls the same helpers the version blocks do (DeriveMemorySettings, and DeriveWorkerSettings extracted here), so it can only ever differ from them by being fresher, never by disagreeing.

It compares the last fingerprint, not any. A host resized 16 -> 32 -> 16 GB carries both; a Contains test would find the stale first one and skip, leaving the 32 GB block still winning by last-occurrence-wins on a box that no longer has 32 GB. The check has to ask the question postgresql.conf itself answers.

What it emits

setting	why
effective_cache_size	the setting the issue was raised for — planner hint, no allocation
maintenance_work_mem	per-operation ceiling PostgreSQL grows into, cannot overcommit
timescaledb.max_background_workers / max_worker_processes	restart-only counts that only grow as collectors are added

Re-stating the worker pair is what finally makes v2's "never goes stale as collectors are added" claim true — that was true of the formula and false of its marker-keyed application.

The omissions are the load-bearing part

• shared_buffers — excluded structurally, not by trusting the cap. The 1 GB cap is the Windows error-487 mitigation (Managed store: cap shared_buffers at 1GB for the co-located reality (v5) + MaxPoolSize=24 — heals the Windows 487 spawn failures #1559, pgsql-bugs BUG #14050 / #18954) and the condition is live on this fleet: 4–205 occurrences/day across the three boxes, zero could not fork — the retry path holding is exactly the margin a larger segment would spend. min(25% RAM, 1 GB) is already 1 GB above 4 GB so a resize cannot move it; the reason to leave it out is the future one. If the cap is ever raised deliberately, that is a reviewed formula change, not something a resize propagates to production on its own.
• work_mem — excluded. The formula would take it 31 -> 63 MB, and the only measurements above 31 MB on this store's heaviest read are worse: PlanRegressionSql at default 26,565 ms, 31 MB 25,617 ms, 512 MB 59,323 ms. 63 MB is not 512 MB and nobody has measured it — which is the point: the evidence that exists points the wrong way. More fundamentally it is the wrong kind of setting for this block. Everything here is a property of the machine; work_mem is a per-sort, per-connection ceiling that follows from the query mix. The hardware changed; the sort behaviour did not.
• max_parallel_workers — untouched. This class has never set it; it sits at the PostgreSQL default of 8 regardless of cores. Deriving it from cores is a plausible want on a 16-core host but it is a behaviour change rather than a staleness fix, and it multiplies the memory story above — each parallel worker gets its own work_mem for its share of a node, so raising parallelism raises peak sort memory on exactly the query that already degrades with more of it. Its own evidence, its own PR. This is also why the fingerprint records RAM and hypertables and not cores: with nothing core-derived to re-state, a core-only change has no work to do, and fingerprinting it would append a block of identical values on every resize.

Reload semantics

effective_cache_size and maintenance_work_mem are SIGHUP-reloadable; the two worker settings are restart-only. The append runs before pg_ctl start on a service-owned start, so in practice the whole block takes effect on that very start — same as v3 and v7. Nothing is applied to production by this PR; it lands and takes effect at the next natural restart.

Red-first evidence

Darling.Tests is net10.0-windows, so the property was proven with a net10.0 reflection harness against the real built PerformanceMonitor.Darling.Service.dll, mutating the fix into each defect its pins guard:

mutation	pin that went red
LastIndexOf -> Contains	resize-back: 32->16 GB still reports STALE
emit shared_buffers	v8 NEVER emits shared_buffers
emit work_mem	v8 NEVER emits work_mem
restored	15 passed, 0 FAILED

Each mutation fails exactly one pin, and the two positive controls (BuildMemorySizingConfAppend resolving, and the v3 block still emitting its own shared_buffers) pass in every run — so a red is the defect, not a harness that stopped resolving methods.

Two things the harness caught before they shipped, both worth recording:

1. A bare "work_mem = " assertion is a substring of "maintenance_work_mem = ", so the unanchored pin was red against correct code. Both the xUnit pin and the harness now anchor on the newline that starts every setting line, with a positive maintenance_work_mem assertion as the guard.
2. The first mutation script had a Python syntax error, so nothing was mutated and the harness "passed" on unmutated code — a false green inside a red-first proof. The script now asserts the file actually changed and that the rebuild succeeded before the harness is believed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

GetTotalPhysicalMemoryBytes() (used for v8RamBytes just above) has three fallback tiers: real RAM, then gcTotal (the GC's live view of available memory) if positive, then the fixed MemoryFallbackRamBytes (4 GB) only as a last resort. BuildHardwareFingerprint's own normalization (totalPhysicalMemoryBytes > 0 ? totalPhysicalMemoryBytes : MemoryFallbackRamBytes) only catches the "≤ 0" case — it never sees a raw 0 here, since GetTotalPhysicalMemoryBytes() already guarantees a positive return before the value reaches the fingerprint.

That leaves the middle tier unguarded. gcTotal is a live snapshot, not a stable hardware property — it can differ from one GlobalMemoryStatusEx failure to the next, and from the true physical RAM. If that Win32 call fails intermittently:

• Each failure can fingerprint under a different gcTotal value, so ConfHasCurrentHardwareFingerprint never matches, and v8 appends a new block every time this happens — the "append a block on every alternating start" append-loop the HardwareFingerprint_NonPositiveRam_MatchesTheFallbackItDerivedUnder test/docstring explicitly says this design prevents, but only for the literal-zero case.
• Worse, that appended block's effective_cache_size / maintenance_work_mem get derived from the GC fallback figure (typically well under true RAM) and take effect immediately on that very start (per the "Reload semantics" note above), so a transient memory-query blip can silently degrade production Postgres sizing until the next start happens to read RAM successfully again.

Unlike v3/v5/v7 (marker-gated, so a bad fallback reading only ever "sticks" once), v8 re-evaluates on every start for the life of the cluster, so this "rare" Win32 failure now gets unbounded chances to trigger it. Consider routing GetTotalPhysicalMemoryBytes()'s fallback tiers through the same fixed MemoryFallbackRamBytes sentinel (or otherwise making the fallback fingerprint-stable) so a transient query failure can't produce a novel fingerprint.

[erikdarlingdata]

Good catch, and it's a real hole in exactly the place the docstring claimed to cover — fixed in d7d0c52.

You're right that the > 0 normalization never sees the middle tier: GetTotalPhysicalMemoryBytes already guarantees a positive return, so the guard was dead for the case that matters. And the value it returns there is GC.GetGCMemoryInfo().TotalAvailableMemoryBytes, which is a live snapshot rather than a hardware property — so it can differ between blips, which is precisely why a value-based guard cannot fix this. Routing the fallback through the fixed sentinel would have worked, but I went further, because I think the stronger statement is available:

A non-authoritative reading is not evidence that the hardware is unchanged — it is the absence of evidence either way. So v8 now does nothing without a trustworthy reading, rather than re-deriving from a stable-but-wrong sentinel. That kills both halves of what you identified: no novel fingerprint, and no sizing derived from a figure below true RAM. It also means the failure mode is "the last known-good block stays in force", which is the correct behaviour under uncertainty.

Concretely:

• TryGetAuthoritativePhysicalMemoryBytes split out — true only on the GlobalMemoryStatusEx success path. GetTotalPhysicalMemoryBytes keeps its existing best-effort tiers unchanged, so v3/v5/v7 behave exactly as before (for a first-ever provision, some number beats none — your point that a marker-gated block can only stick once is what makes that trade fine there and not here).
• ShouldAppendHardwareSizing(conf, ramReadingIsAuthoritative, expectedFingerprint) exposes the whole decision as a pure function, so the property is pinnable without a data directory.
• The call site logs a Warning and skips when the reading isn't authoritative.

Proven red by mutating the gate back out (=> !ConfHasCurrentHardwareFingerprint(...), i.e. the pre-review behaviour): authoritative-gate: an untrusted reading appends NOTHING for any value (append-loop) fails and everything else stays green — 16 passed, 1 FAILED, restored to 17/0. The pin iterates several plausible GC-fallback values rather than one, since the defect is specifically that the fallback is not a single value.

Your framing of "v8 re-evaluates on every start, so this rare failure now gets unbounded chances" is the part I had not reasoned through — the marker-gated blocks made a best-effort guess safe in a way it stops being once the check is continuous.

[claude]

Reviewed the v8 hardware-fingerprint conf-healing logic in DarlingManagedPostgres.cs/DarlingManagedPostgresTests.cs.

Scope check: this is Darling-only (bundled/managed PostgreSQL instance sizing) with no Lite counterpart — Lite has no bundled Postgres to manage, so no parity gap here.

What looks solid:

• ConfHasCurrentHardwareFingerprint correctly compares only the last fingerprint occurrence (matching postgresql.conf's last-occurrence-wins semantics), with a test specifically pinning the resize-back-to-previous-size case that a naive Contains check would get wrong.
• DeriveWorkerSettings extraction keeps the v2 and v8 worker formulas from drifting apart, with a test asserting they agree.
• The shared_buffers/work_mem/max_parallel_workers omissions are well-justified and each has a dedicated "never emits" pin.
• T-SQL style guidelines don't really apply here (pure C#/no SQL in this diff); Darling PostgreSQL-store conventions (schema-qualification, naive UTC timestamps) aren't implicated either since this only touches postgresql.conf text generation, not the store.

One correctness gap flagged inline: the v8 fingerprint is built from whatever GetTotalPhysicalMemoryBytes() returns, but that method's fallback chain has an unguarded middle tier (GC.GetGCMemoryInfo().TotalAvailableMemoryBytes) that isn't normalized the way the explicit zero/non-positive case is. Since v8 (unlike v1–v7) re-evaluates on every start rather than once ever, an intermittent GlobalMemoryStatusEx failure could produce a different fingerprint on each occurrence, defeating the append-loop protection the PR's own test explicitly aims for and writing degraded sizing to production on that start. Details and a suggested fix are in the inline comment.

[claude]

Doc-comment mix-up: the /// <summary> block starting at line 636 ("True when the MOST RECENT fingerprint in the conf matches the current hardware...", describing the LastIndexOf/last-occurrence-wins rationale) reads like it documents ConfHasCurrentHardwareFingerprint, but there's no code line between it and the next /// <summary> block (line 647, "Whether the v8 block should be appended..."), so both attach to ShouldAppendHardwareSizing as one XML doc with two <summary> elements. ConfHasCurrentHardwareFingerprint — the method with the subtler last-occurrence-wins logic that this PR is centrally about — ends up with no doc comment at all, and ShouldAppendHardwareSizing's doc has a stray/duplicate <summary>.

Given CONTRIBUTING.md's C# style calls for XML docs on public APIs, worth moving the first block down onto ConfHasCurrentHardwareFingerprint.

[erikdarlingdata]

Right, and thank you — fixed in e3791ab, and DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks caught it independently in the same CI round, pointing at the same two <summary> openings.

Cause was exactly as you describe: I anchored the insertion on the signature line of ConfHasCurrentHardwareFingerprint, so the new block landed between that method's own summary and its member. Its test message warns specifically not to resolve this by deleting the first summary because it is usually a displaced block whose real member has been left undocumented — which is what happened here, and to the method this PR is centrally about. Moved the new member below the complete host body instead, so each doc sits above what it describes.

One thing that came out of re-reading it: BuildHardwareFingerprint's doc had also become untrue. It claimed the non-positive normalisation is what stops a failed RAM read appending on every alternating start. It is not — and relying on it was the bug you found in the other thread, since the GC fallback arrives as a varying positive. That paragraph now says where the stability actually comes from and warns against relying on the normalisation for it.

[claude]

Reviewed the diff. This PR is Darling-only (DarlingManagedPostgres.cs + its tests, CHANGELOG); Lite has no managed-Postgres equivalent (it's DuckDB-backed), so there's no Lite/parity twin to worry about here per CONTRIBUTING.md's Two-Store Parity section.

Correctness walkthrough:

• ConfHasCurrentHardwareFingerprint's LastIndexOf-based "last fingerprint wins" check correctly matches postgresql.conf's own last-occurrence-wins semantics, and the resize-back-to-previous-size test (HardwareFingerprint_ResizeBackToPreviousSize_StillRederives) proves the Contains pitfall it avoids.
• TryGetAuthoritativePhysicalMemoryBytes cleanly separates "real reading" from "GC-fallback guess," and GetTotalPhysicalMemoryBytes() still preserves the original three-tier fallback for the v1–v7 callers, so no behavior change there.
• Gating v8's append on ramReadingIsAuthoritative (not just a non-zero value) is the right fix for the append-loop risk called out in the PR description — a live/varying GC-fallback value would otherwise mint a new fingerprint on every blip.
• The conf string is read once before any of this call's appends run; that's fine for v8's own check since no other block writes the hardware-fingerprint prefix, so there's no cross-block staleness bug from reusing the pre-append snapshot.
• Structurally excluding shared_buffers and work_mem from the fingerprinted block (rather than trusting the formula's cap) is a good call and is pinned by dedicated "never emits" tests.

Left one inline comment: a doc-comment mix-up where two /// <summary> blocks land back-to-back with no code between them, so the write-up of the last-occurrence-wins rationale attaches to the wrong method (ShouldAppendHardwareSizing) and ConfHasCurrentHardwareFingerprint ends up undocumented.

No security, SQL-injection, or missing-index-DMV concerns — this PR doesn't touch T-SQL at all.

[claude]

Reviewed the diff (CHANGELOG.md, DarlingManagedPostgres.cs, DarlingManagedPostgresTests.cs). No blocking issues found.

Correctness

• ConfHasCurrentHardwareFingerprint correctly compares the last fingerprint occurrence (matches postgresql.conf's last-occurrence-wins semantics), not Contains — verified against the resize-back-to-original-size test case.
• BuildHardwareFingerprint formats invariantly (FormattableString.Invariant), avoiding locale-dependent false mismatches.
• The non-authoritative RAM guard is correct and necessary: TryGetAuthoritativePhysicalMemoryBytes returning false correctly short-circuits ShouldAppendHardwareSizing before the live/varying GC fallback value could mint a new fingerprint on every start.
• GetTotalPhysicalMemoryBytes()'s refactor onto TryGetAuthoritativePhysicalMemoryBytes preserves the original fallback chain (Win32 -> GC -> 4 GB sentinel) exactly.
• DeriveWorkerSettings extraction correctly de-duplicates the v2/v8 worker formula (hypertableCount + 2, 3 + bg + 8) — no drift risk between the two call sites.
• The v8 check reads conf once at the top of EnsureConfAppended (line ~1187), before v1–v7 may append new blocks earlier in the same call. That's benign here since none of v1–v7 write anything matching ConfHardwareFingerprintPrefix, but worth a mental flag if a future version block is added between v8's fingerprint prefix and where conf is captured — the read-once-at-top pattern would silently miss it.

Lite/Darling parity

• No Lite counterpart exists for managed-Postgres conf healing (Lite/ has no references to ConfMarkerV*/ManagedPostgres), which is expected — Lite uses embedded DuckDB, not a locally-managed Postgres install, so this isn't a parity gap under the "Two-Store Parity" rule in CONTRIBUTING.md (that rule targets the shared-interface seams like IAlertStateStore, which this PR doesn't touch).

Security/Performance

• No new external input, no injection surface — purely local file read/append driven by a Win32 memory query and an internal hypertable count. The extra GlobalMemoryStatusEx call per start (now 4 vs. 3 previously) is negligible.

Style

• No T-SQL in this PR, so the T-SQL conventions in CONTRIBUTING.md don't apply here.

Test coverage (constraint pins on never-emit shared_buffers/work_mem, the resize-back case, the non-authoritative-reading case, and the v2/v8 formula-parity check) looks thorough and specifically targets the failure modes called out in the PR description.

[erikdarlingdata]

Thanks — acting on the non-blocking flag rather than just noting it, in 08ea9c5.

You're right that the read-once pattern is benign today only because no v1–v7 block emits a line carrying ConfHardwareFingerprintPrefix. That is an unstated invariant holding up the whole staleness check, and if a future version block broke it, the failure would be silent: the check would keep running and keep concluding "current". That is the same shape as the defect this PR fixes — a check that stops checking without saying so — one level up.

Recorded it at the capture site, naming the invariant, the consequence if it breaks, and the remedy (re-read the file rather than adding a block above). No behaviour change: re-reading unconditionally would add a file read per start to defend against a hypothetical, and the comment puts the constraint where someone adding a v9 will actually see it.

On the parity point — agreed, and worth being explicit since Lite.Tests does not run in PR CI: this touches no shared seam, Lite has no managed Postgres, and the only shared file is TimescaleSupport.HypertableCount, read and not modified. So there is no parity exposure hiding behind the CI gap here.

[claude]

The fingerprint is an exact-MB comparison, and v8 (unlike v1-v7) runs on every start, not just once. GlobalMemoryStatusEx's ullTotalPhys isn't guaranteed bit-identical across reboots on every host class this targets — Hyper-V Dynamic Memory / balloon-driver VMs in particular can report a different "current" physical memory total from one boot to the next even with no operator-intended resize, and a handful of these monitoring boxes are cloud VMs per the PR description. If that happens, ShouldAppendHardwareSizing sees a legitimate, "authoritative" reading that simply differs by a few MB from the last one, and will treat it as a hardware change on every single restart: appending a fresh v8 block, re-logging "Appended v8 hardware sizing..." each time, and growing postgresql.conf by 7 lines per restart forever. Functionally harmless (last-occurrence-wins), but it defeats the "converges immediately" invariant the doc comment relies on, and produces unbounded file growth / log noise on any host whose reported RAM isn't perfectly stable across boots.

Worth considering quantizing RAM before fingerprinting (e.g., round to the nearest GB, or the same granularity the derived settings are keyed on) so a few-MB reboot-to-reboot jitter doesn't read as a hardware change, the same way the rest of this PR is careful to distinguish "the machine changed" from "the reading is noisy."

[erikdarlingdata]

Real defect, and fixed in a46988e — thank you.

You've identified the thing this PR should have caught itself: the whole design is about distinguishing "the machine changed" from "the reading is unreliable", and I handled the second only in its loud form (a failed read) while leaving the quiet form (a successful but jittery read) to be treated as a resize. The evidence was already in front of me — the fleet reports 31.5 GB on a nominally 32 GB host — so the reported total is demonstrably not the nominal figure, and I had no basis for assuming it is stable to the megabyte across reboots.

Quantized to the nearest GB, and applied to both the fingerprint and the derivation rather than the fingerprint alone. That buys a stronger invariant than just suppressing the churn: a v8 block is now exactly reproducible from the fingerprint written above it, so the two can never disagree about the machine they describe. A GB also has the property you're implicitly relying on — far above any plausible jitter, far below the smallest real resize this class sees (4 -> 8 GB), so it cannot mask a genuine change. There's a pin asserting exactly that, since a quantization that hid real resizes would be a worse bug than the one it fixes.

Scoped to the v8 path only: v3/v5/v7 keep deriving from the raw reading, so nothing shifts on a store that never reaches v8.

Proven red by dropping the quantization — jitter: 31.5GB / 32GB / 32GB-7MB all read as the SAME machine and jitter: block is exactly reproducible from its fingerprint both fail, 18 passed / 2 FAILED, restored to 20/0.

Incidentally the rounding is arguably more correct on its own merits: ullTotalPhys is always slightly under the nominal size because of firmware reservation, so the nearest GB is closer to the quantity the sizing formulas conceptually mean.

[claude]

Minor consistency nit: BuildHardwareFingerprint, ConfHasCurrentHardwareFingerprint (line 650) and ShouldAppendHardwareSizing (line 675) are all public, but they're decision/derivation helpers in the same spirit as DeriveMemorySettings and DeriveWorkerSettings, which this PR (correctly) keeps internal. Tests already reach internal members via InternalsVisibleTo, so nothing requires these to be public — making them internal would keep the class's public surface limited to the Build*ConfAppend entry points, matching the established pattern in this file.

[erikdarlingdata]

Agreed and done in a46988e. BuildHardwareFingerprint, ConfHasCurrentHardwareFingerprint, ShouldAppendHardwareSizing and BuildHardwareSizingConfAppend are all internal now, matching DeriveMemorySettings/DeriveWorkerSettings and keeping the public surface to the Build*ConfAppend entry points callers outside the class actually use.

public was unconsidered rather than deliberate on my part — I mirrored the older Build*ConfAppend members without asking whether these needed the same reach, and they don't: EnsureConfAppended is the only production caller and the tests reach internals via InternalsVisibleTo already.

One knock-on worth recording: my out-of-repo verification harness reflects into this class and was binding BindingFlags.Public | BindingFlags.Static, so it silently resolved nothing after the change and would have reported the API as absent rather than failing on behaviour. Updated to include NonPublic, and re-ran to confirm all 20 checks still pass against the real built assembly rather than trusting the compile.

[claude]

Reviewed the v8 hardware re-derivation change (DarlingManagedPostgres.cs / tests). This is Darling-only — Lite has no managed-Postgres store, so there's no Lite/Darling parity concern here; no T-SQL touched. Left two inline comments on the source file:

1. RAM fingerprint has no tolerance for boot-to-boot jitter — v8 runs unconditionally on every start (unlike the marker-gated v1-v7) and compares an exact-MB fingerprint. If GlobalMemoryStatusEx ever reports a slightly different "authoritative" total across reboots without an actual operator-intended resize (plausible on Dynamic-Memory/balloon-driver VMs), the check can't distinguish that from a real hardware change, and will append a fresh v8 block + log line on every restart indefinitely. Harmless functionally (Postgres takes the last occurrence) but defeats the stated "converges immediately" invariant and grows the conf file unbounded over the service's lifetime.
2. Minor encapsulation nit — the new BuildHardwareFingerprint / ConfHasCurrentHardwareFingerprint / ShouldAppendHardwareSizing helpers are public, whereas the existing analogous derivation helpers (DeriveMemorySettings, DeriveWorkerSettings) are internal. Tests already use InternalsVisibleTo, so nothing requires the wider visibility.

One more thing that doesn't anchor to a diff line: Darling/README.md's "What first run does" section documents v1, v2, v3, v5, v6, v7 in detail (markers, formulas, when each fires) but isn't updated here to mention the new v8 hardware-sizing block, even though it's a real behavior change (a new, unconditional-on-every-start conf append). Worth a short addition there so operators inspecting postgresql.conf aren't surprised by an undocumented eighth block.

Everything else checks out well: the shared DeriveWorkerSettings/DeriveMemorySettings extraction correctly prevents v2/v8 formula drift, the shared_buffers/work_mem exclusions are well-justified and pinned by tests, the last-occurrence-wins fingerprint comparison (vs. a naive Contains) is correct for the resize-back-and-forth case, and the non-authoritative-RAM guard correctly avoids treating a GC-fallback guess as evidence of a hardware change.

[claude]

Reviewed the diff (CHANGELOG, DarlingManagedPostgres.cs, DarlingManagedPostgresTests.cs).

Scope check: this is Darling-only (the bundled managed PostgreSQL store's postgresql.conf healing). Lite has no managed-Postgres equivalent (confirmed via grep — no postgresql.conf/effective_cache_size references under Lite/), so there's no Lite/Darling parity gap to flag here; this isn't a shared-collector or store-schema change subject to the Two-Store Parity rules.

Correctness: traced through the v8 fingerprint mechanism carefully — the "compare the last occurrence, not Contains" logic in ConfHasCurrentHardwareFingerprint correctly handles the resize-back-to-previous-size case, the non-authoritative-RAM guard in ShouldAppendHardwareSizing/TryGetAuthoritativePhysicalMemoryBytes correctly prevents the GC-fallback figure (a live, varying quantity) from being fingerprinted and causing an append-loop, and DeriveWorkerSettings being extracted out and shared between v2 and v8 means the two writers of max_worker_processes can't drift. The conf string is read once up front and v1–v7 never emit a ConfHardwareFingerprintPrefix line, so reusing that stale snapshot for the v8 comparison is safe, as the comment there explains. The structural exclusion of shared_buffers/work_mem from the hardware block (vs. relying on the formula's cap) is a good defensive choice given the Windows error-487 history.

Nothing jumped out as a functional bug. The property-based tests (fingerprint stability across a failed read, resize-back, non-authoritative appends-nothing, shared_buffers/work_mem never emitted, worker-formula parity with v2) cover the edge cases I'd otherwise have asked about.

Minor, non-blocking: TryGetAuthoritativePhysicalMemoryBytes is now called up to 4 times per bootstrap on a fresh cluster (once each for v3/v5/v7 via GetTotalPhysicalMemoryBytes(), plus once directly for v8), so a genuinely flaky GlobalMemoryStatusEx call could log the same "could not query total physical memory" warning multiple times in one start and, in principle, let v3 size from a real reading while v8 skips in the same boot. Extremely unlikely in practice and not worth restructuring for.

No security, injection, or secrets-handling concerns — this only formats fixed-shape strings into a local conf file already owned by the service account.

[claude]

BuildHardwareSizingConfAppend(v8RamBytes, hypertableCount) on the line above internally quantizes RAM (DeriveMemorySettings(QuantizeRam(totalPhysicalMemoryBytes))) before writing effective_cache_size / maintenance_work_mem into the conf. This line then recomputes DeriveMemorySettings(v8RamBytes) with the raw, unquantized reading purely for the log message.

Near a GB boundary these disagree. E.g. a 31.5 GB reading quantizes to 32 GB for the actual write (effective_cache_size = 24576MB per the HardwareSizingConfAppend_EmitsHostDerivedSettings test), but this log line would report EffectiveCache = 24192MB (75% of the raw 31.5 GB) — a value that was never written to postgresql.conf. Same mismatch for maintenance_work_mem (1638MB written vs. ~1612MB logged).

Given how much this PR leans on "the block is exactly reproducible from its own fingerprint" as an invariant, the log message should reflect the same quantized value that was actually persisted:

var v8Settings = DeriveMemorySettings(QuantizeRam(v8RamBytes));

(QuantizeRam is internal static in this class already, so it's reachable here.)

[erikdarlingdata]

Correct, and it's a defect I introduced two commits ago — fixed in the push above.

Your framing is the right one: the PR leans on "the block is exactly reproducible from its own fingerprint", and a log line reporting numbers that appear nowhere in the file breaks that in the place an operator would actually look. It's the same failure class the PR exists to fix — a record that disagrees with reality while looking authoritative — reintroduced by the fix for the previous round.

I took your suggested change one step further rather than applying it literally. DeriveMemorySettings(QuantizeRam(v8RamBytes)) fixes the two settings but leaves RamMb in the same message still logging the raw figure, so a 31.5 GB host would report "host RAM 32256 MB -> effective_cache_size 24576MB" — internally inconsistent in a different way. Quantizing once at the call site and passing that value down makes the logged values necessarily the written ones by construction, rather than by two call sites agreeing to apply the same transform:

var v8QuantizedRam = QuantizeRam(v8RamBytes);
File.AppendAllText(confPath, BuildHardwareSizingConfAppend(v8QuantizedRam, hypertableCount));
var v8Settings = DeriveMemorySettings(v8QuantizedRam);

QuantizeRam is idempotent, so the callee still quantizes and still lands on the same answer — the internal call stays as the guarantee for any other caller.

Harness still 20/20 against the real built assembly.

[claude]

Reviewed. This is a well-scoped, thoroughly self-reviewed change (the branch already carries four internal review/fixup commits addressing quantization jitter, non-authoritative RAM reads, and doc placement). One remaining issue found, left as an inline comment:

• Log message doesn't match what's written to postgresql.conf (DarlingManagedPostgres.cs:1315) — BuildHardwareSizingConfAppend quantizes RAM to the nearest GB before deriving effective_cache_size/maintenance_work_mem, but the subsequent _logger.LogInformation call re-derives those same settings from the raw, unquantized reading. Near a GB boundary the logged values disagree with what actually landed in the conf file (e.g. 31.5 GB logs effective_cache_size=24192MB but writes 24576MB).

Other notes:

• Lite/Darling parity: no concern — the managed-Postgres bootstrap (DarlingManagedPostgres.cs) has no Lite counterpart; Lite has no bundled Postgres store, so this is correctly Darling-only.
• CONTRIBUTING conventions: N/A here (no T-SQL, no schema migration rung — this only touches postgresql.conf generation), and the PostgreSQL-store style rules (naive UTC timestamps, schema-qualified migration objects) don't apply to this file.
• Test coverage for the new v8 path (fingerprinting, quantization, non-authoritative-RAM gating, worker-formula sharing with v2) is thorough and each pin is documented with the failure it guards against.

[claude]

Review

Scope: Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs + Darling/Darling.Tests/DarlingManagedPostgresTests.cs (C# only — no T-SQL, no Lite files touched).

Correctness — traced the full v8 path (EnsureConfAppended → TryGetAuthoritativePhysicalMemoryBytes → ShouldAppendHardwareSizing → ConfHasCurrentHardwareFingerprint → BuildHardwareSizingConfAppend/BuildHardwareFingerprint → QuantizeRam/DeriveMemorySettings/DeriveWorkerSettings). Checked in particular:

• conf is captured once at the top of EnsureConfAppended before v1–v7 may append, and the code correctly documents/relies on the invariant that none of v1–v7 emit a line carrying ConfHardwareFingerprintPrefix — confirmed true against the current v1–v7 blocks.
• ConfHasCurrentHardwareFingerprint correctly uses LastIndexOf rather than Contains, matching postgresql.conf's last-occurrence-wins semantics (the resize-back-to-16GB case is directly pinned).
• QuantizeRam is applied consistently on both the fingerprint and the derived settings, and is idempotent, so the "quantize once at the call site, pass down" refactor in the final commit doesn't introduce a fingerprint/written-value mismatch — verified by hand across BuildHardwareFingerprint and BuildHardwareSizingConfAppend.
• TryGetAuthoritativePhysicalMemoryBytes vs. the GC-fallback-bearing GetTotalPhysicalMemoryBytes: v8 only ever calls the authoritative variant, so a transient GlobalMemoryStatusEx failure can't mint a novel fingerprint from the live/varying GC figure. v3/v5/v7 are untouched and still use the original fallback chain.
• TimescaleSupport.HypertableCount is a static count derived from the compiled collector catalog (CollectorCatalog.All.Count + 1), not a live DB query, so calling it before Postgres has started (same as the existing v2 usage) is safe.
• The internal visibility change on the new helpers has no callers outside DarlingManagedPostgres.cs and Darling.Tests, and PerformanceMonitor.Darling.Service.csproj already has InternalsVisibleTo for Darling.Tests, so this doesn't break the build.

Nothing else in the diff calls GlobalMemoryStatusEx/file I/O in a new way that would introduce a security or resource-handling concern, and there's no SQL construction here (this is postgresql.conf text, not a SQL statement).

Lite/Darling parity — no drift. DarlingManagedPostgres sizes Darling's own bundled/managed PostgreSQL process; Lite has no analogous managed-server component (it writes directly to embedded DuckDB), so this isn't one of the shared-library seams CONTRIBUTING.md's "Two-Store Parity" section is about, and no Lite-side counterpart is expected.

Process note, not a defect: this PR's own commit history (6 commits) already walks through and fixes exactly the subtle failure modes a reviewer would look for — RAM-reading jitter across reboots (quantization), a non-authoritative/GC-fallback reading masquerading as a hardware change, a misplaced XML-doc block, and a log-line/written-value mismatch near a GB boundary. I re-derived each of those independently while reading and didn't find anything left over.

No findings to post inline. Missing-index DMV recommendations are out of scope per project convention and don't apply here regardless.

[claude]

Nit: this method's opening brace/body are indented one level deeper than every other test in the file (8 spaces instead of 4 for the brace, and the body doesn't gain the usual extra 4 spaces past it). Doesn't affect behavior, just inconsistent with the surrounding style.

[erikdarlingdata]

Fixed in 3180397 — whitespace only, one line. You were right that it was the brace: it sat at 8 spaces, so the body at 8 read as flush with it rather than nested. Correcting the brace to 4 makes the existing body indentation correct without touching it.

I checked the property rather than just this one site: every parameterless test method in the file now opens its brace at 4 spaces (33 of them), so this was the only instance.

[claude]

Review

Scope: this PR only touches Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs and its test file, plus a CHANGELOG.md entry. All C#, no T-SQL, so the CONTRIBUTING.md T-SQL style section doesn't apply here.

Lite/Darling parity: No parity issue — managed-Postgres bootstrap/conf-healing is a Darling-only concept (Lite runs on DuckDB and has no equivalent EnsureConfAppended/postgresql.conf machinery), confirmed by grepping for any Lite analogue of ManagedPostgres/ConfMarkerV*/DeriveMemorySettings — there is none.

Correctness:

• Traced the fingerprint/quantization logic by hand: QuantizeRam rounds to the nearest GB and is idempotent, so the value baked into the written fingerprint always matches the value baked into the settings computed from it (BuildHardwareSizingConfAppend calls QuantizeRam internally on the same input used for the fingerprint) — no drift between what's logged and what's written, which the code explicitly guards against via the "quantize once, pass down" comment at the call site.
• ConfHasCurrentHardwareFingerprint correctly compares only the last fingerprint occurrence (not Contains), so a host resized 16→32→16 GB is handled correctly — verified this against the dedicated HardwareFingerprint_ResizeBackToPreviousSize_StillRederives test.
• The non-authoritative-RAM-reading guard (TryGetAuthoritativePhysicalMemoryBytes) is the right fix for the real hazard here: since v8 runs unconditionally on every start (unlike the marker-gated v1–v7), acting on the GC-fallback figure would mint a new fingerprint — and append a growing block — on every transient GlobalMemoryStatusEx blip. Refusing to act without an authoritative reading avoids that, and GetTotalPhysicalMemoryBytes() (used by v1/v3/v5/v7) preserves its original three-tier fallback behavior unchanged via the extracted TryGetAuthoritativePhysicalMemoryBytes helper.
• Verified the worked example in the tests by hand: 32 GB, 40 hypertables → effective_cache_size = 24576MB (75%), maintenance_work_mem = 1638MB (5% floored at 1.5 GB, capped at 2 GB), max_background_workers = 42, max_worker_processes = 53 — all match DeriveMemorySettings/DeriveWorkerSettings.
• The shared_buffers/work_mem exclusions are structural (never emitted regardless of RAM), not just "currently the formula doesn't move them," which is the right call given the Windows error-487 mitigation and the work_mem regression data cited in the PR description — and it's pinned by dedicated theory tests across RAM sizes.
• v2 and v8 sharing DeriveWorkerSettings so the two writers of max_worker_processes can't disagree is a nice touch, and is itself pinned by HardwareSizingConfAppend_WorkerCountsMatchV2Formula.

Security: No new I/O surface beyond the existing postgresql.conf read/append pattern already used by v1–v7; no user-controlled input reaches these strings (RAM comes from GlobalMemoryStatusEx, hypertable count from TimescaleSupport), so no injection concern.

Performance: Negligible — one extra LastIndexOf over an already-in-memory small conf file per service start, no new syscalls beyond the existing memory query.

No blocking issues found. Left one inline nit (test method indentation, purely cosmetic).

[claude]

Reviewed the diff (CHANGELOG + Darling's DarlingManagedPostgres.cs/tests). No T-SQL in this PR, so the style-guide checks there don't apply.

Correctness — traced the load-bearing invariants:

• conf is read once at the top of EnsureConfAppended (line 1210) before any v1-v7 append; the v8 fingerprint check correctly relies on none of those blocks emitting ConfHardwareFingerprintPrefix, which holds today.
• QuantizeRam is idempotent, so quantizing the already-quantized v8QuantizedRam a second time inside BuildHardwareSizingConfAppend (called both directly and via BuildHardwareFingerprint) is intentional, not a bug — matches the "reproducible from its own fingerprint" test.
• ConfHasCurrentHardwareFingerprint correctly compares the last fingerprint occurrence rather than Contains, which is what makes the 16→32→16 GB resize-back case re-derive instead of latching — verified against the test that pins exactly this.
• TryGetAuthoritativePhysicalMemoryBytes returning (false, 0) on failure is handled: the !v8Authoritative branch short-circuits before the unused zero-based fingerprint is ever acted on.
• v3/v5/v7 still go through the old GetTotalPhysicalMemoryBytes() (GC fallback → 4 GB sentinel) unchanged; only the new v8 path requires an authoritative reading. No behavior change to the existing marker-gated blocks.
• TimescaleSupport.HypertableCount is a static/compiled catalog count, not a live DB query, so there's no fresh-initdb ordering hazard for the v8 block running before hypertables exist.

Parity — this is Darling-only (managed PostgreSQL instance sizing); Lite embeds DuckDB and has no postgresql.conf to heal, so there's no Lite counterpart to drift from. Confirmed no Lite files reference this logic.

Security — no new input handling, file paths are all internal (dataDirectory/confPath from existing config), no injection surface.

Minor/non-blocking observation: TryGetAuthoritativePhysicalMemoryBytes already logs a warning internally on failure, and the v8 call site logs a second warning on the same failure — slightly duplicated log noise, not worth a change.

Nothing else stood out. The mutation-tested pins described in the PR body (quantization jitter, resize-back-to-previous-size, non-authoritative reading, shared_buffers/work_mem exclusion) line up with what the implementation actually does.