Cooling-stage clock sweep (batched sweep + HOT/COOL evictor + strategy removal)

[gburd]

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.

1. Batch the clock sweep to reduce nextVictimBuffer atomic contention
2. Replace the usage_count clock sweep with a cooling-stage evictor
3. Remove BufferAccessStrategy; scan resistance is now intrinsic

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

[github-actions]

📜 Change history & discussion (Agora / pg.ddx.io)

Confirmed. The referenced NUMA awareness thread by Tomas Vondra exists. No mailing-list discussion of "cooling"/"evictor" exists — those PR commits (cooling-stage evictor, BufferAccessStrategy removal) are not reflected on-list; only the batched clock sweep was posted. I have enough to write the report.

🧵 Related discussion

• Primary thread (confirmed): [PATCH] Batched clock sweep to reduce cross-socket atomic contention — Greg Burd, 2026-04-25 (thread_id 1769). This is the origin of commit f7173347573 ("Batch the clock sweep..."). Patch attributed to Jim Mlodgenski + Burd; batches 64 consecutive buffer IDs from nextVictimBuffer per backend to cut atomic cache-line bouncing on multi-socket/NUMA boxes. CC'd Andres Freund, Tomas Vondra, Nathan Bossart.
• Review reply from Andres Freund, 2026-04-27: Re: [PATCH] Batched clock sweep....
• Thread ran through 2026-07-14 (latest: Re: ...); by July the messages had grown to ~320 KB, suggesting the patch series expanded substantially over its life.
• Referenced prior context ([1] in the opening post): Adding basic NUMA awareness — Tomas Vondra, 2025-07 (same cache-line hotspot called out there and in Andres's pgconf.eu 2024 talk).

🔗 Related commits / prior art

• BufferAccessStrategy origin: BufferAccessStrategy for bulk insert — Robert Haas, 2008 (Tom Lane, Simon Riggs on thread). This is the mechanism commit 54a7350c5b4 removes. No on-list discussion found proposing/justifying its removal.

📋 Commitfest

• No commitfest entry found. find_entries_for_thread on the thread root and find_entries_for_author (greg@burd.me) both returned empty. Either not registered, or the mailing-list↔CF cross-reference isn't indexed. (moderate confidence it is simply unregistered)

🧭 Context for reviewers

• Only one of the three functional commits maps to a posted, reviewed patch: f7173347573 (batched clock sweep) ← thread 1769. That part has real prior discussion and buy-in from the relevant reviewers (Andres/Tomas/Nathan), and is a targeted, minimal fix with sweep order preserved.
• 2d138935dea (cooling-stage evictor) and 54a7350c5b4 (remove BufferAccessStrategy) have no traceable mailing-list thread. Searches for "cooling"/"evictor"/"usage_count clock sweep replacement" and "BufferAccessStrategy removal/scan resistance" returned nothing on pgsql-hackers. These are far more invasive than the batching change — replacing the usage_count algorithm and deleting ring-buffer scan resistance touches nearly every AM and contrib module (hence the sprawling diffstat). Treat them as unproposed/unvetted design changes, not as the batching patch that was discussed. (high confidence: no discussion exists in the index)
• The batching patch author explicitly disclosed AI-assisted (Kiro) exploration and framed the fix as coordination vs. serialization — deliberately scoped to avoid architectural change. The evictor-replacement and strategy-removal commits contradict that stated framing, so do not assume the thread's reviewers endorsed them.
• 1bcc96723d0 is local dev scaffolding (.clangd, .gdbinit, flake.nix, shell.nix, pg-aliases.sh) and should not be in an upstream submission.

_{Generated by pg-history via the Agora MCP server (pg.ddx.io).}

[github-actions]

🔍 OCR found 12 issue(s).

• 12 inline, 0 in summary
• ⚠️ 26 warning(s) during review

[github-actions]

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.

Suggested change

	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

[github-actions]

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)

[github-actions]

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)

[github-actions]

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)

[github-actions]

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.

Suggested change

	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;

[github-actions]

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

[github-actions]

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)

[github-actions]

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.

Suggested change

	current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer);
	if (current >= (uint32) NBuffers)
	{
	wrapped = current % NBuffers;
	if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer,
	&current, 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,
	&current, wrapped))
	StrategyControl->completePasses += current / NBuffers;

[github-actions]

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

[github-actions]

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.

Suggested change

	#include "storage/subsystems.h"
	#include "port/pg_numa.h"
	#include "storage/subsystems.h"

[github-actions]

🔍 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:

1. This else if increments completePasses for a batch that crosses the wrap point without wrapping the shared counter. The counter is then >= NBuffers, so a subsequent StrategySyncStart adds nextVictimBuffer / NBuffers (>=1) on top of the increment already applied here -> the same pass is counted twice.

2. The if (start >= NBuffers) branch above wraps current % NBuffers but increments completePasses by only 1 even when current drifted 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)

[github-actions]

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

Suggested change

	# LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock.
	# LiteLLM proxy config - bridges Open Code Review (OpenAI protocol) to AWS Bedrock.

[github-actions]

Non-ASCII em-dash character (—) in this comment. Replace with an ASCII - to comply with the ASCII-only rule for source and diffs.

Suggested change

	# controlled by output_config.effort. Set it DIRECTLY here — NOT via
	# controlled by output_config.effort. Set it DIRECTLY here - NOT via

[github-actions]

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)

Suggested change

	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

[github-actions]

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)

[github-actions]

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.

Suggested change

	check-model:
	runs-on: ubuntu-latest
	steps:
	check-model:
	runs-on: ubuntu-latest
	timeout-minutes: 10
	steps:

[github-actions]

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.

Suggested change

	sync:
	runs-on: ubuntu-latest
	permissions:
	contents: write
	issues: write
	sync:
	runs-on: ubuntu-latest
	timeout-minutes: 30
	permissions:
	contents: write
	issues: write

[github-actions]

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

[github-actions]

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.

Suggested change

	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

[github-actions]

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)

[github-actions]

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)

[github-actions]

🔍 OCR found 45 issue(s).

• 25 inline, 20 in summary (inline capped at 25)

---

📄 src/include/storage/bufmgr.h

This removal empties the /* in freelist.c */ section: every prototype under it (GetAccessStrategy, GetAccessStrategyWithSize, GetAccessStrategyBufferCount, GetAccessStrategyPinLimit, FreeAccessStrategy) is gone, leaving an orphaned section header followed by two blank lines and no declarations. That is leftover scaffolding; a code_search confirms bufmgr.h declares nothing else from freelist.c. Drop the stale /* in freelist.c */ comment (and the extra blank line) so the diff is minimal and the header reads as if it had always been written this way. (high confidence)

