The lock-wait fact counts each logged line once and each lock wait once - #4886
Merged
Merged
Conversation
erikdarlingdata
marked this pull request as ready for review
October 1, 2026 00:37
…d make the two-waits pin depend on the time bound
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
PgTargetLockWaitEventsSql, the PostgreSQL target analysis's lock-wait fact, counted RAWcollect.pg_log_eventsrows. It computedstill_waitingasCOUNT(*) FILTER (WHERE message LIKE 'process % still waiting for %'), documented as "one per wait", along withlines, the max/sum of "after N ms", and the top relation and fingerprint. Two things inflate those numbers.pg_log_eventshas no unique key, and only a non-unique(server_id, collection_time)index.DarlingPgLogEventReaderdedupes withDISTINCT ON (raw_line_hash).ProcSleep(src/backend/storage/lmgr/proc.c,REL_18_STABLE) logsprocess %d still waiting for %s on %s after %ld.%03d mson every latch wakeup while the backend still waits, once the first deadlock check has run.The same store had no
lock_waitrows in that week, so there is no field number for the inflation. The fix rests on the code reading and the measured duplicate rate.What changed
Only
PgTargetLockWaitEventsSqlchanges. Its output columns, their order, thelookedCTE and thecollection_timewindow are all unchanged: "an event is in the window when it was COLLECTED in it".Each stored line counts once, in the window of its first sighting.
in_windowkeeps one row per distinctraw_line_hashcollected in the window.first_seendrops a line that an earlier pass already stored. A line is never collected before it occurred, so an earlier sighting can only lie in[occurred_at, $2). That range is bounded by the line's own time, with no fixed lookback, so it's exact however late the re-sighting comes. A NULLoccurred_atsearches all history.Each lock wait counts once, at the line that opens it.
waitskeeps a "still waiting" line only when no EARLIER line of the same wait is stored. An earlier line of the same wait means:occurred_atwithin[L.occurred_at − (N + 1 s), L.occurred_at];A backend waits on one lock at a time, so the probe is bounded by the line's own N. The 1 s pad covers whole-second
%tstamps. Two separate waits by one backend count twice. A line with no parsable lock text, duration or time counts as its own wait.What the columns mean now:
still_waiting(the fact'swait_events, and sowait_events_per_hour) is the count of WAITS, as the fact's metadata always described it;acquired,deadlocks,lines,max_wait_ms,acquired_wait_msandlast_event_atare over the distinct lines first sighted in the window;top_relationandtop_fingerprintweigh each wait once, at its opener.Residuals, stated in the constant's doc:
DarlingPgLogEventReadermakes the same assumption, and both are left as they are.Wording fixed in this PR, since each claimed one "still waiting" line per wait, which is wrong for PostgreSQL 18:
wait_events/wait_events_per_hourmetadata-key docs (PgTargetScorer.Blocking.cs);PgTargetAdvice.Blocking.cs). It now reads "…writes a 'process N still waiting…' line when a lock wait outlives deadlock_timeout (1 s by default), and may write it again while the wait continues…".Every reader of
pg_log_events, checkedChanged:
Darling/PerformanceMonitor.Darling.Analysis/PgTargetFactCollector.Blocking.csPgTargetLockWaitEventsSql. It is the only aggregate over raw rows.Already count each line once (they read deduped rows):
DarlingPgLogEventReader.EventsSql(DISTINCT ON (raw_line_hash), earliest sighting;window_totalis counted after the distinct, andtimes_seencounts sightings by design);DarlingPgLogEventReader.AutovacuumRunsSql, with the same dedupe;get_pg_log_events(DarlingMcpPgLogEventTools,total_events = WindowTotal);get_pg_autovacuum_health's recent runs and sums (computed in C# over the deduped rows);ViewerDataService.Postgres,ViewerServerTab.Postgres);DarlingWebEndpoints, which call the MCP tool) andserver-tabs.js, which renders the deduped payload with no client-side count.Duplicates can't change the answer:
EXISTSschema probes inViewerDataService;DarlingCollectorRunner'sMAX(...)watermarks;PgTargetScorer.BlockingandPgTargetAdvice.Blocking, which grade fact metadata and contain no SQL;DarlingPgLoggingAudit,PgTargetToolRecommendationsandServerHealthBands, which are text or name lists only;PgDeadlockRemask, which doesn't read this table.Writers and ingest (no schema or writer change here):
PgLogEventsCollector(it refuses rows without a hash),RdsLogEventIngestor,RdsResumeStore, theDarlingWorkerandDarlingCollectorRunnerroutes, retention, and the DDL.Pins
Live pins in
PgTargetBlockingTests. Each runs the constant against seeded rows with fixed timestamps. Each FAILS on dev and passes here.ARepeatedLineHash_InsideTheWindow_CountsOnce: one line stored twice in the window → 1 wait, 1 line.ALine_FirstStoredBeforeTheWindow_AndRestoredInside_IsNotCounted→ 0 / 0.ALine_ReSightedThreeHoursLater_IsNotCounted_WhereAFixedTwoHourLookbackWould→ 0 / 0. This is what tells the exact rule apart from a fixed lookback.OutcomeLines_StoredTwice_CountOnce:acquired1,acquired_wait_ms30,000 once, deadlocks 1.TwoLinesOfOneWait_CountOneWait: "after 1000 ms" and "after 5000 ms" → 1 wait, top-relation weight 1, max 5,000 ms.AWaitSplitAcrossTwoWindows_IsCountedInTheWindowOfItsOpeningLineOnly→ 0 in the later window, 1 in the earlier.TwoSeparateWaitsByOnePid_OnTheSameLock_CountTwice_EachLineStoredTwice, with and without anacquiredline between them → 2.Unchanged: the existing end-to-end pin still reads 6 / 6 / 12, 65,000.5 ms, 1.5 per hour and a top-relation weight of 6.
Source pins on the new SQL:
DISTINCT ON (e.raw_line_hash),e.collection_time AS first_collected, theoccurred_at-bounded first-sighting probe,p.collection_time < $2,p.collection_time <= l.first_collected,* INTERVAL '1 millisecond'and(SELECT COUNT(*) FROM waits).Verification
A clean
--no-incrementalRelease build: 0 warnings and 0 errors.In-process on macOS, against a migrated local TimescaleDB 2.30.1 / PostgreSQL 18.6 store:
PgTargetBlockingTestsPgTargetFactCollectorTestsFactCollectorCommandTimeoutTestsDocCommentHygieneTestsLiveCleanupConversionRatchetTestsMcpPayloadContractCensusTestsStorageCommandTimeoutTestsRepoFileAdoptionTestsCommentFilterAdoptionTestsRED on dev: each new live pin was run against
origin/dev's SQL in a detached worktree, and each failed there.Plan cost (
EXPLAIN (ANALYZE, BUFFERS), local store):lock_waitrows for one server, 1,801 of them "still waiting" lines inside the one-hour window. That's an extreme contention rate: the production fleet measured above logged none in a week.idx_pg_log_events_timeindex range per candidate line:collection_timefromoccurred_at − (N + 1 s)to the line's own first sighting. That's 1,801 index searches, about 5 rows filtered per probe, and 5.7k shared-buffer hits.CHANGELOG
None:
collect.pg_log_events(V129) and the lock-wait fact are new in 3.9 (v3.8.0 is schema 125), so no released version counted a wait twice. The 3.9 entry for #3691 carries the change:PG_LOCK_WAIT_EVENTScounts each wait once, even when PostgreSQL 18 logs it again while it goes on, and each line once, even when the log is read again.Findings: 1 substantial (this one, fixed here); 3 polish fixed in this PR (the three wording claims); 0 dropped.