Re: Checkpointer write combining

On Sun, Jul 19, 2026 at 4:05 AM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
>
>
> I started reviewing the write-combining series and want to send feedback in
> groups.

Thanks for the review!

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

Yea, you're right. I dropped this from attached v16.

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

I think it's not important enough to backpatch. It become worth fixing
because of my refactor. I will probably commit it next week to master.

> Non-rejection of not-BM_PERMANENT and not needing WAL flush is a bit mind
> melting due to lots of negations.

Yes, it is hard to understand -- that is how it was already though.

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

I added an assert to it

> * "Simplify victim buffer selection"
> 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.

So, concurrent COPY FROMs could not easily hit something like this
because with multi-insert the different backends will extend the table
and then use the blocks (without adding to FSM) and there won't be any
contention for those blocks and they will be reusing their own ring
buffers without contention for those from other COPY FROMs. Even for
single-insert, the FSM should hand them out different blocks, so they
shouldn't have contention for the same buffers.

You could perhaps have a scenario where other backends are reading the
data in the same table we are bulk loading new data into -- and then
you get some contention. I was able to simulate this and get some
evictions due to concurrent readers of the buffers we were copying,
but no more with the patch than on master. I also tried it with
synchronized large sequential scans (no COPY FROM) and did not see
more evictions with my patch than on master.

I also thought about testing the scenarios where this refactor would
be beneficial and decided it was too difficult because the timing has
to line up perfectly to get
the buffer eviction to fail in the right way. I look at the refactor
as more of a way of making the code structurally clearer that has no
downside.

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

Yea, I mean we won't do more than one full revolution of the ring, but we don't
want to leave the pinned/used buffers in the ring forever -- we eventually have
to get rid of them and replace them with new shared buffers if the pinner
doesn't release. So, it's more complicated and probably not worth it.
I've dropped it.

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

That's a good point. I split the refactoring and the logic changes
into more commits and reordered them to avoid this problem and to
simplify the individual commits.

One of the new commits -- the one that changes behavior -- is still
rather large but I tried splitting it into two -- one for moving
ClaimVictimBuffer() into GetBufferFromRing() and one for moving
ClaimVictimBuffer() into GetBufferFromClocksweep() and it was ugly.
Only one of them substantially changes behavior and the intermediate
state was very odd looking. So, I kept it together even though it is a
bigger commit.

> * "Refactor victim buffer selection and add helpers"
...
> 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.

This is just under the amount of duplication I think makes sense to
put in a helper.

> * "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 do have a comment on the enum about this. Should it be more
descriptive in some way?

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

eager_clean_max_batch_size was only for development -- I thought it
would be useful for people to experiment but anyone can just add it
themselves. I've dropped it from this stack.

> * 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?

When you fill up the ring sequentially by the time you get to a buffer
to reuse, the blocks from before it should already have been cleaned
and evicted in the course of reusing slots in the ring.

The attached v16 has all of the above changes plus some larger changes
including renaming structures and functions to be more similar to read
combining and reordering the patch series.

The biggest change in the series is that I dropped the ring-sweep
approach for COPY FROM. Write combining is now done entirely through
neighbor-based eager flushing.

I dropped the ring-sweep method because, while testing write combining
for bulkread and vacuum, I found the ring-sweep approach was often
worse than neighbor-based
eager flushing alone for those strategies, for several reasons I
detail below. That made me question how much it was helping COPY FROM
in the first place — and without AIO, the answer appears to be not
much. It carried a lot of extra code complexity given that it does not
appear to provide benefits.

Note that once we have AIO writes, we will need the ring sweep method
(or something like it) for all strategies. I had an LLM layer AIO
writes on top of write combining and the ring-sweep method beats
neighbor-based write combining for all strategies.

Now onto ring-sweep vs neighbor-based write combining comparisons:

Mixed buffer hits leading to non-contiguous rings:
--
With concurrent access, a block can already be resident in shared
buffers and be already dirty. In this case it is not pulled into the
strategy ring. When this happens, the ring no longer holds a
contiguous run of blocks, so the ring-sweep method gets little
combining. But those buffers are still dirty; they're just not in the
ring. Neighbor-based flushing does combine them, because it looks at
blocks adjacent to a ring buffer regardless of whether they're in the
ring. If there is pressure for clean shared buffers, cleaning these
buffers in combined writes improves performance (neighbor-based beats
ring-sweep).