---

📄 .github/workflows/sync-upstream.yml (L123-L126)

Redundant git config user.name/user.email — these were already set in the dedicated 'Configure Git' step and are not needed again here (each step runs in the same checkout). Remove to avoid dead duplication.

---

📄 .github/workflows/sync-upstream.yml (L134-L137)

The continue-on-error: true on this step is load-bearing: the conflict path does exit 1, and this flag is what lets the downstream 'Create issue on failure' step (gated on merge_status == 'conflict') still run. This coupling is subtle; a future maintainer could remove it and silently break the issue-creation flow. Add a brief comment explaining why it must stay.

---

📄 .github/workflows/sync-upstream.yml (L47-L48)

Non-ASCII glyphs (checkmark, warning, cross-mark, siren emoji) are used throughout the echo/summary/issue strings. The project's contribution standard requires ASCII-only content in source and diffs, and these can also render inconsistently across terminals and log viewers. Replace with ASCII equivalents (e.g., [OK], WARNING:, ERROR:).

---

📄 src/backend/access/heap/heapam.c (L397-L401)

Style nit (low confidence): the if branch now contains a single statement but keeps its braces, while the else branch is unbraced. Previously the braces were justified by two assignments (allow_strat and allow_sync); after removing allow_strat the block should read as if always written this way. PostgreSQL style / pgindent convention omits braces for a single-statement if when the paired else is also single-statement. Drop the braces for consistency.

💡 Suggested change

Before:

	{
		allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
	}
	else
		allow_sync = false;

After:

		allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
	else
		allow_sync = false;

---

📄 src/backend/access/nbtree/nbtree.c (L1732-L1732)

Stale comment. This change removes the info->strategy argument from the ReadBufferExtended() call (the buffer access strategy concept is being removed tree-wide, along with IndexVacuumInfo.strategy). The comment still claims "we want to use a nondefault buffer access strategy," which no longer describes the code below it. Remove that sentence so the comment only justifies the remaining reason for not using _bt_getbuf() (the all-zero page recycling case).

Confidence: high.

---

📄 src/backend/commands/analyze.c (L124-L124)

The vac_strategy = bstrategy; assignment that this comment described has been removed, but the comment /* Set up static variables */ was left behind. It is now an orphaned, stale comment sitting over an empty block (there are no more static variables to set up in this function). Remove it to keep the diff clean; leaving orphaned comments is a common pgsql-hackers rejection point. (high confidence)

---

📄 src/backend/storage/buffer/README (L228-L231)

This README change is incomplete relative to the code change it accompanies. The diff in freelist.c does two things beyond removing the buffer ring: (1) it rewrites ClockSweepTick to claim batches of clock-hand values via a NUMA-gated batch size, and (2) it replaces the usage-counter clock sweep with a two-state HOT/COOL cooling-stage algorithm (BUF_COOLSTATE_COOL/HOT + a second-chance BUF_REFBIT, plus the force_cool escalation). Deleting the ring section here is correct, but the immediately preceding "Normal Buffer Replacement Strategy" section (unchanged, lines 172-203) still describes the old algorithm: it references a per-buffer "usage counter" incremented "up to a small limit value", a "nonzero usage count" test, and "Decrement its usage count", and states buffer_strategy_lock is obtained on every tick. None of that matches the new code, so the README is now stale. Documentation must describe what the code does now; please update the "Normal Buffer Replacement Strategy" section to describe the cooling-stage sweep and the batched clock hand instead of only deleting the ring section.

---

📄 src/backend/storage/buffer/freelist.c (L147-L148)

completePasses accounting drifts under batching, and this straddle-branch increment is a source of over-counting. The invariant StrategySyncStart() relies on is completePasses*NBuffers + nextVictimBuffer == total_raw_ticks (it returns completePasses + nextVictimBuffer/NBuffers). This branch bumps completePasses WITHOUT subtracting NBuffers from the counter, so it breaks that invariant. Concrete case (NBuffers=100, batch=32, counter=90): backend X gets start=90 -> takes this branch, completePasses++; backend Y gets start=122 (>=100) -> first branch folds 122->22-ish and completePasses++ again. Only one boundary was crossed, yet completePasses was incremented twice -> StrategySyncStart over-reports passes. Conversely the >=NBuffers branch below only ever does completePasses++ even when current/NBuffers >= 2 (possible under concurrent batched fetch-adds), so it can also under-count. The comment concedes this only affects bgwriter pacing, not correctness, but the drift is real and bidirectional; the wrap folding and the pass counter must be kept in lockstep (fold exactly one NBuffers per increment, or add current/NBuffers increments).

---

📄 src/backend/storage/buffer/freelist.c (L139-L142)

The >=NBuffers wrap path increments completePasses by exactly 1 but wrapped = current % NBuffers removes ALL multiples of NBuffers from the counter. With batching and concurrent backends, current can exceed 2*NBuffers, so current / NBuffers may be >= 2, yet only one pass is counted -- and because the counter is folded all the way down, the nextVictimBuffer / NBuffers compensation in StrategySyncStart() can no longer recover the lost passes. Increment by current / NBuffers instead of a bare ++ to keep the pass count consistent with the folded counter.

💡 Suggested change

Before:

					wrapped = current % NBuffers;
					if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer,
													   &current, wrapped))
						StrategyControl->completePasses++;

After:

					wrapped = current % NBuffers;
					if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer,
													   &current, wrapped))
						StrategyControl->completePasses += current / NBuffers;

---

📄 src/backend/storage/buffer/bufmgr.c (L2320-L2325)

This comment sits directly above the if (relpersistence == RELPERSISTENCE_PERMANENT ...) block, which sets BM_PERMANENT and has nothing to do with the cooling state. COOL admission actually happens by omission -- the previous BUF_USAGECOUNT_ONE was dropped from the set_bits |= BM_TAG_VALID; line above, so the field defaults to 0 (== BUF_COOLSTATE_COOL). As written, the comment describes an effect achieved by the preceding line's absence, not by the code it annotates, which will mislead future maintainers about where COOL admission occurs. Move the comment up to the set_bits |= BM_TAG_VALID; line (the actual admission site) or make it explicit that COOL is the default zeroed state.

---

📄 src/backend/storage/buffer/bufmgr.c (L4188-L4191)

