Hi Kirill,
Thanks again for the review. Did you get a chance to look at v2? It
removes the vacuum_delay_point() call in hashbucketcleanup() as you
suggested, and keeps the check at the top of hashbulkdelete()'s
per-bucket loop.
cfbot is green on all platforms:
https://commitfest.postgresql.org/patch/7216/The entry has been moved to the November commitfest (PG20-3). If v2
looks good to you, would you mind adding yourself as a reviewer there
and marking it Ready for Committer?
Regards,
Mostafa Nabil
Hi Kirill,
Thanks for the review.
v2 attached, i removed the vacuum_delay_point() call inside
hashbucketcleanup()'s per-page loop as you suggested. Good catch because it's dead code.
Regards,
Mostafa
Hi Kirill,
Thanks for the review.
v2 attached, i removed the vacuum_delay_point() call inside
hashbucketcleanup()'s per-page loop as you suggested. Good catch because it's dead code.
Regards,
Mostafa
On Sat, 22 Aug 2026 at 19:54, mostafa nabil
<mostafa.nabil.nafie@gmail.com> wrote:
>
> Hi Sergei,
>
> Thanks for the report and root cause analysis.
>
> Attached is a patch adding a vacuum_delay_point() call at the top of
> hashbulkdelete()'s per-bucket loop, before any buffer lock is taken.
> The function currently has no interrupt check outside the one inside
> hashbucketcleanup(), which is ineffective for the reason you already
> found (InterruptHoldoffCount stays > 0 for the whole bucket once
> LockBufferForCleanup() is called). This adds a check at the one spot
> where nothing is locked yet, so a pending shutdown or cancel is
> noticed at the next bucket boundary instead of only after the whole
> index scan finishes.
>
> No automated test: both patched and unpatched code eventually honor
> the cancel, so a TAP test would need a hardcoded time threshold,
> which risks flaking on slower CI hosts. Verified manually instead
> (script attached): on an 8M-row table with ~90% dead tuples, cancelling
> a VACUUM during the "vacuuming indexes" phase took ~2.08s on unpatched
> master vs ~0.014s with the patch.
>
> Not addressed: a single bucket with a very long overflow chain.
> hashbucketcleanup() uses lock chaining (locks the next overflow page
> before releasing the current one) to prevent a race with concurrent
> scans overtaking a partially vacuumed bucket, per the hash AM README.
> So interrupts are never truly clear during one bucket's own cleanup,
> and this patch can't help there without changing the locking scheme.
> I'd treat that as a separate, riskier follow-up.
>
> Likely a backpatch candidate (real bug in shipped versions), but
> I'll leave that call to whoever reviews this.
>
> Regards,
> Mostafa
>
I think our fix is fine, we also need to remove vacuum_delay_point
from hashbucketcleanup function, since it is ineffective if called was
Interrupt holdoff. This will follow existing coding practice, see also
how GIN vacuum works with buffer lock/vacuum_delay_point
Also, we can actually test this deterministically using injection
points, but I dont think this test is worth cpu cycles in buildfarm.
Too much for this.
--
Best regards,
Kirill Reshke
--
Mostafa Nabil Software Engineer
--
Mostafa Nabil Software Engineer
-- Mostafa Nabil Software Engineer
