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

On Fri, Jul 11, 2025, at 2:50 PM, Nathan Bossart wrote:
> 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].

Thank you for spending time reviewing my proposal/patch!

I briefly considered how one might use what was left after surgery to produce some similar boolean signal to no avail.
Ithink that autoprewarm was simply trying to at most warm NBuffers then stop.  The freelist at startup was just a
convenientthing to drain and get that done.  Maybe I'll try adapting autoprewarm to consider that global instead.
 

>> 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.

Good thought.  I'll review carefully and see if I can either explain here solid reasons I believe that they don't need
fullbarrier semantics or change the patch accordingly.
 

again, thank you for your time, best.

-greg

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