Horton
Menu

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

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Chao Li <li.evan.chao@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Melanie Plageman <melanieplageman@gmail.com>, Matthias van de Meent <boekewurm+postgres@gmail.com>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Noah Misch <noah@leadboat.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael.paquier@gmail.com>
Date: 2026-01-15T16:43:14Z
Lists: pgsql-hackers 
Hi,

On 2026-01-15 14:22:09 +0800, Chao Li wrote:
> 3 - 0005 - bufmgr.c
> ```
> +inline void
> +MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
> ```
> 
> It’s quite uncommon to extern an inline function. I think usually if we want
> to make an inline function accessible from external, we define it “static
> inline” in a header file. So, I guess “inline” is a typo here.

It works just fine to put an inline into the function definition, even if it's
an external function. That hints the compiler to inline it inside the
translation unit. Which is useful here, because it leads to
MarkBufferDirtyHint() being inlined into BufferFinishSetHintBits().



> 4 - 0005 - bufmgr.c
> ```
> /*
> * MarkBufferDirtyHint
> *
> * Mark a buffer dirty for non-critical changes.
> *
> * This is essentially the same as MarkBufferDirty, except:
> *
> * 1. The caller does not write WAL; so if checksums are enabled, we may need
> * to write an XLOG_FPI_FOR_HINT WAL record to protect against torn pages.
> * 2. The caller might have only a share-exclusive-lock instead of an
> * exclusive-lock on the buffer's content lock.
> ```
> 
> For point 2, do we need to mention “For shared buffers”. Because this function also handles local buffer that doesn’t require a lock.

That's an old comment, just slightly rephrased. And I don't think it matters
that temp buffers don't need to be locked.



> 8 - 0005 - fsmpage.c
> ```
>  * needs to be updated. exclusive_lock_held should be set to true if the
> * caller is already holding an exclusive lock, to avoid extra work.
> ```
> 
> The function comment of fsm_search_avail() needs to be
> updated. exclusive_lock_held should be set to true if the caller is already
> holding a **share-exclusive or** exclusive lock.

Why?


> 9 - 0005 - fsmpage.c
> ```
> +		if (!exclusive_lock_held)
> +			BufferFinishSetHintBits(buf, false, true);
> ```
> 
> Nit: just curious why set the third parameter as “true”? When the second (mark_dirty) is false, the third parameter is not used at all.

That's wrong, indeed. Not because of the mark_dirty, but because they aren't
standard pages.


> 10 - 0005 - freespace.c
> ```
> -	 * Reset the next slot pointer. This encourages the use of low-numbered
> -	 * pages, increasing the chances that a later vacuum can truncate the
> -	 * relation. We don't bother with marking the page dirty if it wasn't
> -	 * already, since this is just a hint.
> +	 * Try to reset the next slot pointer. This encourages the use of
> +	 * low-numbered pages, increasing the chances that a later vacuum can
> +	 * truncate the relation. We don't bother with marking the page dirty if
> +	 * it wasn't already, since this is just a hint.
>  	 */
>  	LockBuffer(buf, BUFFER_LOCK_SHARE);
> -	((FSMPage) PageGetContents(page))->fp_next_slot = 0;
> +	if (BufferBeginSetHintBits(buf))
> +	{
> +		((FSMPage) PageGetContents(page))->fp_next_slot = 0;
> +		BufferFinishSetHintBits(buf, false, false);
> +	}
> ```
> 
> Before this patch, we unconditionally set fp_next_slot, now the setting
> might be skipped. You have add “Try to” in the comment that has explained
> the possibility of skipping setting fp_next_slot. Would it be better to add
> a brief statement for what will result in when skipping setting
> fp_next_slot? Something like “Skipping the update only affects reuse, not
> correctness”.

I don't see the point. The whole paragraph is about how this isn't crucial.


> 11 - 0005 Maybe not a problem. In nbtree.h:
> ```
> /*
> * We need to be able to tell the difference between read and write
> * requests for pages, in order to do locking correctly.
> */
> #define BT_READ BUFFER_LOCK_SHARE
> #define BT_WRITE BUFFER_LOCK_EXCLUSIVE
> ```
> 
> Can the new lock type BUFFER_LOCK_SHARE_EXCLUSIVE be used by nbt?

I don't think at the moment. If we introduce a user, we can add the
define. Orthogonally, I think stuff like BT_READ/BT_WRITE should eventually be
removed, the only thing it does is to make it harder to search the code.


> Maybe implicitly upgrading from BT_READ to BUFFER_LOCK_SHARE_EXCLUSIVE is good enough?

I don't see where that would trivially be safe.


> 12 - 0005 - heapam_visibility.c
> 
> After this commit, tuple hint bits may remain unset if we can’t obtain
> share-exclusive permission. That’s fine because hint bits are advisory and
> optional, but this is a behavior change. Would it make sense to mention this
> explicitly and briefly in the SetHintBitsExt() comment?

That's not new though, we already could skip setting hint bits due to async
commits.

Greetings,

Andres Freund

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