[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.

Attached is v5, which takes Tom's suggestion for a spin.

0002 adds asserts at the top of vacuum_delay_point(). I picked separate asserts instead of INTERRUPTS_CAN_BE_PROCESSED(), just like Neil.

Running check-world with that assert tripped on hashbucketcleanup().

Since hashbucketcleanup is called assuming a lock is in place, no place in it is safe. Instead, I moved the delay to the per-bucket loop. It's a switch from a per-page delay to a per-bucket delay, but the per-page delay we're removing wasn't valid anyway.

Andrey, let me know if you have concerns about lock chaining/cleanup with this approach. There shouldn't be any issues since we're outside the locked function entirely.

With both patches applied, check-world passes with assertions enabled, and each patch passes on its own.

- Kevin Rocker

Hi Kevin,

Andrey, let me know if you have concerns about lock chaining/cleanup
with this approach.

I don't see a problem with the move. It is before the cleanup lock on
the primary bucket is acquired, while the lock chaining within
hashbucketcleanup() is unchanged. A bucket with a long overflow chain
can accumulate more vacuum cost before delaying, but there is no safe
boundary inside that chain. The per-bucket placement seems like the
best available option.

There is now a production report of exactly this hash problem [0]/messages/by-id/19628-c2b17d358181a1ea@postgresql.org. A
shutdown waited about six minutes for an autovacuum in
hashbucketcleanup(). Mostafa independently posted the same per-bucket
move and measured cancellation improving from about 2.08 seconds to
0.014 seconds. That confirms the placement and makes the hash fix a
backpatch candidate.

One detail in 0002: the two assertions do not check
QueryCancelHoldoffCount. Since vacuum_delay_point() is also expected to
service query cancellation, I think this should simply be:

Assert(INTERRUPTS_CAN_BE_PROCESSED());

This also expresses Tom's proposed invariant directly. Did you run
the full CI after this change? check-world on one system might be not
enough.

Best regards, Andrey Borodin.

[0]: /messages/by-id/19628-c2b17d358181a1ea@postgresql.org

Glad to hear the per-bucket logic makes sense. I reached the same conclusion: if nothing in per-page was safe, per-bucket is as close as we can get.

v6 updates the assert and rebases on 8aa49d34e977af, and full CI is clean on my fork [0]https://github.com/kjrocker/postgres/tree/delay-point-v6.

I'll flag this in CF to be backpatched to 14, let me know if there's anything else I need to do for that or if we want to try to carve out the hash fix.

- Kevin Rocker

[0]: https://github.com/kjrocker/postgres/tree/delay-point-v6

Hi Kevin,

v6 addresses my concerns. Thank you for running the full CI.

I would keep the fixes in 0001 together. They all restore the same
invariant, and the production report for hash strengthens the backpatch
case without making that particular move a separate kind of change. A
committer can choose a narrower set for a particular branch if
needed (especially wrt just comments change).

I would consider 0002 separately, though. Adding the Assert on HEAD is
useful, but backpatching the fixes does not necessarily mean that we
should risk extra buildfarm noise in all stable branches.

Two tiny commit message typos in 0001: "it's duration" should be "its
duration", and "vaccuum" should be "vacuum".

With these minor points, the patch set looks Ready for Committer to me.

Thank you!

Best regards, Andrey Borodin.

That sounds like a good plan to me, backpatch 0001 with the actual fixes and keep 0002 with the assert on master .

v7 is identical to v6 except with typos fixed.

- Kevin Rocker