Re: Checkpointer write combining

> On 20 Jun 2026, at 03:13, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> Attached is v15

Hi Melanie,

I started reviewing the write-combining series and want to send feedback in
groups. This first group covers the preparatory/refactor patches that come
before any actual write combining: the ScheduleBufferTagForWriteback
cleanup, the pg_stat_io fix, the StrategyRejectBuffer/PinBuffer tweaks, and
the two victim-buffer-selection refactors.

Per-patch
=========

* "Make ScheduleBufferTagForWriteback static"
LGTM as a preparatory cleanup. Note that a later patch in the series
("Write combining for background writer") removes the function entirely.
So this intermediate static-ification is churn that goes away.

* "pg_stat_io: Count buffer eviction after invalidation"
This is a bug fix. Is it too insignificant for backpatching?

* "Make StrategyRejectBuffer encapsulate more logic"
I agree that there's no need to check from_ring in StrategyRejectBuffer().
Assert(strategy) documents it well.

Non-rejection of not-BM_PERMANENT and not needing WAL flush is a bit mind
melting due to lots of negations.
But it seems correct new heuristic. Maybe it is already in effect somewhere
else in the code, IDK.

Moving BufHdrGetBlock()/BufferGetLSN() into buf_internals.h is reasonable.
"shared buffers only" macro is now visible to more code, so it's a bit easier
to misuse on a local buffer. Not something worth bothering, just maybe Assert
somewhere... But it's fine as is.

* "Simplify victim buffer selection"
This is the big one and I think the direction is right: ClaimVictimBuffer()
loop is much easier to follow than the old goto-again control flow.

The commit message is commendably explicit about the three behavior
changes. It seems to me they're all improvements, and not adding a shared
buffer to the ring until it's fully claimed is clearly better.

These changes only matter under contention, so an experiment would be good.
I imagine something like: run several concurrent COPY FROMs (a contended
BAS_BULKWRITE workload) and compare pg_stat_io in the "bulkwrite" context
before and after the patch. The number to watch is reuses vs evictions.
If evictions go up noticeably, the strategy is giving up on its ring and
evicting from shared buffers more than before, which would work against the
ring's whole point of not polluting the buffer pool. If the ratio stays
about the same, the behavior change is harmless. I'll try to allocate time
to do this check myself, if you think such experiment is of an interest.

On your XXX ("should we keep looking in the ring when a buffer can't be
reused due to usage/pin count, instead of going to shared buffers?"): my
vote is to keep the simpler "go to the clocksweep" behavior. Re-scanning
the ring for a hot/pinned slot risks spinning on the same busy buffers,
and replacing the slot from the clocksweep is what master effectively does
today anyway.

One modular note: ClaimVictimBuffer() now performs I/O (FlushBuffer and
potentially XLogFlush) but is called from the freelist.c selection
helpers, and it's declared "For use in freelist.c but defined in bufmgr.c".
That blurs the old freelist.c (pure selection) / bufmgr.c (I/O) boundary a
bit. I'm not sure such convention exists, but anyway.

This patch folds buffer claiming into the existing StrategyGetBuffer() (so
it now both selects and claims a victim), and the very next patch then
splits StrategyGetBuffer() into GetBufferFromRing()/GetBufferFromClocksweep().
So this intermediate shape doesn't survive.

* "Refactor victim buffer selection and add helpers"
I like API with which this patch ends. GetBufferFromRing()/
GetBufferFromClocksweep() are much clearer than the old StrategyGetBuffer(),
and dropping the redundant "strategy &&" in front of from_ring seems correct.
The duplicated buf_valid + EVICT/REUSE counting between the
two helpers is the small price you declare in the message. I think that's
acceptable, though a tiny static helper for "count and return" could remove.

* "Allow PinBuffer to skip increasing usage count"
LGTM. Made me look at where usage_count actually climbs. Always +1,
capped at BM_MAX_USAGE_COUNT (5). nbtree re-pins internal pages (a splitting
insert re-pins the parent). Expected enough, the cap is just small, so those
pages sit at max.
The BufferUsageCountChange enum is more readable than the old
strategy==NULL overloading. BUC_ZERO is unused here (introduced for later
patches), which is fine to mention in the message. Two tiny nits: BUC_ONE
vs BUC_MAX_ONE are easy to mix up at a glance, and the
"strategy ? BUC_MAX_ONE : BUC_ONE" mapping is now repeated at three call
sites. A one-line helper or a comment on the enum would help.

I didn't find any correctness problems in this group. The buf_valid
staleness handling and the conditional-lock/reject/invalidate ordering
all look correct to me.

Design-level questions (looking ahead at patches not reviewed yet)
==================================================================

* eager_clean_max_batch_size vs io_combine_limit. I realize the batch-size
sizing has been discussed at length already, and I couldn't tell what was
finally settled, so treat this as a DBA-side note. In the code, eager
cleaning is already bounded by io_combine_limit (MaxWriteBatchSize), and
eager_clean_max_batch_size is an additional cap on top of that
(CurrentMaxEagerWriteBatchSize). That only becomes clear after reading
bufmgr.c. From the docs and postgresql.conf alone it does not: io_combine_limit
reads fine, but eager_clean_max_batch_size gives no hint of which "cleaning"
it means, or why it needs a separate knob next to io_combine_limit. If it
survives (the commit notes "XXX: remove development-only GUCs"), the
docs should spell out how it relates to io_combine_limit.

* Batch construction geometry. The strategy path only looks forward from the
target buffer, while the client-backend/bgwriter path looks both ways
around the target block. Isn't forward-only case leaving easy combining
on the table?

I'll follow up with the bulkwrite and checkpointer patches next.

Best regards, Andrey Borodin.