Give timescaledb.enable_job_execution_logging its own conf marker, so existing stores can heal (#3175) - #3177
Conversation
) The GUC that makes timescaledb_information.job_history record anything was written into the v1 postgresql.conf block, which is the one block EnsureConfAppended cannot heal: its marker is already present on every pre-existing cluster, so the append carrying the setting was skipped and it only ever reached a fresh initdb. A maximum over the resulting empty job_history returns zero rows, which reads as "no run exceeded the line" rather than "this instrument is off". The setting moves to its own v11 marker, the way every setting added after v1 is handled. get_store_metrics now reports the GUC's effective value, source and sourcefile on every response, because that tool's own description redirects a maximum question to job_history and had no way to say whether the route was live.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewScope: Darling-only ( Correctness:
Security: No new file/process/network surface; SQL is parameterized; nothing user-controlled reaches the query. Performance: Adds one small, indexed No missing-index suggestions per your instructions, and none were relevant here anyway (no new T-SQL). |
…t reference The gated E2E asserted "exactly one shared_preload_libraries" with a substring count, which reads 2 on a healthy cluster: initdb's generated postgresql.conf already documents the setting as a commented line. CI caught it; the ungated twin could not, because its fixture held only the product's own blocks. Both now count live assignments through one comment-skipping helper, and the ungated fixture carries initdb's commented decoys so the same instrument error goes red locally. Also from review: the v11 marker's doc comment referenced a test name that does not exist, and the pg_settings probe now lets OperationCanceledException through rather than reporting a cancellation as an unreadable measurement. The sourcefile column is superuser-only, so the note and the tool description no longer promise it unconditionally.
|
Reviewed the full diff (both commits) against CONTRIBUTING.md's conventions. Summary: No correctness, parity, or security issues found. This is a Darling-only change (TimescaleDB conf healing + What I specifically checked and found solid:
Nothing rises to a blocking or even a nit-level comment; the PR's own description already documents the two review findings that were applied (dangling |
|
Reviewed the diff (Darling-only: Correctness: the v11 marker/block, the removal from One doc-drift nit (not in the diff, so flagging here instead of inline): No security, injection, or performance concerns — the new SQL is a single parameterized |
CI's second round said pg_settings had no row for the GUC on a cluster whose conf carried the preload line, and the test's failure message named the wrong cause. Measured: timescaledb in shared_preload_libraries loads the LOADER, and the loader pulls in the versioned library that defines this GUC only for a database that has the extension. From a database created TEMPLATE template0 the GUC has zero pg_settings rows while the loader-defined timescaledb.max_background_workers still has one. EnsureRunningAsync creates the store database; TimescaleSupport creates the extension later in the worker's bootstrap. The gated E2E now stands in for that step before reading the setting, and its failure message names both causes. The NotRegistered note said "this store has no TimescaleDB library loaded", which is one of the two causes and not the one a mid-bootstrap store would hit. It now states both and the consequence they share: job_history does not exist on this connection either way.
…E EXTENSION A bare CREATE EXTENSION IF NOT EXISTS timescaledb TERMINATES THE BACKEND when the library is on disk but absent from shared_preload_libraries (#1922), and with no SCHEMA clause it lands the extension on whatever schema resolves first. LiveTimescaleProbe handles both and returns a flag, so the precondition is asserted instead of assumed: a store that could not be enabled now says so rather than presenting as a setting that is absent. The meta-test that bans the masking shape matches TimescaleSupport.TryEnableAsync call sites, so a hand-rolled statement passed it by not being one — which is worse than tripping it.
|
Reviewed this PR end-to-end (all 5 commits) against CONTRIBUTING.md's conventions, correctness/edge-cases, Lite/Darling parity, security, and performance. No issues found — this is a clean, well-verified fix. Notes from the review: Correctness
Lite/Darling parity
Security / performance
Nice catch-and-fix cycle on the CI failures baked into this branch's history (substring-vs-assignment counting, the loader-vs-versioned-library preload distinction) — both fixes are correctly reflected in the final diff. |
|
Reviewed against CONTRIBUTING.md conventions and the correctness/parity/security/perf checklist. No T-SQL touched here (pure C#/Npgsql, Darling-only — TimescaleDB provisioning and MCP read path), so the T-SQL style rules don't apply. Parity: No Lite counterpart exists for TimescaleDB conf management or Correctness:
Security: No new input handling, no secrets, no file/process/network surface beyond the existing Npgsql/postgresql.conf append pattern already used for v2–v10. Performance: The new No findings to raise — this is a solid, narrowly-scoped fix with strong test coverage (unit + gated E2E) for the actual defect (v1 marker being unhealable) and the MCP-side blind spot it created. |
|
Reviewed. This is a Darling-only change (TimescaleDB Checked specifically:
No correctness bugs, parity gaps, or security issues found. The one thing knowingly left stale — |
…on, and re-derive the compression grid with it (#3178) * Re-derive HeaviestHourlyRefreshObservedCeilingSeconds from a job_history census The constant was 594 s, the maximum of a 16-run record that mixed a census of the boundary day's tail with a sample of the days after it. The census read the comment named as the fix has been done: every run of job policy_refresh_continuous_aggregate query_store_stats_interval_hourly since the 13:44:23 boundary, one row per run from timescaledb_information.job_history, read at 2026-09-08 01:37Z. 57 succeeded runs after the boundary, 194 s to 896 s, median 418 s, none failed and none without a finish time; 304 runs at or before it, median 1081.7 s, maximum 13300.7 s. The estimator is unchanged, so the constant is 896. Two pre-registered triggers fired together. The 95th percentile is no longer the maximum at 57 readings (830 s, 66 s below it), so the sample-size half of the maximum-versus-percentile argument is retired and the decision is re-taken on the half that never referenced n. And the sizing figure now sits 146 s ABOVE RefreshSlotWarningSeconds, so every assertion holding the grid clear of it is false. Those are left with their conditions exactly as written. * Drop the "well inside one phase slot" margin from the two places that state it At 896 s of a 900 s slot the refresh still fits, so "inside one phase slot" is true and load-bearing; the margin that made it "well inside" is 4 s and naming it as comfort is the claim that has stopped being true. * Give every deliberately-red assertion its own note, not three of six Three of the six failing methods carried a comment explaining why they are left failing and which decision owns the remedy; three did not, so the same inversion read as an oversight in half the places it appears. Conditions are unchanged in all six. NoCompressionMinuteStartsWhileTheHeaviestRefreshIsStillRunning also needed the distinction its two halves now have: the grid clearance genuinely passes (the nearest compression minute is 1,320 s past the heaviest start), and only the watch-line ordering and the 26%-gap literal derived from it are false. * Retire the second reason the excluded run was out, where it was still asserted The open-question paragraph closed on "the excluded run is disqualified by its duration alone", which was the belt to the boundary's braces. At a 896 s population maximum 864 s is an ordinary member of the post-boundary range, so that belt is gone and the sentence was contradicting the paragraph above it. The positional rule needs no second reason, which is why it was chosen over one. * Re-derive the refresh grid so contention cannot depend on list position The hourly phase map is injective now: the heaviest refresh is answered by identity and every other policy takes its own consecutive minute, so thirteen policies hold thirteen distinct minutes at any list order. The hour is three derived bands - the light band and its guard, the heaviest refresh's window, the compression band - and the window is the remainder, so nothing sizes itself from the ceiling it has to hold. The compression grid is re-derived from the same rule rather than renumbered, the guard band comes from the light refreshes' own ceiling, and the band's width comes from the catalog and a stated per-minute spread. * Re-derive OtherHourlyRefreshObservedCeilingSeconds from a job_history census 226.8 s over 874 runs of the twelve non-heaviest hourly refresh policies, post-boundary, with nothing removed by the succeeded/finish filter. The midnight regime stays in, because the guard band is now derived from this figure and a readout of a margin has to include the runs that consumed it. The census's shape, the midnight mechanism behind the two largest runs, and the guard band the ceiling sizes are all pinned against the constants rather than restated. * Point four figures at the re-derived window instead of the 15-minute slot PercentOfSlot's doc quoted 99.6%, which was 896 against a 900 s slot; against the window the hour can spare it is 71.1%, which is what the test asserts. The narrowing pin's margin, the population clause's warning-band note and one watch-line figure were reading the same way. * Scope both job_history censuses to the one store whose GUC is on timescaledb_information.job_history only records executions where timescaledb.enable_job_execution_logging is on, it is off by default, and the conf block that sets it cannot be healed onto a cluster that predates it - so a maximum over that view on an older store returns zero rows and reads as nothing exceeded the line. Both censuses are one store's, said as a precondition of the read rather than as a caveat about the workload. The refresh-slot warning line names that route, so it now names the condition too: a pin on a pointer's presence cannot tell an operator that following it may return nothing. * Test injectivity through the shipped phase map, not a copy of its rule The rotation control re-implemented the counting rule inline, because the public map can only be called at the one order HourlyRefreshPhaseOrder has. That proved the rule injective under permutation and left the shipped method exercised at exactly one order. The map now takes the order as a parameter and the product passes its own list. Only the order is a parameter: the geometry still comes from the registry, so the seam cannot fabricate a different grid. * Make the rotation control assert the map responded, per view Comparing the two minute sequences passes for an overload that ignores its order parameter, because that still returns a permutation of the unrotated minutes. What has to change is the minute a named view gets. * Say the census scope is a property of the read, now that the GUC heals #3175/#3177 gave timescaledb.enable_job_execution_logging its own conf marker, so existing stores heal. That does not widen either census: both reads predate the heal, and a later one would have to state its own scope rather than inherit this one's.
1 participant
Closes #3175.
timescaledb.enable_job_execution_loggingwas written into the v1postgresql.confblock by #1681. That is the one blockEnsureConfAppendedcannot heal: it skips a block whose marker it finds, and v1's marker is present on every cluster that already exists — so the append carrying the GUC only ever reached a fresh initdb. Every store older than that release has hadtimescaledb_information.job_historyempty the whole time, and a maximum over an emptyjob_historyreturns zero rows, which reads as "no run exceeded the line" rather than "this instrument is off".The change
The setting gets its own v11 marker, the way every setting added after v1 is handled, and it is moved rather than duplicated — leaving a copy in
BuildConfAppendwould cost nothing at runtime and would leave the repository asserting this setting in the block that provably cannot deliver it, which is the reading that produced the defect.TheJobExecutionLoggingGuc_IsInV11AndNotInTheUnhealableV1Blockpins both halves so it cannot drift back.get_store_metricsnow reports the GUC's effective value, source and sourcefile on every response, in ajob_historyblock present whatever the state — the #2813 precedent one step further, because here the absence being guarded is the absence of rows in the instrument this tool's own description redirects a maximum question to. Four states, never a bool: "the probe did not run", "this server has no such setting" and "it is switched off" call for three different readings of an emptyjob_history, andOffsplits again on whether anything set it off, because those need different actions.The GUC name is a single constant on the platform-neutral side (
StoreSelfMetrics), so the name the conf block writes and the name the probe binds cannot drift. A retyped copy would not error:pg_settingswould return no row for a name nobody registered, which this read reports as "the server has no such setting" — indistinguishable from a plain-PostgreSQL store.Why not widen the v1 marker, measured rather than asserted
Making v1's check ask "is the GUC line present?" would re-append the whole shared v1 block to every pre-existing cluster. Measured on TimescaleDB 2.30.0 / PG 17 in a throwaway container: a conf carrying an operator's
shared_preload_libraries = 'timescaledb,pg_stat_statements'came back up serving'timescaledb'alone once the v1 block was appended behind it — the GUC is list-valued and the last assignment replaces the list rather than extending it.listen_addresseswould likewise re-assert loopback over a conf-configured exposure, andportwould override a hand-edited one.HealingAConfWithoutV11_AppendsOnlyThatBlock_AndReAppliesNoV1Settingasserts every v1 setting still has exactly one assignment after a heal, with the v1 names derived from the v1 builder rather than listed.ConfV1Block_ContentIsFrozen_ANewSettingNeedsItsOwnMarkeris the pin whose absence let this happen: adding anything to the v1 builder now goes red on the commit that types it.EveryConfMarker_IsDistinct_AndNoneIsASubstringOfAnothercloses the other route —EnsureConfAppendedasks each question asContains, so a marker that became a substring of another would let a cluster carrying only the longer block answer "present" for the shorter one and never gain it. The v1 marker is nearly a prefix of every later one and is saved only by the parenthesised version segment; that is not obvious by eye.Reload: the append is enough where it matters, and nothing reloads where it is not
EnsureConfAppendedruns beforepg_ctl start, so on a service-owned start the setting is live on the very start that writes it — no reload needed. Measured for the record rather than assumed: the GUC'spg_settings.contextissighup, and a conf append plus onepg_reload_conf()moved it fromoff/source = defaulttoon/source = configuration file. The gated E2E pinscontext = 'sighup'against the bundled TimescaleDB, so if it ever became restart-only that reasoning goes red instead of the comment quietly becoming fiction.The exception is the adopted-listener path: when a postmaster is already running, this service neither stops nor signals it, so the heal waits for the next service-owned start. No reload is issued, and that is a decision. v9 and v10 carry the same exposure and the same choice; signalling a server this service did not start is the same class of act as stopping one; and a reload would apply this block while leaving the restart-only settings the same heal may have just appended (v2/v3/v4/v5/v7) inert. A half-applied conf is worse than a consistently deferred one — it removes the operator's ability to reason about the server's state from "did the service own this start".
Existing clusters: logging starts now, and there is nothing to recover
A store that has been running without the GUC wrote no per-run rows, and TimescaleDB does not retain what it was told not to record. The append's log line states that rather than implying it, and so does the
Onnote in the MCP response: coverage begins at the moment logging was switched on, so an emptyjob_historywindow that predates the heal is expected and is not evidence about those runs.job_statsremains the unconditional surface — which is why every shipped read already uses it, and why no shipped read had to change here.One thing the append cannot beat, and the reason the check reads the effective value rather than the marker:
postgresql.auto.confis read afterpostgresql.conf, so anALTER SYSTEM SET ... = offwins. Measured — with the appended block last inpostgresql.conf, the effective value stayedoffwithsourcefilenamingpostgresql.auto.conf. A marker check would have called that store healed.Legibility: the check, not the doc
Recommended and built: a check, in
get_store_metrics. Not documentation, for two reasons. The redirect is issued by the product, in that tool's own description, so a product that sends a caller to an instrument should say whether it is switched on. And the documentation half already existed —TheDescription_...NotAMaximumhas required the description to namejob_historysince #3119, and passed the entire timejob_historywas empty on every older store. Naming a route is not the route working.TheDescription_SaysJobHistoryNeedsItsGucOn_AndTheResponseReportsItasserts the sentence and the shipped read together, so either half disappearing lands there rather than leaving advice to go look somewhere this tool declines to look.No shipped read queries
job_history— checked; every one usesjob_stats, deliberately, andTimescaleSupportcitesjob_historyonly as the provenance of a constant. So there was no read to gate; the surface that needed the check is the one that redirects.Verification, and what it does not cover
CI caught two, both mine, and the second one taught the more interesting thing. Round 1:
Darling Linux buildpass 1m56s,reviewpass 5m13s,verifypass 5m43s,check-branchespass 4s,description-driftpass 8s, andDarling PostgreSQL testsFAIL at 4m41s — 8,161 tests, 8,154 passed, 6 not executed, 1 failed:ExistingStore_GainsJobExecutionLogging_OnNextStart_Gated,Expected: 1 Actual: 2. Not the product: the assertion countedshared_preload_libraries =as a substring, and initdb's generated postgresql.conf already documents the setting as a commented line, so a healthy cluster reads 2. The ungated twin could not have caught it, because its fixture held only the product's own blocks and none of initdb's decoys — an isolated fixture hiding the class of bug. Both now count live assignments through one comment-skipping helper, and the ungated fixture carries a stock commented preamble so the same instrument error goes red locally.Round 2 got past that and failed differently:
pg_settings has no row for timescaledb.enable_job_execution_logging— on a cluster whose conf demonstrably carried the preload line, with the count assertions above it all green. The test's own failure message named the wrong cause ("the library is not preloaded"). Measured:timescaledbinshared_preload_librariesloads the loader, and the loader pulls in the versioned library — the one that defines this GUC — only for a database that has the extension installed. From a database createdTEMPLATE template0on 2.30.0/PG17, the GUC has zeropg_settingsrows while the loader-definedtimescaledb.max_background_workersstill has one.EnsureRunningAsynccreates the store database;TimescaleSupportcreates the extension later in the worker's bootstrap, so the E2E now stands in for that step before reading the setting, and the message names both causes.Enabling it goes through
LiveTimescaleProbe.TryEnableAsyncrather than a bareCREATE EXTENSION, and the flag is asserted: that statement terminates the backend when the library is on disk but unpreloaded (#1922), and with noSCHEMAclause it lands the extension on whatever schema resolves first. Worth noting the meta-test that bans the masking shape matchesTimescaleSupport.TryEnableAsynccall sites, so the hand-rolled statement passed it by not being one — passing a guard by falling outside it is worse than tripping it.That finding also corrected shipped prose: the
NotRegisterednote said "this store has no TimescaleDB library loaded", which is one of the two causes and not the one a mid-bootstrap store hits. It now states both and the consequence they share —job_historydoes not exist on this connection either way. A second probe to split the two causes was considered and left out, because no case is known where they lead to different advice.The Windows-only suites cannot run on macOS, so the real
DarlingManagedPostgresTests.cs,DarlingMcpStoreMetricsToolsTests.csandStoreLogSeverityLocaleTests.cswere compiled into a throwawaynet10.0console against the shipped build via an xunit shim: 11 executed, 0 failed, 0 missing (the harness treats a wanted-but-absent test as a failure, so a rename cannot read as a clean run). Then 11 mutations killed, each confirmed applied by both a byte check and a non-emptygit diff --numstatbefore the build: the GUC put back in v1, the v11 block writingoff, the v1 marker widened to a prefix, a new setting added to v1,Recordingleaking to every non-Offstate, the twoOffnotes collapsed, the description'sdefaults OFFclause dropped, the GUC name retyped into the probe SQL instead of bound, the v11 factory call deleted fromEnsureConfAppended, and the ungated pin reverted to the substring instrument (which reproduces CI'sExpected: 1 Actual: 2locally — the category, closed).The first correction there is mine: deleting the v11 arm looked like it survived until
StoreLogSeverityLocaleTestswas pulled into the harness. The pre-existingEveryDeclaredConfMarkerIsWrittenByAFactoryEnsureConfAppendedCallsparsesEnsureConfAppended's body and requires the factory writing each declared marker to be called from it, so it kills that mutation, ungated. The wiring was already guarded and the harness was under-claiming its own scope.Two measured survivors, reported because they scope the claim rather than weaken it. Inverting the v11 gate to
if (conf.Contains(...))survives every ungated pin — the body still names the factory — and that is the exact shape of the original defect, a gate that skips when it should append. It is covered only byExistingStore_GainsJobExecutionLogging_OnNextStart_Gated, which needs a real cluster and therefore only runs in the Windowsbuildjob (DARLING_TEST_PGRUNTIMEis set there, and the run above proves the gate is live rather than skipping). And removing the stock commented decoys from the ungated fixture also survives — correctly: with an assignment-counting instrument the decoys change no answer. They are a trap for a future wrong instrument, which is what the pair of mutations measures.The CI path itself was rehearsed locally against a real cluster, so the fix was not shipped on reasoning: a store-like database created
TEMPLATE template0on a TimescaleDB cluster starts at zeropg_settingsrows for the GUC,LiveTimescaleProbe.TryEnableAsyncreturns true, the count goes to one, and the read then returns exactly what the gated E2E asserts —setting = on,source = configuration file,boot_val = off,context = sighup. The only assertion that cannot be rehearsed off Windows is thesourcefilepath comparison.All four states of the new read were measured against real servers, not one measured and three reasoned about — the shipped
GetJobExecutionLoggingAsyncand the shippedJobHistoryNotedriven from anet10.0harness against three throwaway containers: TimescaleDB with the GUC on (On,recording=true,source = configuration file), TimescaleDB with it unset (Off,source = default,overridden=false), plain PostgreSQL 17 (NotRegistered, every field null), and TimescaleDB with anALTER SYSTEM ... = off(Off,source = configuration file,overridden=true, and the separate note) — plus the second route intoNotRegistered, a TimescaleDB cluster queried from a database with no extension. CI can only show one of those on one cluster.The gated test rewinds a provisioned conf to the exact field shape — v1 marker present, no v11 block, and no assignment of the GUC anywhere, an absence rather than an
off— then takes the reading together withboot_val,sourceandsourcefilein one row, becauseonon its own would be worthless:boot_val = 'off'is what proves the observedonis not the value this server would have served regardless.Review findings, both applied: a dangling
<c>reference to a test name that does not exist, and the broadcatchnow lettingOperationCanceledExceptionthrough rather than reporting a cancellation as an unreadable measurement. One more came out of measuring rather than review:pg_settings.sourcefileis superuser-only, so the MCP's least-privilege role gets a null there — verified with a plain LOGIN role, wheresetting,source,boot_valandcontextall came back andsourcefiledid not.OffByExplicitOverridekeys onsourcefor that reason, and the note and the description no longer promise the filename unconditionally.One review finding deliberately not taken, and it is a file-ownership call rather than a disagreement.
TimescaleSupport.cs:1822-1824still attributes the GUC toBuildConfAppend, which this PR makes stale. Two open PRs are rewriting that file (#3178 at +528/-229, #3168 at +152/-70) and the sentence sits insideHeaviestHourlyRefreshObservedCeilingSeconds' doc comment — the constant #3168 exists to re-derive — so a one-line fix from here conflicts with both and gets rewritten anyway. Handed over with the replacement text, whose second clause matters more than the builder name: #3168's census premise is scoped to a store where this GUC is on. On any store initdb'd before the boundary the same query returns nothing, and nothing distinguishes that from a clean result — so a ceiling re-derived fromjob_historyis a statement about one store until this PR has been deployed and logging has accumulated.The heal is confirmed against a real cluster carrying the v1 marker.
Darling PostgreSQL testson the final commit: 4m28s,Total: 8161, Errors: 0, Failed: 0, Skipped: 6, Not Run: 0, Time: 94.412s(and the same shape at 4m36s on5bcda8a0), withDarling Linux build1m51s,review,verify,check-branchesanddescription-driftall green. The step list is a real run rather than a fast-path no-op —Initialize and start throwaway PostgreSQLandRun Darling PG testsboth success, the pg-runtime build skipped on a cache hit, the failure-artifact upload skipped. And the six SKIPs are enumerated in the log and none of them is new here: twoDarlingStoreUpgradeTestsupgrade E2Es, threeDarlingCollectorRunnerTestslive-SQL-Server E2Es (DARLING_TEST_SQLis intentionally unset) and oneNpgsqlRootCertificateValidationTestsplatform case. SoExistingStore_GainsJobExecutionLogging_OnNextStart_Gatedand the live probe test both executed — the total is unchanged from the rounds where the gated test failed, which is what rules out "it stopped running".All eight checks are green on the final commit (
6abdff6):build8m45s —Lite.Tests Total: 3510, Failed: 0, Skipped: 0, Not Run: 0andDarling.Tests Total: 8161, Failed: 0, Skipped: 331, Not Run: 0(331 skips there rather than 6, because that job leavesDARLING_TEST_PGandDARLING_TEST_PGRUNTIMEunset — a strict subset of the suite the PG job ran) — plusDarling PostgreSQL tests4m28s,Darling Linux build1m49s,Darling whole-tree guards20s,verify2m41s,review2m9s,check-branches3s,description-drift14s. Worth recording thatbuildwas cancelled in two earlier rounds by the next push landing mid-run: acancelledconclusion with a plausible five-to-six-minute duration reads like a result and is not one.One review finding deliberately not taken, and it is a file-ownership call rather than a disagreement.
TimescaleSupport.cs:1822-1824still attributes the GUC toBuildConfAppend, which this PR makes stale. Two open PRs are rewriting that file (#3178 at +528/-229, #3168 at +152/-70) and the sentence sits insideHeaviestHourlyRefreshObservedCeilingSeconds' doc comment — the constant #3168 exists to re-derive — so a one-line fix from here conflicts with both and gets rewritten anyway. Handed over with the replacement text, whose second clause matters more than the builder name: #3168's census premise is scoped to a store where this GUC is on. On any store initdb'd before the boundary the same query returns nothing, and nothing distinguishes that from a clean result — so a ceiling re-derived fromjob_historyis a statement about one store until this PR has been deployed and logging has accumulated.The heal is confirmed against a real cluster carrying the v1 marker.
Darling PostgreSQL testson the final commit: 4m28s,Total: 8161, Errors: 0, Failed: 0, Skipped: 6, Not Run: 0, Time: 94.412s(and the same shape at 4m36s on5bcda8a0), withDarling Linux build1m51s,review,verify,check-branchesanddescription-driftall green. The step list is a real run rather than a fast-path no-op —Initialize and start throwaway PostgreSQLandRun Darling PG testsboth success, the pg-runtime build skipped on a cache hit, the failure-artifact upload skipped. And the six SKIPs are enumerated in the log and none of them is new here: twoDarlingStoreUpgradeTestsupgrade E2Es, threeDarlingCollectorRunnerTestslive-SQL-Server E2Es (DARLING_TEST_SQLis intentionally unset) and oneNpgsqlRootCertificateValidationTestsplatform case. SoExistingStore_GainsJobExecutionLogging_OnNextStart_Gatedand the live probe test both executed — the total is unchanged from the rounds where the gated test failed, which is what rules out "it stopped running".The Windows
buildjob was cancelled twice by the next push landing while it ran, andcancelledis notpassed— a plausible five-to-six-minute duration that reads like a result and is not one. Its partial log is worth more than its conclusion, though: on5bcda8a0it got as far asLite.Tests Total: 3508, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0, Time: 287.100sbefore the cancellation landed on a later step. Its remaining step,Run Darling tests, carries noenv:block (verified inbuild.yml), so it runs the sameDarling.Testssuite withDARLING_TEST_PGandDARLING_TEST_PGRUNTIMEunset — a strict subset of whatDarling PostgreSQL testsjust ran green. It is running again on the final commit.Not verified: the heal has not been run against a real pre-existing FIELD store — only against a real cluster rewound to exactly that shape (v1 marker present, no v11 block, no assignment of the GUC anywhere). What a field store adds over that is age and accumulated content, neither of which
EnsureConfAppendedreads. The container measurements are TimescaleDB 2.30.0 / PG 17, not the bundled 2.28.1 / PG 18 — thecontext = 'sighup'claim is re-pinned against the bundled runtime by the gated test, but theshared_preload_librariesclobber and theALTER SYSTEMprecedence are PostgreSQL semantics measured on 17 and not re-measured on 18. No live store was touched; this changes provisioning code, not a running cluster.CHANGELOG entry text (not committed — coordinator appends)
Goes under
### Fixedin[Unreleased], and needs one line added to the link-reference block at the bottom of the file:[#3175]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/3175. The#1681mention is deliberately a bare number, not a reference, so it needs no definition.timescaledb.enable_job_execution_loggingis what makestimescaledb_information.job_historyrecord one row per background-job run, and Self-alert FIRINGS are never logged — only resolutions are, so the log shows 'Recovered' with no preceding event #1681 put it in the v1postgresql.confblock - the one blockEnsureConfAppendedcannot heal, because it skips a block whose marker it finds and v1's marker is present on every cluster that already exists. So the setting only ever reached a fresh initdb, and every older store has hadjob_historyempty the whole time. That is worse than a plainly missing setting: a maximum over an emptyjob_historyreturns zero rows, which reads as "no run exceeded the line" rather than "this instrument is off" - an absence that reads as health. Measured on two field stores running the same binary: one carried all ten markers, no GUC line, an effectiveoffwithsource = defaultand ONE history row for 110 jobs; the other carried the line and 39,020 rows. The setting now has its own v11 marker and is moved out of v1 rather than duplicated, so the repository no longer asserts it in the block that cannot deliver it. Existing clusters gain it on their next service-owned start - the append runs beforepg_ctl start, so it is live on that very start - and there is no history to recover: logging starts then, which the log line states rather than implies. Not widened, and the harm is measured: re-appending the shared v1 block would clobbershared_preload_libraries, which is list-valued and takes the last assignment as a REPLACEMENT - a conf carrying an operator's'timescaledb,pg_stat_statements'came back up serving'timescaledb'alone.get_store_metricsalso now reports that GUC's effective value, source and sourcefile on every response, in four states rather than a bool, because that tool's own description redirects a maximum question tojob_historyand had no way to say whether the route was live - and it reads the effective value rather than the marker because anALTER SYSTEMinpostgresql.auto.confbeats the append, measured. Three new pins close the category rather than the instance: the v1 block's content is frozen (adding a setting there now fails on the commit that types it - the pin whose absence let this through), no marker may be a substring of another (EnsureConfAppendedasks withContains), and a heal must re-apply no v1 setting