[PATCH] Fix vacuum_delay_point happening inside lock
ginInsertCleanup() calls vacuum_delay_point while still holding a lock on the current pending-list page, so the delay runs with interrupts held off.
This is the same problem 2d7f6947293 fixed in btbulkdelete() back in 2006, and it's related to 8a045f760f6 which went in last week.
The patch keeps the other vacuum_delay_point() calls that exist after the buffer is released, in both the flush branch and the main branch at the end of the loop.
I haven't seen this hang directly and this is my first contribution, so I'd appreciate a second opinion on anything. The issue exists in multiple supported branches, but should be easy to backpatch (happy to help with that).
The issue was surfaced by Opus, I've verified the logic against those two commits myself.
- Kevin Rocker
Attachments:
v1-0001-Don-t-call-vacuum_delay_point-while-holding-a-buf.patchtext/x-patch; name="=?UTF-8?Q?v1-0001-Don-t-call-vacuum=5Fdelay=5Fpoint-while-holding-a-buf.?= =?UTF-8?Q?patch?="Download+3-3
Hi Kevin,
On Sun, Aug 2, 2026 at 12:58 AM Kevin Rocker <me@kevinrocker.com> wrote:
ginInsertCleanup() calls vacuum_delay_point while still holding a lock on
the current pending-list page, so the delay runs with interrupts held off.This is the same problem 2d7f6947293 fixed in btbulkdelete() back in 2006,
and it's related to 8a045f760f6 which went in last week.The patch keeps the other vacuum_delay_point() calls that exist after the
buffer is released, in both the flush branch and the main branch at the end
of the loop.I haven't seen this hang directly and this is my first contribution, so
I'd appreciate a second opinion on anything. The issue exists in multiple
supported branches, but should be easy to backpatch (happy to help with
that).The issue was surfaced by Opus, I've verified the logic against those two
commits myself.- Kevin Rocker
Thanks for the patch. The analysis looks correct to me. But one thought:
Would it be better to move the call just after
LockBuffer(buffer, GIN_UNLOCK);
rather than remove it? The following comment explicitly notes that moving
the collected data can take significant time. Placing the call after the
unlock would allow a pending cancellation to be handled before that work,
while preserving the existing cost-delay point.
Perhaps I’m missing something, but it may be worth considering.
Best regards,
--
Ze Chen (Neil)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Neil,
The `LockBuffer(buffer, GIN_UNLOCK)` you mentioned does call the vacuum delay as part of inserting each entry to disk, so the gain would be one additional delay_point before the scan and the first insertion. That unlock is also only in the flush-to-disk path, so it's not a direct replacement for the removed one.
The unconditional part of the loop is 'processPendingPage' then release the buffer and call vacuum_delay right after the if/else. Given all that, it's probably fine as is? Let me know what you think. I've attached a patch with your suggestion as well.
- Kevin Rocker