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