The catch is that the extra combining is cleaning buffers the strategy
won't reuse (they're not in our ring). That's only beneficial under
memory pressure, when those buffers would have had to be written out
by someone anyway. If there isn't memory pressure, we are writing out
buffers that don't need to be written out.

I tested this for vacuum in a scenario with no memory pressure where
the buffer isn't immediately dirtied again (random pgbench-style
dirtying of pages), it just costs a little extra bandwidth but didn't
affect TPS or vacuum duration.

However, in the worst case where it does dirty the buffer again, I did
see it affect vacuum duration (but not TPS). I built a workload where
every 16th block of the table is unmodified and the rest are
constantly modified by a concurrent workload. On the first vacuum of
the table, the unmodified blocks are read into the ring while the
modified ones stay in shared buffers. Once the ring fills and we reuse
a buffer, neighbor-based flushing writes out all of that buffer's
dirty neighbors that aren't in the ring -- but those are being
constantly modified, so the write-out is wasted. This did increase
vacuum time from master. It only applies to the first vacuum, though:
afterward the unmodified blocks are all-visible and get skipped so
nothing lands in the ring.

I wonder whether it's worth a heuristic to avoid this. The problem
with a usage_count filter (don't eagerly write out buffers with a high
usage count) is that it would stop us from combining in cases where we
want to. If we're vacuuming a page that was recently modified and
won't be modified again, it's good to write it out — and it will
commonly be outside the ring, where ring-sweep can't help. If we defer
writing it until the buffer is reused by some other backend for some
other purpose, we're less likely to find contiguous dirty buffers to
combine it with. When I applied a usage count filter, many of my
experiments performed worse than with it.

Timing effects with concurrent workloads:
--
Sweeping the ring and preemptively writing dirty buffers changes when
the write happens, which can interact with concurrent workloads.
Concurrent workloads may pin pages at a moment that prevents combining
or they may flush WAL enabling more combining had the strategy waited
to write out some buffers (they would be eagerly combinable and
flushable because they would not require WAL flush after the
concurrent workload flushed WAL). I generally saw the ring sweep lose
here.

Read-ahead and clipped writes:
--
Vacuum and bulkread both read ahead, so more than one buffer in the
ring is pinned at any given time. Preemptively cleaning the whole ring
can therefore produce clipped (undersized) writes. That's potentially
solvable by refusing to issue undersized writes. It would be
non-trivial to make sure this heuristic didn't stop write combining of
non fully sized writes in general and was only avoiding it due to this
read-ahead pin frontier problem.

CPU cost of the neighbor method:
--
I checked whether the extra shared-buffer lookups cost meaningful CPU.
In my bulkread experiments they increased CPU cycles only slightly. I
also found that for a sparsely dirty ring (every other buffer dirty),
the ring-sweep method pins buffers one extra time compared to the
neighbor method — once when writing them out and again when reusing
them — whereas the neighbor method reuses them under the same pin it
wrote them out under. This only applies to sparsely dirty rings, where
we aren't getting combining.

COPY FROM:
--
COPY FROM doesn't seem to benefit from ring-sweep more than the
neighbor-based method. The combining is the same, and in some narrow
cases neighbor-based may be better (e.g. single-insert COPY into an
already-dirty, resident page, which leaves the ring non-contiguous
[this is largely theoretical, I couldn't come up with an example where
this made a difference]). The extra buffer-lookup CPU seems
negligible.

When ring sweep is needed:
--
The real benefits come with AIO: starting more writes at once reduces
overall runtime for all strategies. And once we add that code, we may
decide to start writing dirty buffers out sooner — before we need to
reuse them — to cut stalls further. At that point we'd be changing the
ring code substantially anyway.

I ran so many experiments that I'm hesitant to fill this email with
numbers and reproducers — if there's a particular scenario people want
to see, let me know.
The bottom line: once we have AIO we'll want both the neighbor-based
and ring-sweep methods, and the neighbor-based method is required for
non-strategy combining regardless. The ring-sweep method is a lot of
extra code that, without AIO, has no obvious benefit. So my focus is
on implementing the neighbor-based method without any regression from
master.

- Melanie