Restore vacuum_delay_point() in GIN posting-tree leaf vacuum
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:
v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patchtext/x-patchDownload+2-1
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]/messages/by-id/31a702a.14dd.166c1366ac1.Coremail.chjischj@163.com -- Michael, 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]: /messages/by-id/31a702a.14dd.166c1366ac1.Coremail.chjischj@163.com -- Michael
--
Michael
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.
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
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.
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:
v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patchapplication/octet-stream; name=v1-0001-Restore-vacuum_delay_point-in-GIN-posting-tree-le.patchDownload+7-1
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