Re: [PATCH] Let's get rid of the freelist and the buffer_strategy_lock - Mailing list pgsql-hackers

On Fri, Jul 11, 2025 at 01:26:53PM -0400, Greg Burd wrote:
> This change does remove the have_free_buffer() function used by the
> contrib/pg_prewarm module.  On the surface this doesn't seem to cause any
> issues, but honestly I've not thought too deeply on this one.

Hm.  ISTM we'll either need to invent another similarly inexpensive way to
test for this or to justify to ourselves that it's not necessary.  My guess
is that we do want to keep autoprewarm from evicting things, but FWIW the
docs already say "prewarming may also evict other data from cache" [0].

> Once removed [2] and tests passing [3] I took a long hard look at the
> buffer_strategy_lock that used to serialize concurrent access to members
> of BufferStrategyControl and I couldn't find a good reason to keep it
> around.  Let's review what it is guarding:
> 
> completePasses: a count of the number of times the clock-sweep hand wraps
> around.  StrategySyncStart() provides this to the bgwriter which in turn
> uses it to compute a strategic location at which to start scanning for
> pages to evict.  There's an interesting comment that indicates both a
> "requirement" and an equivocal "but that's highly unlikely and wouldn't
> be particularly harmful" statement conflicting with itself.  I tried to
> find a reason that nextVictimBuffers could overflow or that the use of
> the completePasses value could somehow cause harm if off by one or more
> in the bgwriter and either I missed it (please tell me) or there isn't
> one.  However, it does make sense to change completePasses into an atomic
> value so that it is consistent across backends and in the bgwriter.
> 
> bgwprocno: when not -1 is the PID of the allocation notification latch
> (ProcGlobal->allProcs[bgwprocno].procLatch).  This is a "power savings"
> feature where the goal is to signal the bgwriter "When a backend starts
> using buffers again, it will wake us up by setting our latch."  Here the
> code reads, "Since we don't want to rely on a spinlock for this we force
> a read from shared memory once, and then set the latch based on that
> value." and uses INT_ACCESS_ONCE() to read the value and set the latch.
> The function StrategyNotifyBgWriter() is where bgwprocno is set, I see no
> reason to use atomic or other synchronization here.
> 
> And that's all there is to it now that the freelist is gone.  As a
> result, IMO it seems unnecessary to require a spin lock for access to
> BufferStrategyControl.

I haven't followed your line of reasoning closely yet, but I typically
recommend that patches that replace locks with atomics use functions with
full barrier semantics (e.g., pg_atomic_read_membarrier_u32(),
pg_atomic_fetch_add_u32()) to make things easier to reason about.  But that
might not be as straightforward in cases like StrategySyncStart() where we
atomically retrieve two values that are used together.  Nevertheless,
minimizing cognitive load might be nice, and there's a chance it doesn't
impact performance very much.

[0] https://www.postgresql.org/docs/devel/pgprewarm.html

-- 
nathan