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

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