Re: Buffer locking is special (hints, checksums, AIO writes)

Hi,

On 2025-12-02 10:01:06 +0200, Heikki Linnakangas wrote:
> On 25/11/2025 22:46, Andres Freund wrote:
> > > > > 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.

Hm. I thought we had always assumed that 4byte values can be read/written
tear-free. Hence thinking that garbled couldn't refer to reading entire
garbage due to a concurrent write.

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

It'd make the locking a lot more complicated without much of a benefit.

The new share-exclusive lock mode only requires one additional bit of lock
state, for the single allowed holder. If we wanted a new lockmode that
prevented the page from being written out concurrently, but could be held
multiple times, we'd need at least MAX_BACKENDS bits for the lock level
allowing hint bits to be set and another lock level to acquire while writing
out the buffer.

At the same time, there seems to be little benefit in setting hint bits on a
page concurrently. A very common case is that the same hint bit(s) would be
set by multiple backends, we don't gain anything from that. And in the cases
where hint bits were intended to be set for different tuples, the window in
which that is not allowed is very narrow, and the cost of not setting right in
that moment is pretty small and the cost of not setting the hint bit right
then and there isn't high.

Makes sense?

Greetings,

Andres Freund