Skip to content

Commit 3588941

Browse files
DmitryNFominclaude
andcommitted
Address review_6.md issue #9: drop speculative consumer naming in docs/comments
review_6.md issue #9 flagged that documentation comments over-reference "future ASH/AWR/10046-style background workers" as justification for the cross-backend trace surface. On -hackers this reads as speculative -- naming hypothetical consumers to motivate present code. PG style prefers factual statements about what the code does, with neutral framing for consumers ("any extension that reads via the snapshot pattern"). The framing is also now doubly stale: since review_6 was written we landed pg_get_wait_event_trace (review_6.md issue #1) as the in-tree cross-backend consumer. The "future" tense is wrong as well as speculative. Sweep through wait_event_timing.h, wait_event_timing.c, and monitoring.sgml. In each speculative reference replace the "future ASH/AWR/10046-style background workers" phrasing with neutral framing that: * names the in-tree consumer (pg_get_wait_event_trace) where that is what callers should use today, * refers to "extensions following the snapshot pattern documented on WaitEventTraceControl" for the direct-shared-memory case, * drops the speculative consumer enumeration entirely. References to "10046-style trace" as a descriptive label for the technique (e.g. "Per-session wait event trace ring buffer (10046-style)" on WaitEventTraceState) are retained -- that is a recognised term for the per-session-event-trace profiling style, used here as a descriptor of the design, not as a forward-looking promise about a particular consumer. Files touched: * doc/src/sgml/monitoring.sgml: pg_backend_wait_event_trace cross-backend-tools paragraph, pg_get_wait_event_trace description, and pg_stat_clear_orphaned_wait_event_rings description. * src/include/utils/wait_event_timing.h: slot-lifecycle comment block (3 spots). * src/backend/utils/activity/wait_event_timing.c: wait_event_trace_before_shmem_exit doc, the pg_backend_wait_event_trace SRF doc, the pg_get_wait_event_trace SRF doc, and the pg_stat_clear_orphaned_wait_event_rings doc. No code change. Pure documentation rewrite. Builds clean in both --enable-wait-event-timing and stub configurations. Main regress (249 tests) and test_wait_event_stress TAP (15 subtests) pass on the timing build; main regress (249 tests) passes on the stub build. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent d1553b9 commit 3588941

3 files changed

Lines changed: 55 additions & 41 deletions

File tree

doc/src/sgml/monitoring.sgml

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4128,16 +4128,19 @@ ORDER BY b.bucket_idx;
41284128
</para>
41294129

