Re: Unlogged rel fake lsn vs GetVictimBuffer()

On Sat, Feb 28, 2026 at 7:16 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > +BufferNeedsWALFlush(BufferDesc *bufdesc, bool exclusive_locked)
> > +{
> > + uint32 buf_state = pg_atomic_read_u64(&bufdesc->state);
> > + Buffer buffer;
> > + char *page;
> > + XLogRecPtr lsn;
> > +
> > + /*
> > + * Unlogged buffers can't need WAL flush. See FlushBuffer() for more
> > + * details on unlogged relations with LSNs.
> > + */
> > + if (!(buf_state & BM_PERMANENT))
> > + return false;
> > +
> > + buffer = BufferDescriptorGetBuffer(bufdesc);
> > + page = BufferGetPage(buffer);
> > +
> > + if (!XLogHintBitIsNeeded() || BufferIsLocal(buffer) || exclusive_locked)
> > + lsn = PageGetLSN(page);
> > + else
> > + {
> > + /* Buffer is either share locked or not locked */
> > + LockBufHdr(bufdesc);
> > + lsn = PageGetLSN(page);
> > + UnlockBufHdr(bufdesc);
> > + }
>
> I buy the exclusive_locked branch, but the rest seems a bit dubious:
>
> - BufferIsLocal(buffer) is impossible, the buffer would not be permanent, and
> I don't think we use strategy

Changed it to an assert.

> - XLogHintBitIsNeeded() isn't *that* cheap and if we dirtied the buffer, the
> relative cost of locking the buffer header isn't going to matter.

BufferGetLSNAtomic() checks XLogHintBitIsNeeded() as part of what it
calls a "fast path". I suppose in the case of BufferNeedsWALFlush() we
expect it only to be called on dirty buffers, so perhaps we don't need
to worry about being "fast". But why would XLogHintBitIsNeeded() be
slower than the buffer header lock? Is accessing the ControlFile
variable expensive or is it just the external function call?

> > diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
>
> > @@ -795,8 +800,14 @@ StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, bool from_r
> > strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf))
> > return false;
> >
> > + Assert(BufferIsLockedByMe(BufferDescriptorGetBuffer(buf)));
> > + Assert(!(pg_atomic_read_u64(&buf->state) & BM_LOCKED));
> > +
> > + if (!BufferNeedsWALFlush(buf, false))
> > + return false;
>
> It's a bit unfortunate that now you have an external function call from
> bufmgr.c (for StrategyRejectBuffer()) and then an external call back to
> bufmgr.c, just to then return back to freelist.c and then to bufmgr.c. And
> you need to re-read the buf_state in BufferNeedsWALFlush() again.
>
> I'd probably just call BufferNeedsWALFlush() in GetVictimBuffer() and pass it
> the old buf_state, to avoid having to re-fetch it.

My idea was to avoid taking the buffer header lock if it is a
non-bulkread strategy -- which you can't do if you need to call
BufferNeedsWALFlush() from bufmgr.c. You have to check
BufferNeedsWALFlush() before StrategyRejectBuffer() because
StrategyRejectBuffer() sets the buffer to InvalidBuffer in the ring.
So you think that the external function call will be more expensive
than taking the buffer header lock?

And, I find the current StrategyRejectBuffer() kind of silly and hard
to understand -- its comment mentions needing WAL flush, but, of
course it doesn't check that at all. I've changed it as you suggest,
but I did really prefer it the other way from a code readability
perspective.

- Melanie