Repository navigation
The migration advisory-lock wait cannot cover an arbitrary number of data-moving rungs #2894
Description
Summary
#2888 gave the migration advisory-lock acquire its own budget, MigrationLockWaitTimeoutSeconds = 5 * MigrationCommandTimeoutSeconds (1500 s), because the previous value was one statement's bound applied to a whole-session wait. That is correct as far as a constant can go, and it deliberately does not close the failure. This issue records the residual so it is not mistaken for closed.
The residual
MigrateAsync takes the advisory lock ONCE and holds it while MigrateLockedAsync applies every pending rung in the same session. The 1500 s floor is derived from today's rung census: exactly four of the 108 rungs touch pre-existing data, each is one command already capped at MigrationCommandTimeoutSeconds, so four multiples is the floor and the fifth is margin.
Two things still outlast it:
- A fifth data-moving rung. The bound is pinned to a census taken at one point in time. Nothing fails when a new
migrate_data,CREATE INDEX-on-populated-table, whole-tableUPDATEor hypertable conversion rung lands, so the constant silently stops covering the ladder. - One rung genuinely needing longer than its own 300 s bound. Then the holder dies at its statement bound, but a waiter that arrived early enough can still have burned its 1500 s first.
The failure is not graceful. The waiter throws out of PgMigrations.MigrateAsync, DarlingWorker.RunCollectionLoopAsync catches it, logs LogCritical and returns out of the collection loop — that instance then collects nothing until an operator restarts it. There is no retry and no enclosing budget: one production caller, plain stopping token, no CancelAfter, no configured HostOptions.StartupTimeout, no health check or readiness probe, no container HEALTHCHECK, no orchestrator manifest.
The census this rests on, so it is re-derivable
108 rungs (V1-V109, V45 absent). Every rung's SQL body extracted (100 const string, 8 from PgSchemaGenerator, plus V23 which is the file's only private static string V23Sql => property) and scanned for data-moving shapes. Exactly four touch pre-existing data:
| rung | operation | evidence |
|---|---|---|
| V22 | CREATE INDEX on populated collect.index_object_stats |
rung's own doc: "a plain CREATE INDEX is applied across every chunk by Timescale" |
| V23 | create_hypertable(..., migrate_data => true) on collection_log |
the only migrate_data in the file |
| V39 | two partial indexes on query_stats + procedure_stats |
rung's own doc: "propagates the index to existing and future chunks" |
| V104 | CREATE INDEX on populated collect.pg_deadlocks |
index-only rung; table created in V103 |
Everything else creates the table it then indexes, or is metadata-only (ADD COLUMN ... DEFAULT <constant>, ALTER ... SET SCHEMA, DROP NOT NULL, CREATE OR REPLACE VIEW). Measured on a local PostgreSQL 17.11 / TimescaleDB 2.29.2 rig: the whole 108-rung ladder runs in 0.301 s end to end on an empty store, advisory lock and version stamps included. Seeded and warm, the four are the only ones with any cost: V22 1.287 s over 907 MB / 90 chunks, V23 9.372 s over a 608 MB heap, V39 1.439 s over 1.26 GB. For scale, the largest live store carries 2.72 GB, 0.69 GB and 24.5 GB in those same tables across 42 monitored servers.
Note this also corrects MigrationCommandTimeoutSeconds' own framing, which singles out V23 as the data-moving rung. V22, V39 and V104 build btrees over already-populated hypertables, and V39's two targets are the store's largest fact tables.
Suggested shapes — deliberately NOT built in #2888
Any of these is a bigger change than a constant, which is why #2888 stopped at the smaller correct fix:
- Scale the wait with pending rung count. Read
MAX(version)before taking the lock and size the wait from the number of rungs that would actually apply. Makes the bound track the ladder instead of a snapshot of it, at the cost of a read outside the lock. pg_try_advisory_lockin a diagnostic retry loop. Poll instead of blocking, so a long wait produces periodic "still waiting on a sibling migrator, N s elapsed" log lines rather than one silent stall and then aTimeoutExceptionthat reads as a network fault. Directly addresses the 133 NpgsqlCommand sites still inherit Npgsql's undocumented 30s default timeout #2874 misdiagnosis shape: the current failure surfaces asException while reading from stream.- A rung-census pin. Assert the count of data-moving rungs matches the number the constant is derived from, so adding a fifth one fails the build and forces whoever adds it back to this constant. This is the cheapest of the three and is worth doing regardless of which mechanism wins.
Not urgent
No production instance has hit this. The 1500 s bound covers the ladder as it stands today with one full statement-bound of margin. This is filed so the margin is a recorded decision rather than an assumption, and so option 3 gets built before the margin is quietly spent.
Activity
Merge pull request #2920 from erikdarlingdata/fix/2894-rung-census-pin
erikdarlingdata commentedon Sep 4, 2026
The census pin landed (PR #2920). This issue stays open: it closes one residual, not both.
Merged as
Part of #2894deliberately. The pin makes a fifth data-moving rung impossible to add silently, which is residual 1. Residual 2 is untouched — nothing bounds a single rung that overruns its ownMigrationCommandTimeoutSeconds, and no pin can fix that.The census in this issue was wrong: SIX rungs touch pre-existing data, not four
Re-derived on current
dev(109 rungs, V1–V110):CREATE INDEXcollect.index_object_statscreate_hypertable+migrate_data => truecollect.collection_logCREATE INDEX×2query_stats,procedure_statsADD CONSTRAINT … CHECKconfig.config_serviceDELETE FROM×2collect.collector_stateCREATE INDEXcollect.pg_deadlocksV62's
CHECKvalidates every existing row and V77'sDELETEs are real DML, so both genuinely touch pre-existing data — "exactly four" was too strong. Neither moves the5×: V62's target is a single-row control-plane table and V77's is a few rows per server per collector. Both are now declared at zero cost rather than left to be rediscovered.V110 is not data-moving — verified rather than assumed: ten
ADD COLUMN IF NOT EXISTS … integer, nullable, noDEFAULT, plus aCREATE OR REPLACE VIEW. Catalog-only.Also confirmed absent from the entire ladder, each grep with a positive control through the identical form:
ALTER … TYPE,SET NOT NULL,ADD PRIMARY KEY/UNIQUE,ADD COLUMN … DEFAULT fn(),INSERT INTO … SELECT,refresh_continuous_aggregate,VACUUM/CLUSTER/REINDEX, and dynamic SQL.A premise in #2888's
5×derivation was wrong — the conclusion survives, the reasoning didn'tThat derivation (and my restatement of it) said each data-moving rung "is ONE command, so
MigrationCommandTimeoutSecondsalready caps it."MigrationCommandTimeoutSecondsis a per-RUNG bound, not per-statement: the applier issuesnew NpgsqlCommand(migration.Sql, connection, transaction) { CommandTimeout = … }— one command carrying the rung's entire SQL. V39 contains twoCREATE INDEXstatements, so "each is a single command" is true only because the unit is the rung.That distinction is load-bearing. Under a per-statement reading the floor would be 5, not 4, and the
5×multiple would have no margin at all. The value holds; the sentence justifying it was wrong, and it is corrected in the doc comment.The pin
Darling/Darling.Tests/MigrationDataMovingRungCensusPins.cs, 27 cases. It scans the realPgMigrations.Scriptsstrings — so the ten generator-assembled rungs are covered — for 14 data-moving shapes, and asserts the matching rung set equals a declared register carrying each rung's reason and aSetsTheFloorflag. A second[Fact]reads the constant out of source and asserts its multiple equals floor-setting-count + 1, so the derived form is pinned rather than the compiled integer — a future= 1500literal fails.Distinguishing a free
CREATE INDEXfrom an expensive one was the real work, and it matters because a pin that flags all of them gets switched off within a week. Every targeted shape resolves its statement's target table and is dropped when that same rung created it: 130 of the ladder's 134CREATE INDEXstatements are same-rung. Table identity uses the bare name, because V1 creates the collector tables unqualified inpublicand V8 moves them tocollect— 37 bare names exist under both spellings.Proven red twelve ways, one mutation at a time, each with its anchor count asserted first and a byte-identical restore after. Two worth highlighting:
INSERT INTO … SELECTregex fails only the shape-theory test — the census itself stays green. That is precisely why the shape theory exists as its own assertion: it catches a scanner that has stopped scanning.Review caught a real silent miss and it is fixed.
CREATE INDEX ON t (a)is valid PostgreSQL (auto-named), and the original pattern consumedONas the index name, then found no secondON— producing no match at all rather than a wrong match.ALTER TABLE ONLY tlikewise reportedONLYas the target. Both corrected; neither moved the ladder's numbers (still 134/130, same six rungs).Existing pins checked rather than assumed.
CollectionLogDrainForensicsStoreTestsslicesPgMigrations.csfromIndexOf("2864"); after this edit the anchor still resolves, the edited doc comment sits inside the slice, bothDoesNotContainassertions hold, and a planted-phrase control confirms they can still fail. All 109 rung SQL strings were dumped before and after and are byte-identical.Not verified
No live store — this change adds no SQL and no runtime path. The four floor-setting rungs' timings are this issue's own, quoted rather than re-measured.
Darling.Testscannot run on macOS; the 27 cases were executed in a throwaway host compiling the real source file, and CI is the arbiter.Merge pull request #2935 from erikdarlingdata/fix/2894-rung-overrun-b…
Clear the fleet-identifier scrub, credit the polling waiter to #2935,…
Part of #2894 (#2975)
erikdarlingdata commentedon Sep 5, 2026
Closed by #2975 (
2bd7ad2d). Closing by hand — this repository merges todev, so a closing keyword never fires.The answer is "the bound binds, and the residual is a rung shape rather than a missing mechanism" — measured against a real ladder (109 rungs, PostgreSQL 17.11 / TimescaleDB 2.29.2) with the targets seeded to 13.5M rows / 90 chunks each, ~14.7 GB, and
maintenance_work_memsqueezed to force external sorts. Unbounded, the four data-moving rungs run 4.28–25.61 s. Applied through the applier's exact command shape at a scaled budget, every one aborts within ~50 ms of it, the backend is gone inside 0.4 s, and the client connection stays usable. At the real 300 s, V22 parked behind a peer'sRowExclusiveLockgave up at 300.06 s with nothing applied and nothing stamped.But it can exceed 300 s, by a mechanism this issue did not name.
CommandTimeoutis a socket read timeout, and every backend message restarts it — so the bound is 300 s of backend silence, not of work. Matched pair, same statement and same 40 s of work against a 5 s budget: the chatty version (oneRAISE NOTICE/s) ran to completion at 40.18 s with the budget never firing; the silent version aborted at 5.02 s. That also accounts for all of V23's overshoot — exactly 3.00 s at two different budgets, equal to its lastcreate_hypertablemessage. Today's ladder is quiet (zeroLOOPs; V23's is the onlyRAISE, in a handler that fires only on failure), so V23's ceiling is 303 s and the rest 300.05 s.The genuine defect found on the way, now pinned:
EXCEPTION WHEN OTHERSdoes not catch a cancel, butWHEN query_canceleddoes — and then the applier sees SUCCESS, commits, and stamps the version while the rung's work never happened (probe table present, zero rows). That destroys the "a stamp of N proves rung N committed" property #2935's expiry split reads. A rung can no longer trap its own cancellation.Three alternatives declined, with measurements rather than argument. A watchdog buys nothing:
pg_cancel_backendstopped the same four rungs at 2.01–2.03 s andpg_terminate_backendat 2.01–2.05 s, against the applier's own 2.04–2.07 s — all three rideCHECK_FOR_INTERRUPTS, and terminate is worse, because its57P01is retryable toStartupFailureTriage. Splitting rungs helps one of four (three are single statements) and raises the floor from 4 to 5 multiples. Server-side timeouts are unchanged from #2935.Two corrections to this issue's own text
NpgsqlException <- TimeoutExceptionpassesIsRetryable, but 300 s already exceedsStartupFailureTriage.RetryBudget(120 s), so it lands on the sameLogCritical-and-return arm the waiter does at 1500 s. Declining is correct — the schema genuinely cannot reach the build — but it is permanent rather than a delay.Also corrected
#2935's shipped doc comment said "so the holder stays unbounded." That was too pessimistic and is replaced by the measurement above.
Not verified
No production store, and no SQL or runtime path was added — the change is doc comments plus one pin. Local NVMe, so the rung durations are floors. A phase that never reaches an interrupt check could not be produced on this rig: the measurement shows no phase of these four rungs is one, which is evidence about reachability rather than a proof that none exists.
Record #2975's three entries, and correct a claim its measurements di…
Pin the rung census MigrationLockWaitTimeoutSeconds is derived from