Message-ID: <e6a8f734-2198-4958-a028-aba863d4a204@iki.fi>
Date: Tue, 2 Dec 2025 10:01:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
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>
References: <6kmid26do57ykqfpvq6iieniy4djsymhrypkjccazq5g4bbe6a@2y6owwv7qpex>
 <CAEze2WgGe8vjj3jiWqUugWuwLJ9cLryaGrnASjm-yJ=tEALX2A@mail.gmail.com>
 <pmto7djq64mei53p7r5smfync2waittilhbuzc7j7lpflf2b3y@laz7r76y5pux>
 <CAEze2WjeK9CY003S4dmCugv_H4tz9AaXgnqW+wTc=BaPDg+2xg@mail.gmail.com>
 <3je3ahgf7rrmmurxo6hnlhg5d3ffwfrtjwjxd6jm5srlv5iebp@vxqk5qtgmowr>
 <3w7v3w6a57jnssokap4k7thoekig72flnyhd4wp3yftzdd7lm7@f6lpcfen6hr7>
 <6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar>
 <CAAKRu_ZcZCXNUWfhEg4qOYuRO8T1x3_gvaUFFrwgCfY0RnVhkg@mail.gmail.com>
 <3nce7i72ayzkunai6mkz24ckbxk74jodz4ua2chcdrwppxlxcd@w6x5kfkjrkru>
 <CAAKRu_bsQ5KmtAjFsOEHGZCiirp_wgTPyfAQfsyJsGU=g7rkCQ@mail.gmail.com>
 <uuukiok2b5gbbae6blokwim2ohxxvp5235epd5zrbc5d3fojb6@vsqee77athye>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <uuukiok2b5gbbae6blokwim2ohxxvp5235epd5zrbc5d3fojb6@vsqee77athye>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/e6a8f734-2198-4958-a028-aba863d4a204%40iki.fi>
Precedence: bulk

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