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: Melanie Plageman <melanieplageman@gmail.com>
Cc: Noah Misch <noah@leadboat.com>, Heikki Linnakangas <hlinnaka@iki.fi>, 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-03-25T22:35:55Z
Lists: pgsql-hackers 
Hi,

On 2026-03-25 17:34:33 -0400, Melanie Plageman wrote:
> On Wed, Mar 11, 2026 at 6:40 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > I pushed this and many of the later patches in the series.  Here are updated
> > versions of the remaining changes.  The last two previously were one commit
> > with "WIP" in the title. The first one has, I think, not had a lot of review -
> > but it's also not a complicated change.
> 
> 0001 looks good except for the comment above PageSetChecksum() that
> says it is only for shared buffers and a stray reference to the
> no-longer-present bufToWrite variable in a comment around line 4490 in
> bufmgr.c

Thanks for catching these.

Updated the PageSetChecksum() comment to

 * Set checksum on a page.
 *
 * If the page is in shared buffers, it needs to be locked in at least
 * share-exclusive mode.
...


> 0002
> diff --git a/src/backend/access/nbtree/nbtpage.c
> b/src/backend/access/nbtree/nbtpage.c
> index cc9c45dc40c..ad700e590e8 100644
> --- a/src/backend/access/nbtree/nbtpage.c
> +++ b/src/backend/access/nbtree/nbtpage.c
> @@ -1011,24 +1011,48 @@ _bt_relandgetbuf(Relation rel, Buffer obuf,
> BlockNumber blkno, int access)
>     Assert(BlockNumberIsValid(blkno));
>     if (BufferIsValid(obuf))
> -       _bt_unlockbuf(rel, obuf);
> -   buf = ReleaseAndReadBuffer(obuf, rel, blkno);
> -   _bt_lockbuf(rel, buf, access);
> +   {
> +       if (BufferGetBlockNumber(obuf) == blkno)
> +       {
> +           /* trade in old lock mode for new lock */
> +           _bt_unlockbuf(rel, obuf);
> +           buf = obuf;
> +       }
> +       else
> +       {
> +           /* release lock and pin at once, that's a bit more efficient */
> +           _bt_relbuf(rel, obuf);
> +           buf = ReadBuffer(rel, blkno);
> +       }
> +   }
> +   else
> +       buf = ReadBuffer(rel, blkno);
> 
> Not related to this patch, but why do we unlock and relock it when
> obuf has the block we need? Couldn't we pass lock mode and then just
> do nothing if it is the right lockmode?

I think it's very unlikely that it's called at any frequency with the same
buffer and lockmode. What would be the point of calling _bt_relandgetbuf() if
that's the case.


> Setting that aside, I presume we don't need to check the fork and
> relfilelocator (as ReleaseAndReadBuffer() did) because this code knows
> it will be the same?

Yea, it's a single index, so there can't be a different relfilenode.


> 0003
> AFAICT, this does what you claim. I don't really know what else to
> look when reviewing it, if I'm being honest. As such, I diligently fed
> it through AI which suggested you may have lost a
>         VALGRIND_MAKE_MEM_NOACCESS(BufHdrGetBlock(buf), BLCKSZ);
> which sounds right to me and like something you should fix.

Good catch Melai.


> Also, I'd say this comment
> +   /*
> +    * Now okay to allow cancel/die interrupts again, were held when the lock
> +    * was acquired.
> +    */
> 
> needs a "which" after the comma to read smoothly.

Fixed.


Running it through valgrind and then will work on reading through one more
time and pushing them.

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