Horton

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

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>, pgsql-hackers@postgresql.org, 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-12-01T20:28:09Z
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-11-21 12:52:38 -0500, Melanie Plageman wrote:
> > 0004: Use 64bit atomics for BufferDesc.state - at this point nothing uses the
> > additional bits yet, though.  Some annoying reformatting required to avoid
> > long lines.
>
> I noticed that the BUF_STATE_GET_REFCOUNT and BUF_STATE_GET_USAGECOUNT
> macros cast the return value to a uint32. We won't use the extra bits
> but we did bother to keep the macro result sized to the field width
> before so keeping it uint32 is probably more confusing now that state
> is 64 bit.

I can't really follow - why would we want to return a 64bit value if none of
the values ever can get anywhere near that big?


> Not related to this patch, but I noticed GetBufferDescriptor() calls
> for a uint32 and all the callers pretty much pass a signed int —
> wonder why it calls for uint32.

It's not strictly required - no Buffers exist bet INT32_MAX and
UINT32_MAX. However, GetBufferDescriptor() cannot be used for local buffers
(which would have a negative buffer id), therefore a uint32 is fine. Many of
the callers have dedicated branches to deal with local buffers and therefore
couldn't use a uint32.


> > 0006+0007: This is preparatory work for 0008, but also worthwhile on its
> > own. The private refcount stuff does show up in profiles. The reason it's
> > related is that without these changes the added information in 0008 makes that
> > worse.
>
> I found it slightly confusing that this commit appears to
> unnecessarily add the PrivateRefCountData struct (given that it
> doesn't need it to do the new parallel array thing). You could wait
> until you need it in 0008, but 0008 is big as it is, so it probably is
> fine where it is.

It seemed too annoying to whack the code around multiple times...


> in InitBufferManagerAccess(), why do you have
>
> memset(&PrivateRefCountArrayKeys, 0, sizeof(Buffer));
> seems like it should be
> memset(PrivateRefCountArrayKeys, 0, sizeof(PrivateRefCountArrayKeys));

Ugh, indeed.


> I wonder how easy it will be to keep the Buffer in sync between
> PrivateRefCountArrayKeys and the PrivateRefCountEntry — would a helper
> function help?

I don't think we really need it, the existing helper functions are where it
should be manipulated.


> ForgetPrivateRefCountEntry doesn’t clear the data member — but maybe
> it doesn’t matter...

It asserts that the fields are reset, which seems to suffice.


> in ReservePrivateRefCountEntry() there is a superfluous clear
>
> memset(&victim_entry->data, 0, sizeof(victim_entry->data));
> victim_entry->data.refcount = 0;

It's indeed superfluous today. I guess I put it in as a belt and suspenders
approach to future members... The compiler is easily be able to optimize that
redundancy away.


> 0007
> needs a commit message. overall seems fine though.

This is what I've since written:

    bufmgr: Add one-entry cache for private refcount

    The private refcount entry for a buffer is often looked up repeatedly for the
    same buffer, e.g. to pin and then unpin a buffer. Benchmarking shows that it's
    worthwhile to have a one-entry cache for that case. With that cache in place,
    it's worth splitting GetPrivateRefCountEntry() into a small inline
    portion (for the cache hit case) and an out-of-line helper for the rest.

    This is helpful for some workloads today, but becomes more important in an
    upcoming patch that will utilize the private refcount infrastructure to also
    store whether the buffer is currently locked, as that increases the rate of
    lookups substantially.



> You should probably capitalize the "c" of "count" in
> PrivateRefcountEntryLast to be consistent with the other names.

Ooops, yes.

Greetings,

Andres Freund