From: Heikki Linnakangas <hlinnaka@iki.fi>
To: mostafa nabil <mostafa.nabil.nafie@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
To: pgsql-bugs@lists.postgresql.org
To: sk@zsrv.org
Subject: Re: BUG #19628: Uninterruptible vacuum during hash index processing
Date: Sat, 3 Oct 2026 00:55:25 +0300
Message-ID: <3e128ea7-8cae-4a48-9d7c-9a032e7633f4@iki.fi> (raw)
In-Reply-To: <CAOwWfmw2u4NUy6S9e+zbt2tOMD3LhmEq5PXiwag6SxTWs9qgnQ@mail.gmail.com>
References: <19628-c2b17d358181a1ea@postgresql.org>
<CAOwWfmwvEfJ+_-AQdX_+F_ffRBeTxQn_ru1s19ppX_qTeHnWYg@mail.gmail.com>
<CALdSSPhMOpUYFFViHSZma=enFkGjjp-T_xoBGGrmL3TNTS+REA@mail.gmail.com>
<CAOwWfmycksenYFimk-ohmk4Q+tPzUurnaAJ7WD+N-do=TdPs3A@mail.gmail.com>
<CAOwWfmy+7Xgu_8O0iWZ8-hnXedhW=0Kv2yqO0yNXVbc6_g3H2A@mail.gmail.com>
<CAOwWfmw2u4NUy6S9e+zbt2tOMD3LhmEq5PXiwag6SxTWs9qgnQ@mail.gmail.com>
On 02/10/2026 12:21, mostafa nabil wrote:
> Hi Kirill,
>
> Thanks again for the review. Did you get a chance to look at v2? It
> removes the vacuum_delay_point() call in hashbucketcleanup() as you
> suggested, and keeps the check at the top of hashbulkdelete()'s
> per-bucket loop.
I had a look at this. Some further fixes:
Do we have more places where we call vacuum_delay_point() while holding
locks? It seems like a bad idea to ever do that -- you don't want to
sleep while holding locks. I added an
"Assert(INTERRUPTS_CAN_BE_PROCESSED())" into vacuum_cost_delay() and ran
the regression tests, and that indeed revealed a few more places that
did that. The attached patch removes or moves those vacuum_delay_point()
calls too. I didn't include the assertion in the patch, because I'm
afraid there might be more places that we've missed, including in
extensions.
After fixing those, if you ever do call vacuum_cost_delay_point() while
holding a lock, e.g. from an extension or if we missed a caller, I think
you don't really want to sleep. And you definitely don't want to call
ProcessConfigFile() while in a critical section. So I added a quick exit
to vacuum_delay_point() if it's called with !INTERRUPTS_CAN_BE_PROCESSED().
What do you think?
- Heikki
Attachments:
[text/x-patch] v3-0001-Don-t-call-vacuum_delay_point-while-holding-locks.patch (5.3K, ../3e128ea7-8cae-4a48-9d7c-9a032e7633f4@iki.fi/2-v3-0001-Don-t-call-vacuum_delay_point-while-holding-locks.patch)
download | inline diff:
From 80cdf4f3407d34bdcfc862a8b1150ab53524d706 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Sat, 3 Oct 2026 00:53:18 +0300
Subject: [PATCH v3 1/1] Don't call vacuum_delay_point() while holding locks
It's a bad idea to sleep while holding locks, because you might block
another process that wants to acquire the same lock. So avoid calling
vacuum_delay_point() while holding locks.
There were a few places that did that, which I found by adding an
"Assert(INTERRUPTS_CAN_BE_PROCESSED())" into vacuum_delay_point() and
running the regression tests. I didn't include that Assert in this
commit, because there might be more places that do that that are not
covered by the regression tests, including extensions. But I did add
a runtime check that bails out of vacuum_delay_point() quickly if
interrupts cannot be processed at the time. That prevents the sleep
that might block others, and it also avoids potentially calling
ProcessConfigFile() while in a critical section, if
vacuum_delay_point() was ever called in one.
In ginInsertCleanup(), there was already another vacuum_delay_point()
call later in the loop, while not holding any locks, so just remove
the other call that was made while holding the lock.
In hashbucketcleanup(), move the vacuum_delay_point() call up the
stack to its caller. The call in hashbucketcleanup() was not able to
handle interrupts because it held a lock, and because there were no
vacuum_delay_point() or CHECK_FOR_INTERRUPTS() calls in the caller's
loop, the whole hash index vacuuming phase was uninterruptible by
pending shutdown or query cancel. Now it can be interrupted between
buckets. Unfortunately, hashbucketcleanup() has to process all the
bucket's pages in one go without pausing; fixing that would require
changing how hashbucketcleanup()'s lock chaining works.
In acquire_sample_rows(), move the vacuum_delay_point() call in the
loop to between pages, to a time where we're not holding the buffer
lock.
Reported-by: Sergei Kornilov <sk@zsrv.org>
Author: Mostafa <mostafa.nabil.nafie@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org
---
src/backend/access/gin/ginfast.c | 6 +++---
src/backend/access/hash/hash.c | 5 +++--
src/backend/commands/analyze.c | 3 +--
src/backend/commands/vacuum.c | 8 ++++++++
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.cindex 46fc60115a8..3779b43fa1a 100644--- a/src/backend/access/gin/ginfast.c+++ b/src/backend/access/gin/ginfast.c@@ -895,8 +895,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
@@ -1002,10 +1000,12 @@ ginInsertCleanup(GinState *ginstate, bool must_empty_list,
UnlockReleaseBuffer(buffer);
}
+ /* call vacuum_delay_point while not holding any buffer lock */+ vacuum_delay_point(false);+
/*
* Read next page in pending list
*/
- vacuum_delay_point(false);
buffer = ReadBuffer(index, blkno);
LockBuffer(buffer, GIN_SHARE);
page = BufferGetPage(buffer);
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.cindex b2e34d2d45e..63846c10840 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;
+ /* call vacuum_delay_point while not holding any buffer lock */+ 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/commands/analyze.c b/src/backend/commands/analyze.cindex d0498b14da1..518d7526255 100644--- a/src/backend/commands/analyze.c+++ b/src/backend/commands/analyze.c@@ -1339,8 +1339,6 @@ acquire_sample_rows(Relation onerel, int elevel,
/* Outer loop over blocks to sample */
while (table_scan_analyze_next_block(scan, stream))
{
- vacuum_delay_point(true);-
while (table_scan_analyze_next_tuple(scan, &liverows, &deadrows, slot))
{
/*
@@ -1388,6 +1386,7 @@ acquire_sample_rows(Relation onerel, int elevel,
pgstat_progress_update_param(PROGRESS_ANALYZE_BLOCKS_DONE,
++blksdone);
+ vacuum_delay_point(true);
}
read_stream_end(stream);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.cindex d8c2f33c615..a257dd8d21e 100644--- a/src/backend/commands/vacuum.c+++ b/src/backend/commands/vacuum.c@@ -2482,6 +2482,14 @@ vacuum_delay_point(bool is_analyze)
{
double msec = 0;
+ /*+ * If we're holding locks or holding interrupts for some other reason,+ * don't sleep, because we don't want to hold locks any longer than+ * necessary.+ */+ if (!INTERRUPTS_CAN_BE_PROCESSED())+ return;+
/* Always check for interrupts */
CHECK_FOR_INTERRUPTS();
--
2.47.3
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: hlinnaka@iki.fi, mostafa.nabil.nafie@gmail.com, reshkekirill@gmail.com, pgsql-bugs@lists.postgresql.org, sk@zsrv.org
Subject: Re: BUG #19628: Uninterruptible vacuum during hash index processing
In-Reply-To: <3e128ea7-8cae-4a48-9d7c-9a032e7633f4@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox