Re: [PATCH] Batched clock sweep to reduce cross-socket atomic contention

Hi,

v6 is attached, rebased on master (d7cd5d6).

0001 Batch the clock sweep to reduce nextVictimBuffer atomic contention
0002 Replace the usage_count clock sweep with a cooling-stage evictor
0003 Remove BufferAccessStrategy; scan resistance is now intrinsic

Two kinds of change since v5: three correctness fixes, and one bgwriter
tweak (folded into 0002) that deals with a regression I found while chasing
down whether removing StrategyRejectBuffer would hurt. Most of this mail is
that second thing, with numbers, because it is the part I most want picked
apart.

The three fixes first, briefly:

stats.out was a real bug. Once the rings are gone in 0003, pg_stat_io's
"reuses" column is NULL for every row, so the reset test's sum(reuses) came
back NULL, the \gset made an empty variable, and the next line died with
"syntax error at or near ':'". I dropped the now-meaningless reuses term
from the reset-test sums and regenerated the expected output.

force_cool now honours the reference bit. When the sweep has found no COOL
victim in a full pass and starts demoting HOT buffers, it applies the same
single second-chance ref-bit rule the bgwriter uses instead of demoting
unconditionally, so a buffer touched mid-pass isn't demoted a tick later.
And completePasses now counts an intra-batch wrap: with 0001's batched claim
a batch can straddle the NBuffers wrap, and that wasn't being reflected, so
the bgwriter's pass accounting could drift by up to a batch.

Let's talk regression...

The buffer cache ring strategy didn't only bound cache pollution, it also
deferred writeback. A backend running a big COPY, bulk UPDATE or VACUUM
reused a small ring and, via StrategyRejectBuffer, pushed dirty victims back
for the bgwriter and checkpointer to write instead of writing (and
WAL-flushing) them inline on its own allocation path. Pull the rings (as in
0003) and, done naively, that deferral goes too.

I wrote a small benchmark to see how much that costs. It's on an AWS
m6id.metal (Sapphire Rapids, 128 vCPU, 512 GB), with the data and WAL on the
instance-local NVMe (mdraid-0), not EBS. Both builds are meson
debugoptimized, cassert off, libnuma on; stock is master at d15a6bc2e16,
patched is the series on the same base.

Server pinned with "numactl --cpunodebind=0 --interleave=all",
non-default GUCs, same for both:

shared_buffers = 8GB # small enough that a bulk op spills
huge_pages = try
maintenance_work_mem = 1GB
max_wal_size = 64GB
min_wal_size = 8GB
checkpoint_timeout = 30min
checkpoint_completion_target = 0.9
wal_level = replica
fsync = on
synchronous_commit = on
track_io_timing = on

The table is ~4GB (20M rows, 180-byte payload, fillfactor 90), half of
shared_buffers, so a whole-table dirtying op has to evict dirty victims.
Each of the three statements runs on its own fresh cluster, three times,
median reported; IO stats are reset right before each timed statement so
the write count is per-statement, not cumulative:

CREATE TABLE bulk (id int, pad text) WITH (fillfactor=90);
INSERT INTO bulk SELECT g, repeat('x',180) FROM generate_series(1,2e7) g;
VACUUM (FREEZE) bulk; CHECKPOINT;

-- (1) UPDATE, dirties every page
UPDATE bulk SET id = id + 1;

-- (2) bulk INSERT into an empty table
CREATE TABLE ins (id int, pad text);
INSERT INTO ins SELECT g, repeat('y',180) FROM generate_series(1,2e7) g;

-- (3) VACUUM after dirtying half the pages
UPDATE bulk SET pad = repeat('z',180) WHERE id % 2 = 0; CHECKPOINT;
VACUUM bulk;

One caveat up front: (2) is INSERT...SELECT, not the COPY command. It hits
the same relation-extend + WAL-logged-write path BAS_BULKWRITE served, which
is what I wanted to stress, but if that distinction matters to anyone's
conclusion I'll re-run with COPY FROM.

For each statement I record time_s (wall seconds), bwrites (buffers the
backend wrote itself -- sum(writes) from pg_stat_io where object='relation'
after a forced flush, i.e. the writes the rings used to hand off), and
wal_gb (LSN delta, just a check the two builds did the same work). Full
series, Option B included:

workload metric stock patched impact of patches
------------ --------- ---------- ---------- ------------------
bulk UPDATE time_s 26.0 27.3 5% slower
bwrites 90754 158165 74% more
wal_gb 5.83 5.90 +1%
bulk INSERT time_s 14.2 15.6 10% slower
bwrites 0 0 none; extend path
wal_gb 5.00 5.11 +2%
VACUUM time_s 11.4 9.3 19% faster
bwrites 996255 390945 61% fewer
wal_gb 6.80 6.77 -1%

Per-run time_s so you can see the spread (stock / patched):
UPDATE 29.0 25.9 26.0 / 29.4 27.2 27.3
INSERT 15.1 14.2 13.4 / 15.5 15.6 15.7
VACUUM 11.5 11.4 11.4 / 9.2 9.3 9.4

