[PATCH] Fix vacuum_delay_point happening inside lock

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

Hi Kevin,

On Mon, Aug 3, 2026 at 11:21 PM Kevin Rocker <me@kevinrocker.com> wrote:

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

Thanks, that makes sense. I agree that the original patch is sufficient.

The additional call could improve cancellation responsiveness in the narrow
case where an interrupt becomes pending before the flush, but that window is
probably small, and the existing per-entry delay points already cover the
long-running part. So I have no objection to keeping the patch as is.

Best regards,
--
Ze Chen (Neil)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi Kevin and Neil!

On 3 Aug 2026, at 20:20, Kevin Rocker <me@kevinrocker.com> wrote:

Looks like an interesting and useful improvement.

I've instrumenetd vacuum_delay_point() and CHECK_FOR_INTERRUPTS() with __FILE__ and
__LINE__ and found several other cases when we always check for interrupts where
we cannot actually process them. Let's make a combined patch?

There are couple of cases where I do not know what to do.

Best regards, Andrey Borodin.

Hi Andrey,

On Tue, Aug 4, 2026 at 1:45 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Looks like an interesting and useful improvement.

I've instrumenetd vacuum_delay_point() and CHECK_FOR_INTERRUPTS() with
__FILE__ and
__LINE__ and found several other cases when we always check for interrupts
where
we cannot actually process them. Let's make a combined patch?

There are couple of cases where I do not know what to do.

Thanks for the broader audit and the patch.

The GIN and ANALYZE changes look reasonable to me. In particular, the
ANALYZE change restores the ordering that existed before 041b96802ef moved
next_block() into the while condition.

I think the hash case deserves separate discussion. With the current lock
chaining, there is no lock-free per-page boundary. Moving the delay point to
the bucket boundary avoids sleeping while holding a buffer content lock and
should make little difference for short overflow chains. For a long chain,
however, the accumulated delay can be capped at four times
vacuum_cost_delay and then reset, so this may reduce overall throttling
rather than merely making it coarser.

For discussion, I have attached a second, experimental patch on top of v3.
It moves the hash delay point to the bucket boundary and adds these
assertions to vacuum_delay_point():

Assert(InterruptHoldoffCount == 0);
Assert(CritSectionCount == 0);

With assistance from OpenAI Codex (Sol), I tried this locally; it builds and
passes the core regression tests. I have not benchmarked the performance
tradeoff described above.

I am not deeply familiar with the hash AM code, so review from someone more
familiar with this area would be very welcome.

Best regards,

--
Ze Chen (Neil)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 4 Aug 2026, at 13:13, Neil Chen <carpenter.nail.cz@gmail.com> wrote:

Assert(InterruptHoldoffCount == 0);
Assert(CritSectionCount == 0);

With assistance from OpenAI Codex (Sol), I tried this locally; it builds and
passes the core regression tests. I have not benchmarked the performance
tradeoff described above.

I think that asserts are a bit too much. I observed several other
non-interruptible cases of check for interrupts. But all other cases
that I found were called sometimes without LWlocks too.

Also, make check-world is completely different beast than make check.
I will take a lot more time, but also cover a lot more cases. I used
regression tests, isolation and some subset of other tests. With Sol,
perhaps, you can scan through logs of whole make check-world.

Best regards, Andrey Borodin.

Andrey's extension makes sense to me, happy to make a combined patch.

If there are other places where this happens, we can either go one patch at a time or try to do it all together. I'm leaning towards one patch at a time. My only remaining question is if we need to backpatch this.

/messages/by-id/178447127453.110.12276981925360691905@mail.gmail.com also adjusted vacuum_delay_point calls and was backpatched back to 14. That might not be necessary in our case, but I want to ask.

- Kevin Rocker

Hi Kevin and Andery,

My second patch was intended as an experiment, with the assertions also
serving as instrumentation while testing the hash relocation. Following
your suggestion, I ran the Meson equivalent of check-world and checked the
logs with Sol; there were no assertion failures. Still, I agree that the
assertions are too strong to keep permanently.

I am happy to leave hash aside for now, and the current patch looks good to
me. I also agree that a backpatch does not seem necessary here.

Best regards,

--
Ze Chen (Neil)
HighGo Software Co., Ltd.
https://www.highgo.com/

Just bumping the patch to v4 for CFBot and pointed 'Discussion' to the thread root. Also confirmed check-world passes.

"Kevin Rocker" <me@kevinrocker.com> writes:

Just bumping the patch to v4 for CFBot and pointed 'Discussion' to the thread root. Also confirmed check-world passes.

I wonder if we could enforce (via an Assert) that vacuum_delay_point
is only called from places where an interrupt can be accepted?
We found that that wasn't practical for CHECK_FOR_INTERRUPTS itself,
but maybe we could be stricter about vacuum_delay_point placement?

regards, tom lane

On 10 Aug 2026, at 04:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wonder if we could enforce (via an Assert) that vacuum_delay_point
is only called from places where an interrupt can be accepted?

I'd say even sleeping with LWLock is not a very good idea.

Best regards, Andrey Borodin.