UnlockBufHdrExt already applies the masked store atomically and returns the previous state, but here the header lock is dropped and immediately re-acquired (LockBufHdr) in both branches. This adds two extra atomic ops (unlock + relock) per buffer on the frequently-run bgwriter scan, and opens a TOCTOU window: the buf_state re-read after LockBufHdr can differ from the one the cooling decision was based on (a concurrent PinBuffer may re-promote to HOT or bump refcount between the unlock and relock). The subsequent BUF_REUSABLE test relies on the re-read state, so correctness holds, but the round-trip is avoidable churn. Consider whether the demotion can be folded into the single UnlockBufHdrExt that already terminates this function's header-lock ownership, or otherwise document why the drop-and-relock is required.

---

📄 src/backend/utils/activity/pgstat_io.c (L443-L450)

This comment block is now orphaned. The if statements it described (the B_CHECKPOINTER/B_BG_WRITER/B_AUTOVAC checks against IOCONTEXT_BULKREAD/BULKWRITE/VACUUM) were just deleted in this hunk, and no code remains under this comment -- it sits directly above the bare return true;. Leaving stale documentation that describes removed behavior is a comment-drift defect. Remove this comment block (and the trailing blank line) as part of the same change.

Confidence: high.

💡 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 (L60-L60)

This removes a shipped, documented, user-visible CLI option (--buffer-usage-limit, present since PostgreSQL 16). Removing it is a backward-compatibility break: any existing script or automation invoking vacuumdb --buffer-usage-limit=... will now fail with "unrecognized option". The vacuumingOptions.buffer_usage_limit field and all its handling in vacuuming.c are removed as well, so the capability is gone entirely. There is no justification in the change for dropping a supported feature. Unless there is an explicit deprecation/removal decision (with a -hackers thread and matching server-side changes), this should not be removed. (high confidence)

---

📄 src/bin/scripts/vacuumdb.c (L205-L207)

The option handling is removed here but doc/src/sgml/ref/vacuumdb.sgml (line ~135) still documents --buffer-usage-limit, and that file is not part of this change. Code and docs are now inconsistent. If the removal is truly intended, the SGML documentation must be updated in the same patch; otherwise make check-docs/user expectations diverge from reality. (high confidence)

---

📄 src/bin/scripts/vacuuming.h (L52-L53)

Removing this field (together with the coordinated removals in vacuumdb.c and vacuuming.c) deletes the shipped, documented --buffer-usage-limit option from vacuumdb. This is a user-visible backward-compatibility break: existing scripts passing --buffer-usage-limit will now fail with "unrecognized option". Such a removal needs extraordinary justification and a pgsql-hackers design discussion (Message-Id), neither of which is referenced here. Additionally, doc/src/sgml/ref/vacuumdb.sgml still documents this option (around line 135), so the docs are now stale and inconsistent with the code. If this removal is intentional, it must be justified and the documentation removed in the same change; otherwise it looks like an accidental drop. (high confidence)

---

📄 src/include/pgstat.h (L294-L294)

Reducing IOCONTEXT_NUM_TYPES from 5 to 2 shrinks the bytes/counts/times arrays in PgStat_BktypeIO (embedded in the persisted PgStat_IO struct, lines 326-344). This changes the on-disk layout of the cumulative statistics file. Per the contract stated at line 216 ("PGSTAT_FILE_FORMAT_ID should be changed whenever any of these data structures change"), PGSTAT_FILE_FORMAT_ID (line 221) must be bumped. Without it, a stats file written by an old binary is read straight into the new, smaller struct at startup, silently misreading the file and corrupting the loaded IO stats instead of discarding the stale file. Confidence: high.

---

📄 src/include/storage/buf_internals.h (L141-L142)

BUF_STATE_GET_USAGECOUNT now has zero callers in the entire tree (every reader was switched to BUF_STATE_GET_COOLSTATE), yet it is left defined here. Worse than dead code: it masks the full BUF_USAGECOUNT_MASK, which now includes the second-chance ref bit at bit 1. A future caller reaching for the "natural" accessor name would read 0..3 with mixed HOT/COOL + ref-bit meaning instead of the intended HOT/COOL. Per the minimalism/YAGNI discipline, remove this unused accessor (and, if likewise unused, BUF_USAGECOUNT_ONE) rather than leaving a latent footgun that silently conflicts with BUF_STATE_GET_COOLSTATE. Confidence: high.

---

📄 src/include/storage/buf_internals.h (L139-L140)

