pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRestore vacuum_delay_point() in GIN posting-tree leaf vacuum
9+ messages / 5 participants
[nested] [flat]
* Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-19 14:27 Paul Kim <mok03127@gmail.com>
0 siblings, 2 replies; 9+ messages in thread
From: Paul Kim @ 2026-07-19 14:27 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Hi,
ginVacuumPostingTreeLeaves() sweeps the leaf pages of a GIN posting tree
via their rightlinks, but that loop has no vacuum_delay_point() (and hence
no CHECK_FOR_INTERRUPTS()), unlike the sibling loops in ginbulkdelete() and
ginvacuumcleanup().
The call used to be there: it was dropped when fd83c83d094 rewrote this
cleanup from a recursive walk into the current iterative sweep, and never
re-added to the new loop. That commit was only about fixing a deadlock, so
the removal looks inadvertent.
A posting tree holds all TIDs for a single key, so for a common key it can
span many leaf pages. While one such tree is being vacuumed, the operation
now ignores vacuum_cost_delay and cannot be interrupted (query cancel,
statement_timeout, or autovacuum cancellation when another backend wants a
conflicting lock) until the whole sweep finishes; a corrupted rightlink
forming a cycle turns it into an uninterruptible hang.
The attached patch restores the call, placed where the buffer has already
been released so no content lock is held across the delay. It is against
master; on branches before 18 vacuum_delay_point() takes no argument (the
is_analyze parameter was added by e5b0b0ce150), so the back-branch version
is just "vacuum_delay_point();".
I left the initial leftmost-descent loop in the same function as-is, since
it mirrors the entry-tree descent in ginbulkdelete(), which likewise has no
delay point.
Thanks,
Paul
Attachments:
[text/x-patch] v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patch (2.2K, ../../178447127453.110.12276981925360691905@mail.gmail.com/2-v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patch)
download | inline diff:
From d9b5ddb3cb0cfb55d418a09f571a78ee069eb3e1 Mon Sep 17 00:00:00 2001
From: Paul Kim <mok03127@gmail.com>
Date: Sun, 19 Jul 2026 23:08:12 +0900
Subject: [PATCH v1] Restore vacuum_delay_point() in GIN posting-tree leaf
vacuum
Commit fd83c83d094 turned the recursive posting-tree cleanup in
ginVacuumPostingTreeLeaves() into an iterative sweep that follows the
tree's leaf pages via their rightlinks. The recursive version called
vacuum_delay_point() while processing the tree, but that call was removed
and never re-added to the new loop. As that commit only set out to fix a
deadlock, the removal appears to have been unintentional.
Consequently the leaf-page sweep of a single posting tree runs with no
vacuum_delay_point(), and therefore no CHECK_FOR_INTERRUPTS(). A posting
tree stores all the TIDs for one indexed key, so for a frequently-occurring
key it can span a large number of leaf pages. While such a tree is being
vacuumed the operation ignores vacuum_cost_delay and does not respond to
query cancellation or statement_timeout; an autovacuum worker likewise
cannot be interrupted mid-sweep when another backend requests a conflicting
lock. A corrupted rightlink that forms a cycle makes the loop run
uninterruptibly forever.
Restore the call, placed after the current page has been unlocked and
released so that no buffer content lock is held across a potential delay
(cf. 21c27af65fb). The sibling loops in ginbulkdelete() and
ginvacuumcleanup() already call vacuum_delay_point() once per page.
Back-patch to all supported branches, which all contain fd83c83d094.
---
src/backend/access/gin/ginvacuum.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index 840543eb664..a0b81a215a9 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -429,6 +429,8 @@ ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno)
if (blkno == InvalidBlockNumber)
break;
+ vacuum_delay_point(false);
+
buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno,
RBM_NORMAL, gvs->strategy);
LockBuffer(buffer, GIN_EXCLUSIVE);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-20 07:16 Michael Paquier <michael@paquier.xyz>
parent: Paul Kim <mok03127@gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2026-07-20 07:16 UTC (permalink / raw)
To: Paul Kim <mok03127@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Alexander Korotkov <aekorotkov@gmail.com>
On Sun, Jul 19, 2026 at 11:27:54PM +0900, Paul Kim wrote:
> ginVacuumPostingTreeLeaves() sweeps the leaf pages of a GIN posting tree
> via their rightlinks, but that loop has no vacuum_delay_point() (and hence
> no CHECK_FOR_INTERRUPTS()), unlike the sibling loops in ginbulkdelete() and
> ginvacuumcleanup().
>
> The call used to be there: it was dropped when fd83c83d094 rewrote this
> cleanup from a recursive walk into the current iterative sweep, and never
> re-added to the new loop. That commit was only about fixing a deadlock, so
> the removal looks inadvertent.
Hmm, yeah, that looks like an oversight. We have been calling
vacuum_delay_point() for each page cleaned up.
> The attached patch restores the call, placed where the buffer has already
> been released so no content lock is held across the delay. It is against
> master; on branches before 18 vacuum_delay_point() takes no argument (the
> is_analyze parameter was added by e5b0b0ce150), so the back-branch version
> is just "vacuum_delay_point();".
This suggested location looks sensible as well, after unlocking a
buffer that has been cleaned up, before taking a lock on the next one.
Looking at the thread of fd83c83d0943 at [1], the first patch has
worked on the problem differently, while the first version of the
patch that has led to the commit does not mention a reason why
vacuum_delay_point() should be removed, so I think that it's simply an
oversight.
Adding Alexander Korotkov in CC, as the original committer of
fd83c83d0943.
[1]: https://www.postgresql.org/message-id/31a702a.14dd.166c1366ac1.Coremail.chjischj%40163.com
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../al3LFmvcsdBF_Gqn@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-20 10:45 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Paul Kim <mok03127@gmail.com>
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Borodin @ 2026-07-20 10:45 UTC (permalink / raw)
To: Paul Kim <mok03127@gmail.com>; +Cc: pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
> On 19 Jul 2026, at 19:27, Paul Kim <mok03127@gmail.com> wrote:
>
> no CHECK_FOR_INTERRUPTS()
As far as I remember, we hold a cleanup lock on posting tree root.
We cannot process interrupts while holding a LWLock.
I'm afraid to fix this problem we need a real page deletion with
full fledged concurrency protocol as we do it in B-tree or GiST.
I've written a patch for this in 2018. I hope to return to this one day.
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-20 11:59 Alexander Korotkov <aekorotkov@gmail.com>
parent: Andrey Borodin <x4mmm@yandex-team.ru>
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Korotkov @ 2026-07-20 11:59 UTC (permalink / raw)
To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Paul Kim <mok03127@gmail.com>; pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
On Mon, Jul 20, 2026 at 1:45 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > On 19 Jul 2026, at 19:27, Paul Kim <mok03127@gmail.com> wrote:
> >
> > no CHECK_FOR_INTERRUPTS()
>
> As far as I remember, we hold a cleanup lock on posting tree root.
> We cannot process interrupts while holding a LWLock.
>
> I'm afraid to fix this problem we need a real page deletion with
> full fledged concurrency protocol as we do it in B-tree or GiST.
>
> I've written a patch for this in 2018. I hope to return to this one day.
We're holding cleanup lock on posting tree root while
ginScanPostingTreeToDelete(), not ginVacuumPostingTreeLeaves(). So, I
think yes, vacuum_delay_point() was deleted unintentionally. I'm
going to get closer look on this in the next couple of days.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-20 19:10 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Andrey Borodin @ 2026-07-20 19:10 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Paul Kim <mok03127@gmail.com>; pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
> On 20 Jul 2026, at 16:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> We're holding cleanup lock on posting tree root while
> ginScanPostingTreeToDelete(), not ginVacuumPostingTreeLeaves(). So, I
> think yes, vacuum_delay_point() was deleted unintentionally.
Yup, my recollection was wrong. ginVacuumPostingTreeLeaves() seems fine for a
delay point.
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-22 19:28 Alexander Korotkov <aekorotkov@gmail.com>
parent: Andrey Borodin <x4mmm@yandex-team.ru>
0 siblings, 3 replies; 9+ messages in thread
From: Alexander Korotkov @ 2026-07-22 19:28 UTC (permalink / raw)
To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Paul Kim <mok03127@gmail.com>; pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
On Mon, Jul 20, 2026 at 10:10 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 20 Jul 2026, at 16:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > We're holding cleanup lock on posting tree root while
> > ginScanPostingTreeToDelete(), not ginVacuumPostingTreeLeaves(). So, I
> > think yes, vacuum_delay_point() was deleted unintentionally.
>
> Yup, my recollection was wrong. ginVacuumPostingTreeLeaves() seems fine for a
> delay point.
Please, check the patch attached.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patch (2.5K, ../../CAPpHfdtoMnMPSksdjWx8tsM0RCXSpmYLQGYF+rzbK00Tf-q6HQ@mail.gmail.com/2-v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patch)
download | inline diff:
From dff250e499f993f8e5ac7a2e6bd2bbf6b38fd153 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 22 Jul 2026 22:28:01 +0300
Subject: [PATCH v1] Restore vacuum_delay_point() in GIN posting-tree leaf
vacuum
Commit fd83c83d094 turned the recursive posting-tree cleanup in
ginVacuumPostingTreeLeaves() into an iterative sweep that follows the
tree's leaf pages via their rightlinks. The recursive version called
vacuum_delay_point() while processing the tree, but that call was removed
and never re-added to the new loop. As that commit only set out to fix a
deadlock, the removal appears to have been unintentional.
Consequently the leaf-page sweep of a single posting tree runs with no
vacuum_delay_point(), and therefore no CHECK_FOR_INTERRUPTS(). A posting
tree stores all the TIDs for one indexed key, so for a frequently
occurring key it can span a large number of leaf pages. While such a
tree is being vacuumed the operation ignores vacuum_cost_delay and does
not respond to query cancellation or statement_timeout; an autovacuum
worker likewise cannot be interrupted mid-sweep when another backend
requests a conflicting lock. A corrupted rightlink that forms a cycle
makes the loop run uninterruptibly forever.
Restore the call, placed after the current page has been unlocked and
released so that no buffer content lock is held across a potential delay
(cf. 21c27af65fb). The sibling loops in ginbulkdelete() and
ginvacuumcleanup() already call vacuum_delay_point() once per page.
Reported-by: Paul Kim <mok03127@gmail.com>
Discussion: https://postgr.es/m/178447127453.110.12276981925360691905%40mail.gmail.com
Backpatch-through: 14
---
src/backend/access/gin/ginvacuum.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index 840543eb664..040f21a92e3 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -429,6 +429,13 @@ ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno)
if (blkno == InvalidBlockNumber)
break;
+ /*
+ * A safe point to delay/accept interrupts: the previous page has been
+ * unlocked and released, so we hold no buffer content lock (nor any
+ * other LWLock) here and CHECK_FOR_INTERRUPTS() can do its job.
+ */
+ vacuum_delay_point(false);
+
buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno,
RBM_NORMAL, gvs->strategy);
LockBuffer(buffer, GIN_EXCLUSIVE);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-23 02:26 Paul Kim <mok03127@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
2 siblings, 0 replies; 9+ messages in thread
From: Paul Kim @ 2026-07-23 02:26 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: aekorotkov@gmail.com, michael@paquier.xyz, x4mmm@yandex-team.ru, Paul Kim <mok03127@gmail.com>
On 2026-07-22, Alexander Korotkov wrote:
> Please, check the patch attached.
Thanks, Alexander. The patch looks good to me -- it's the same change
I posted upthread, and the placement is right: the call sits after the
page has been unlocked and released, so no buffer content lock (nor any
other LWLock) is held across the delay, matching the sibling loops in
ginbulkdelete() and ginvacuumcleanup(). The added comment reads well.
One note for the back-branches, since it's marked Backpatch-through:
14: vacuum_delay_point() only gained its bool argument in e5b0b0ce150
(v18), so on 14-17 the call would need to be the no-argument
vacuum_delay_point().
--
Paul Kim
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-26 16:15 solai v <solai.cdac@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
2 siblings, 0 replies; 9+ messages in thread
From: solai v @ 2026-07-26 16:15 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>; Paul Kim <mok03127@gmail.com>; pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
Hi all,
On Sun, Jul 26, 2026 at 8:09 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Mon, Jul 20, 2026 at 10:10 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> >
> > > On 20 Jul 2026, at 16:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >
> > > We're holding cleanup lock on posting tree root while
> > > ginScanPostingTreeToDelete(), not ginVacuumPostingTreeLeaves(). So, I
> > > think yes, vacuum_delay_point() was deleted unintentionally.
> >
> > Yup, my recollection was wrong. ginVacuumPostingTreeLeaves() seems fine for a
> > delay point.
>
> Please, check the patch attached.
>
Thank you for the updated patch. I reviewed and tested this patch by
creating a large GIN index on a table containing 10 million rows. Then
deleted 9 million rows, and ran VACUUM (VERBOSE). For validation, I
temporarily added instrumentation to ginVacuumPostingTreeLeaves()
function. On the unpatched build, I confirmed that the posting-tree
leaf traversal code was executed. Also after applying the patch, I
verified that the newly added vacuum_delay_point(false) is reached and
executed during the traversal of posting-tree leaf pages. I did not
observe any functional regressions or unexpected behavior during
testing. The patch restores the missing vacuum_delay_point() call
while preserving the existing vacuum behavior. The patch looks good to
me.
Regards,
Solai
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
@ 2026-07-27 06:55 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Alexander Korotkov <aekorotkov@gmail.com>
2 siblings, 0 replies; 9+ messages in thread
From: Andrey Borodin @ 2026-07-27 06:55 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Paul Kim <mok03127@gmail.com>; pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
> On 23 Jul 2026, at 00:28, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Mon, Jul 20, 2026 at 10:10 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>
>>> On 20 Jul 2026, at 16:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>>>
>>> We're holding cleanup lock on posting tree root while
>>> ginScanPostingTreeToDelete(), not ginVacuumPostingTreeLeaves(). So, I
>>> think yes, vacuum_delay_point() was deleted unintentionally.
>>
>> Yup, my recollection was wrong. ginVacuumPostingTreeLeaves() seems fine for a
>> delay point.
>
> Please, check the patch attached.
I'm slightly against mentioning corruption in the commit message for two reasons:
1. With corruption any effect is possible
2. We are not dealing with corruption here, fortunately
Besides this patch LGTM. Thanks!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-07-27 06:55 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-19 14:27 Restore vacuum_delay_point() in GIN posting-tree leaf vacuum Paul Kim <mok03127@gmail.com>
2026-07-20 07:16 ` Michael Paquier <michael@paquier.xyz>
2026-07-20 10:45 ` Andrey Borodin <x4mmm@yandex-team.ru>
2026-07-20 11:59 ` Alexander Korotkov <aekorotkov@gmail.com>
2026-07-20 19:10 ` Andrey Borodin <x4mmm@yandex-team.ru>
2026-07-22 19:28 ` Alexander Korotkov <aekorotkov@gmail.com>
2026-07-23 02:26 ` Paul Kim <mok03127@gmail.com>
2026-07-26 16:15 ` solai v <solai.cdac@gmail.com>
2026-07-27 06:55 ` Andrey Borodin <x4mmm@yandex-team.ru>
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