Conversation
github-actions
Bot
force-pushed
the
master
branch
from
14db4ac to
bad9884
Compare
github-actions
Bot
force-pushed
the
master
branch
4 times, most recently
from
b481005 to
ac73e0f
Compare
github-actions
Bot
force-pushed
the
master
branch
2 times, most recently
from
3b1d505 to
7dda555
Compare
github-actions
Bot
force-pushed
the
master
branch
3 times, most recently
from
7e77e41 to
f9645af
Compare
github-actions
Bot
force-pushed
the
master
branch
4 times, most recently
from
15913cb to
06a2a69
Compare
Keeps gburd/postgres rebased hourly on postgres/postgres master with only .github/ changes on top (sync-upstream automatic + manual). Drops the bespoke Windows dependency-builder workflow: Windows is already built and tested in CI by upstream's pg-ci.yml (Visual Studio + MinGW meson jobs), so a separate dependency prebuild that nothing consumed was redundant.
The Open Code Review system: ocr-review and ocr-model-check workflows plus .github/ocr config (LiteLLM->Bedrock Claude Opus 4.8, rule.json, context.md, pg-history.py).
📜 Change history & discussion (Agora / pg.ddx.io)The NUMA-awareness thread (Tomas Vondra, 2025-2026) touches 🧵 Related discussion
🔗 Related commits / prior art
📋 Commitfest
🧭 Context for reviewers
Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| SELECT sum(reads) AS stats_bulkreads_before | ||
| FROM pg_stat_io WHERE context = 'bulkread' \gset | ||
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset |
There was a problem hiding this comment.
The \gset variables are still named stats_bulkreads_before/stats_bulkreads_after, but this query now deliberately measures the normal context (the comment above was rewritten to say the reads are no longer bulkreads). The names now contradict what they hold. Since these exact lines are already being touched, rename them to reflect the new semantics (e.g. stats_normal_reads_before) for both \gset sites and the comparison on the following line. This also requires the matching update in contrib/amcheck/expected/check_heap.out.
| SELECT sum(reads) AS stats_bulkreads_before | |
| FROM pg_stat_io WHERE context = 'bulkread' \gset | |
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset | |
| SELECT sum(reads) AS stats_normal_reads_before | |
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset |
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This silently changes the user-visible semantics of the pg_buffercache.usagecount column. BUF_STATE_GET_USAGECOUNT returned the full clock-sweep count (0..BM_MAX_USAGE_COUNT, historically 0..5), whereas BUF_STATE_GET_COOLSTATE returns only bit 0 (0=COOL, 1=HOT). The column is still declared smallint and documented as "Clock-sweep access count", so users querying usagecount will now silently get only 0/1 with no doc or column-name update. This is a backward-incompatible behavior change that needs the documentation (pgbuffercache.sgml) updated and, arguably, the column renamed to reflect HOT/COOL semantics. (high confidence)
| { | ||
| buffers_used++; | ||
| usagecount_total += BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount_total += BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
usagecount_total now accumulates only 0/1 per buffer instead of 0..BM_MAX_USAGE_COUNT, so the derived usagecount_avg column changes range/meaning without any doc update. The SGML still labels it a clock-sweep average. Update the documentation to match the new HOT/COOL semantics. (high confidence)
| CHECK_FOR_INTERRUPTS(); | ||
|
|
||
| usage_count = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usage_count = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
pg_buffercache_usage_counts() now buckets buffers only into indices 0 and 1 (COOL/HOT); the usage_count output column will never exceed 1. This is fine for array bounds since BM_MAX_USAGE_COUNT is redefined to BUF_COOLSTATE_HOT (=1), but the SQL column name usage_count and its doc ("A possible buffer usage count") are now misleading. Update pgbuffercache.sgml and the expected regression output to reflect the reduced value domain. (high confidence)
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | ||
| } | ||
| else | ||
| allow_strat = allow_sync = false; | ||
|
|
||
| if (allow_strat) | ||
| { | ||
| /* During a rescan, keep the previous strategy object. */ | ||
| if (scan->rs_strategy == NULL) | ||
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | ||
| } | ||
| else | ||
| { | ||
| if (scan->rs_strategy != NULL) | ||
| FreeAccessStrategy(scan->rs_strategy); | ||
| scan->rs_strategy = NULL; | ||
| } | ||
| allow_sync = false; |
There was a problem hiding this comment.
The if body is now a single statement after removing the allow_strat assignment, but the braces are retained. PostgreSQL style (and pgindent-adjacent convention) omits braces around single-statement bodies. Since this block was directly modified by the diff, drop the braces to keep the code reading as if it had always been written this way:
if (!RelationUsesLocalBuffers(scan->rs_base.rs_rd) &&
scan->rs_nblocks > NBuffers / 4)
allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
else
allow_sync = false;Confidence: moderate.
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | |
| } | |
| else | |
| allow_strat = allow_sync = false; | |
| if (allow_strat) | |
| { | |
| /* During a rescan, keep the previous strategy object. */ | |
| if (scan->rs_strategy == NULL) | |
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | |
| } | |
| else | |
| { | |
| if (scan->rs_strategy != NULL) | |
| FreeAccessStrategy(scan->rs_strategy); | |
| scan->rs_strategy = NULL; | |
| } | |
| allow_sync = false; | |
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | |
| else | |
| allow_sync = false; |
| @@ -124,7 +122,6 @@ analyze_rel(Oid relid, RangeVar *relation, | |||
| elevel = DEBUG2; | |||
|
|
|||
| /* Set up static variables */ | |||
There was a problem hiding this comment.
This comment is now dangling. It solely described the removed vac_strategy = bstrategy; assignment; the only other static in this file (anl_context) is set up in do_analyze_rel, not here. Leaving an empty comment block referring to nonexistent code violates the minimal-diff/comment-accuracy discipline. Remove the comment (and the now-doubled blank line).
| read_stream_next_block(ReadStream *stream) | ||
| { | ||
| *strategy = stream->ios[0].op.strategy; | ||
| return read_stream_get_block(stream, NULL); | ||
| } |
There was a problem hiding this comment.
With strategy removed, read_stream_next_block() no longer has any distinguishing behavior: it is now an exported thin wrapper around read_stream_get_block(stream, NULL) and has no callers anywhere in the tree (verified: only the callback functions apw_read_stream_next_block and collect_corrupt_items_read_stream_next_block match by name; no invocation of read_stream_next_block(...) exists). Its sole prior reason to exist over calling the internal helper directly was reporting the strategy, which this patch deletes. Recommend removing the function (and its prototype in read_stream.h) as dead transitional scaffolding, or, if it is intentionally retained as public API, keep it clearly justified. Leaving a caller-less exported wrapper contradicts the minimal-diff / YAGNI discipline. (moderate confidence)
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | ||
| if (current >= (uint32) NBuffers) | ||
| { | ||
| wrapped = current % NBuffers; | ||
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | ||
| ¤t, wrapped)) | ||
| StrategyControl->completePasses++; |
There was a problem hiding this comment.
completePasses can be under-counted under concurrency, corrupting bgwriter pacing. StrategySyncStart() relies on nextVictimBuffer staying below 2*NBuffers so its pending-wrap compensation (nextVictimBuffer / NBuffers) is at most 1. With batching, many concurrent backends can each land a fetch_add(batch_size) before any of them reaches the spinlock, so the shared counter can grow to NBuffers + MaxBackends*batch_size. When it exceeds 2*NBuffers, wrapped = current % NBuffers subtracts multiple NBuffers-multiples but completePasses is incremented only once, permanently losing pass counts. This is reachable in the small-NBuffers case (minimum 16, batch capped at NBuffers): 2*NBuffers is easily exceeded by concurrent claims. Consider incrementing completePasses by current / NBuffers instead of a flat +1, and/or bounding growth so the counter cannot exceed 2*NBuffers.
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | |
| if (current >= (uint32) NBuffers) | |
| { | |
| wrapped = current % NBuffers; | |
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | |
| ¤t, wrapped)) | |
| StrategyControl->completePasses++; | |
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | |
| if (current >= (uint32) NBuffers) | |
| { | |
| wrapped = current % NBuffers; | |
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | |
| ¤t, wrapped)) | |
| StrategyControl->completePasses += current / NBuffers; |
| MyBatchPos = start; | ||
| MyBatchEnd = start + batch_size; |
There was a problem hiding this comment.
A batch can straddle the NBuffers boundary without any wrap being registered on the shared counter. When start < NBuffers but start + batch_size > NBuffers, the wrap branch (start >= NBuffers) is skipped, yet victim = MyBatchPos % NBuffers wraps internally as MyBatchPos crosses NBuffers. The completePasses increment for that crossing is deferred until a later fetch-add happens to return start >= NBuffers. Between those events, StrategySyncStart() sees nextVictimBuffer already advanced past NBuffers and relies solely on its nextVictimBuffer / NBuffers term to compensate. That compensation is only correct while the counter stays below 2*NBuffers (see the related finding). This coupling is fragile and should be documented or handled explicitly, since a mid-batch wrap is now the common case (batch_size defaults to 32).
| #include "storage/subsystems.h" | ||
| #include "port/pg_numa.h" |
There was a problem hiding this comment.
Include is out of group/alphabetical order: port/pg_numa.h is placed after the storage/* block. Per the tree's convention (and pgindent grouping), it belongs with the other port/* headers, immediately after port/atomics.h.
| #include "storage/subsystems.h" | |
| #include "port/pg_numa.h" | |
| #include "storage/subsystems.h" |
github-actions
Bot
force-pushed
the
master
branch
from
0b3e646 to
3ed29c4
Compare
StrategyGetBuffer() advances the shared clock hand, nextVictimBuffer, with a pg_atomic_fetch_add_u32(..., 1) on every tick. On a multi-socket system the cache line holding that counter has to travel over the interconnect on each operation, pushing a sweep tick from ~20ns (same-socket, line warm in L1/L2) into the ~100-200ns range. Under eviction pressure with hundreds of backends in StrategyGetBuffer() concurrently, that single cache line becomes the dominant cost of the sweep, visible as elevated bus-cycles and cache-misses in a perf profile. Have each backend claim a run of consecutive buffer IDs from the shared hand with a single fetch-add and then iterate through them privately. The sweep still advances through the pool in order, each buffer is still visited exactly once per complete pass, and the meaning of the clock state is unchanged; only the temporal ordering of visits within a pass changes, which the algorithm does not depend on. The contended atomic now fires roughly once per batch rather than once per buffer. The batch is one cache line's worth of clock-hand values -- PG_CACHE_LINE_SIZE / sizeof(uint32) -- capped at NBuffers so a claim can never wrap the pool more than once. Batching only helps when the counter's cache line actually bounces between sockets, so it is enabled only on multi-node NUMA hardware (pg_numa_get_max_node() >= 1); on a single socket, or where libnuma is unavailable, the batch size stays 1 and the code path is byte-identical to the stock clock sweep. Wraparound handling is adjusted: with batching, several backends can each see a fetch-add return a value past NBuffers within the same pass. Any such backend takes buffer_strategy_lock, re-reads the counter, and if it is still out of range wraps it with a single CAS and increments completePasses. StrategySyncStart() continues to see a consistent (nextVictimBuffer, completePasses) pair. This is the batched-clock-sweep idea from Jim Mlodgenski's pgsql-hackers thread, adapted to derive the batch size from the platform cache-line size. Co-authored-by: Jim Mlodgenski <mlodj@amazon.com>
Replace the 0..5 usage_count buffer-replacement policy with a cooling-stage
clock (the LeanStore / 2Q-A1 model): a buffer is simply HOT (recently used)
or COOL (an eviction candidate), with "pinned" being the existing refcount.
There is no per-buffer access counter.
- A demand-loaded page is admitted COOL (probationary), not HOT. A second
access via PinBuffer promotes it COOL -> HOT (the rescue). So a page
touched once -- a sequential scan -- fills and drains the COOL stage and
is evicted from it without ever displacing the HOT working set. Scan
resistance is intrinsic to the replacement algorithm, which is what lets a
later commit remove the BufferAccessStrategy ring buffers entirely.
- The foreground sweep in StrategyGetBuffer() reclaims an already-COOL,
unpinned buffer, pinning it with a CAS so a racing PinBuffer always wins.
Promotion (COOL -> HOT) and demotion (HOT -> COOL) are single-bit
transitions; only the eviction claim is a CAS.
- The background writer maintains the supply of COOL victims. As its LRU
scan runs ahead of the clock hand it demotes HOT buffers to COOL, so the
foreground finds a victim in a single pass rather than having to cool
buffers itself. The demotion is demand-driven -- bounded by the predicted
allocation for the next cycle -- so it stages just enough COOL buffers
without cooling the whole pool, and it is done under the buffer header
lock the scan already holds. A single second-chance reference bit
(set by PinBuffer, cleared on the bgwriter's first pass over a HOT buffer)
spares a recently-accessed buffer one cooling pass, keeping the genuinely
hot set out of the COOL stage under scan pressure. BgBufferSync also
tracks the reusable-buffer density it directly observes on a shorter
smoothing window, so a burst of probationary/scan COOL pages is followed
promptly rather than averaged away.
The 4-bit usage_count field is reinterpreted in place: bit 0 is the HOT/COOL
state (BUF_COOLSTATE_ONE), bit 1 the reference bit (BUF_REFBIT). The 64-bit
buffer-state layout -- refcount, flag and lock offsets and their StaticAsserts
-- is unchanged; only the meaning of the field and the instructions that touch
it change. A StaticAssert requires the field to be at least 2 bits wide so a
future width change cannot push the reference bit into the flag bits.
BM_MAX_USAGE_COUNT becomes BUF_COOLSTATE_HOT (1), so the pin fast path
saturates at HOT. Local (temp-table) buffers get the same two-state
treatment; being single-backend they need no background cooler.
Because the reference bit shares the field, the full 4-bit value can be 0..3;
readers that mean "the cooling state" must use BUF_STATE_GET_COOLSTATE(), which
masks to bit 0 and returns only 0 (COOL) or 1 (HOT), never the raw field.
contrib/pg_buffercache is updated accordingly: its usagecount column and the
pg_buffercache_summary average report the cooling state (0 = COOL, 1 = HOT),
and pg_buffercache_usage_counts() buckets on it. Using the raw 4-bit getter
there would index its BM_MAX_USAGE_COUNT+1 = 2-element arrays with values up
to 3 and overrun the stack; masking to the cooling bit keeps the index in
range. The reference bit is deliberately not exposed as usagecount.
Depends on the batched clock sweep from the previous commit.
The cooling-stage evictor admits demand-loaded pages COOL and promotes them to
HOT only on a second access, so a one-touch sequential scan fills and drains
the COOL stage without displacing the hot working set. Scan resistance is
therefore a property of the replacement algorithm itself, and the
BufferAccessStrategy ring buffers that previously provided it are dead weight.
Remove them end to end.
Deleted:
- the BufferAccessStrategy type and the BufferAccessStrategyType enum
(BAS_NORMAL/BULKREAD/BULKWRITE/VACUUM);
- the ring machinery in freelist.c (GetAccessStrategy[WithSize],
GetAccessStrategyBufferCount, GetAccessStrategyPinLimit,
FreeAccessStrategy, GetBufferFromRing, AddBufferToRing,
StrategyRejectBuffer, IOContextForStrategy);
- the strategy parameter from ReadBufferExtended, ReadBufferWithoutRelcache,
the ExtendBufferedRel* family, StrategyGetBuffer, read_stream_begin_*,
and every scan/vacuum/analyze/index-AM caller;
- the strategy fields on HeapScanDescData, IndexScanDescData,
BulkInsertStateData, ReadBuffersOperation, and ReadStream;
- _hash_getbuf_with_strategy (identical to _hash_getbuf without a strategy).
pg_stat_io's per-strategy IO contexts collapse: IOCONTEXT_BULKREAD,
IOCONTEXT_BULKWRITE and IOCONTEXT_VACUUM are removed, leaving IOCONTEXT_INIT
and IOCONTEXT_NORMAL. IOOP_REUSE only ever occurred while recycling a ring
buffer, so it is no longer tracked; GetVictimBuffer counts IOOP_EVICT only.
The vacuum_buffer_usage_limit GUC and the VACUUM/ANALYZE (BUFFER_USAGE_LIMIT
...) option are removed, along with the VacuumBufferUsageLimit global, the
ring-size plumbing through VacuumParams and parallel vacuum, and vacuumdb's
--buffer-usage-limit client option. read_stream's per-backend pin budget is
now enforced solely by GetPinLimit()/GetLocalPinLimit(), which already applied
and is unchanged for the (formerly universal) no-strategy case.
Documentation and the stats/amcheck regression tests are updated to drop the
removed contexts and options.
There was a problem hiding this comment.
🔍 OCR found 43 issue(s).
- 25 inline, 18 in summary (inline capped at 25)
📄 src/backend/access/nbtree/nbtree.c (L1732-L1732)
The strategy argument was removed from this ReadBufferExtended call, but the comment above still claims "we want to use a nondefault buffer access strategy." This comment is now stale and misleading -- no strategy is passed anymore. Update the comment to reflect that the buffer access strategy is now handled elsewhere (or drop that sentence). (moderate confidence)
📄 src/backend/commands/analyze.c (L124-L124)
The /* Set up static variables */ comment is now stale: this diff removed the only statement it introduced (vac_strategy = bstrategy;), leaving the comment describing nothing. Remove the dangling comment so the code reads as if it had always been written this way (minimal-diff hygiene). (high confidence)
📄 src/backend/commands/vacuum.c (L178-L179)
This removal makes VACUUM (BUFFER_USAGE_LIMIT ...) and ANALYZE (BUFFER_USAGE_LIMIT ...) a hard error. The grammar (gram.y utility_option_list) still accepts any option name as a generic DefElem, so with this branch gone the option now falls through to the unrecognized "VACUUM"/"ANALYZE" option ereport below. This is a backward-compatibility break of a documented, user-visible SQL option, and it breaks existing dump/restore scripts and tooling that pass BUFFER_USAGE_LIMIT. If the intent is to fully retire the feature, the corresponding user-facing surfaces (SQL docs in doc/src/sgml/ref/{vacuum,analyze}.sgml and psql tab-completion in tab-complete.in.c, which still advertise BUFFER_USAGE_LIMIT) must be updated in the same change so the option isn't offered while silently erroring. Confidence: high.
📄 src/backend/storage/buffer/freelist.c (L161-L163)
completePasses accounting is inconsistent with StrategySyncStart's compensation term and can double-count / go backwards.
StrategySyncStart() still returns *complete_passes = completePasses + nextVictimBuffer / NBuffers. But now:
-
This
else ifincrements completePasses for a batch that crosses the wrap point without wrapping the shared counter. The counter is then >= NBuffers, so a subsequent StrategySyncStart addsnextVictimBuffer / NBuffers(>=1) on top of the increment already applied here -> the same pass is counted twice. -
The
if (start >= NBuffers)branch above wrapscurrent % NBuffersbut increments completePasses by only 1 even whencurrentdrifted to e.g.2*NBuffers; the sync-start compensation term drops from 2 to 0 across that wrap, so the visible pass count decreases.
Because BgBufferSync does passes_delta = strategy_passes - prev_strategy_passes; strategy_delta += passes_delta*NBuffers; Assert(strategy_delta >= 0), a non-monotonic completePasses can trip that assertion and corrupts bgwriter pacing. Confidence: high. The batched wrap accounting needs to be made consistent with StrategySyncStart's nextVictimBuffer / NBuffers term (i.e. don't both pre-count the crossing here and let sync-start count it again).
📄 src/backend/storage/buffer/freelist.c (L137-L142)
This if (start >= NBuffers) wrap path can lose passes when the counter has drifted more than one NBuffers period. current may be >= 2*NBuffers under batching/contention, but wrapped = current % NBuffers collapses it and completePasses is bumped by only 1, while StrategySyncStart's nextVictimBuffer / NBuffers compensation was >1 before the wrap and becomes 0 after. Net effect: the reported completed-pass count can decrease across the wrap, which BgBufferSync's Assert(strategy_delta >= 0) does not tolerate. Confidence: high. Consider incrementing completePasses by current / NBuffers (the number of whole periods being discarded) so the (nextVictimBuffer, completePasses) pair stays monotonic.
📄 src/backend/storage/buffer/freelist.c (L324-L330)
Under force_cool, resetting trycounter = NBuffers on the ref-bit-clear transition (not just on the HOT->COOL demote) means a highly contended, all-HOT pool where PinBuffer keeps re-setting BUF_REFBIT (it does so on every pin, bufmgr.c:3302) can keep this backend clearing ref bits and resetting the counter without ever landing on a reclaimable COOL buffer. The intended "second unproductive full pass -> elog(ERROR, no unpinned buffers)" backstop can then be deferred well beyond the ~3 passes the comment claims. Consider only resetting trycounter on the actual HOT->COOL demotion (real forward progress toward a victim), and treating a bare ref-clear as non-progress so the escalation/backstop stays bounded. Confidence: moderate.
📄 src/backend/storage/buffer/bufmgr.c (L86-L86)
Dead flag: BUF_COOLED is defined here, documented in the SyncOneBuffer header, and set via result |= BUF_COOLED, but no caller ever inspects it. SyncOneBuffer's only cool_if_hot=true caller is BgBufferSync, which reacts only to BUF_WRITTEN and BUF_REUSABLE. Per the minimalism/YAGNI discipline, either wire BUF_COOLED into a consumer (e.g. bgwriter pacing/instrumentation) or drop the flag, its result |= assignment, and the doc line. (high confidence)
📄 src/backend/storage/buffer/bufmgr.c (L2320-L2325)
Comment-drift / misleading placement. This "Admit the newly loaded page COOL" comment sits directly above the unrelated BM_PERMANENT conditional, but the actual COOL admission is the removal of BUF_USAGECOUNT_ONE from the set_bits |= BM_TAG_VALID; line above (a freshly admitted buffer now has coolstate 0 == COOL). As written the comment misleads readers into thinking the BM_PERMANENT block performs the cooling admission. Move the comment to sit above the set_bits |= BM_TAG_VALID; line. (moderate confidence)
📄 src/backend/storage/buffer/bufmgr.c (L2964-L2968)
Same comment-placement issue as the other admission site: the "Admit COOL (probation)" comment is above the BM_PERMANENT conditional rather than the set_bits |= BM_TAG_VALID; line where BUF_USAGECOUNT_ONE was previously set. Relocate it above set_bits |= BM_TAG_VALID; so the comment describes the line that actually performs COOL admission. (moderate confidence)
📄 src/backend/utils/activity/pgstat_io.c (L443-L449)
This comment block is now stale/dangling. The if blocks it described (the B_CHECKPOINTER/B_BG_WRITER/autovac restrictions on the removed bulkread/bulkwrite/vacuum IOContexts) were deleted, leaving the comment sitting directly above return true; describing nothing. Per PG comment discipline (comments must describe what the code does now) and minimal-diff hygiene, remove the orphaned comment so the function reads as if it had always been written this way.
💡 Suggested change
Before:
/*
* Some BackendTypes do not currently perform any IO in certain
* IOContexts, and, while it may not be inherently incorrect for them to
* do so, excluding those rows from the view makes the view easier to use.
*/
return true;
After:
return true;
📄 src/bin/scripts/vacuumdb.c (L355-L355)
This removes the user-visible --buffer-usage-limit option (getopt entry, handler, validation, and help text), but the corresponding documentation is not removed: doc/src/sgml/ref/vacuumdb.sgml still contains a full <varlistentry> for --buffer-usage-limit (around line 135). A user-visible removal must delete the matching doc entry in the same patch, otherwise the docs describe an option the binary no longer accepts. Please remove the SGML block as part of this change. (high confidence)
📄 src/bin/scripts/vacuuming.c (L267-L272)
This removes the --buffer-usage-limit handling from vacuumdb, a user-visible option shipped since PG16. Together with the coordinated removals in vacuumdb.c and vacuuming.h, this deletes an established CLI feature. This is a backward-compatibility break: existing scripts invoking vacuumdb --buffer-usage-limit=... will now fail with an unknown-option error. Such a removal needs extraordinary justification and a design discussion on -hackers; nothing in the diff explains why. Additionally, doc/src/sgml/ref/vacuumdb.sgml still documents --buffer-usage-limit and is not part of this change set, so the docs will describe an option that no longer exists. (high confidence)
📄 src/include/storage/bufmgr.h (L357-L360)
Dead section header and whitespace churn. All prototypes that lived under "/* in freelist.c /" (GetAccessStrategy, FreeAccessStrategy) were removed, so this comment now labels nothing, and the removal left a stray double blank line before "/* inline functions /". This will trip git diff --check / pgindent style expectations. Remove the empty "/ in freelist.c */" section entirely and collapse to a single blank line.
💡 Suggested change
Before:
/* in freelist.c */
/* inline functions */
After:
/* inline functions */
📄 src/include/storage/bufmgr.h (L221-L222)
API/ABI break on widely-used exported entry points (moderate confidence on impact). ReadBufferExtended(), ReadBufferWithoutRelcache(), and the ExtendBufferedRel*() family drop their BufferAccessStrategy parameter, while GetAccessStrategy()/GetAccessStrategyWithSize()/GetAccessStrategyBufferCount()/GetAccessStrategyPinLimit()/FreeAccessStrategy() are removed outright. These are core buffer-manager APIs used pervasively by out-of-tree extensions; every such extension will fail to compile against this header. On HEAD a signature change is allowed, but wholesale removal of the strategy concept with no replacement is a substantial, non-obvious semantic change (ring buffers gone; scan/VACUUM no longer bounded to a small buffer ring). For pgsql-hackers this needs (a) a reference to the design thread / commitfest entry justifying dropping ring buffers, and (b) confirmation it is a single logical change -- as submitted it bundles at least three independent things (remove BufferAccessStrategy, replace usage_count clock sweep with HOT/COOL cooling state, add NUMA-batched clock sweep), which should be split into separately-committable, individually-bisectable patches.
📄 src/include/pgstat.h (L294-L294)
This change reduces IOCONTEXT_NUM_TYPES from 6 to 2, which shrinks the fixed-stats structs PgStat_BktypeIO/PgStat_PendingIO/PgStat_IO (dimensioned [IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES], lines 328-337). PgStat_IO is a fixed stats kind that is serialized to the persisted stats file. The comment above PGSTAT_FILE_FORMAT_ID (line 216-217) explicitly requires bumping the format ID "whenever any of these data structures change", and on startup pgstat_read_statsfile() only discards a stale file when format_id != PGSTAT_FILE_FORMAT_ID. Since PGSTAT_FILE_FORMAT_ID (0x01A5BCBC) is unchanged in this patch, a stats file written by a pre-patch server will be accepted and read with the new, smaller layout, corrupting the fixed-stats block (and desynchronizing subsequent parsing). Bump PGSTAT_FILE_FORMAT_ID. Unlike catversion.h this stamp is the author's responsibility. Confidence: high.
📄 src/include/storage/buf_internals.h (L136-L137)
BUF_STATE_GET_USAGECOUNT now has no callers anywhere in the tree -- every consumer was switched to BUF_STATE_GET_COOLSTATE. Leaving the old accessor behind is dead code and a footgun: the field it reads now mixes the cooling bit (bit 0) and the ref bit (bit 1), so any new caller that reaches for the familiar name gets a 0..3 value conflating two orthogonal flags. Remove it. (high confidence)
📄 src/include/storage/buf_internals.h (L139-L140)
pgindent-nonconforming multi-line comment: the block opens with text on the same line as /* and closes with an inline */, unlike every other multi-line comment in this header (which puts /* on its own line with asterisk-aligned continuation). This will not survive pgindent cleanly. (moderate confidence)
💡 Suggested change
Before:
/* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
* the second-chance ref bit (bit 1) does not perturb the HOT/COOL test. */
After:
/*
* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
* the second-chance ref bit (bit 1) does not perturb the HOT/COOL test.
*/
📄 src/include/storage/buf_internals.h (L191-L191)
BM_MAX_USAGE_COUNT now means "the HOT cool-state value" (=1), not a usage-count maximum, yet it is still consumed as a numeric range bound -- e.g. pg_buffercache sizes usage_counts[BM_MAX_USAGE_COUNT + 1] and iterates over it. Retaining a count-named macro for a boolean cool-state violates POLA and invites a future reader to treat it as a real max count. Prefer a name that reflects the new semantics (e.g. BM_MAX_COOLSTATE) and update call sites, rather than aliasing the old name for the single expression that happens to read naturally. (moderate confidence)
| @@ -0,0 +1,41 @@ | |||
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |||
There was a problem hiding this comment.
Non-ASCII em-dash character (—) in a source/config comment. The project's contribution standards require ASCII-only in source and diffs (no smart quotes, em-dashes, or ellipsis characters). Replace with an ASCII - (or --).
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |
| # LiteLLM proxy config - bridges Open Code Review (OpenAI protocol) to AWS Bedrock. |
| aws_region_name: os.environ/AWS_REGION | ||
|
|
||
| # "High effort" review. Claude Opus 4.8 on Bedrock uses *adaptive* thinking | ||
| # controlled by output_config.effort. Set it DIRECTLY here — NOT via |
There was a problem hiding this comment.
Non-ASCII em-dash character (—) in this comment. Replace with an ASCII - to comply with the ASCII-only rule for source and diffs.
| # controlled by output_config.effort. Set it DIRECTLY here — NOT via | |
| # controlled by output_config.effort. Set it DIRECTLY here - NOT via |
| import boto3 | ||
| from botocore.config import Config |
There was a problem hiding this comment.
The module docstring promises the script "exits 0 even on soft failures (writes a note)", and every other failure path here honors that (MCP unreachable at line 168, no tools at line 172, Bedrock error at line 212 all write a note to OUT and return). But import boto3 / from botocore.config import Config are not guarded: if the SDK is missing/unimportable this raises an uncaught ImportError, exits non-zero, and writes NO note to PG_HISTORY_OUT, leaving the downstream "Upsert PR comment" step with empty output. The workflow's || true masks the exit code but not the missing note. Wrap the import in the same try/except-and-write-note pattern used for the MCP setup for consistency with the documented contract. (moderate confidence)
| import boto3 | |
| from botocore.config import Config | |
| try: | |
| import boto3 | |
| from botocore.config import Config | |
| except Exception as e: | |
| open(OUT, "w").write(f"_pg-history: boto3/botocore unavailable: {e}_\n") | |
| print(f"boto3 unavailable: {e}") | |
| return |
| open(OUT, "w").write(body) | ||
| print(body) |
There was a problem hiding this comment.
open(OUT, "w").write(...) leaves the file handle to be closed by refcount-based GC, so written bytes are only flushed at interpreter cleanup. This pattern is repeated at every write site (lines 158, 168, 172, 220). On CPython in this short-lived script it usually works, but it is not guaranteed on other interpreters and violates the tree's resource-management/portability expectations. Prefer with open(OUT, "w") as f: f.write(...). (low confidence, minor)
| check-model: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
This job has no timeout-minutes, unlike every job in .github/workflows/pg-ci.yml (which sets it on all jobs). The OIDC credential step and the inline Python subprocess invoking the AWS CLI can hang on network/throttling/OIDC issues, letting the job run to the default 6-hour runner limit. Add a short explicit timeout for this scheduled maintenance job.
| check-model: | |
| runs-on: ubuntu-latest | |
| steps: | |
| check-model: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: |
| sync: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| issues: write |
There was a problem hiding this comment.
The sync job has no timeout-minutes. git fetch upstream master against the full-history PostgreSQL repository plus a rebase can hang on network problems, and with an hourly schedule stalled runs can pile up and burn runner minutes indefinitely. Other workflows in this repo (pg-ci.yml) set timeout-minutes; add an explicit timeout here for consistency and safety.
| sync: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write | |
| issues: write | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 | |
| permissions: | |
| contents: write | |
| issues: write |
| echo "⚠️ Local master has $DIVERGED commits not in upstream" | ||
|
|
||
| # Check commit messages for "dev setup" or "dev v" pattern | ||
| DEV_SETUP_COMMITS=$(git log --format=%s upstream/master..origin/master | grep -iE "^dev (setup|v[0-9])" | wc -l) |
There was a problem hiding this comment.
The decision to rebase and force-push over master rests on brittle string matching: grep -iE "^dev (setup|v[0-9])" on commit subjects and grep -v "^\.github/" on changed paths. A squashed commit that mixes .github/ and non-.github/ changes, a merge commit, or a commit whose subject coincidentally starts with "dev v1" can be misclassified. A false positive here permits git rebase upstream/master followed by --force-with-lease, which can silently drop legitimate local commits on master. This is a data-loss footgun — the guard protecting destructive history rewriting should be more robust than subject-line pattern matching (e.g., fail closed on any non-.github/ change rather than trusting the commit subject).
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
|
|
||
| if git rebase upstream/master; then |
There was a problem hiding this comment.
git config user.name/user.email is already set in the earlier "Configure Git" step, and git config persists across steps within a job on the runner. Re-setting it here is redundant and can be removed.
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| if git rebase upstream/master; then | |
| if git rebase upstream/master; then |
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This change alters the user-visible semantics of the usage_count column exposed by all three functions (pg_buffercache_pages, pg_buffercache_summary, pg_buffercache_usage_counts). BUF_STATE_GET_USAGECOUNT returned the full 0..N usage count, whereas BUF_STATE_GET_COOLSTATE returns only the 0/1 cooling state (bit 0 of the field). The documentation in doc/src/sgml/pgbuffercache.sgml still describes and shows a multi-valued "usage count" (e.g. the sample output table and the column descriptions "A possible buffer usage count"), and is not updated in this change. A user-visible SQL-interface change without matching docs is incomplete for a pgsql-hackers patch. Update pgbuffercache.sgml to reflect the new HOT/COOL cooling-state semantics (and consider whether the exposed column/variable naming should still be usage_count). (high confidence)
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | ||
| } | ||
| else | ||
| allow_strat = allow_sync = false; | ||
|
|
||
| if (allow_strat) | ||
| { | ||
| /* During a rescan, keep the previous strategy object. */ | ||
| if (scan->rs_strategy == NULL) | ||
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | ||
| } | ||
| else | ||
| { | ||
| if (scan->rs_strategy != NULL) | ||
| FreeAccessStrategy(scan->rs_strategy); | ||
| scan->rs_strategy = NULL; | ||
| } | ||
| allow_sync = false; |
There was a problem hiding this comment.
This diff removes the bulk-read access strategy from initscan(), but the comment above this block (lines 383-390) still reads "use a bulk-read access strategy and enable synchronized scanning ... only two behaviors to tune rather than four ... some callers need to be able to disable one or both of these behaviors". After this change there is only one behavior left here (sync scan); the bulk-read strategy no longer exists. The comment now describes code that is gone and should be updated to mention only synchronized scanning, otherwise it will mislead readers into looking for strategy handling that isn't there. (moderate confidence)
1 participant
Cooling-stage clock sweep for the buffer manager — CI check.
This PR is against gburd/postgres:master (my fork), not upstream, purely
to exercise CI on the three-patch series before posting to pgsql-hackers.
Net +510/-1642 over 83 files (almost all deletion is patch 3).
Each commit builds -Werror + cassert + injection_points and passes
regress/regress(245 tests) independently; verified on both the fork baseand a clean upstream/master rebase. See the draft cover letter for the
design reasoning and the m6i / r8i (6-node NUMA) / huge-pages / local-NVMe
real-IO benchmark data.
Not for merge — this is the review/CI vehicle for the -hackers submission.