Comment-style nit: PostgreSQL/pgindent convention for a multi-line block comment is to put /* on its own line, with the text starting on the next line. Here the comment text begins on the same line as /*. Match the surrounding style (e.g. the BUF_COOLSTATE and BUF_REFBIT comments just above) to keep pgindent clean. Confidence: moderate.

💡 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)

Retaining the historical name BM_MAX_USAGE_COUNT for what is now a single cooling-state bit is a naming footgun: the identifier no longer describes what it holds, and it is still used as an array bound in pg_buffercache (usage_counts[BM_MAX_USAGE_COUNT + 1]) where the exposed usage_count column silently collapses from a 0..5 range to 0..1. The justification in the comment ("reads naturally" in the pin fast path) is weak given the readability cost everywhere else. Consider renaming to something like BM_MAX_COOLSTATE (or BUF_COOLSTATE_HOT directly at call sites) so the name matches the semantics. Confidence: moderate.

[github-actions]

Non-ASCII em-dash character in source. The project requires ASCII-only source (no em-dashes, smart quotes, or ellipsis). Replace the em-dash with an ASCII hyphen or --.

Suggested change

	# LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock.
	# LiteLLM proxy config -- bridges Open Code Review (OpenAI protocol) to AWS Bedrock.

[github-actions]

Non-ASCII em-dash character in source. Per the ASCII-only rule, replace the em-dash with an ASCII hyphen or --.

Suggested change

	# controlled by output_config.effort. Set it DIRECTLY here — NOT via
	# controlled by output_config.effort. Set it DIRECTLY here -- NOT via

[github-actions]

File handles are opened but never explicitly closed (open(OUT, "w").write(...)), relying on CPython refcount finalization to flush and close. This same pattern is repeated at lines 158, 168, 172, 212, and 220. Prefer a context manager so the write is deterministically flushed/closed across Python implementations, e.g. with open(OUT, "w") as f: f.write(...), or factor a small write_out(text) helper. Low severity — practical risk is small for a short-lived script — and non-blocking. (low confidence this causes a real failure)

Suggested change

	open(OUT, "w").write(body)
	with open(OUT, "w") as f:
	f.write(body)

[github-actions]

The AWS CLI subprocess result is never checked for success. On any failure (missing/expired OIDC credentials, throttling, a region where Bedrock is unavailable, IAM denial), out.stdout is empty, so json.loads(out.stdout or "[]") yields [], opus is empty, and the script prints newer=false and exits 0. The failure is silently swallowed and the maintainer is never notified, and the workflow reports success. Check out.returncode and surface out.stderr (fail the step) so a broken check is visible instead of masquerading as "no newer model".

Suggested change

	ids = json.loads(out.stdout or "[]")
	if out.returncode != 0:
	print(f"::error::aws bedrock list-inference-profiles failed: {out.stderr}", file=__import__('sys').stderr)
	raise SystemExit(1)
	ids = json.loads(out.stdout or "[]")

[github-actions]

When current (vars.OCR_BEDROCK_MODEL) does not match the claude-opus-<major>-<minor> regex, ver(current) returns None and newer is forced to true. Any misconfiguration or naming-scheme change makes the workflow report a newer model every week, repeatedly opening/updating the notification issue with cur_ver=unknown. Consider treating an unparseable configured value as a distinct "cannot determine" state (e.g., emit a warning / skip the notify) rather than unconditionally signalling an upgrade.

[github-actions]

First-party actions are pinned only to mutable major tags (actions/checkout@v4 here, actions/github-script@v7 below). Because this job holds contents: write + issues: write and a token that can force-push master, a hijacked tag would run with those privileges. Per the security standard, pin to a full commit SHA (with the version as a trailing comment). Note also that sibling workflows in this repo already use newer majors (actions/checkout@v6, actions/github-script@v9); this file is inconsistent with them.

[github-actions]

The explicit token: ${{ secrets.GITHUB_TOKEN }} is redundant — actions/checkout uses GITHUB_TOKEN by default. More importantly, the default GITHUB_TOKEN is the token later used by git push origin master --force-with-lease; if branch protection is enabled on master, that push will be rejected and the sync silently reduced to a failure loop. This is worth confirming against the repo's branch protection.

[github-actions]

Comment/behavior drift: this comment plus the cron '0 * * * *' mean the workflow runs hourly, but downstream user-facing text calls it a 'daily sync' / 'Daily Sync Summary' (see the failure-issue body at line ~156 and the Summary step at line ~245). Operators reading generated issues will be misled about the actual cadence. Align the wording (either make it truly daily, or replace 'daily' with 'hourly'/'automatic' everywhere).

[github-actions]

Force-push to the primary branch is a data-loss hazard. The rebase logic above rewrites local master history and this force-push then overwrites origin/master. Whether local commits are preserved is gated by a fragile heuristic: grep -iE "^dev (setup|v[0-9])" on commit subjects and a triple-dot git diff --name-only upstream/master...origin/master (merge-base diff, not the exact rebased commit set). A misclassified or newly-landed commit on origin/master between checkout and push can be silently discarded. --force-with-lease without an explicit expected ref only guards against changes since the local fetch, not against races with a concurrent run (see the missing concurrency note). At minimum, re-fetch origin/master immediately before the push and use --force-with-lease=master:<expected-sha>.

[github-actions]

The classification heuristic here is brittle: any legitimate non-.github/ local commit whose subject does not match ^dev (setup|v[0-9]) causes a hard exit 1 (blocking all syncs), while the triple-dot merge-base diff used to compute NON_GITHUB_CHANGES may not correspond to the exact commits being rebased. This same logic is duplicated verbatim in sync-upstream-manual.yml (DRY concern). Consider a more robust gate (e.g., verifying each individual commit's changed paths) or extracting the shared logic.

[github-actions]

🔍 OCR found 17 issue(s).

• 16 inline, 1 in summary

---

📄 src/include/storage/bufmgr.h

GetAccessStrategy, GetAccessStrategyWithSize, GetAccessStrategyBufferCount, GetAccessStrategyPinLimit, and FreeAccessStrategy were exported (extension-facing) functions used by contrib and countless out-of-tree extensions (e.g. pg_prewarm, pgstattuple, custom TAMs). Removing their declarations wholesale deletes a public API with no replacement or deprecation path — any extension that constructs a BufferAccessStrategy (the overwhelmingly common pattern for bulk operations) will no longer compile or link. If the strategy subsystem is truly being retired, this must go through the pgsql-hackers process with the removal justified against ABI/API compatibility guarantees. Confidence: high.

[github-actions]

Now that only a single statement remains in this branch, the braces are inconsistent with PostgreSQL style (single-statement if bodies omit braces, matching the braceless else below). Removing the braces keeps the diff pgindent-clean and reads as if it had always been written this way.

(low confidence / style)

Suggested change

	{
	allow_strat = (scan->rs_base.rs_flags & SO_ALLOW_STRAT) != 0;
	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;

[github-actions]

Stale comment. This hunk removes the info->strategy argument from ReadBufferExtended (the strategy parameter no longer exists in the API), yet the comment directly above still cites "we want to use a nondefault buffer access strategy" as a reason for preferring ReadBufferExtended over _bt_getbuf. That rationale no longer applies. Update the comment to remove the obsolete access-strategy justification so it describes what the code does now. (high confidence)

[github-actions]

This comment is now stale/orphaned. It previously described the vac_strategy = bstrategy; assignment, which was the only "static variable" set up here. With that line removed, the comment describes nothing and should be deleted to keep the diff clean (per patch-hygiene: comments must describe code that exists now).

Confidence: high.

[github-actions]

BUF_COOLED is dead scaffolding: it is defined here and set into result in SyncOneBuffer (result |= BUF_COOLED), but no caller ever reads it. BgBufferSync, the only caller passing cool_if_hot=true, inspects only BUF_WRITTEN and BUF_REUSABLE. Per the YAGNI/minimalism discipline, drop the flag and the result |= BUF_COOLED; assignment (and the corresponding doc line) unless a consumer is added in this same patch.

[github-actions]

write_limit silently overrides the operator-configured bgwriter_lru_maxpages cap whenever upcoming_alloc_est exceeds it. That GUC is documented as the hard per-round write ceiling for the bgwriter; raising it from inside the code defeats an explicit operator knob and changes IO-scheduling behavior under bulk workloads. This is a user-visible behavior change that needs documentation (config.sgml / the GUC description) and a reproducible benchmark, and it should be justified against a -hackers thread. At minimum, POLA argues against transparently exceeding a limit the DBA set.

[github-actions]

Removing the strategy parameter from ReadBufferExtended, ReadBufferWithoutRelcache, and the ExtendBufferedRel* family silently changes the signatures of exported, extension-facing buffer manager APIs. These are among the most widely used entry points in third-party extensions, custom table access methods, and bulk loaders. Dropping the parameter is a hard ABI/API break with no compatibility shim, so every out-of-tree caller fails to compile and any prebuilt extension binary breaks. Beyond the API break, this eliminates the entire BufferAccessStrategy ring-buffer mechanism, which prevents cache-blowout for VACUUM/COPY/large seqscans; removing it is a fundamental behavioral change to the buffer manager that needs an explicit -hackers design thread, benchmarks, and extraordinary justification. Confidence: high (API break) / high (behavioral impact).

[github-actions]

After removing all GetAccessStrategy*/FreeAccessStrategy declarations, the /* in freelist.c */ section comment is now orphaned with nothing beneath it, followed by two blank lines. If freelist.c no longer exposes any prototypes here, drop the dangling section header (and the extra blank line) so the header reads as if it had always been written that way. Confidence: high (style/hygiene).

