Horton
Menu

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

Chao Li <li.evan.chao@gmail.com>

View thread
From: Chao Li <li.evan.chao@gmail.com>
To: Andres Freund <andres@anarazel.de>
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-16T02:36:55Z
Lists: pgsql-hackers 

> On Jan 16, 2026, at 00:43, Andres Freund <andres@anarazel.de> wrote:
> 
> 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().
> 

Technically, that for sure works. I raised the comment basically because it’s uncommon in the source tree. I just did a search for “inline” across all .c files, only found two non-static inline functions: ReadBufferExtended() and TrackNewBufferPin(), and they are all in bufmgr.c. So, adding a new one should be fine. I should have done the search yesterday while reviewing.

>> 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?

Ah, I see. We will never use a share-exclusive lock for fsm_search_avail? Currently, there are two callers of fsm_search_avail, one use a share lock, the other uses an exclusive lock. My bad.

>> 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.
> 

Yeah, I could understand the comment expresses that isn’t crucial, but just felt that’s not “explicit”. I am fine with the current comment. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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