The 5% and 10% are only a couple of run-to-run wobbles apart, but the
direction is steady across runs; the VACUUM win and the bwrites jump are
well clear of the noise.

That +74% bwrites on UPDATE is the crux: the backend is doing the writeback
the rings used to defer. Before I added the bgwriter tweak it was worse --
8% slower, 77% more writes -- and VACUUM was only 8% faster rather than 19%.
Two things I tried. First, having the sweep prefer a clean COOL victim and
skip dirty ones (bounded, so it can't turn into a full-pool rescan). That
made UPDATE *slower still*, 14% over stock, because under bulk dirtying the
clean victims simply aren't there and the skipping is wasted work. I threw
that out.

What stuck instead was letting the bgwriter keep up. Its per-cycle
clean-write cap is bgwriter_lru_maxpages, default 100; under bulk dirtying
the pool fills with dirty COOL buffers faster than 100/cycle can clean, the
bgwriter stalls at the cap, and the backend is left flushing victims itself.
So when the bgwriter's own predicted demand for the next cycle already
exceeds the cap, I let the cap follow demand:

write_limit = bgwriter_lru_maxpages;
if (upcoming_alloc_est > write_limit)
write_limit = upcoming_alloc_est;

It's still bounded -- by demand and by lapping the strategy point -- so it
can't run away, and anything whose demand sits below the cap
(i.e. everything that isn't bulk-dirtying) is untouched. A small change in
BgBufferSync, folded into 0002 in v6.

That halves the UPDATE slowdown and flips VACUUM into a clear win, but it
doesn't erase the residual, so I ran one more comparison to see where the
residual actually lives: patches 0001+0002 (rings still in place, 0003 not
applied). Same instance type, same three runs:

workload metric stock 0001+0002 impact
------------ --------- ---------- ---------- ------------------
bulk UPDATE time_s 25.8 25.4 1% faster
bwrites 90797 122416 35% more
bulk INSERT time_s 14.0 15.2 8% slower
bwrites 0 ~0 extend path
VACUUM time_s 11.5 9.7 15% faster
bwrites 986865 500190 49% fewer

So with the rings still doing their deferral, the new evictor on its own is
a touch faster on UPDATE and already a solid win on VACUUM. The 5% UPDATE
slowdown in the full series is therefore the price of removing the rings
(0003) specifically -- not the evictor -- and now I can say that against a
rings-kept baseline rather than just asserting it. The bgwriter tweak moves
those writes off the critical path (why the time recovers to 5%) but doesn't
reduce their number; some of that is just the cost of not having a private
ring to write-behind from.

Bulk INSERT is the more interesting one, because it's 8% slower even with
the rings kept -- so it's not about ring removal at all, it's something in
0001+0002 on the extend/write path (the pages are extended, not evicted:
bwrites is ~0, and it's WAL-bound at +2%). 0003 adds only the last two
points. I haven't fully root-caused it; my guess is lost write coalescing
on the extend path that BULKWRITE gave for free. It's small and contained,
and since it shows up without 0003 I expect it's fixable without bringing
the strategy machinery back. I'd rather understand it than wave at it, so
I'll follow up on that specifically.

The harness is just three psql scripts and the pg_stat_io / pg_stat_wal
deltas above; happy to post them if anyone wants to reproduce.

One loose end: 0003 drops the vacuum_buffer_usage_limit GUC, which only ever
sized the BAS_VACUUM ring. With no ring to size it does nothing, so I
removed it.

So the questions I'd most like opinions on:

Is a 5%-slower bulk UPDATE (the cost of retiring the rings) and an 8-10%
slower bulk INSERT (an extend-path effect independent of 0003) an acceptable
price for making scan resistance an algorithm property and dropping the
rings? Or would you rather the rings stayed and 0002 rode alongside them --
which the isolation run shows is a real option, since 0001+0002 with the
rings kept is neutral-to-better on the eviction-bound workloads? My hope
was to offer both a better/simpler algorithm for clock-sweep as well as
remove a lot of code.

Separately: is admitting demand-loaded pages COOL (probationary, promoted on
the second touch) an acceptable basis for scan resistance in core at all?
Where is my in-cache testing flattering the sweep, and what would you want
measured instead? And is reinterpreting the usage_count field as
{HOT/COOL, ref} bits, plus collapsing pg_stat_io's contexts, something you'd
accept, or is there a representation you'd want to see first?

This idea started with the question of, "I wonder if we really need a 5
count on each buffer in the clock-sweep eviction path or if that is simply
overhead and carries no signal at all for eviction or information useful to
the bgwriter?" FWIW, I feel that the COOL/HOT clock is simplier and that
tests show it is better than the 0..5 clock. I'm not thrilled by *any*
regression, especially not one that induces more WAL traffic, so I'm open to
thoughts on this.

That's all for now.

-greg