Horton

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>, Melanie Plageman <melanieplageman@gmail.com>
Cc: 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: 2025-12-02T08:01:06Z
Lists: pgsql-hackers

Commits

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

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

  2. lwlock: Remove ForEachLWLockHeldByMe

  3. bufmgr: Implement buffer content locks independently of lwlocks

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

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

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

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

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

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

  10. bufmgr: Separate keys for private refcount infrastructure

  11. Add pg_atomic_unlocked_write_u64

  12. Rename BUFFERPIN wait event class to BUFFER

  13. bufmgr: Turn BUFFER_LOCK_* into an enum

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

  15. bufmgr: Use atomic sub for unpinning buffers

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

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

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

  19. bufmgr: fewer calls to BufferDescriptorGetContentLock

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

  21. bufmgr: Introduce FlushUnlockedBuffer

  22. Improve ReadRecentBuffer() scalability

On 25/11/2025 22:46, Andres Freund wrote:
> On 2025-11-25 15:02:02 -0500, Melanie Plageman wrote:
>> On Tue, Nov 25, 2025 at 11:54 AM Andres Freund <andres@anarazel.de> wrote:
>>>> --- a/src/backend/storage/freespace/freespace.c
>>>> +++ b/src/backend/storage/freespace/freespace.c
>>>> @@ -904,14 +904,22 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
>>>>      max_avail = fsm_get_max_avail(page);
>>>>      /*
>>>> -    * 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 a lock here, nor 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 a lock here, nor with
>>>> +    * marking the page dirty if it wasn't already, since this is just a hint.
>>>> +    *
>>>> +    * To be allowed to update the page without an exclusive lock, we have to
>>>> +    * use the hint bit infrastructure.
>>>>       */
>>>>
>>>> What the heck? This didn't even take a share lock before...
>>>
>>> It is insane. I do not understand how anybody thought this was ok.  I think I
>>> probably should split this out into a separate commit, since it's so insane.
>>
>> Aren't you going to backport having it take a lock? Then it can be
>> separate (e.g. have it take a share lock in the "fix" commit and then
>> this commit bumps it to share-exclusive).
> 
> I wasn't thinking we should backport that. It's been this way for ages,
> without having caused known issues....

I wrote that originally :-). The FSM code always treats the FSM page 
contents as potentially corrupted garbage. FSM page updates are not 
WAL-logged, for starters. And as the comment says, the next slot pointer 
is just a hint for where within the page to start looking for free space.

Page checksums were added later, and now we know about the problems of 
modifying a page a write() is in progress, on some filesystems with 
filesystem-level checksums. So it's a good idea to tighten it up, but it 
was fine back then.

>>>> diff --git a/src/backend/storage/freespace/fsmpage.c
>>>> b/src/backend/storage/freesp>
>>>> /*
>>>> * Update the next-target pointer. Note that we do this even if we're only
>>>> * holding a shared lock, on the grounds that it's better to use a shared
>>>> * lock and get a garbled next pointer every now and then, than take the
>>>> * concurrency hit of an exclusive lock.
>>>>
>>>> We appear to avoid the garbling now?
>>>
>>> I don't think so. Two backends concurrently can do fsm_search_avail() and
>>> one backend might set a hint to a page that is already used up by the other
>>> one. At least I think so?
>>
>> Maybe I don't know what it meant by garbled, but I thought it was
>> talking about two backends each trying to set fp_next_slot. If they
>> now have to have a share-exclusive lock and they can't both have a
>> share-exclusive lock at the same time, then it seems like that
>> wouldn't be a problem. It sounds like you may be talking about a
>> backend taking up the freespace of a page that is referred to by the
>> fp_next_slot?
> 
> Yes, a version of the latter. The value that fp_next_slot will be set to can
> be outdated by the time we actually set it, unless we do all of
> fsm_search_avail() under some form of exclusive lock - clearly not something
> desirable.

I'm pretty sure the "garbled" in the comment means the former, not the 
latter. I.e. it means that the pointer itself might become garbage. 
Would be good to update the comment if that's no longer possible.

But speaking of that: why do we not allow two processes to concurrently 
set hint bits on a page anymore?

- Heikki