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: Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>
Cc: Alexander Lakhin <exclusion@gmail.com>, Chao Li <li.evan.chao@gmail.com>, 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-29T17:27:10Z
Lists: pgsql-hackers

Attachments

Hi,

On 2026-01-24 19:54:36 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2026-01-24 16:11:47 -0500, Tom Lane wrote:
> >> In the case at hand I think it is probably driven by two recursion levels
> >> trying to acquire free space out of the same buffer.  SPGist is expecting
> >> the lower level to fail to get the lock and then go find some free space
> >> elsewhere.  Yeah, we could probably re-code it to get that outcome in
> >> another way, but why?
> 
> > Regardless of the assertion, it still feels like there may be something off
> > here. Why is the page marked as empty in the FSM, despite actually not being
> > empty?
> 
> Who said anything about its being empty?

In the crashes due to Alexander's repro that I have looked at, the page is
returned by SpGistNewBuffer()->GetFreeIndexPage(). Which afaict should only
contain empty pages. It's easy to see how that could happen with concurrency,
but there's none in the test.

I was just trying to repro this again while writing this message, and
interestingly I got the same issue in nbtree this time. Which a) confirms
Peter's statement that the "conditionally locking a buffer we already locked"
issue exists for nbtree b) makes me suspect something odd is happening around
indexfsm.


Anyway, independent of that, the behavior clearly needs to be allowed. Here's
a proposed patch.

At first I was thinking of just removing the assertion without anything else
in place - but I think that's not quite right: We could e.g. be trying to
acquire a share or share-exclusive lock when holding a share lock (or the
reverse), but we can't currently don't keep track of two different lock modes
for the same lock.  Therefore it seems safer to just define it so that
acquiring a conditional lock on a buffer that is already locked by us will
always fail, regardless of what existing lock mode we already hold.  I think
all current callers good with that.

Does that sound reasonable?

We could add support for locking the same buffer multiple times, but I don't
think it'd be worth the complexity and (small) overhead that would bring with
it?  It also seems like allowing that would make it more likely for a backend
to trample over its own state higher up in the call tree.

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