[github-actions]

BUF_STATE_GET_USAGECOUNT now has zero callers: every consumer (pg_buffercache_pages/summary/usage_counts, bufmgr.c, freelist.c, localbuf.c) was converted to BUF_STATE_GET_COOLSTATE. Retaining the unused macro is dead code. Per the YAGNI/minimalism discipline, either remove it or, if you want to keep raw field access for debugging, say so explicitly. Confidence: high.

[github-actions]

Comment inaccuracy: this says the ref bit "stays a plain masked store under the header lock", but PinBuffer() sets BUF_REFBIT inside a lock-free pg_atomic_compare_exchange_u64 CAS loop (bufmgr.c), not under the buffer header lock. Only the clear/demote side in SyncOneBuffer() runs under LockBufHdr. Comments must describe what the code does now; reword to note the set side is via the pin CAS and only the bgwriter clear side is under the header lock. Confidence: high.

[github-actions]

Style: multi-line block comments in this file (and per pgindent convention) open with /* on its own line. This one starts the text on the /* line. Also, the two adjacent bit accessors use inconsistent idioms -- BUF_STATE_GET_REFBIT masks ((state) & BUF_REFBIT) while this shifts-and-masks (>> BUF_USAGECOUNT_SHIFT) & 1). Aligning them (and the comment format) improves readability. Confidence: high (style).

[github-actions]

🔍 OCR found 35 issue(s).

• 25 inline, 10 in summary (inline capped at 25)

---

📄 src/backend/utils/activity/pgstat_io.c (L506-L508)

Use ASCII prose; the em-dash-style -- runs violate PostgreSQL's comment conventions. Rewrite as separate sentences, e.g. "Buffer access strategies (ring buffers) have been removed; scan resistance is now intrinsic to the cooling-stage clock sweep, so IOOP_REUSE never occurs." (low confidence)

💡 Suggested change

Before:

	 * Buffer access strategies (ring buffers) have been removed -- scan
	 * resistance is now intrinsic to the cooling-stage clock sweep -- so
	 * IOOP_REUSE never occurs.

After:

	 * Buffer access strategies (ring buffers) have been removed; scan
	 * resistance is now intrinsic to the cooling-stage clock sweep, so
	 * IOOP_REUSE never occurs.

---

📄 src/backend/storage/buffer/freelist.c (L317-L318)

Misleading comment (moderate confidence). It claims ~3 passes is "still within the stock clock's worst case (up to BM_MAX_USAGE_COUNT+1 passes)". In this tree BM_MAX_USAGE_COUNT was redefined to BUF_COOLSTATE_HOT (= 1), so BM_MAX_USAGE_COUNT+1 = 2 passes -- which is LESS than the ~3 passes just described, so the new algorithm is not "within" that bound. The comparison silently refers to upstream's old BM_MAX_USAGE_COUNT (5 -> 6 passes), a value that no longer exists here. Either drop the comparison or state the concrete bound in terms of this tree's constants.

---

📄 src/bin/scripts/vacuumdb.c (L60-L60)

This patch removes the user-visible --buffer-usage-limit option from vacuumdb, but the documentation for it is left intact in doc/src/sgml/ref/vacuumdb.sgml (lines 134-145), which is not part of this change. A user-visible change without a matching doc update is a hard requirement violation and will cause the docs to describe a nonexistent option. Remove the corresponding <varlistentry> in vacuumdb.sgml. (high confidence)

Separately, the underlying server feature is untouched: VACUUM (BUFFER_USAGE_LIMIT ...) SQL syntax, the vacuum_buffer_usage_limit GUC, and psql tab-completion for BUFFER_USAGE_LIMIT all still exist. Removing only the vacuumdb convenience wrapper for a fully-functional server feature is a gratuitous loss of user-facing functionality; the rationale for this removal should be justified in the commit message / -hackers thread. (moderate confidence)

---

📄 src/include/pgstat.h (L294-L294)

Shrinking the IOContext enum changes IOCONTEXT_NUM_TYPES from 5 to 2, which changes the array dimensions of PgStat_BktypeIO.bytes/counts/times (pgstat.h:328-330), and thus the on-disk layout of PgStat_IO in the persisted stats file. Per the comment above this block ("PGSTAT_FILE_FORMAT_ID should be changed whenever any of these data structures change"), you must bump PGSTAT_FILE_FORMAT_ID in this same patch. Without the bump, a stats file written by a prior binary is read back with the new (smaller) struct size while the format-ID check passes, silently mis-reading the IO stats. Bump PGSTAT_FILE_FORMAT_ID. (high confidence)

---

📄 src/include/storage/bufmgr.h (L221-L222)

This diff removes the strategy parameter from exported functions (ReadBufferExtended, ReadBufferWithoutRelcache, ExtendBufferedRel, ExtendBufferedRelBy, ExtendBufferedRelTo) and deletes the entire public BufferAccessStrategy API (the BufferAccessStrategyType enum, GetAccessStrategy, GetAccessStrategyWithSize, GetAccessStrategyBufferCount, GetAccessStrategyPinLimit, FreeAccessStrategy). These are among the most widely used PGDLLIMPORT/public buffer-manager APIs; ReadBufferExtended(..., strategy) in particular is the canonical entry point used by virtually every table/index AM and out-of-tree extension. Removing these signatures and the enum is a hard API/ABI break that silently breaks the compile of essentially all extensions that do buffered I/O, with no compatibility shim or deprecation path. This is the kind of change that needs extraordinary justification and an explicit -hackers design thread; at minimum it must be called out prominently and the removal of a stable, long-standing API must be justified rather than done as a side effect of the internal clock-sweep rework. (high confidence)

---

📄 src/include/storage/bufmgr.h (L357-L357)

The /* in freelist.c */ section is now empty after all of its prototypes (GetAccessStrategy*, FreeAccessStrategy) were removed, leaving a stale section comment followed by two consecutive blank lines. Remove the now-meaningless /* in freelist.c */ comment and collapse the double blank line so the header reads as if it had always been written that way. (moderate confidence)

---

📄 src/test/regress/pg_regress.c (L1246-L1246)

