Re: BUG #19628: Uninterruptible vacuum during hash index processing

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

On Fri, Sep 4, 2026 at 12:47 PM mostafa nabil <mostafa(dot)nabil(dot)nafie(at)gmail(dot)com>
wrote:

> 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 Fri, Sep 4, 2026 at 12:42 PM mostafa nabil <
> mostafa(dot)nabil(dot)nafie(at)gmail(dot)com> wrote:
>
>> 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 Thu, Sep 3, 2026 at 9:48 AM Kirill Reshke <reshkekirill(at)gmail(dot)com>
>> wrote:
>>
>>> On Sat, 22 Aug 2026 at 19:54, mostafa nabil
>>> <mostafa(dot)nabil(dot)nafie(at)gmail(dot)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