Обсуждение: freespace buffer locking

Hi,

In the context of [1] I started looking at the freespace code, in particular
fsm_vacuum_page(), which has this comment:
    /*
     * 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.
     */

I.e. we modify the buffer without even holding a share lock on the page. That
seems ... not ok.

What if, e.g., the page were included in a WAL record? Then this would corrupt
the record checksum. Now, this normally won't happen, was the FSM isn't WAL
logged, but still. And I think there may be special circumstances where the
page is included in a WAL record, e.g. as part of an CREATE DATABASE. And
there's FreeSpaceMapPrepareTruncateRel() - which hopefully can't run
concurrently with fsm_vacuum_page(), but would seem to court WAL corruption,
if it ever did.

Besides modifying the page while not even share locked, there are a few other
oddities:


There seem to be some other oddities:
- GetRecordedFreeSpace() does fsm_get_avail() without locking
- fsm_vacuum_page() does fsm_get_avail(), fsm_get_max_avail() without locking


ISTM we clearly should take a lock in fsm_vacuum_page() to reset fp_next_slot,
that just seems like a nasty hard to find bug waiting to happen.  Changing it
to not look at the page without a lock seems a bit more challenging.

I suspect the omission of the lock in GetRecordedFreeSpace() in 15c121b3ed7e
wasn't intentional? Heikki, you probably don't remember? :).  I think we
should fix that - none of the callers look like they'd be anywhere near
frequent enough in real workloads to make that a problem?

Greetings,

Andres Freund

[1] https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7%40az2pljabhnff

On Tue, 2 Dec 2025 at 03:41, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,

Hi!

> I.e. we modify the buffer without even holding a share lock on the page. That
seems ... not ok.

I was recently confused by these lines too doing some related hacking
on my work.

>
> Now, this normally won't happen, was the FSM isn't WAL
> logged, but still. And I think there may be special circumstances where the
> page is included in a WAL record, e.g. as part of an CREATE DATABASE. And
> there's FreeSpaceMapPrepareTruncateRel() - which hopefully can't run
> concurrently with fsm_vacuum_page(), but would seem to court WAL corruption,
> if it ever did.

Yep, also extension may want to run log_newpage_range for FSM fork for
whatever reason.
Extension then should rely on that when running log_newpage_range,
other activity will not
cause any hazards or corruptions. Is it?

> Besides modifying the page while not even share locked, there are a few other
> oddities:
>
>
> There seem to be some other oddities:
> - GetRecordedFreeSpace() does fsm_get_avail() without locking
> - fsm_vacuum_page() does fsm_get_avail(), fsm_get_max_avail() without locking

IIRC, Given these all are single-byte reads will there be no actual
troubles? Like, we already can
read corrupted info from FSM fork, because this is just a "hint", and
fsm callers are ready to not-trust the results. Just my 2c

>
> ISTM we clearly should take a lock in fsm_vacuum_page() to reset fp_next_slot,
> that just seems like a nasty hard to find bug waiting to happen.  Changing it
> to not look at the page without a lock seems a bit more challenging.
>

I agree on the grounds of enforcing good examples of buf lock/pin
management along the internals.
Like, maybe all of this actually works on HEAD as expected, but I'm
not sure it is worth its additional complexity.

-- 
Best regards,
Kirill Reshke