This execl -> execlp change is unjustified and should be dropped. shellprog is only ever the compile-time SHELLPROG, which is an absolute path in every supported build (/bin/sh in meson, $(SHELL) in make) and is never reassigned at runtime. Since execlp only consults $PATH when the program name has no slash, this is a functional no-op for all real configurations. Worse, it introduces a latent footgun: if SHELLPROG were ever a bare name, the shell would be resolved via $PATH, making a test harness's shell selection non-deterministic and PATH-hijackable. This looks like unrelated churn with no stated purpose (minimal-diff violation) and adds no value. Revert to execl. [high confidence]

💡 Suggested change

Before:

		execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

After:

		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

---

📄 src/include/storage/buf_internals.h (L139-L140)

Style nit (low confidence, pgindent): this multi-line block comment opens with text on the same line as /*. Every other block comment added in this file (lines 70, 90, 105, 185) and the surrounding tree convention put /* on its own line for multi-line comments. pgindent will not reformat the comment body, so this stylistic inconsistency will persist through the buildfarm's pgindent check. Reformat to the standard form.

💡 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 (L141-L142)

Latent footgun (moderate confidence): the repurposed field now packs the cooling-state bit (bit 0) and the second-chance ref bit (bit 1), but the pre-existing BUF_STATE_GET_USAGECOUNT macro just above still masks the whole BUF_USAGECOUNT_MASK (all BUF_USAGECOUNT_BITS). After this change it returns 0..3 (cool/ref bits combined), no longer a meaningful "usage count", and it now has no in-tree callers (all were converted to BUF_STATE_GET_COOLSTATE). Leaving a same-named, wrong-semantics accessor next to the new one invites a future caller to grab the wrong macro and read the ref bit as part of the count. Either remove BUF_STATE_GET_USAGECOUNT or add a comment marking it obsolete/mask it to bit 0.

---

📄 src/tools/pgindent/pgindent (L1-L1)

This shebang change is inconsistent with the rest of the tree and appears unrelated to any stated purpose of the patch. Every other Perl script in the PostgreSQL source (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~30 others) uses #!/usr/bin/perl; there are zero uses of #!/usr/bin/env perl in the tree. Changing only this file makes pgindent the lone exception to a tree-wide convention. On pgsql-hackers, an unrelated one-off deviation like this will draw a rejection: either drop this hunk (keep the minimal diff) or, if the goal is genuinely to switch conventions, propose it as its own discussion applied consistently across all scripts. Note also /usr/bin/env perl is arguably a downgrade for a canonical dev tool since it picks up whatever perl is first on $PATH rather than the system perl.

[github-actions]

This .clangd file is personal editor/LSP tooling configuration and should not be part of a patch destined for pgsql-hackers/commitfest. Per PostgreSQL patch hygiene, a patch must be a minimal diff containing only what the stated change requires; committing local IDE/tooling config (this file, plus .gdbinit and pg-aliases.sh in this same change) is unrelated churn and a reliable rejection reason. The PostgreSQL tree does not carry .clangd. If you need it locally, put it in .git/info/exclude or your global gitignore rather than committing it. Remove this file from the patch.

Also note the hardcoded relative include path -I../../../../src/include (line 43) is brittle and workstation/subdirectory-specific, which further confirms this is a local artifact, not a tree-wide config.

[github-actions]

The \gset target variables are still named stats_bulkreads_before/stats_bulkreads_after, but this hunk (and the reworded comments) removes the "bulkread" concept entirely -- the query now counts the normal context. The names are now misleading. Since you are already touching this block and rewording the comments, consider renaming to e.g. stats_reads_before/stats_reads_after for consistency. (low confidence / low severity nit -- weigh against minimal-diff discipline.)

[github-actions]

This .gdbinit is a developer-local debugging artifact and does not belong in a PostgreSQL source patch. It is added alongside two other personal tooling files (.clangd, pg-aliases.sh), none of which are part of any functional change. Committing them violates the minimal-diff discipline (unrelated files are the top rejection reason on pgsql-hackers) and will pollute the tree root. Remove all three from the patch.

Worse, the file is orphaned from the feature it targets: the breakpoints reference a "HOT indexed updates" implementation that is NOT present in this tree. Symbols such as heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_tuple_size, heap_hot_indexed_read_bitmap, heap_xlog_indexed_update, and macros HEAP_INDEXED_UPDATED / XLOG_HEAP2_INDEXED_UPDATE exist ONLY inside this .gdbinit — a codebase search finds them nowhere else. GDB would reject those break commands at load time. This indicates a mis-scoped commit: the debug file was committed without (or ahead of) the feature it targets, which also breaks the atomic/bisectable-commit rule.

[github-actions]

Hardcoded file:line breakpoints are inaccurate and brittle. For example, heapam.c:3210 is labeled "Main entry: heap_update", but line 3210 is actually the variable declaration bool all_visible_cleared_new = false; inside the function body — not the function entry. Since heapam.c is heavily modified in this very change set, all such line-based breakpoints (4019, 4024, 4033, 4101, 4147, etc.) are already stale. Prefer function-name breakpoints; but ultimately this file should not be tracked in the repository at all.

[github-actions]

This changes user-visible semantics: the pg_buffercache view's usage_count column now reports the 0/1 HOT/COOL cooling state instead of the historical 0..BM_MAX_USAGE_COUNT usage count (BM_MAX_USAGE_COUNT is now BUF_COOLSTATE_HOT == 1). doc/src/sgml/pgbuffercache.sgml still documents usage_count as "A possible buffer usage count" with old sample output and is not part of this change. A user-visible behavior change must ship with the doc update in the same patch. (high confidence)

[github-actions]

This silently overrides the operator-configured bgwriter_lru_maxpages cap. config.sgml documents it as a hard per-round bound ("In each round, no more than this many buffers will be written by the background writer"), and bgwriter_lru_multiplier's doc reaffirms "no more than bgwriter_lru_maxpages buffers will be written per round". Under bulk-dirtying demand, write_limit = upcoming_alloc_est exceeds the configured cap, breaking that documented contract and the operator's intent to leave writes to backends (small maxpages). This behavior change requires a documentation update in config.sgml and a design justification with a reproducible benchmark for the "bgwriter stays ahead" claim; otherwise it is a POLA violation on a documented GUC. (high confidence)

[github-actions]

Removing VacuumBufferUsageLimit leaves a stray blank line between the /* GUC parameters for vacuum */ comment and the VacuumCost* group it now heads. The blank line previously separated VacuumBufferUsageLimit from the cost variables; with that variable gone it is leftover churn. Drop the blank line so the comment sits directly above the code it describes.

