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

On Tue, Jun 23, 2026 at 10:44 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> 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!

Here is the updated patch. Please take a look.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.