Re: 048_vacuum_horizon_floor.pl hangs due to wakeup lost inside LockBufferForCleanup

On Tue, Jun 23, 2026 at 4:11 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2026-06-22 16:14:08 +0800, Xuneng Zhou wrote:
> > This looks like a real lost-wakeup race in LockBufferForCleanup().
>
> I don't think any wakeup is lost. The problem is that we're waiting for a
> wakeup when it is not guaranteed that one will arrive.

I think this is true.

> > The relevant sequence in failed test 048 is:
> >
> > - Session B opens a cursor and fetches one heap tuple, leaving a heap
> > buffer pinned.
> > - Session A starts VACUUM (FREEZE).
> > - VACUUM reaches that page and waits in LockBufferForCleanup().
> > - Session B later advances/closes the cursor, releasing the pin.
> > - VACUUM is expected to wake up and finish.
> >
> > In the failure, the tap test had already passed its three sql
> > assertions, but then it timed out waiting for VACUUM completion:
> >
> > ok 3 - Cursor query returned 7 from second fetch
> > poll_query_until timed out:
> > SELECT vacuum_count > 0 ...
> > last actual query output: f
> > Looks like your test exited with 29 just after 3.
> >
> > The diagnostic log shows the actual race:
> >
> > LOG: !!!LockBufferForCleanup| wakeup lost
> > CONTEXT: while scanning block 90 of relation "public.vac_horizon_floor_table"
>
> > LOG: !!!LockBufferForCleanup]| before ProcWaitForSignal
> > CONTEXT: while scanning block 90 of relation "public.vac_horizon_floor_table"
>
> I don't think this is meaningful output, as written upthread.

I agree that the output could be misleading. The missing post-wait log
seems to be an indication of 'no one wakes us after falling asleep
unnecessary' in this specific test case since no later backend
pins/unpins that same table buffer. Sure this is just the symptom.

> > We can fix this with the state returned by UnlockBufHdrExt() when
> > publishing BM_PIN_COUNT_WAITER. If the wait refcount is 1, do not
> > enter the wait path. Instead, fall through to the existing waiter-bit
> > cleanup and retry the loop to acquire the cleanup lock normally. The
> > reproducer test passed after applying the patch.
>
> Why are we retrying, when we already achieved everything we wanted? Seems we
> should just unset BM_PIN_COUNT_WAITER / wait_backend_pgprocno. I think the
> easiest way to do this would be to not unlock the buffer header lock, but just
> atomically-or-in BM_PIN_COUNT_WAITER and then recheck if we already are done,
> while still holding the header lock.

Yeah, there's no need to retry. What you proposed looks cleaner and
better. I'll update the patch as suggested. Thanks!
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.