41304130
<para>
4131-
Cross-backend monitoring tools &mdash; future Active Session
4132-
History, Automatic Workload Repository, or 10046-style
4133-
trace-replay background workers that read wait events losslessly
4134-
from every backend's ring &mdash; should <emphasis>not</emphasis>
4135-
consume through this view. The underlying SQL function returns only the
4136-
calling backend's own ring; a background worker invoking
4131+
Cross-backend monitoring tools &mdash; extensions and background
4132+
workers that read wait events losslessly from every backend's
4133+
ring &mdash; should <emphasis>not</emphasis> consume through this
4134+
view. The in-tree cross-backend reader is
4135+
<function>pg_get_wait_event_trace</function>
4136+
(see <xref linkend="monitoring-stats-funcs"/>); the underlying
4137+
per-session SQL function returns only the calling backend's own
4138+
ring, so a background worker invoking
41374139
<command>SELECT * FROM pg_backend_wait_event_trace</command> via
41384140
SPI would receive only its own (typically empty) ring, not the
4139-
target backend's data. Such tools must instead use the
4140-
shared-memory snapshot pattern documented on
4141+
target backend's data. External tools that need cross-backend
4142+
access without going through SQL use the shared-memory snapshot
4143+
pattern documented on
41414144
<structname>WaitEventTraceControl</structname> in
41424145
<filename>src/include/utils/wait_event_timing.h</filename>:
41434146
snapshot <structfield>trace_slots[procNumber].generation</structfield>,
@@ -4168,9 +4171,9 @@ ORDER BY b.bucket_idx;
41684171
procnumbers. The lifecycle change matters for short-lived
41694172
backends that exit before any monitoring tool has read their
41704173
data: parallel workers in particular exit in milliseconds at
4171-
end-of-parallel-query, well below typical sampling cadences,
4172-
and without orphan-persistence their final waits would be
4173-
lost. Orphaned rings are reclaimed automatically when a new
4174+
end-of-parallel-query, well below typical reader polling
4175+
intervals, and without orphan-persistence their final waits
4176+
would be lost. Orphaned rings are reclaimed automatically when a new
41744177
backend takes over the same <literal>procNumber</literal>
41754178
slot, and the DBA can force a sweep at any time via
41764179
<function>pg_stat_clear_orphaned_wait_event_rings</function>.
@@ -6405,10 +6408,10 @@ ORDER BY b.bucket_idx;
64056408
</para>
64066409
<para>
64076410
This is the canonical cross-backend reader. External
6408-
monitoring background workers (ASH/AWR/10046-style ring
6409-
readers) should follow the same snapshot pattern documented
6410-
on <structname>WaitEventTraceControl</structname> in
6411-
<filename>src/include/utils/wait_event_timing.h</filename>;
6411+
monitoring extensions that need cross-backend access without
6412+
going through SQL should follow the same snapshot pattern
6413+
documented on <structname>WaitEventTraceControl</structname>
6414+
in <filename>src/include/utils/wait_event_timing.h</filename>;
64126415
this function serves as both the reference implementation and
64136416
a DBA-facing diagnostic tool. The
64146417
<parameter>procnumber</parameter> argument can be obtained
@@ -6524,8 +6527,10 @@ SELECT * FROM pg_get_wait_event_trace(
65246527
When a backend that had <varname>wait_event_capture</varname> =
65256528
<literal>trace</literal> exits, its ~4 MB trace ring is
65266529
intentionally <emphasis>not</emphasis> freed at exit so that
6527-
cross-backend monitoring tools (ASH/AWR/10046-style background
6528-
workers) can still read the dying backend's final waits. The
6530+
cross-backend consumers
6531+
(<function>pg_get_wait_event_trace</function> and extensions
6532+
following the snapshot pattern) can still read the dying
6533+
backend's final waits. The
65296534
memory is reclaimed lazily: in the common case, the ring is
65306535
freed automatically when a new backend takes over the same
65316536
<literal>procNumber</literal> slot. This function is the

src/backend/utils/activity/wait_event_timing.c

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1111,12 +1111,14 @@ wait_event_trace_ensure_dsa(void)
11111111
* dsm_backend_shutdown() detaches the DSA.
11121112
*
11131113
* Crucially, we do NOT free the ring here. The ring stays allocated in
1114-
* DSA so that cross-backend consumers (ASH/AWR/10046-style monitoring
1115-
* background workers, future or external) can read the dying backend's
1116-
* final waits -- the original "free at exit" design lost data the
1117-
* instant a worker terminated, which was particularly bad for parallel
1118-
* workers exiting in milliseconds at end-of-parallel-query. See the
1119-
* lifecycle comment on WaitEventTraceControl for the full design
1114+
* DSA so that cross-backend consumers -- the in-tree
1115+
* pg_get_wait_event_trace SRF and any extension following the
1116+
* snapshot pattern documented on WaitEventTraceControl -- can read
1117+
* the dying backend's final waits. The original "free at exit"
1118+
* design lost data the instant a worker terminated, which was
1119+
* particularly bad for parallel workers exiting in milliseconds at
1120+
* end-of-parallel-query. See the lifecycle comment on
1121+
* WaitEventTraceControl for the full design
11201122
* rationale and the bounded-memory cost we accept in exchange.
11211123
*
11221124
* The ORPHANED slot is reclaimed in one of two ways:
@@ -2215,9 +2217,11 @@ pg_stat_get_wait_event_timing(PG_FUNCTION_ARGS)
22152217
* own-session diagnostics from psql.
22162218
*
22172219
* This SRF is NOT the path for cross-backend monitoring tools --
2218-
* cross-backend readers (ASH/AWR/10046-style background workers that
2219-
* consume the per-backend trace rings) should NOT call this function
2220-
* via SPI.
2220+
* cross-backend readers should use pg_get_wait_event_trace for SQL
2221+
* access, or follow the shared-memory snapshot pattern documented
2222+
* on WaitEventTraceControl in wait_event_timing.h to consume the
2223+
* per-backend trace rings directly. They should NOT call this
2224+
* function via SPI.
22212225
* It is hard-coded to return only the calling backend's own ring,
22222226
* so a bgworker calling SELECT * FROM pg_backend_wait_event_trace
22232227
* would get only the bgworker's own (typically empty) ring, not the
@@ -2738,8 +2742,8 @@ emit_wait_event_trace_for_procnumber(int procNumber, ReturnSetInfo *rsinfo)
27382742
* ring allocated in DSA in ORPHANED state, and this function reads it
27392743
* until either a new backend takes over the same procNumber or the
27402744
* DBA calls pg_stat_clear_orphaned_wait_event_rings(). External
2741-
* monitoring background workers (ASH/AWR/10046-style readers) follow
2742-
* the same snapshot pattern documented on WaitEventTraceControl in
2745+
* extensions that need cross-backend access follow the same
2746+
* snapshot pattern documented on WaitEventTraceControl in
27432747
* wait_event_timing.h; this function serves as both the reference
27442748
* implementation and a DBA-facing diagnostic tool.
27452749
*
@@ -3075,8 +3079,10 @@ pg_stat_reset_wait_event_timing_all(PG_FUNCTION_ARGS)
30753079
* Why this exists. When a backend that had wait_event_capture = trace
30763080
* exits, we deliberately do NOT free its ~4 MB trace ring (see the
30773081
* lifecycle discussion on WaitEventTraceControl): the data must remain
3078-
* readable by cross-backend consumers (ASH/AWR/10046-style monitoring
3079-
* background workers), and an exit-time dsa_free would defeat that.
3082+
* readable by cross-backend consumers -- the in-tree
3083+
* pg_get_wait_event_trace SRF and any extension following the
3084+
* snapshot pattern on WaitEventTraceControl -- and an exit-time
3085+
* dsa_free would defeat that.
30803086
* The reclaim instead happens lazily in two places:
30813087
*
30823088
* (a) wait_event_trace_clear_orphan_at_init(): when a new backend

src/include/utils/wait_event_timing.h

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -361,8 +361,10 @@ typedef struct WaitEventTraceState
361361
* backend exits we deliberately do NOT free its ring. Instead we
362362
* transition the slot to ORPHANED and leave the ring allocated in DSA.
363363
* That preserves trace data past backend exit so it remains readable by
364-
* cross-backend consumers (ASH/AWR/10046-style monitoring background
365-
* workers) -- the original per-backend-ring design lost data the
364+
* cross-backend consumers: the in-tree pg_get_wait_event_trace SRF and
365+
* any extension that follows the snapshot pattern documented on
366+
* WaitEventTraceControl below. The original per-backend-ring design
367+
* lost data the
366368
* instant a parallel worker (or any short-lived backend) terminated,
367369
* because the worker's before_shmem_exit callback ran dsa_free before
368370
* any consumer could observe the final waits. See "Slot lifecycle and
@@ -414,8 +416,9 @@ typedef enum WaitEventTraceSlotState
414416
* lifecycle plus admin cleanups).
415417
*
416418
* state is pg_atomic_uint32 only for cheap unlocked "is this slot
417-
* worth visiting" probes (e.g. an ASH reader iterating MaxBackends
418-
* slots and skipping FREE ones without taking the lock). Authoritative
419+
* worth visiting" probes (e.g. an iterating reader that walks all
420+
* MaxBackends slots and skips FREE ones without taking the lock).
421+
* Authoritative
419422
* reads of state-and-ring_ptr together MUST be done under
420423
* WaitEventTraceCtl->lock in LW_SHARED, paired with the
421424
* generation-snapshot retry loop above. Writers always hold the lock
@@ -465,10 +468,10 @@ typedef struct WaitEventTraceSlot
465468
* older design that called dsa_free in the backend's
466469
* before_shmem_exit callback. That older design lost trace data the
467470
* instant a backend exited, because the data was gone before any
468-
* cross-backend reader (e.g. an ASH/AWR-style ring reader) could observe
469-
* it. This was particularly acute for parallel workers, which exit
470-
* in milliseconds at end-of-parallel-query; a sampler running at
471-
* 1 Hz would never see their waits.
471+
* cross-backend reader could observe it. This was particularly
472+
* acute for parallel workers, which exit in milliseconds at
473+
* end-of-parallel-query; a reader polling at 1 Hz would never
474+
* observe their waits before the data was freed.
472475
*
473476
* Persisting the ring past backend exit pays a bounded memory cost:
474477
* up to NUM_WAIT_EVENT_TIMING_SLOTS orphaned rings can simultaneously
@@ -495,9 +498,9 @@ typedef struct WaitEventTraceSlot
495498
* (parallel workers, autovacuum, walsender, all transient backends
496499
* preserve their data), DSA's lazy-allocation property is preserved
497500
* (capture=off pays zero memory), and the cross-backend reader
498-
* pattern below works for the future ASH/AWR/10046 background worker
499-
* with no further plumbing. See review_5.md issue #26 for the
500-
* design discussion.
501+
* pattern below is what pg_get_wait_event_trace uses; extensions
502+
* implementing similar tools follow the same pattern with no further
503+
* plumbing. See review_5.md issue #26 for the design discussion.
501504
*
502505
* External reader pattern (cross-backend consumers)
503506
* -------------------------------------------------

0 commit comments

Comments
 (0)