Suggested change

	/* GUC parameters for vacuum */
	int VacuumBufferUsageLimit = 2048;
	int VacuumCostPageHit = 1;
	/* GUC parameters for vacuum */
	int VacuumCostPageHit = 1;

[github-actions]

completePasses double-count (moderate confidence). This straddle branch increments completePasses but does NOT wrap nextVictimBuffer back below NBuffers. StrategySyncStart() then adds nextVictimBuffer / NBuffers (freelist.c:393) as a correction for "wraparounds that happened before completePasses could be incremented". Because the counter is left >= NBuffers here, that same crossing is counted twice: once by this increment and again by the /NBuffers term. Unlike the start >= NBuffers branch (which wraps the counter under the spinlock so the /NBuffers term drops back to 0), this path leaves the counter high, so completePasses drifts high, not low as the comment claims. It feeds bgwriter pacing, so this skews scan-pacing rather than correctness, but the stated rationale is inverted. Consider wrapping the counter here as well, or accounting for the straddle without also leaving it uncorrected in StrategySyncStart.

[github-actions]

Trailing comments aligned with tabs on code lines of differing length; pgindent will realign these and likely reflow them. Run pgindent and keep the comment placement consistent to avoid whitespace churn.

[github-actions]

Stale lead sentence. The only remaining check under this comment is now the unconditional if (io_op == IOOP_REUSE) return false;, which has nothing to do with an IOOp being "valid in certain contexts" -- the IOContext-dependent checks (BULKREAD/EXTEND, strategy-context REUSE/FSYNC) were all removed. This comment describes behavior that no longer exists and should be dropped/rewritten so it reads as if the code had always been written this way. (moderate confidence)

[github-actions]

🔍 OCR found 36 issue(s).

• 25 inline, 11 in summary (inline capped at 25)

---

📄 src/backend/utils/activity/pgstat_io.c

This comment block is now dangling. The if blocks it described (the B_CHECKPOINTER/B_BG_WRITER/B_AUTOVAC exclusions for BULKREAD/BULKWRITE/VACUUM) were removed just below, leaving the comment followed by a blank line and return true;. It documents code that no longer exists. Remove the whole comment (lines "Some BackendTypes do not currently perform any IO...") along with the now-empty blank line so the function reads as if it had always been written this way. (high confidence)

---

📄 src/backend/utils/activity/pgstat_io.c (L446-L449)

Dangling comment. The if blocks that this comment described (the B_CHECKPOINTER / B_BG_WRITER / B_AUTOVAC exclusions for the removed BULKREAD/BULKWRITE/VACUUM IOContexts) were deleted directly below it, so this comment now documents nonexistent code and is immediately followed by a blank line and return true;. Remove the comment block along with the trailing blank line. (high confidence)

💡 Suggested change

Before:

	 * do so, excluding those rows from the view makes the view easier to use.
	 */
 
 	return true;

After:

	return true;

---

📄 src/backend/utils/init/globals.c (L152-L152)

Removing this line leaves a dangling blank line between the /* GUC parameters for vacuum */ comment and the first cost variable (VacuumCostPageHit), so the comment now reads as a header followed by an empty line. Drop the stray blank line to keep the block tidy (git diff --check / pgindent expect no gratuitous whitespace churn). (low confidence, cosmetic)

---

📄 src/bin/scripts/vacuuming.h (L52-L52)

This removes the buffer_usage_limit field, which is the header side of dropping the whole vacuumdb --buffer-usage-limit CLI option (option parsing, the version guard, the VACUUM-FULL restriction, and the help text are all deleted in vacuumdb.c/vacuuming.c). This is a backward-incompatible removal of a documented option that has shipped since PG16, while the server-side BUFFER_USAGE_LIMIT VACUUM option, psql tab-completion, and the --buffer-usage-limit entry in doc/src/sgml/ref/vacuumdb.sgml (line 135) all remain. The result: users lose a working feature with no deprecation, and the docs describe an option that no longer exists. Unless there is explicit -hackers justification for removing this feature, this field (and the corresponding vacuumdb.c/vacuuming.c hunks) should be kept. If the removal is intended, the SGML docs must be updated in the same patch. (high confidence)

---

📄 src/bin/scripts/vacuuming.c (L868-L874)

This coordinated removal deletes the entire user-visible --buffer-usage-limit option (here the SQL emission, plus the struct field, arg parsing, mutual-exclusion check, and help text in vacuuming.h/vacuumdb.c). However the documentation entry for this option still exists (doc/src/sgml/ref/vacuumdb.sgml, the --buffer-usage-limit size varlistentry). Removing a documented CLI option without dropping its docs in the same patch leaves stale documentation for a flag that no longer exists. Remove the corresponding doc entry as part of this change. (high confidence)

---

📄 src/include/storage/bufmgr.h (L357-L357)

Removing the GetAccessStrategy/FreeAccessStrategy declarations leaves the /* in freelist.c */ section comment with nothing under it, plus a stray double blank line. The dangling section marker and blank-line churn should be cleaned up so the header reads as if it had always been written this way. Drop the now-empty /* in freelist.c */ comment (bufmgr.h no longer declares anything from freelist.c) and collapse the extra blank line. (moderate confidence)

---

📄 src/test/regress/pg_regress.c (L1246-L1246)

This execl -> execlp change is unexplained and, as it stands, either a no-op or a footgun. execlp only consults PATH when its first argument contains no slash. Here shellprog is SHELLPROG, defined as /bin/sh (meson) or $(SHELL) (make) -- both absolute paths containing a slash -- so execlp behaves identically to execl and this hunk changes nothing. If the intent is to allow a bare shell name to be resolved via PATH, that is undesirable for a test harness: it makes which sh runs depend on the caller's environment, defeating deterministic, reproducible test execution and introducing a PATH-injection surface. Either revert this hunk (minimal-diff discipline: it is not required for any stated purpose) or justify it against a concrete need and keep the guarantee that an absolute shell path is always used. Confidence: high.

💡 Suggested change

Before:

		execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

After:

		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

---

📄 src/include/storage/buf_internals.h (L141-L142)

This new accessor sits directly below the pre-existing BUF_STATE_GET_USAGECOUNT (line 136-137), which is left unchanged and still masks the full BUF_USAGECOUNT_MASK (all 4 bits of the field). Now that bit 1 of that field is BUF_REFBIT, BUF_STATE_GET_USAGECOUNT returns 0..3 (cooling bit + ref bit), no longer a 0..1 HOT/COOL value. It has no remaining in-tree callers, but buf_internals.h is exposed to extensions; any surviving/out-of-tree user of BUF_STATE_GET_USAGECOUNT will silently read a HOT+ref buffer as "3" and mis-drive eviction/statistics. Either remove BUF_STATE_GET_USAGECOUNT entirely (there are no callers) or redefine it to mask bit 0 only, so exactly one accessor exists for this field. Confidence: high.

