Attached is v5, which takes Tom's suggestion for a spin.
0002 adds asserts at the top of vacuum_delay_point(). I picked separate asserts
instead of INTERRUPTS_CAN_BE_PROCESSED(), just like Neil.
Running check-world with that assert tripped on hashbucketcleanup().
Since hashbucketcleanup is called assuming a lock is in place, no place in it
is safe. Instead, I moved the delay to the per-bucket loop. It's a switch from
a per-page delay to a per-bucket delay, but the per-page delay we're removing
wasn't valid anyway.
Andrey, let me know if you have concerns about lock chaining/cleanup with this
approach. There shouldn't be any issues since we're outside the locked function
entirely.
With both patches applied, check-world passes with assertions enabled, and each
patch passes on its own.
- Kevin Rocker
From 647b8b6c6689de7e500e7a15dc8bfeba86100644 Mon Sep 17 00:00:00 2001
From: Kevin Rocker <[email protected]>
Date: Wed, 12 Aug 2026 15:58:04 +0200
Subject: [PATCH v5 1/2] Move interrupt checks out of locked regions
vacuum_delay_point() and CHECK_FOR_INTERRUPTS() cannot process pending
interrupts while interrupts are held. A vacuum delay point may additionally
sleep while retaining a buffer content lock. Several call sites make these
calls while a lock is held.
Move the ANALYZE delay point before scan_analyze_next_block(). Move the
first GIN pending-list cleanup delay point before its locks are acquired;
an existing delay point already covers transitions between pages.
Move the hash bucket cleanup delay point from hashbucketcleanup() up to
hashbulkdelete()'s per-bucket loop, before the bucket's cleanup lock is
acquired. hashbucketcleanup() is called assuming a lock exists for
it's duration, so no part of it is a valid call site. Its other callers,
the split-cleanup paths called from insertion, lose the call entirely.
A backend running INSERT doesn't do vaccuum cost accounting and there's
an active lock, so the call couldn't sleep there anyway.
dshash sequential iteration returns each stats entry with its partition lock
held and provides no unlocked per-entry boundary, so mark that call with a
grep-friendly comment instead.
Author: Kevin Rocker <[email protected]>
Author: Andrey Borodin <[email protected]>
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
---
src/backend/access/gin/ginfast.c | 5 +++--
src/backend/access/hash/hash.c | 5 +++--
src/backend/commands/analyze.c | 5 ++++-
src/backend/utils/activity/pgstat.c | 4 ++++
4 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index f50848eb65a..174610b455a 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -797,6 +797,9 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
bool fsm_vac = false;
int workMemory;
+ /* Delay or accept interrupts before acquiring the pending-list locks. */
+ vacuum_delay_point(false);
+
/*
* We would like to prevent concurrent cleanup process. For that we will
* lock metapage in exclusive mode using LockPage() call. Nobody other
@@ -892,8 +895,6 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
*/
processPendingPage(&accum, &datums, page, FirstOffsetNumber);
- vacuum_delay_point(false);
-
/*
* Is it time to flush memory to disk? Flush if we are at the end of
* the pending list, or if we have a full row and memory is getting
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 8d8cd30dc38..1735f85fcc9 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -560,6 +560,9 @@ bucket_loop:
Page page;
bool split_cleanup = false;
+ /* Delay or accept interrupts before locking the next bucket. */
+ vacuum_delay_point(false);
+
/* Get address of bucket's start page */
bucket_blkno = BUCKET_TO_BLKNO(cachedmetap, cur_bucket);
@@ -797,8 +800,6 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf,
bool retain_pin = false;
bool clear_dead_marking = false;
- vacuum_delay_point(false);
-
page = BufferGetPage(buf);
opaque = HashPageGetOpaque(page);
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index f66e80b757c..82f6c45e922 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1310,10 +1310,13 @@ acquire_sample_rows(Relation onerel, int elevel,
0);
/* Outer loop over blocks to sample */
- while (table_scan_analyze_next_block(scan, stream))
+ for (;;)
{
vacuum_delay_point(true);
+ if (!table_scan_analyze_next_block(scan, stream))
+ break;
+
while (table_scan_analyze_next_tuple(scan, &liverows, &deadrows, slot))
{
/*
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 50cd07822b4..916089a3c2f 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1729,6 +1729,10 @@ pgstat_write_statsfile(void)
PgStatShared_Common *shstats;
const PgStat_KindInfo *kind_info = NULL;
+ /*
+ * CHECK_FOR_INTERRUPTS_WITH_INTERRUPTS_HELD: dshash_seq_next()
+ * returns with the current hash partition lock still held.
+ */
CHECK_FOR_INTERRUPTS();
/*
--
2.54.0
From eb8bd08dd65c57c46aeb745e962a3f53fc766ad8 Mon Sep 17 00:00:00 2001
From: Kevin Rocker <[email protected]>
Date: Tue, 11 Aug 2026 22:45:12 +0200
Subject: [PATCH v5 2/2] Assert that vacuum_delay_point() is called only when
interruptible.
A delay point may sleep and is expected to service query cancel, so it
must not be reached while interrupts are held off, e.g. with an LWLock
or buffer content lock held. Enforce that in assert-enabled builds so
new call sites in locked regions trip the buildfarm rather than
silently delaying with interrupts held.
Per suggestion from Tom Lane.
Author: Kevin Rocker <[email protected]>
Author: Neil Chen <[email protected]>
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
---
src/backend/commands/vacuum.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 38539a6fd3d..7c81ffd6b56 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2439,6 +2439,13 @@ vacuum_delay_point(bool is_analyze)
{
double msec = 0;
+ /*
+ * A delay point may sleep and must service query cancel, so it cannot be
+ * reached while interrupts are held off (LWLock or buffer lock held).
+ */
+ Assert(InterruptHoldoffCount == 0);
+ Assert(CritSectionCount == 0);
+
/* Always check for interrupts */
CHECK_FOR_INTERRUPTS();
--
2.54.0
