pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #19628: Uninterruptible vacuum during hash index processing
8+ messages / 5 participants
[nested] [flat]
* BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-08-18 12:02 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 8+ messages in thread
From: PG Bug reporting form @ 2026-08-18 12:02 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: sk@zsrv.org
The following bug has been logged on the website:
Bug reference: 19628
Logged by: Sergei Kornilov
Email address: sk@zsrv.org
PostgreSQL version: 16.14
Operating system: ubuntu
Description:
Hello!
During the installation of minor update (from 16.14 to 16.15), I discovered
that PostgreSQL was shutting down for a long time (about 6 minutes) because
it was waiting for the autovacuum process to terminate.
I was able to find the cause in the vacuum implementation for hash indexes
(this database has a fairly big hash index):
1) vacuum executes hashbulkdelete ( src/backend/access/hash/hash.c )
2) LockBufferForCleanup in loop_top calls HOLD_INTERRUPTS() under the hood
3) then we execute hashbucketcleanup , which can take a long time. Here, the
loop calls vacuum_delay_point (which calls CHECK_FOR_INTERRUPTS), but it
does nothing because HOLD_INTERRUPTS is still in effect.
4) Only at the end of hashbucketcleanup RESUME_INTERRUPTS is called from
LWLockRelease
I checked with gdb: hashbucketcleanup is actually executed with a non-zero
InterruptHoldoffCount.
Breakpoint 1, hashbucketcleanup (rel=rel@entry=0x7f1cf95ec080,
cur_bucket=cur_bucket@entry=0,
bucket_buf=bucket_buf@entry=11847, bucket_blkno=bucket_blkno@entry=1,
bstrategy=0x55f74ae62ca8, maxbucket=32573,
highmask=32767, lowmask=16383, tuples_removed=0x7fff95dc5a98,
num_index_tuples=0x7fff95dc5a90, split_cleanup=false,
callback=0x55f7395a6fa8 <vac_tid_reaped>, callback_state=0x7f1ce9b6d048)
at hash.c:691
691 {
(gdb) n
700 if (split_cleanup)
(gdb) n
717 vacuum_delay_point();
(gdb) p InterruptHoldoffCount
$1 = 1
I'm not sure how to properly fix hashbucketcleanup. At the very least, an
additional vacuum_delay_point in hashbulkdelete before LockBufferForCleanup
would improve the situation slightly.
regards, Sergei
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-08-22 14:54 mostafa nabil <mostafa.nabil.nafie@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 8+ messages in thread
From: mostafa nabil @ 2026-08-22 14:54 UTC (permalink / raw)
To: sk@zsrv.org; pgsql-bugs@lists.postgresql.org
Hi Sergei,
Thanks for the report and root cause analysis.
Attached is a patch adding a vacuum_delay_point() call at the top of
hashbulkdelete()'s per-bucket loop, before any buffer lock is taken.
The function currently has no interrupt check outside the one inside
hashbucketcleanup(), which is ineffective for the reason you already
found (InterruptHoldoffCount stays > 0 for the whole bucket once
LockBufferForCleanup() is called). This adds a check at the one spot
where nothing is locked yet, so a pending shutdown or cancel is
noticed at the next bucket boundary instead of only after the whole
index scan finishes.
No automated test: both patched and unpatched code eventually honor
the cancel, so a TAP test would need a hardcoded time threshold,
which risks flaking on slower CI hosts. Verified manually instead
(script attached): on an 8M-row table with ~90% dead tuples, cancelling
a VACUUM during the "vacuuming indexes" phase took ~2.08s on unpatched
master vs ~0.014s with the patch.
Not addressed: a single bucket with a very long overflow chain.
hashbucketcleanup() uses lock chaining (locks the next overflow page
before releasing the current one) to prevent a race with concurrent
scans overtaking a partially vacuumed bucket, per the hash AM README.
So interrupts are never truly clear during one bucket's own cleanup,
and this patch can't help there without changing the locking scheme.
I'd treat that as a separate, riskier follow-up.
Likely a backpatch candidate (real bug in shipped versions), but
I'll leave that call to whoever reviews this.
Regards,
Mostafa
On Sat, Aug 22, 2026 at 5:34 PM PG Bug reporting form <
noreply@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 19628
> Logged by: Sergei Kornilov
> Email address: sk@zsrv.org
> PostgreSQL version: 16.14
> Operating system: ubuntu
> Description:
>
> Hello!
>
> During the installation of minor update (from 16.14 to 16.15), I discovered
> that PostgreSQL was shutting down for a long time (about 6 minutes) because
> it was waiting for the autovacuum process to terminate.
>
> I was able to find the cause in the vacuum implementation for hash indexes
> (this database has a fairly big hash index):
>
> 1) vacuum executes hashbulkdelete ( src/backend/access/hash/hash.c )
> 2) LockBufferForCleanup in loop_top calls HOLD_INTERRUPTS() under the hood
> 3) then we execute hashbucketcleanup , which can take a long time. Here,
> the
> loop calls vacuum_delay_point (which calls CHECK_FOR_INTERRUPTS), but it
> does nothing because HOLD_INTERRUPTS is still in effect.
> 4) Only at the end of hashbucketcleanup RESUME_INTERRUPTS is called from
> LWLockRelease
>
> I checked with gdb: hashbucketcleanup is actually executed with a non-zero
> InterruptHoldoffCount.
>
> Breakpoint 1, hashbucketcleanup (rel=rel@entry=0x7f1cf95ec080,
> cur_bucket=cur_bucket@entry=0,
> bucket_buf=bucket_buf@entry=11847, bucket_blkno=bucket_blkno@entry=1,
> bstrategy=0x55f74ae62ca8, maxbucket=32573,
> highmask=32767, lowmask=16383, tuples_removed=0x7fff95dc5a98,
> num_index_tuples=0x7fff95dc5a90, split_cleanup=false,
> callback=0x55f7395a6fa8 <vac_tid_reaped>,
> callback_state=0x7f1ce9b6d048)
> at hash.c:691
> 691 {
> (gdb) n
> 700 if (split_cleanup)
> (gdb) n
> 717 vacuum_delay_point();
> (gdb) p InterruptHoldoffCount
> $1 = 1
>
> I'm not sure how to properly fix hashbucketcleanup. At the very least, an
> additional vacuum_delay_point in hashbulkdelete before LockBufferForCleanup
> would improve the situation slightly.
>
> regards, Sergei
>
>
>
>
>
>
>
--
Mostafa Nabil Software Engineer
Attachments:
[text/x-patch] 0001-Check-for-interrupts-between-hash-index-vacuum-bucke.patch (2.1K, ../../CAOwWfmwvEfJ+_-AQdX_+F_ffRBeTxQn_ru1s19ppX_qTeHnWYg@mail.gmail.com/3-0001-Check-for-interrupts-between-hash-index-vacuum-bucke.patch)
download | inline diff:
From 7f994f1ad50a64cf18a69d0c5063a7612b58dec9 Mon Sep 17 00:00:00 2001
From: Mostafa <mostafa.nabil.nafie@gmail.com>
Date: Sat, 22 Aug 2026 17:25:50 +0300
Subject: [PATCH] Check for interrupts between hash index vacuum buckets
hashbulkdelete() acquires a cleanup lock on each bucket's primary
page and holds it for the duration of hashbucketcleanup(), which
walks every page in that bucket's overflow chain. While that lock
is held, interrupts are held off, so the vacuum_delay_point() call
inside hashbucketcleanup()'s per-page loop is ineffective. The
outer per-bucket loop in hashbulkdelete() had no interrupt check at
all, so a pending shutdown or query cancel was not honored until the
entire index had been scanned, which can take minutes on a table
with many or large buckets.
Add a vacuum_delay_point() call at the top of the per-bucket loop,
before any lock is acquired, so a pending interrupt is noticed at
the next bucket boundary instead of only after the whole index scan
completes. Verified with a manual reproduction: cancelling a VACUUM
mid-"vacuuming indexes" phase on a hash index with many buckets went
from ~2.1s to honor the cancel down to ~0.01s with this change.
This does not address the case of a single bucket with a very long
overflow chain, where the checkpoint added here still cannot fire
until that bucket's own cleanup finishes; that would require
changing how hashbucketcleanup()'s lock chaining works, and is left
as a follow-up.
Reported-by: Sergei Kornilov <sk@zsrv.org>
Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org
---
src/backend/access/hash/hash.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index b2e34d2d45e..796a178f3ec 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -562,6 +562,12 @@ bucket_loop:
Page page;
bool split_cleanup = false;
+ /*
+ * Check for interrupts before acquiring the cleanup lock on the next
+ * bucket.
+ */
+ vacuum_delay_point(false);
+
/* Get address of bucket's start page */
bucket_blkno = BUCKET_TO_BLKNO(cachedmetap, cur_bucket);
--
2.43.0
[application/x-shellscript] test_hash_vacuum_cancel.sh (2.2K, ../../CAOwWfmwvEfJ+_-AQdX_+F_ffRBeTxQn_ru1s19ppX_qTeHnWYg@mail.gmail.com/4-test_hash_vacuum_cancel.sh)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-09-03 06:48 Kirill Reshke <reshkekirill@gmail.com>
parent: mostafa nabil <mostafa.nabil.nafie@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Kirill Reshke @ 2026-09-03 06:48 UTC (permalink / raw)
To: mostafa nabil <mostafa.nabil.nafie@gmail.com>; +Cc: sk@zsrv.org; pgsql-bugs@lists.postgresql.org
On Sat, 22 Aug 2026 at 19:54, mostafa nabil
<mostafa.nabil.nafie@gmail.com> wrote:
>
> Hi Sergei,
>
> Thanks for the report and root cause analysis.
>
> Attached is a patch adding a vacuum_delay_point() call at the top of
> hashbulkdelete()'s per-bucket loop, before any buffer lock is taken.
> The function currently has no interrupt check outside the one inside
> hashbucketcleanup(), which is ineffective for the reason you already
> found (InterruptHoldoffCount stays > 0 for the whole bucket once
> LockBufferForCleanup() is called). This adds a check at the one spot
> where nothing is locked yet, so a pending shutdown or cancel is
> noticed at the next bucket boundary instead of only after the whole
> index scan finishes.
>
> No automated test: both patched and unpatched code eventually honor
> the cancel, so a TAP test would need a hardcoded time threshold,
> which risks flaking on slower CI hosts. Verified manually instead
> (script attached): on an 8M-row table with ~90% dead tuples, cancelling
> a VACUUM during the "vacuuming indexes" phase took ~2.08s on unpatched
> master vs ~0.014s with the patch.
>
> Not addressed: a single bucket with a very long overflow chain.
> hashbucketcleanup() uses lock chaining (locks the next overflow page
> before releasing the current one) to prevent a race with concurrent
> scans overtaking a partially vacuumed bucket, per the hash AM README.
> So interrupts are never truly clear during one bucket's own cleanup,
> and this patch can't help there without changing the locking scheme.
> I'd treat that as a separate, riskier follow-up.
>
> Likely a backpatch candidate (real bug in shipped versions), but
> I'll leave that call to whoever reviews this.
>
> Regards,
> Mostafa
>
I think our fix is fine, we also need to remove vacuum_delay_point
from hashbucketcleanup function, since it is ineffective if called was
Interrupt holdoff. This will follow existing coding practice, see also
how GIN vacuum works with buffer lock/vacuum_delay_point
Also, we can actually test this deterministically using injection
points, but I dont think this test is worth cpu cycles in buildfarm.
Too much for this.
--
Best regards,
Kirill Reshke
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-09-04 09:47 mostafa nabil <mostafa.nabil.nafie@gmail.com>
parent: Kirill Reshke <reshkekirill@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: mostafa nabil @ 2026-09-04 09:47 UTC (permalink / raw)
To: Kirill Reshke <reshkekirill@gmail.com>; pgsql-bugs@lists.postgresql.org; sk@zsrv.org
Hi Kirill,
Thanks for the review.
v2 attached, i removed the vacuum_delay_point() call inside
hashbucketcleanup()'s per-page loop as you suggested. Good catch because
it's dead code.
Regards,
Mostafa
On Fri, Sep 4, 2026 at 12:42 PM mostafa nabil <mostafa.nabil.nafie@gmail.com>
wrote:
> Hi Kirill,
>
> Thanks for the review.
>
> v2 attached, i removed the vacuum_delay_point() call inside
> hashbucketcleanup()'s per-page loop as you suggested. Good catch because
> it's dead code.
>
>
> Regards,
> Mostafa
>
> On Thu, Sep 3, 2026 at 9:48 AM Kirill Reshke <reshkekirill@gmail.com>
> wrote:
>
>> On Sat, 22 Aug 2026 at 19:54, mostafa nabil
>> <mostafa.nabil.nafie@gmail.com> wrote:
>> >
>> > Hi Sergei,
>> >
>> > Thanks for the report and root cause analysis.
>> >
>> > Attached is a patch adding a vacuum_delay_point() call at the top of
>> > hashbulkdelete()'s per-bucket loop, before any buffer lock is taken.
>> > The function currently has no interrupt check outside the one inside
>> > hashbucketcleanup(), which is ineffective for the reason you already
>> > found (InterruptHoldoffCount stays > 0 for the whole bucket once
>> > LockBufferForCleanup() is called). This adds a check at the one spot
>> > where nothing is locked yet, so a pending shutdown or cancel is
>> > noticed at the next bucket boundary instead of only after the whole
>> > index scan finishes.
>> >
>> > No automated test: both patched and unpatched code eventually honor
>> > the cancel, so a TAP test would need a hardcoded time threshold,
>> > which risks flaking on slower CI hosts. Verified manually instead
>> > (script attached): on an 8M-row table with ~90% dead tuples, cancelling
>> > a VACUUM during the "vacuuming indexes" phase took ~2.08s on unpatched
>> > master vs ~0.014s with the patch.
>> >
>> > Not addressed: a single bucket with a very long overflow chain.
>> > hashbucketcleanup() uses lock chaining (locks the next overflow page
>> > before releasing the current one) to prevent a race with concurrent
>> > scans overtaking a partially vacuumed bucket, per the hash AM README.
>> > So interrupts are never truly clear during one bucket's own cleanup,
>> > and this patch can't help there without changing the locking scheme.
>> > I'd treat that as a separate, riskier follow-up.
>> >
>> > Likely a backpatch candidate (real bug in shipped versions), but
>> > I'll leave that call to whoever reviews this.
>> >
>> > Regards,
>> > Mostafa
>> >
>>
>> I think our fix is fine, we also need to remove vacuum_delay_point
>> from hashbucketcleanup function, since it is ineffective if called was
>> Interrupt holdoff. This will follow existing coding practice, see also
>> how GIN vacuum works with buffer lock/vacuum_delay_point
>>
>> Also, we can actually test this deterministically using injection
>> points, but I dont think this test is worth cpu cycles in buildfarm.
>> Too much for this.
>>
>> --
>> Best regards,
>> Kirill Reshke
>>
>
>
> --
> Mostafa Nabil Software Engineer
>
--
Mostafa Nabil Software Engineer
Attachments:
[text/x-patch] v2-0001-Check-for-interrupts-between-hash-index-vacuum-bu.patch (2.8K, ../../CAOwWfmy+7Xgu_8O0iWZ8-hnXedhW=0Kv2yqO0yNXVbc6_g3H2A@mail.gmail.com/3-v2-0001-Check-for-interrupts-between-hash-index-vacuum-bu.patch)
download | inline diff:
From 1e69f738bd4bb0499f029f34ee3c9d575b7fe68d Mon Sep 17 00:00:00 2001
From: Mostafa <mostafa.nabil.nafie@gmail.com>
Date: Fri, 4 Sep 2026 12:33:21 +0300
Subject: [PATCH v2] Check for interrupts between hash index vacuum buckets
hashbulkdelete() acquires a cleanup lock on each bucket's primary
page and holds it for the duration of hashbucketcleanup(), which
walks every page in that bucket's overflow chain. While that lock
is held, interrupts are held off, so the vacuum_delay_point() call
inside hashbucketcleanup()'s per-page loop was never effective. The
outer per-bucket loop in hashbulkdelete() had no interrupt check at
all, so a pending shutdown or query cancel was not honored until the
entire index had been scanned, which can take minutes on a table
with many or large buckets.
Add a vacuum_delay_point() call at the top of the per-bucket loop,
before any lock is acquired, so a pending interrupt is noticed at
the next bucket boundary instead of only after the whole index scan
completes. Verified with a manual reproduction: cancelling a VACUUM
mid-"vacuuming indexes" phase on a hash index with many buckets went
from ~2.1s to honor the cancel down to ~0.01s with this change.
Also remove the now-redundant vacuum_delay_point() call inside
hashbucketcleanup()'s per-page loop, since it never had any effect
there: it runs while the bucket's cleanup lock is held, so
InterruptHoldoffCount is always > 0 at that point. This matches how
GIN vacuum places its delay points only where no buffer lock is
held (see ginVacuumPostingTreeLeaves()).
This does not address the case of a single bucket with a very long
overflow chain, where interrupts still cannot be noticed until that
bucket's own cleanup finishes; that would require changing how
hashbucketcleanup()'s lock chaining works, and is left as a
follow-up.
Reported-by: Sergei Kornilov <sk@zsrv.org>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org
---
src/backend/access/hash/hash.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index b2e34d2d45e..7684a2ae5fe 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -562,6 +562,12 @@ bucket_loop:
Page page;
bool split_cleanup = false;
+ /*
+ * Check for interrupts before acquiring the cleanup lock on the next
+ * bucket.
+ */
+ vacuum_delay_point(false);
+
/* Get address of bucket's start page */
bucket_blkno = BUCKET_TO_BLKNO(cachedmetap, cur_bucket);
@@ -799,8 +805,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);
--
2.43.0
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-10-02 09:21 mostafa nabil <mostafa.nabil.nafie@gmail.com>
parent: mostafa nabil <mostafa.nabil.nafie@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: mostafa nabil @ 2026-10-02 09:21 UTC (permalink / raw)
To: Kirill Reshke <reshkekirill@gmail.com>; pgsql-bugs@lists.postgresql.org; sk@zsrv.org
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.
cfbot is green on all platforms:
https://commitfest.postgresql.org/patch/7216/
The entry has been moved to the November commitfest (PG20-3). If v2
looks good to you, would you mind adding yourself as a reviewer there
and marking it Ready for Committer?
Regards,
Mostafa Nabil
On Fri, Sep 4, 2026 at 12:47 PM mostafa nabil <mostafa.nabil.nafie@gmail.com>
wrote:
> Hi Kirill,
>
> Thanks for the review.
>
> v2 attached, i removed the vacuum_delay_point() call inside
> hashbucketcleanup()'s per-page loop as you suggested. Good catch because
> it's dead code.
>
>
> Regards,
> Mostafa
>
>
> On Fri, Sep 4, 2026 at 12:42 PM mostafa nabil <
> mostafa.nabil.nafie@gmail.com> wrote:
>
>> Hi Kirill,
>>
>> Thanks for the review.
>>
>> v2 attached, i removed the vacuum_delay_point() call inside
>> hashbucketcleanup()'s per-page loop as you suggested. Good catch
>> because it's dead code.
>>
>>
>> Regards,
>> Mostafa
>>
>> On Thu, Sep 3, 2026 at 9:48 AM Kirill Reshke <reshkekirill@gmail.com>
>> wrote:
>>
>>> On Sat, 22 Aug 2026 at 19:54, mostafa nabil
>>> <mostafa.nabil.nafie@gmail.com> wrote:
>>> >
>>> > Hi Sergei,
>>> >
>>> > Thanks for the report and root cause analysis.
>>> >
>>> > Attached is a patch adding a vacuum_delay_point() call at the top of
>>> > hashbulkdelete()'s per-bucket loop, before any buffer lock is taken.
>>> > The function currently has no interrupt check outside the one inside
>>> > hashbucketcleanup(), which is ineffective for the reason you already
>>> > found (InterruptHoldoffCount stays > 0 for the whole bucket once
>>> > LockBufferForCleanup() is called). This adds a check at the one spot
>>> > where nothing is locked yet, so a pending shutdown or cancel is
>>> > noticed at the next bucket boundary instead of only after the whole
>>> > index scan finishes.
>>> >
>>> > No automated test: both patched and unpatched code eventually honor
>>> > the cancel, so a TAP test would need a hardcoded time threshold,
>>> > which risks flaking on slower CI hosts. Verified manually instead
>>> > (script attached): on an 8M-row table with ~90% dead tuples, cancelling
>>> > a VACUUM during the "vacuuming indexes" phase took ~2.08s on unpatched
>>> > master vs ~0.014s with the patch.
>>> >
>>> > Not addressed: a single bucket with a very long overflow chain.
>>> > hashbucketcleanup() uses lock chaining (locks the next overflow page
>>> > before releasing the current one) to prevent a race with concurrent
>>> > scans overtaking a partially vacuumed bucket, per the hash AM README.
>>> > So interrupts are never truly clear during one bucket's own cleanup,
>>> > and this patch can't help there without changing the locking scheme.
>>> > I'd treat that as a separate, riskier follow-up.
>>> >
>>> > Likely a backpatch candidate (real bug in shipped versions), but
>>> > I'll leave that call to whoever reviews this.
>>> >
>>> > Regards,
>>> > Mostafa
>>> >
>>>
>>> I think our fix is fine, we also need to remove vacuum_delay_point
>>> from hashbucketcleanup function, since it is ineffective if called was
>>> Interrupt holdoff. This will follow existing coding practice, see also
>>> how GIN vacuum works with buffer lock/vacuum_delay_point
>>>
>>> Also, we can actually test this deterministically using injection
>>> points, but I dont think this test is worth cpu cycles in buildfarm.
>>> Too much for this.
>>>
>>> --
>>> Best regards,
>>> Kirill Reshke
>>>
>>
>>
>> --
>> Mostafa Nabil Software Engineer
>>
>
>
> --
> Mostafa Nabil Software Engineer
>
--
Mostafa Nabil Software Engineer
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-10-02 21:55 Heikki Linnakangas <hlinnaka@iki.fi>
parent: mostafa nabil <mostafa.nabil.nafie@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Heikki Linnakangas @ 2026-10-02 21:55 UTC (permalink / raw)
To: mostafa nabil <mostafa.nabil.nafie@gmail.com>; Kirill Reshke <reshkekirill@gmail.com>; pgsql-bugs@lists.postgresql.org; sk@zsrv.org
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.c
index 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.c
index 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.c
index 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.c
index 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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-10-03 04:05 shihao zhong <zhong950419@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 8+ messages in thread
From: shihao zhong @ 2026-10-03 04:05 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: mostafa nabil <mostafa.nabil.nafie@gmail.com>; Kirill Reshke <reshkekirill@gmail.com>; pgsql-bugs@lists.postgresql.org; sk@zsrv.org; me@kevinrocker.com
Hi Heikki,
On 02/10/2026, Heikki Linnakangas wrote:
> Do we have more places where we call vacuum_delay_point() while holding
> locks?
I tried to run tests under assertion you had and I did not see any new
failures.
I also asked Fable to list 28 use cases and manually check the code path
and did not find any vacuum_delay_point with LWLock holding.
But, there is a thread on hackers with the same three fixes, in case you
have not seen it. Kevin Rocker's v8 is here:
https://postgr.es/m/d46104e6-19e0-4dfe-bb64-8ff21d53b1dd@app.fastmail.com
Tom suggested the Assert there, and v8-0003 adds it.
Thanks,
Shihao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #19628: Uninterruptible vacuum during hash index processing
@ 2026-10-05 10:20 Heikki Linnakangas <hlinnaka@iki.fi>
parent: shihao zhong <zhong950419@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Linnakangas @ 2026-10-05 10:20 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: mostafa nabil <mostafa.nabil.nafie@gmail.com>; Kirill Reshke <reshkekirill@gmail.com>; pgsql-bugs@lists.postgresql.org; sk@zsrv.org; me@kevinrocker.com
On 03/10/2026 07:05, shihao zhong wrote:
> But, there is a thread on hackers with the same three fixes, in case you
> have not seen it. Kevin Rocker's v8 is here:
>
> https://postgr.es/m/d46104e6-19e0-4dfe-
> bb64-8ff21d53b1dd@app.fastmail.com <https://postgr.es/m/
> d46104e6-19e0-4dfe-bb64-8ff21d53b1dd@app.fastmail.com>
>
> Tom suggested the Assert there, and v8-0003 adds it.
Ah, thanks for the pointer! I thought issue sounded familiar but didn't
remember that other thread.
I committed the fix, taking the discussion from that other thread into
account.
- Heikki
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-10-05 10:20 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-18 12:02 BUG #19628: Uninterruptible vacuum during hash index processing PG Bug reporting form <noreply@postgresql.org>
2026-08-22 14:54 ` mostafa nabil <mostafa.nabil.nafie@gmail.com>
2026-09-03 06:48 ` Kirill Reshke <reshkekirill@gmail.com>
2026-09-04 09:47 ` mostafa nabil <mostafa.nabil.nafie@gmail.com>
2026-10-02 09:21 ` mostafa nabil <mostafa.nabil.nafie@gmail.com>
2026-10-02 21:55 ` Heikki Linnakangas <hlinnaka@iki.fi>
2026-10-03 04:05 ` shihao zhong <zhong950419@gmail.com>
2026-10-05 10:20 ` Heikki Linnakangas <hlinnaka@iki.fi>
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