Re: [PATCH] Let's get rid of the freelist and the buffer_strategy_lock

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. I think that autoprewarm was simply trying to at most warm NBuffers then stop. The freelist at startup was just a convenient thing 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 full barrier semantics or change the patch accordingly.

again, thank you for your time, best.

-greg

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