Horton

Re: Buffer locking is special (hints, checksums, AIO writes)

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: pgsql-hackers@postgresql.org, Melanie Plageman <melanieplageman@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Noah Misch <noah@leadboat.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael.paquier@gmail.com>
Date: 2025-10-06T22:55:23Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Require share-exclusive lock to set hint bits and to flush

  2. lwlock: Remove ForEachLWLockHeldByMe

  3. bufmgr: Implement buffer content locks independently of lwlocks

  4. bufmgr: Change BufferDesc.state to be a 64-bit atomic

  5. heapam: Add batch mode mvcc check and use it in page mode

  6. freespace: Don't modify page without any lock

  7. heapam: Move logic to handle HEAP_MOVED into a helper function

  8. bufmgr: Optimize & harmonize LockBufHdr(), LWLockWaitListLock()

  9. bufmgr: Add one-entry cache for private refcount

  10. bufmgr: Separate keys for private refcount infrastructure

  11. Add pg_atomic_unlocked_write_u64

  12. Rename BUFFERPIN wait event class to BUFFER

  13. bufmgr: Turn BUFFER_LOCK_* into an enum

  14. lwlock: Fix, currently harmless, bug in LWLockWakeup()

  15. bufmgr: Use atomic sub for unpinning buffers

  16. bufmgr: Allow some buffer state modifications while holding header lock

  17. bufmgr: Fix valgrind checking for buffers pinned in StrategyGetBuffer()

  18. bufmgr: Don't lock buffer header in StrategyGetBuffer()

  19. bufmgr: fewer calls to BufferDescriptorGetContentLock

  20. bufmgr: Fix signedness of mask variable in BufferSync()

  21. bufmgr: Introduce FlushUnlockedBuffer

  22. Improve ReadRecentBuffer() scalability

Hi,

On 2025-10-04 09:05:45 +0200, Matthias van de Meent wrote:
> On Tue, 23 Sept 2025 at 00:14, Andres Freund <andres@anarazel.de> wrote:
> > On 2025-09-15 19:05:37 -0400, Andres Freund wrote:
> > > Here are the first few cleaned up patches implementing the above steps, as
> > > well as some cleanups.  I included a commit from another thread, as it
> > > conflicts with these changes, and we really should apply it - and it's
> > > arguably required to make the changes viable, as it removes one more use of
> > > PinBuffer_Locked().
> > >
> > > Another change included is to not return the buffer with the spinlock held
> > > from StrategyGetBuffer(), and instead pin the buffer in freelist.c. The reason
> > > for that is to reduce the most common PinBuffer_locked() call. By definition
> > > PinBuffer_locked() will become a bit slower due to 0003. But even without 0003
> > > it 0002 is faster than master. And the previous approach also just seems
> > > pretty unclean.   I don't love that it requires the new TrackNewBufferPin(),
> > > but I don't really have a better idea.
> > >
> > > I invite particular attention to the commit message for 0003 as well as the
> > > comment changes in buf_internals.h within.
> >
> > Robert looked at the patches while we were chatting, and I addressed his
> > feedback in this new version.
> 
> I like these changes, and have some minor comments:

Thank for reviewing!


> 0001 ensures that ReadRecentBuffer increments the usage counter, which
> someone who uses an access strategy may want to prevent. I know this
> isn't exactly new behaviour, but something I noticed anyway. Apart
> from that observation, LGTM

Are you proposing to change behaviour? Right now ReadRecentBuffer doesn't even
accept a strategy, so I don't really see this as something that needs to be
tackled at this point.

I'm not sure I see any real use cases for ReadRecentBuffer() that would
benefit from a strategy, but I very well might just not be thinking wide
enough.


> 0002 has a FIXME in a comment in GetVictimBuffer. Assuming it's about
> the comment itself needing updates

Indeed.


> , how about:
> 
> +     * Ensure, before we pin a victim buffer, that there's a free refcount
> +     * entry, and a resource owner slot for the pin.
> 
> Again, LGTM.

WFM.


> 0003's UnlockBufHdrExt:
> This is implemented with CAS, even when we only want to change bits we
> know the state of (or could know, if we spent the effort).
> Given its inline nature, wouldn't it be better to use atomic_sub
> instructions? Or is this to handle cases where the bits we want to
> (un)set might be (un)set by a concurrent process?

Yes, it's to handle concurrent changes to the buffer state.

> If the latter, could we specialize this to do a single atomic_sub
> whenever we want to change state bits that we know can be only changed
> whilst holding the spinlock?

We probably could optimize some cases as an atomic-sub, some others as an
atomic-and and others again as an atomic-or. The latter to however are
implemented as a CAS on x86 anyway...

After 0004 I don't think any of the paths using this are actually particularly
hot, so I'm somewhat doubtful it's worth to try to optimize this too much. If
there are hot paths, we really should try to work towards not even needing the
buffer header spinlock, that has a bigger impact that improving the code for
unlocking the buffer header...

Greetings,

Andres Freund