---

📄 src/include/storage/buf_internals.h (L191-L191)

Retaining the name BM_MAX_USAGE_COUNT for what is now a HOT/COOL cooling max is a POLA violation. There is no longer any "usage count" -- the field holds a 1-bit cooling state plus a ref bit -- so a reader (or extension author) will reasonably assume a 0..N counter still exists. pg_buffercache even sizes arrays with BM_MAX_USAGE_COUNT + 1 and labels the column "usage_count", now reporting only 0/1. Rename to a cooling-oriented constant (e.g. BM_MAX_COOLSTATE) and update callers; the comment's rationale ("so the pin fast path reads naturally") does not justify a misleading exported name. Confidence: high.

---

📄 src/include/storage/buf_internals.h (L139-L140)

Block-comment style diverges from the surrounding code and the tree convention: the opening "/*" should be on its own line, with the text starting on the next line (see the block comments just above for BUF_REFBIT and BUF_COOLSTATE). As written this will not survive pgindent cleanly. Reformat to the standard multi-line form.

💡 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/tools/pgindent/pgindent (L1-L1)

This shebang change is inconsistent with the rest of the tree and appears unrelated to the change set. Every other Perl script in PostgreSQL (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~30 others) uses #!/usr/bin/perl. After this hunk, pgindent becomes the sole outlier using env perl. This is an unrelated, non-minimal diff that breaks the tree's established convention and will draw an objection on -hackers. Revert it and keep the hardcoded interpreter path for consistency. (high confidence)

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

[github-actions]

This file should not be committed to the repository. The project's root .gitignore explicitly states: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." .clangd is a personal, editor/tooling-local config for the clangd language server and is unrelated to the actual code change in this patch (the buffer/vacuum work). Including it (along with the other added local files .gdbinit and pg-aliases.sh) violates the minimal-diff discipline and will be rejected on pgsql-hackers. Remove it from the patch and add it to your local git exclude instead.

Additionally, note the paths are developer-specific: CompilationDatabase: build/ and -I../../../../src/include assume a particular out-of-tree build layout that will not hold for other contributors, further confirming this is not a shareable, tree-wide file.

[github-actions]

This entire file is a personal debugging aid and must be removed from the patch. Committing a .gdbinit violates PostgreSQL patch hygiene (minimal diff) and will be an automatic rejection on -hackers. It is also a real footgun: GDB auto-loads .gdbinit from the current working directory, so shipping one in the repository root causes arbitrary debugger commands to execute for anyone who launches gdb in a checkout (local code-execution vector). This is not tracked in .gitignore, so it should not be committed at all. The co-added .clangd and pg-aliases.sh are the same class of out-of-scope developer artifacts and should likewise be dropped from the patch.

[github-actions]

Breakpoints anchored to hardcoded source line numbers (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1287/1802/1836/1863/2936) are inherently fragile: they silently drift as surrounding code changes and will land on the wrong statements. Since this file should not be committed at all (see above), this is moot, but even as a private aid, function-name breakpoints are far more robust than line-number ones.

[github-actions]

ASCII-only is required in PostgreSQL source and diffs. This comment block uses non-ASCII em-dashes (e.g., "HOT Indexed Updates —", "returns Bitmapset" lines and the "—" separators throughout). Another reason this file cannot be committed as-is.

[github-actions]

The comment was updated to drop the 'bulkread' terminology (since per-strategy IO contexts no longer exist), but the psql variable names still say stats_bulkreads_before/stats_bulkreads_after. They now hold normal-context relation reads, not bulkreads, so the names are stale and misleading. Since these lines are already being edited, rename them (e.g. stats_reads_before/stats_reads_after) for clarity. Note the matching .out file would need the same rename.

Low confidence on necessity (it's a naming/clarity nit, not a correctness bug), but worth fixing while the lines are touched.

Suggested change

	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_reads_before
	FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset

[github-actions]

MODERATE confidence, portability/style. port/pg_numa.h is appended after the storage/* block, breaking the alphabetical include ordering PostgreSQL enforces (pgindent/headerscheck will flag it). It belongs with the other port/ header, right after port/atomics.h.

Suggested change

	#include "port/pg_numa.h"
	#include "pgstat.h"
	#include "port/atomics.h"
	#include "port/pg_numa.h"
	#include "storage/buf_internals.h"

[github-actions]

MODERATE confidence, style/convention. The My* prefix in PostgreSQL is reserved for globally-visible per-backend state (MyProc, MyProcNumber, MyDatabaseId). These are file-static, backend-local variables private to freelist.c; naming them MyBatchPos/MyBatchEnd is misleading. Prefer a lowercase file-static name (e.g. clock_batch_pos / clock_batch_end).

[github-actions]

LOW confidence, maintainability. A backend that claims a batch (MyBatchPos..MyBatchEnd) and then errors out mid-batch (elog(ERROR) somewhere in GetVictimBuffer's caller) silently abandons the remaining hand values on next entry it just claims a fresh batch, so those buffers are skipped for one pass. For the default single-node case batch_size==1 so this is moot, but with batching enabled it introduces a small eviction bias that is neither reset nor documented. Worth a one-line note that abandoning a partial batch is acceptable for this approximate clock.

[github-actions]

This silently overrides bgwriter_lru_maxpages, a user-facing GUC documented as the hard per-round write cap ("max buffers written/round"). When upcoming_alloc_est > bgwriter_lru_maxpages, the bgwriter now issues far more writes than the operator configured. This is a POLA violation of a documented tunable and needs a corresponding doc update in config.sgml (guc-bgwriter-lru-maxpages) plus justification/benchmark. As a side effect, maxwritten_clean (incremented at num_written >= write_limit) no longer means "hit the configured cap" but "hit the demand-raised limit," shifting a user-visible statistic's meaning. Confidence: high.

[github-actions]

Comment-placement nit: this "Admit COOL" block sits between set_bits |= BM_TAG_VALID; and the BM_PERMANENT conditional, so it visually documents the BM_PERMANENT logic rather than the admission decision it actually describes. The COOL admission is effected by the absence of the old BUF_USAGECOUNT_ONE set bit (the reused victim's cool-state field was already zeroed by InvalidateVictimBuffer). Consider moving this comment to sit with the removed | BUF_USAGECOUNT_ONE at the set_bits |= BM_TAG_VALID; line, or noting explicitly that COOL == not setting any cool-state bit, so it isn't misread as applying to BM_PERMANENT. Confidence: moderate.