Greg, I think this was meant for the list since it opens by greeting
everyone, so I've added pgsql-hackers back and quoted your message in
full.
> Hi Kevin, Andrey, Neil, Tom,
>
> I tested this on my aarch64 Windows/MSVC buildfarm animal (unicorn,
> --enable-cassert, injection_points) which has been crashing repeatedly
> in tests and it fixed the issue. I'd just yesterday started a thread
> [1] on this, but I'll shut that one down and point here instead as I
> think you've got it nailed.
>
> I think this raises the priority of the ANALYZE hunk in v7-0001, and
> argues for landing it ASAP.
>
> In my logs on that animal I find:
>
> TRAP: failed Assert("entry->data.lockmode == BUFFER_LOCK_UNLOCK"),
> bufmgr.c, in BufferLockAcquire (Windows exception 0xC0000409)
>
> And when I reviewed the crash dumps all 10 had the identical stack.
> Reading it outermost to innermost (nearest-symbol mislabels from
> optimized codegen noted):
>
> AutoVacWorkerMain
> -> vacuum -> analyze_rel -> acquire_sample_rows
> -> [heapam_scan_analyze_next_block holds BUFFER_LOCK_SHARE on a
> pg_class page and returns with it held]
> -> vacuum_delay_point(true) <-- analyze.c, delay point
> -> ProcessConfigFile(PGC_SIGHUP) (a config reload landed)
> -> ... -> check_default_text_search_config (GUC check hook)
> -> get_ts_config_oid -> LookupExplicitNamespace
> -> SearchSysCache -> table_open -> relation_open
> -> LockRelationOid -> AcceptInvalidationMessages
> -> RelationCacheInvalidate (rebuild a nailed catalog entry)
> -> systable_getnext -> heapgettup_pagemode
> -> heap_prepare_pagescan -> LockBuffer(BUFFER_LOCK_SHARE)
> -> Assert (second SHARE lock on the pg_class buffer that
> acquire_sample_rows already holds SHARE on)
>
> So this looks to be the same "vacuum_delay_point() reached with a buffer
> content lock held" bug. The ANALYZE call site that v7-0001 moves before
> scan_analyze_next_block(). The extra wrinkle on the ANALYZE path is that
> the delay point can run a SIGHUP config reload, whose GUC check hook
> does a catalog lookup that triggers a relcache rebuild, and that
> rebuild's pagemode pg_class scan re-locks the very buffer the sampling
> scan is still holding SHARE on.
>
> The reason this is more than a cancellation-latency issue on v19 is the
> buffer content-lock rewrite in fcb9c977aa5 tracks only one lock per
> buffer per backend (the single data.lockmode field). A second SHARE
> acquire on an already-share-locked buffer is now:
>
> - a hard Assert/crash in cassert builds (what unicorn shows), and
> - in a non-assert build, an asymmetric leak: BufferLockAttempt() adds
> a second BM_LOCK_VAL_SHARED to the shared state, data.lockmode
> records only one, and release subtracts one -- so that pg_class
> buffer is left permanently one shared-locker too high and can never
> again be locked exclusive. Any later exclusive waiter (VACUUM) on
> that buffer blocks for the life of the cluster.
>
> Pre-v19 the double SHARE was harmless (the held-lwlocks array could
> represent it), which is presumably why the call site survived so long.
>
> The steps to reproduce this are ordinary, an autovacuum ANALYZE of a
> catalog (pg_class here, reached via the text-search-config GUC hook's
> syscache lookup) that overlaps a config reload. My animal happens to
> build with injection_points, but nothing in this stack is an injection
> point, it is the stock ANALYZE -> vacuum_delay_point ->
> ProcessConfigFile -> relcache-rebuild path, so I don't believe
> injection_points is required to hit it.
>
> I have not seen the crash on non-cassert animals because there it silently
> leaks the lock rather than asserting, which is arguably worse.
>
> Given that v7-0001 already contains the fix (thank you), my only ask is
> that the ANALYZE hunk be treated as a v19 crash-regression fix rather than
> a latency improvement, and backpatched to REL_19 before GA. Happy to test
> again on the aarch64/MSVC animal.
>
> best.
>
> -greg
>
> [1] https://postgr.es/m/arKVu9wp5A7EdKkx@floki
I've attached v8 of the patch. The code is the same, but I extracted the
ANALYZE fix to 0001 so it can be backpatched separately.
This is a v19 regression from fcb9c977aa5, so I think it needs an open
item. I don't have wiki edit access yet, so could someone from the RMT
(Cc'd) add it, with Andres as owner?
- Kevin RockerFrom 87efed93b6978443303372cf7b51b069e53c5b8b Mon Sep 17 00:00:00 2001
From: Kevin Rocker <[email protected]>
Date: Mon, 28 Sep 2026 02:32:16 +0200
Subject: [PATCH v8 1/3] Don't call vacuum_delay_point() with a buffer lock
held in ANALYZE.
vacuum_delay_point() and CHECK_FOR_INTERRUPTS() cannot process pending
interrupts while interrupts are held off. A vacuum delay point may
additionally sleep while retaining a buffer content lock.
Since commit fcb9c977aa5, ANALYZE reaching a delay point while holding
a buffer content lock can crash assert-enabled builds and leave the
page permanently blocked for writers in production builds.
Move the ANALYZE delay point before scan_analyze_next_block().
Reported-by: Greg Burd <[email protected]>
Author: Kevin Rocker <[email protected]>
Author: Andrey Borodin <[email protected]>
Reviewed-by: Neil Chen <[email protected]>
Tested-by: Greg Burd <[email protected]>
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
Backpatch-through: 19
---
src/backend/commands/analyze.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d0498b14da1..4cd1cf87fda 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1337,10 +1337,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))
{
/*
--
2.54.0
From 7f91e3d66a924c25d94bca4b39527faf7c067b91 Mon Sep 17 00:00:00 2001
From: Kevin Rocker <[email protected]>
Date: Mon, 28 Sep 2026 02:32:16 +0200
Subject: [PATCH v8 2/3] Move remaining interrupt checks out of locked regions.
vacuum_delay_point() and CHECK_FOR_INTERRUPTS() cannot process pending
interrupts while interrupts are held off.
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
its 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 vacuum 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]>
Reviewed-by: Neil Chen <[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/utils/activity/pgstat.c | 4 ++++
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 46fc60115a8..bb678300b1b 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -797,6 +797,9 @@ ginInsertCleanup(GinState *ginstate, bool must_empty_list,
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
@@ -895,8 +898,6 @@ ginInsertCleanup(GinState *ginstate, bool must_empty_list,
*/
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 b2e34d2d45e..3d48355eb08 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -562,6 +562,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);
@@ -799,8 +802,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/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 6dd13ab9dec..cef49b8a98f 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1745,6 +1745,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 5aac7960f87988a9c41aaf9f27cf9edcdeb39868 Mon Sep 17 00:00:00 2001
From: Kevin Rocker <[email protected]>
Date: Mon, 28 Sep 2026 02:32:16 +0200
Subject: [PATCH v8 3/3] 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 where CHECK_FOR_INTERRUPTS() cannot act, e.g. with
an LWLock or buffer content lock held. Enforce that in assert-enabled
builds, so that a new call site in a locked region trips the buildfarm
rather than silently delaying with interrupts held off.
Author: Kevin Rocker <[email protected]>
Author: Neil Chen <[email protected]>
Suggested-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
---
src/backend/commands/vacuum.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d8c2f33c615..b388c561cdb 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2482,6 +2482,12 @@ vacuum_delay_point(bool is_analyze)
{
double msec = 0;
+ /*
+ * A delay point may sleep and must service query cancel, so it cannot be
+ * reached where CHECK_FOR_INTERRUPTS() would be a no-op.
+ */
+ Assert(INTERRUPTS_CAN_BE_PROCESSED());
+
/* Always check for interrupts */
CHECK_FOR_INTERRUPTS();
--
2.54.0
