Re: freespace.c modifies buffer without any locks

On 29/10/2024 02:50, Andres Freund wrote:
> Hi,
> 
> I just noticed that fsm_vacuum_page() modifies a buffer without even holding a
> shared lock.  That quite obviously seems like a violation of the buffer
> locking protocol:
> 
>     /*
>      * 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.
>      */
>     if (BufferPrepareToSetHintBits(buf))
>     {
>         ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
>         BufferFinishSetHintBits(buf);
>     }
> 
> 
> In the commit (15c121b3ed7) adding the current freespace code, there wasn't
> even a comment remarking upon that oddity.  10 years later Tom added a
> comment, in 2b1759e2675f.
> 
> 
> I noticed this while adding a debug mode in which buffers are mprotected
> PROT_NONE/PROT_READ/PROT_READ|PROT_WRITE depending on the buffer's state.
> 
> 
> Is there any good reason to avoid a lock here? Compared to the cost of
> exclusively locking buffers during RecordAndGetPageWithFreeSpace() the cost of
> doing so during FreeSpaceMapVacuum*() seems small?

Agreed. This is a premature optimization, fsm_vacuum_page() should just 
take the lock.

> Somewhat relatedly, but I don't think I understand why it's a good idea to
> reset fp_next_slot to 0 in fsm_vacuum_page(). At least doing so
> unconditionally.

Per the comment: "This encourages the use of low-numbered pages, 
increasing the chances that a later vacuum can truncate the relation".

Yes, the next GetPageWithFreeSpace() call will need to do a little more 
work to find the first page that actually has free space, if any. But 
that seems insignificant compared to vacuum.

> When extending a relation, it seems we'll constantly reset the search back to
> the start of the range, even though we pretty much know that there's no space
> earlier in the relation - otherwise we'd not have extended.

That's a good point. Before commit a063baaced, relation extension used a 
separate UpdateFreeSpaceMap() function, which didn't reset fp_next_slot.

> And when called from FreeSpaceMapVacuumRange() we'll reset fp_next_slot to
> somewhere that wasn't actually vacuumed, afaict?

Yeah. In the context of actual VACUUM rather than relation extension, 
that seems fine; the next GetPageWithFreeSpace() call will fix it up 
quickly.

-- 
Heikki Linnakangas
Neon (https://neon.tech)