Horton
Menu

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

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>
Cc: Melanie Plageman <melanieplageman@gmail.com>, Noah Misch <noah@leadboat.com>, Kirill Reshke <reshkekirill@gmail.com>, Matthias van de Meent <boekewurm+postgres@gmail.com>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael.paquier@gmail.com>
Date: 2026-02-09T10:14:47Z
Lists: pgsql-hackers 
On 09/02/2026 03:52, Andres Freund wrote:
> On 2026-02-07 14:59:34 +0200, Heikki Linnakangas wrote:
>>> +/*
>>> + * Try to set a single hint bit in a buffer.
>>> + *
>>> + * This is a bit faster than BufferBeginSetHintBits() /
>>> + * BufferFinishSetHintBits() when setting a single hint bit, but slower than
>>> + * the former when setting several hint bits.
>>> + */
>>> +bool
>>> +BufferSetHintBits16(uint16 *ptr, uint16 val, Buffer buffer)
>>
>> This could use some more explanation. The point is that this does "*ptr =
>> val", if it's allowed to set hint bits. That's not obvious. And "single hint
>> bit" isn't really accurate, as you could update multiple bits in *ptr with
>> one call.
> 
> Agreed.  I updated it to
> 
>   * Try to set hint bits on a single 16bit value in a buffer.
>   *
>   * If hint bits are allowed to be set, set *ptr = val, try mark the buffer
>   * dirty and return true. Otherwise false is returned.
>   *
>   * *ptr needs to be a pointer to memory within the buffer.
>   *
>   * This is a bit faster than BufferBeginSetHintBits() /
>   * BufferFinishSetHintBits() when setting hints once in a buffer, but slower
>   * than the former when setting hint bits multiple times in the same buffer.

+1. Instead of "try mark the buffer dirty", I'd say just "mark the 
buffer dirty". The only reason it might not to mark the buffer dirty is 
that it was already marked dirty, right? I wouldn't call that a failure.

>>> 	/*
>>> 	 * If the buffer was dirty, try to write it out.  There is a race
>>> 	 * condition here, in that someone might dirty it after we released the
>>> 	 * buffer header lock above.  We will recheck the dirty bit after
>>> 	 * re-locking the buffer header.
>>> 	 */
>>
>> It's not clear what "above" means in that paragraph. Where do we release the
>> buffer header lock? In StrategyGetBuffer?
>>
>> (This is not actually new with this patch; it goes back to commit
>> 5e89985928. Before that, there was a call to PinBuffer_Locked() which
>> released the spinlock.)
> 
> Yea, looks like I should have edited the comment in that commit. Updated to:
> 
> 	 * If the buffer was dirty, try to write it out.  There is a race
> 	 * condition here, another backend could dirty the buffer between
> 	 * StrategyGetBuffer() checking that it is not in use and invalidating the
> 	 * buffer below. That's addressed by InvalidateVictimBuffer() verifying
> 	 * that the buffer is not dirty.

+1

>>> +/*
>>> + * Helper for BufferBeginSetHintBits() and BufferSetHintBits16().
>>> + *
>>> + * This checks if the current lock mode already suffices to allow hint bits
>>> + * being set and, if not, whether the current lock can be upgraded.
>>> + *
>>> + * Updates *lockstate when returning true.
>>> + */
>>> +static inline bool
>>> +SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64 *lockstate)
>>
>> Would be good to be more explicit what returning true/false here means.
> 
> Hm, ISTM that'd just end up restating the comments for
> BufferBeginSetHintBits(). Given SharedBufferBeginSetHintBits() is just an
> implementation detail for BufferBeginSetHintBits/BufferSetHintBits16, I don't
> think it's worth restating the details here.

Ok fair.

- Heikki

Commits

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

  1. bufmgr: Fix ordering of checks in PinBuffer()

  2. Use UnlockReleaseBuffer() in more places

  3. bufmgr: Make UnlockReleaseBuffer() more efficient

  4. bufmgr: Don't copy pages while writing out

  5. Fix use of wrong variable in _hash_kill_items()

  6. Fix bug due to confusion about what IsMVCCSnapshot means

  7. bufmgr: Switch to standard order in MarkBufferDirtyHint()

  8. bufmgr: Remove the, now obsolete, BM_JUST_DIRTIED

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

  10. heapam: Don't mimic MarkBufferDirtyHint() in inplace updates

  11. bufmgr: Allow conditionally locking of already locked buffer

  12. bufmgr: Avoid spurious compiler warning after fcb9c977aa5

  13. lwlock: Remove ForEachLWLockHeldByMe

  14. lwlock: Remove support for disowned lwlwocks

  15. bufmgr: Implement buffer content locks independently of lwlocks

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

  17. lwlock: Improve local variable name

  18. lwlock: Invert meaning of LW_FLAG_RELEASE_OK

  19. bufmgr: Make definitions related to buffer descriptor easier to modify

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

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

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

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

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

  25. bufmgr: Separate keys for private refcount infrastructure

  26. Add pg_atomic_unlocked_write_u64

  27. Rename BUFFERPIN wait event class to BUFFER

  28. bufmgr: Turn BUFFER_LOCK_* into an enum

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

  30. bufmgr: Use atomic sub for unpinning buffers

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

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

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

  34. bufmgr: fewer calls to BufferDescriptorGetContentLock

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

  36. bufmgr: Introduce FlushUnlockedBuffer

  37. Improve ReadRecentBuffer() scalability