Re: Checkpointer write combining - Mailing list pgsql-hackers

> On 20 Jun 2026, at 03:13, Melanie Plageman <melanieplageman@gmail.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.