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)

Commitfest lookup returns nothing for either author (the cross-reference index is empty, not a confirmation of absence). Writing the report.

🧵 Related discussion

• Adding basic NUMA awareness — Tomas Vondra, 2025-07-01 (thread 8657, ~93 messages, still active into Sept 2026). This is the primary parent. The root message spells out exactly this PR's agenda: explicit NUMA mapping of shared buffers + descriptors into per-node chunks (numa_buffers_interleave), partitioned buffer freelists (numa_partition_freelist with node/cpu/pid/none strategies), and the explicit note that "the clocksweep really needs to be partitioned the same way as a freelists, and each partition will operate on a subset of buffers." Those map directly to commits b16dc8b95e2, 41fc6f8722e, dc073170e34, fb38d05498e, a77de17f762, b93ace66139. Main participants: Vondra, Jakub Wartak, Andres Freund, Greg Burd, Alexey Makhmutov. (high confidence)
• [PATCH] Batched clock sweep to reduce cross-socket atomic contention — Greg Burd, 2026-04-25 (21 messages). This is the direct source of the PR title's "batched sweep + HOT/COOL evictor." Burd's v8 message (2026-07-29) explicitly describes "the HOT/COOL+batching model" with benchmark data — matching commits b8d59c33e98 (single second-chance bit instead of 0..5 usagecount), c83d2913037 (demand-loaded pages on probation), b0eb3ddff8e (bound work per allocation), ea2165cdfaf (three-arm comparison on x86_64/aarch64, cf. paper/data_threearm_arch.dat). Ants Aasma and Andres Freund reviewed. (high confidence)
• [PATCH] Let's get rid of the freelist and the buffer_strategy_lock — Greg Burd, 2025-07-11 (thread 18478). Source of the PR's "strategy removal" arm. Burd quotes Andres from the NUMA thread: "I've started to wonder if we shouldn't just get rid of the freelist entirely." The one-line experiment ("adding 0 && to StrategyGetBuffer()") is in Andres' message. (high confidence)

🔗 Related commits / prior art

• The three threads above are deliberately intertwined — the batched-sweep and freelist-removal threads both spun out of the NUMA-awareness thread. This PR appears to be a combined dev branch stacking all three, plus local dev scaffolding (.clangd, .gdbinit, flake.nix, shell.nix, pg-aliases.sh, the glibc fortify patch — commit 545737033e5 "dev: local dev-setup"). None of that scaffolding is upstreamable.
• Prior art repeatedly cited in these discussions:
  • Scaling shared buffer eviction (Robert Haas / Amit Kapila, 2014) — the "bgreclaimer" effort to offload clock sweep to a background process. Directly relevant to commit 3824f5d8801 (bgwriter spending chances when the sweep starves).
  • Move unused buffers to freelist (Amit Kapila / Andres, 2013) — bgwriter populating the freelist; same design lineage.
  • NUMA shared memory interleaving (Jakub Wartak, 2025-04) — the "recent thread aiming to do something similar [2]" Vondra referenced; the lighter interleave-the-whole-segment alternative. The NUMA-awareness patch deliberately rejects plain numactl --interleave in favor of content-aware chunking (buffer + descriptor co-located per node).
• pg_numa.c / pg_numa.h infrastructure (src/port/pg_numa.c) and pg_shmem_allocations_numa originate from the "Draft for basic NUMA observability" line of work (Jakub Wartak / Bertrand Drouvot, 2025); this PR extends pg_buffercache (new 1.7--1.8 upgrade) to expose per-buffer NUMA node, consistent with that observability work.

📋 Commitfest

• No commitfest entry returned by author lookup for greg@burd.me or Tomas Vondra, and the thread→CF cross-reference index is empty — so I cannot confirm or deny a CF entry from the index. Given the threads were active commitfest material through 2026, a CF entry almost certainly exists but is not retrievable here. (low confidence on absence)

🧭 Context for reviewers

• Nothing here is committed to master. The freelist-removal thread (18478) was still in review as of late 2025; the batched-sweep and NUMA-awareness threads were active into mid/late 2026. Treat this PR as an unmerged research/dev stack, not a backport.
• This PR fuses three separate upstream proposals that the community was deliberately keeping as distinct, reviewable units (NUMA buffer/freelist partitioning; batched HOT/COOL clock sweep; freelist+buffer_strategy_lock removal). Reviewers should expect each to be judged independently upstream — the combined form here is harder to review than any single thread.
• Key design tensions already raised on-list and relevant to this code:
  • Single backend can't use all of shared_buffers once freelists/sweep are partitioned (Vondra's root message); needs a cross-partition "steal" path. Commits fb38d05498e/dc073170e34 ("balancing of allocations", "scan all partitions") are the attempted answer — verify they actually resolve it.
  • New-page usagecount semantics: the shift from a 0..5 usagecount to a single second-chance bit + "probation" (commits b8d59c33e98, c83d2913037) changes long-standing eviction behavior. Tom Lane's historical rationale for starting usagecount at 1 (bounding a new page's minimum lifespan) is the thing to check against — see Page replacement algorithm in buffer cache.
  • Andres warned (in the shared_buffers-resize thread) that buffers kept out of the clock sweep get unfairly high usagecounts; partitioning the sweep must not reintroduce that: Re: Changing shared_buffers without restart.
  • Ants Aasma and Andres actively reviewed the batching math/correctness in July 2026 (Ants, Andres) — read those before trusting the batched-sweep correctness.
• src/test/regress/pg_regress.c and src/tools/pgindent/pgindent edits are one-liners; the typedefs.list additions are the real surface for new structs. Confirm new buffer-internals types are indexed there.

_{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]

This is personal, developer-local tooling (Nix dev shells, flamegraph/pgbench helper scripts) committed to the repo root. For a change destined to pgsql-hackers / a commitfest, this is out-of-scope scaffolding that would be rejected -- the repo root is reserved and the project explicitly directs local workflow files to $GIT_DIR/info/exclude or ~/.gitexclude (see the header of the root .gitignore). Note the hook sets core.excludesFile .local-gitignore, but no .local-gitignore exists in the tree (confirmed via file_find), so none of these dev files (flake.nix, shell.nix, .clangd, .gdbinit, pg-aliases.sh) are actually ignored. Keep these out of the upstream-bound patch.

[github-actions]

-I../../../../src/include hard-codes a fixed directory depth that is only correct for a source file exactly four levels below the repo root; files at other depths (e.g. top-level or contrib/*/) will get a wrong include path. Since CompilationDatabase: build/ already supplies per-file include flags from compile_commands.json, this manual relative -I is both brittle and redundant. Drop it and rely on the compilation database (plus -I.).

[github-actions]

This caller declares no permissions block, yet the header comment states "Auth is GitHub OIDC". OIDC token minting requires id-token: write, which is never part of the default GITHUB_TOKEN permissions. A reusable workflow cannot be granted a permission the caller lacks, so the called ocr-review.yml will be unable to request the OIDC token and the AWS role assumption (aws_role_arn) will fail. Beyond the least-privilege concern already noted, this is a functional break: add an explicit permissions: block including id-token: write (plus contents: read, pull-requests: write as the review needs).

Suggested change

	jobs:
	review:
	permissions:
	id-token: write
	contents: read
	pull-requests: write
	jobs:
	review:

[github-actions]

No permissions block is declared, but this workflow assumes an AWS role via OIDC (it passes aws_role_arn, and the comment says Bedrock auth is GitHub OIDC). OIDC requires id-token: write, which is never granted by default and cannot be requested by the reusable workflow unless the caller grants it. Without it the role assumption fails. Add id-token: write (together with the issues: write/contents: read scope the issue-opening logic needs).

Suggested change

	jobs:
	check:
	permissions:
	id-token: write
	contents: read
	issues: write
	jobs:
	check:

[github-actions]

The output attribute name is misspelled "weigths" (should be "weights"). This is the runtime-constructed tuple descriptor; while the view in pg_buffercache--1.7--1.8.sql aliases the column positionally as "weights", the misspelled name is still user-visible when the SRF is called directly (e.g. SELECT * FROM pg_buffercache_partitions() t(...)) or via get_call_result_type diagnostics. Fix the spelling.

Suggested change

	TupleDescInitEntry(tupledesc, (AttrNumber) 12, "weigths",
	typentry->typarray, -1, 0);
	TupleDescInitEntry(tupledesc, (AttrNumber) 12, "weights",
	typentry->typarray, -1, 0);

[github-actions]

The inner loop variable int i shadows the outer uint32 i = funcctx->call_cntr declared at the top of this block. The partition index (values[0] = Int32GetDatum(i)) relies on the outer i, so the result is correct today, but the shadowing is confusing and a maintenance footgun. Rename the loop variable (e.g. j) to match the style used in ClockSweepSetWeights above.

Suggested change

	dweights = palloc_array(Datum, funcctx->max_calls);
	for (int i = 0; i < funcctx->max_calls; i++)
	dweights[i] = Int32GetDatum(weights[i]);
	dweights = palloc_array(Datum, funcctx->max_calls);
	for (int j = 0; j < funcctx->max_calls; j++)
	dweights[j] = Int32GetDatum(weights[j]);

[github-actions]

pg_buffercache_set_partition is REVOKE'd from PUBLIC but never GRANTed to any role, so after this upgrade only superusers (and the extension owner) can execute it. If that is the intended access model, this is fine; but note pg_buffercache_partitions is granted to pg_monitor while the mutating setter is left superuser-only without comment. Confirm this asymmetry is deliberate and document it. (Not a privilege-escalation issue: the default REVOKE correctly denies PUBLIC.)

[github-actions]

Typo in user-visible column comment: "partitioon" should be "partition".

Suggested change

	numa_node integer, -- NUMA node of the partitioon
	numa_node integer, -- NUMA node of the partition

[github-actions]

The comment for num_req_allocs is copy-pasted from num_allocs ("handled allocs (current cycle)"). Based on the C code (buffer_req_allocs from numRequestedAllocs) and the total_req_allocs comment above, this should read "requested allocs (current cycle)".

Suggested change

	num_req_allocs bigint, -- handled allocs (current cycle)
	num_req_allocs bigint, -- requested allocs (current cycle)

[github-actions]

This upgrade adds two user-visible SQL functions and a view but ships no regression tests and no documentation. The Makefile REGRESS target (pg_buffercache, pg_buffercache_numa) is unchanged, there is no partitions test under sql/expected, and doc/src/sgml/pgbuffercache.sgml has no mention of partitions. Per PostgreSQL commit-readiness standards, a user-visible change needs regression tests (covering the happy path plus error paths like out-of-range partition, mismatched weights length, NULL/multi-dim array for set_partition) and documentation for the new functions and view. Without them this is WIP.

[github-actions]

environment.localBinInPath = true; is a NixOS module option, not a flake output. Here it is placed inside the per-system attribute set returned by flake-utils.lib.eachDefaultSystem, so the flake schema would expose it as environment.localBinInPath.<system>, which is not a recognized flake output. It has no effect (dev shells get their PATH from the shellHooks), and nix flake check will warn about the unknown output. Remove this dead line.

[github-actions]

The dev shells pull in Linux-only packages (linuxPackages.perf, bpftrace, strace, ltrace, perf-tools, valgrind, plus numactl/liburing/libselinux in the glibc branch) and pkgs.pkgsMusl.*, but flake.nix advertises these shells via flake-utils.lib.eachDefaultSystem, which includes x86_64-darwin/aarch64-darwin. On macOS, evaluating devShells.<darwin>.* fails because those attributes do not exist for darwin. If these shells are intended to be Linux-only, restrict the flake to Linux systems instead of eachDefaultSystem.

[github-actions]

This entire file is personal developer scaffolding (shell aliases/functions wrapping meson/valgrind/asan/gdb/rr/perf/pgbench) and does not belong in the PostgreSQL source tree. Together with the companion .clangd, .gdbinit, flake.nix, shell.nix and .github/workflows/fork-*.yml files added in this same change, it is pure fork-local dev tooling, not a source change to PostgreSQL. A patch posted to pgsql-hackers / tracked in a commitfest must be a minimal, single-logical-change diff; bundling this environment setup violates the "minimal diff / one logical change" discipline and would be rejected on sight. These dev-environment files should be split out of any patch intended for upstream (e.g. kept on a fork branch, not on master).

Confidence: high.

[github-actions]

🔍 OCR found 64 issue(s).

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

---

📄 src/backend/storage/buffer/bufmgr.c (L3887-L3888)

smoothed_alloc and smoothed_density are function-local statics but BgBufferSyncPartition() is now called once per partition in a loop. These statics are therefore shared across all partitions within and across cycles, so one partition's allocation/density smoothing contaminates the next -- the per-partition design (acknowledged in the XXX note on BufferSyncPartition) is not realized for the smoothing estimates. Move them into per-partition saved state alongside the other saved fields, or document why global smoothing is intended.

---

📄 src/backend/storage/buffer/bufmgr.c (L4009-L4010)

Commented-out assignment left in place. saved_info_valid is now set in BgBufferSync after the loop; delete the dead commented-out line and the stale XXX rather than leaving commented-out code (violates the no-commented-out-code convention).

---

📄 src/include/storage/bufmgr.h (L403-L404)

BufferGetNode() is declared and exported (PGDLLIMPORT via the extern) with the comment "in buf_init.c", but no definition exists anywhere in the tree (grep for the symbol finds only this declaration). Any caller linking against it will fail to link, and it makes this patch non-bisectable. Either add the definition in buf_init.c or drop the declaration.

---

📄 shell.nix (L16-L18)

./glibc-no-fortify-warning.patch is referenced here but does not exist anywhere in the repository (searched the whole tree). Any time the patchedGlibc derivation is realized -- which happens for the default/clang glibc dev shells via getPostgreSQLDeps false -> patchedGlibc/patchedGlibc.dev -- Nix evaluation fails with a "path does not exist" error. This breaks nix develop and nix flake check on a clean checkout, so the dev environment is non-reproducible as committed. Either add the patch file to the tree or drop the override.

---

📄 src/include/storage/buf_internals.h (L426-L430)

Grammar error introduced in the edited comment: "the space wastage is be a bit more noticeable". Also the leading sentence "XXX: As this is primarily matters ..." is now a sentence fragment after the rewrite. Reword, e.g. "As this primarily matters in highly concurrent workloads which probably are all 64-bit these days, we previously did not pad on 32-bit. We now force the stride to be cache-line sized even on 32-bit systems, where the space wastage is a bit more noticeable, to allow partitioning of shared buffers (which requires the memory page to be a multiple of the buffer descriptor size)."

---

📄 src/include/storage/buf_internals.h (L92-L98)

Dangling StaticAssertDecl comment. This comment block ("The cooling state is one bit, so the field must be at least that wide. Assert it here ...") describes the StaticAssertDecl, but it is immediately followed by a second, unrelated comment block about BUF_COOL_CLAIM_THRESHOLD, leaving the first comment orphaned from the assert it documents (the assert now sits far below, after the threshold definition). Move the StaticAssertDecl(BUF_USAGECOUNT_BITS >= 1, ...) directly under this comment, or merge the two comment blocks.

---

📄 src/backend/port/sysv_shmem.c (L699-L700)

Comment drift: this block references numa_set_membind() twice ("If set the policy to interleaving by numa_set_membind()" and "undo the policy set by numa_set_membind() earlier"), but numa_set_membind() is never called anywhere in this file -- the interleaving policy is set via numa_set_interleave_mask() in CreateAnonymousSegment above. Fix the comment to name the function actually used.

💡 Suggested change

Before:

	 * If set the policy to interleaving by numa_set_membind(), undo it now by
	 * setting the policy to localalloc. With MAP_POPULATE, all the pages were

After:

	 * If we set the interleaving policy via numa_set_interleave_mask(), undo it
	 * now by setting the policy to localalloc. With MAP_POPULATE, all the pages were

---

📄 src/backend/port/sysv_shmem.c (L632-L633)

numa_set_interleave_mask()/numa_set_localalloc()/numa_interleave_memory() return values are ignored. If libnuma fails here (e.g. the policy is rejected), the segment silently falls back to default placement and the operator gets no diagnostic, which is a footgun for a feature whose entire point is placement. At minimum log a WARNING on failure, consistent with how buf_init.c warns on pg_numa_bind_to_node() failures.

---

📄 src/backend/storage/buffer/freelist.c (L1048-L1050)

Inconsistent locking of balance[]. ChooseClockSweep() reads sweep->balance[index] / sweep->balance[current] with no spinlock, while StrategySyncBalance() and ClockSweepSetWeights() mutate balance[] under clock_sweep_lock. The weighted-average update here is a read-modify-write (balance[j] = coeff*old + (1-coeff)*new), so a lock-free reader can observe a torn/transient weight vector -- including, during the memset-then-rebuild path above, a vector that is momentarily all-zero, which can hang the budget loop in ChooseClockSweep (see that function). If this racing is acceptable "heuristic" behavior, state so explicitly and make the budget loop robust to an all-zero snapshot; otherwise the reader must take the lock or the writer must publish atomically.

---

📄 src/backend/storage/buffer/freelist.c (L39-L43)

This whole change is riddled with XXX/FIXME/TODO and aspirational notes (e.g. "We should enforce this in bufmgr.c", "FIXME This may not quite right", "XXX Do we need the lock", "XXX Maybe we should randomize", "TODO:: just guessing,heuristics"). Several of them flag unresolved correctness questions (the MAX_BUFFER_PARTITIONS cap, the last-partition numBuffers sizing, lock necessity), not just polish. For a pgsql-hackers submission these must be resolved or converted into decided design rationale before posting; as-is this reads as WIP.

---

📄 src/backend/storage/buffer/freelist.c (L1240-L1242)

FIXME flags a real sizing inconsistency. BufferPartitionGet() returns the per-partition num_buffers that buf_init.c computed (with the extra-buffer distribution), so for a correctly-sized registry this is fine -- but the comment claims the last partition may have numBuffers "too high". If that is actually possible, the clock-sweep modulo arithmetic (victim % sweep->numBuffers in ClockSweepTick) would address buffers outside the partition and corrupt the per-partition ranges. Confirm whether numBuffers here can ever exceed the partition's true size and remove the FIXME once settled; a lingering FIXME on buffer-range sizing is not committable.

---

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

This redefines the core buffer replacement algorithm: the 0..5 usage_count LRU ladder is replaced by a single HOT/COOL bit (BM_MAX_USAGE_COUNT now == 1), changing eviction aggressiveness for every workload, and the scan-resistance / BUF_COOL_CLAIM_THRESHOLD=128 behavior is justified only by in-comment performance numbers with no reproducible benchmark or regression/TAP tests in this change, and no documentation of the new GUCs/behavior. A change of this blast radius to shared_buffers management needs tests covering eviction/scan-resistance edge cases and docs before it is commit-ready; as submitted it is WIP for -hackers. (Note: dependent consumers such as pg_buffercache's usage_counts histogram, now effectively 2-wide, should be re-examined too.)

---

📄 contrib/pg_buffercache/pg_buffercache_pages.c (L1089-L1090)

Server-crash footgun: pg_buffercache_set_partition is declared LANGUAGE C VOLATILE PARALLEL UNSAFE without STRICT (see pg_buffercache--1.7--1.8.sql), and this code has no PG_ARGISNULL guard. Calling pg_buffercache_set_partition(0, NULL) passes a NULL (zero) datum to PG_GETARG_ARRAYTYPE_P(1), which detoasts/dereferences a NULL pointer and crashes the backend. Any caller granted EXECUTE can trigger it. Fix by either marking the SQL function STRICT, or guarding both args with PG_ARGISNULL() here.

---

📄 contrib/pg_buffercache/pg_buffercache_pages.c (L947-L947)

Unnecessary type-cache lookup. typentry->typarray is used only once below for the weights array element type. For a built-in type this is simply INT4ARRAYOID; use the macro directly and drop the lookup_type_cache() call (and the typentry variable). [low confidence]

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L1-L1)

This upgrade script is missing the leading /* contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql */ header comment that every other upgrade script in this module carries (e.g. 1.6--1.7). Minor, but it breaks the module's convention.

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L1-L1)

Wrong wording for an upgrade script: this file is loaded via ALTER EXTENSION, so the comment should say "rather than via ALTER EXTENSION" (matching 1.6--1.7.sql), not "CREATE EXTENSION". This looks copy-pasted from the base pg_buffercache--1.2.sql. The \echo line correctly says ALTER EXTENSION, so the comment is inconsistent with the very next line.

💡 Suggested change

Before:

+-- complain if script is sourced in psql, rather than via CREATE EXTENSION

After:

+-- complain if script is sourced in psql, rather than via ALTER EXTENSION

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L5-L5)

Use CREATE FUNCTION, not CREATE OR REPLACE FUNCTION, for a brand-new function in an upgrade script. Every other function in this module (including pg_buffercache_set_partition just below) uses plain CREATE FUNCTION. CREATE OR REPLACE here is unnecessary and inconsistent, and would silently mask a name collision instead of erroring.

💡 Suggested change

Before:

+CREATE OR REPLACE FUNCTION pg_buffercache_partitions()

After:

+CREATE FUNCTION pg_buffercache_partitions()

---

📄 src/port/pg_numa.c (L241-L253)

Non-void stubs fall off the end without a return. pg_numa_bind_to_node (returns int) and pg_numa_page_size (returns Size) only contain Assert(false), which is a no-op in non-assert (production) builds. Control then reaches the closing brace with no return value — undefined behavior, and it will trigger -Wreturn-type warnings that become hard errors under -Werror on the buildfarm, breaking every build without libnuma. Follow the existing stub pattern (pg_numa_get_max_node returns 0) and return a value. (high confidence)

💡 Suggested change

Before:

int
pg_numa_bind_to_node(char *startptr, char *endptr, int node)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
}

Size
pg_numa_page_size(void)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
}

After:

int
pg_numa_bind_to_node(char *startptr, char *endptr, int node)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return 0;
}

Size
pg_numa_page_size(void)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return 0;
}

---

📄 src/port/pg_numa.c (L23-L23)

numa_flags is an exported, unsynchronized global with no reader or writer anywhere in the tree (grep across the repo finds only the declaration and definition). Combined with the NUMA_BUFFERS macro — which also has zero consumers — this is speculative scaffolding (YAGNI). Either wire it to a real consumer in this change or drop it. If it is later read concurrently by backends after process fork, note that a plain int global written during startup and read elsewhere needs a defined initialization/visibility contract. (high confidence)

---

📄 src/port/pg_numa.c (L143-L143)

Casting a pointer to signed int64 for the modulo is a portability smell. The established idiom in the tree for pointer-alignment asserts is (uintptr_t) (see md.c: Assert((uintptr_t) buffer == TYPEALIGN(...))). On 32-bit platforms pointer width differs from int64, and a signed cast is wrong for address arithmetic. Use uintptr_t. (high confidence)

💡 Suggested change

Before:

	Assert((int64) startptr % pg_numa_page_size() == 0);

After:

	Assert((uintptr_t) startptr % pg_numa_page_size() == 0);

---

📄 src/port/pg_numa.c (L151-L152)

pg_numa_move_to_node is dead code: it has no caller anywhere (the only references in buf_init.c are inside /* XXX or should we use pg_numa_move_to_node? */ comments). It also silently discards the result of numa_tonode_memory, unlike pg_numa_bind_to_node which returns a status — an unexplained asymmetry. Either remove this helper until it has a caller, or justify and document why its errors are ignored. (high confidence)

---

📄 src/port/pg_numa.c (L135-L136)

The "XXX We only expect to do this during startup, when the shared memory is still being setup" comment is an unenforced assumption, not an invariant. This matters because pg_numa_page_size reads huge_pages_status, which is only finalized by SetConfigOption("huge_pages_status", ...) during shared-memory creation (sysv_shmem.c / win32_shmem.c). If this helper runs before that point, huge_pages_status is still HUGE_PAGES_UNKNOWN (not HUGE_PAGES_OFF), so the code assumes huge pages and may compute the wrong page size, mis-aligning the Assert and placement. Document the actual ordering requirement or guard against the UNKNOWN state. (moderate confidence)

---

📄 src/port/pg_numa.c (L203-L207)

Layering/portability violation: pg_numa.c lives in src/port/ and is compiled with -DFRONTEND into libpgport.a (see src/port/Makefile OBJS list). But pg_numa_page_size() depends on two backend-only symbols: GetHugePageSize() (defined only in src/backend/port/sysv_shmem.c and win32_shmem.c, and itself uses backend-only AllocateFile/FreeFile) and the backend GUC huge_pages_status (declared in storage/pg_shmem.h, defined in the backend). None of these exist in the frontend build. The only current callers are backend-only (buf_init.c), so there is no link failure today, but any frontend tool that references pg_numa_page_size/pg_numa_move_to_node/pg_numa_bind_to_node will fail to link with unresolved symbols. A src/port/ routine must not reach into backend internals; the page-size computation belongs in the backend (e.g. alongside GetHugePageSize) or the huge-page size must be passed in as a parameter. [moderate confidence]

---

📄 src/port/pg_numa.c (L176-L179)

The comment claims this function will "leave the default placement in effect" when mbind() fails, but that is not guaranteed: mbind() with MPOL_BIND|MPOL_MF_MOVE can partially migrate pages before returning an error, so placement may already be altered on failure. Also, this function only returns ret; it does not itself "report the failure" (the caller in buf_init.c emits the WARNING). The comment describes behavior the function does not implement; trim it to describe what the flags do and that the status is returned to the caller. [moderate confidence]

---

📄 src/backend/tcop/postgres.c (L117-L118)

debug_numa_node is defined here as a plain global and exported PGDLLIMPORT in miscadmin.h, but it is never registered with the GUC machinery (no DefineCustomIntVariable / guc_tables.c entry exists anywhere in the tree — confirmed by search). Because of this the variable is permanently stuck at its initial value of -1, so process_debug_numa_node() always short-circuits and the entire feature is dead/unwired scaffolding that a user can never enable. If the GUC is meant to ship, it must be registered (with min/max bounds, GUC_NOT_IN_SAMPLE/developer context, and documentation); otherwise this and the two call sites should be dropped. (high confidence)

---

📄 src/backend/tcop/postgres.c (L90-L92)

This includes <numa.h> and calls libnuma directly (numa_available, numa_run_on_node, numa_run_on_node_mask, numa_max_node, numa_all_nodes_ptr), bypassing the src/port/pg_numa.c abstraction that PostgreSQL deliberately uses to centralize platform-specific NUMA logic (and that this same change set extends with pg_numa_bind_to_node/pg_numa_move_to_node). Raw libnuma usage should stay confined to src/port/pg_numa.c; add a pg_numa_pin_to_node()-style wrapper there and call it from here instead of pulling libnuma into postgres.c. (medium confidence)

---

📄 src/backend/tcop/postgres.c (L1067-L1068)

%m relies on a meaningful errno, but numa_run_on_node() from libnuma does not reliably set errno on all failure paths the way a bare syscall does, so %m may append a stale/irrelevant error string. Either drop %m or route the call through a pg_numa wrapper that guarantees errno is set (as pg_numa_bind_to_node in this change set does for its own failures). (medium confidence)

---

📄 src/backend/tcop/postgres.c (L1035-L1036)

Comment is missing a terminating period for consistency with the surrounding comment style (...-1 disables it (default).). (low confidence)

---

📄 src/backend/tcop/postgres.c (L1061-L1062)

This user-facing WARNING embeds the raw internal GUC identifier debug_numa_node into the message text and uses a trailing ", ignoring" clause. Convention is to not expose internal variable names in errmsg and to phrase as a complete clause; prefer something like errmsg("requested NUMA node %d exceeds the highest available NUMA node %d", ...) with an errdetail/errhint noting it is ignored. (low confidence)

---

📄 src/include/miscadmin.h (L218-L218)

Gratuitous double blank line after the new externs introduces whitespace churn; a single blank line matches the surrounding style and minimal-diff discipline. (low confidence)

---

📄 src/port/pg_numa.c (L157-L158)

This declaration block won't survive pgindent. pgindent aligns the variable name to a single tab stop past the longest type; here int is over-indented (four tabs) to try to line up with struct bitmask *nodemask, which is not how pgindent formats it. Run pgindent to normalize. Same applies to int\tnuma_flags; at the top of the file. [high confidence]

---

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

Two problems here, both high confidence.

1. Unrelated change / scope creep. This diff belongs to a PR whose purpose (per the other touched files) is NUMA-aware buffer management. A change to how pg_regress execs the shell has nothing to do with that. On -hackers this is a top rejection reason: a patch that does more than one thing. Split it out into its own patch with its own justification, or drop it.

2. Behavioral/portability regression. SHELLPROG is $(SHELL) (make) or /bin/sh (meson) — normally an absolute path, for which execlp and execl behave identically, so the change buys nothing in the common case. But when shellprog is a bare name, execlp resolves it through $PATH, meaning the test harness would run whatever shell happens to come first on the caller's PATH. That is a footgun (POLA violation and a mild security concern for a test runner). Note the rest of the tree deliberately does the opposite: src/bin/pg_ctl/pg_ctl.c uses execl("/bin/sh", ...) with a hard-coded absolute path specifically to avoid PATH-based shell lookup. If there is a real motivation (e.g. shellprog being a bare name on some platform), it must be stated and justified; otherwise revert to execl.

💡 Suggested change

Before:

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

After:

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

---

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

Unrelated change / scope creep (high confidence). This shebang edit (/usr/bin/perl -> /usr/bin/env perl) has no connection to the stated purpose of this PR (NUMA-aware buffer management, per the other touched files). It is an independent tooling tweak and should not ride along in this patch — bundling unrelated cleanup is a top rejection reason on -hackers. Note also that the rest of the tree's Perl scripts consistently use #!/usr/bin/perl; changing only pgindent is inconsistent, and /usr/bin/env perl picks up whatever perl is first on PATH, which can select a different interpreter than the build expects. Drop this from the patch (or propose it separately, tree-wide, with justification).

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/include/miscadmin.h (L216-L217)

shmem_populate and shmem_interleave are shared-memory GUCs defined in src/backend/port/sysv_shmem.c. Their externs belong in storage/pg_shmem.h alongside the existing shared-memory GUC externs (huge_pages, huge_pages_status), not in the catch-all miscadmin.h. Putting them here is inconsistent with the subsystem's existing header layout and forces unrelated TUs that include miscadmin.h to see them. (moderate confidence)

---

📄 src/backend/tcop/postgres.c (L90-L92)

System headers (<...>) conventionally go at the top of the file right after the postgres.h/c.h block and before the project "..." includes; this <numa.h> block is placed after all the "utils/..." includes, which breaks the include-ordering convention. (This is moot if the direct libnuma usage is instead routed through the src/port/pg_numa.c abstraction as noted elsewhere.) (low confidence)

---

📄 src/backend/tcop/postgres.c (L1055-L1056)

Inconsistent error handling: the set path checks numa_run_on_node() != 0 and warns on failure, but the reset path ignores the return value of numa_run_on_node_mask(). If un-pinning fails the backend silently stays pinned with no diagnostic. Check and warn here too for symmetry. (low confidence)

---

📄 src/backend/storage/buffer/freelist.c (L750-L756)

numCoolClaims is incremented before the CAS that claims the HOT buffer. If the CAS fails (another backend changed buf->state), the inner for(;;) loop re-runs with the refreshed old_buf_state; if the buffer is still HOT and cooled >= BUF_COOL_CLAIM_THRESHOLD, this fetch_add runs again, double-counting. Worse, if the buffer became pinned/COOL after the failed CAS, the claim never actually happens but numCoolClaims was still bumped. This counter drives the bgwriter's cool_if_hot staging decision (via StrategyCoolClaims), so inflating it under contention makes the background writer over-stage. Increment only after a successful CAS.

💡 Suggested change

Before:

				pg_atomic_fetch_add_u64(&sweep->numCoolClaims, 1);
				local_buf_state += BUF_REFCOUNT_ONE;

				if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
												   local_buf_state))
				{
					/* Found a usable buffer */

After:

				local_buf_state += BUF_REFCOUNT_ONE;

				if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
												   local_buf_state))
				{
					pg_atomic_fetch_add_u64(&sweep->numCoolClaims, 1);
					/* Found a usable buffer */

---

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

Division by zero when a partition has numBuffers == 0. BufferPartitionsCalculate() computes npartitions = nnodes * nparts_per_node (>= MIN_BUFFER_PARTITIONS = 4) with no guard that NBuffers >= npartitions. On a host with many NUMA nodes and a tiny shared_buffers (minimum is 16 buffers), buffers_per_partition can be 0 and some partitions get numBuffers == 0; victim % sweep->numBuffers here (and nextVictimBuffer % numBuffers in StrategySyncStart) then divides by zero and crashes at startup/first allocation. Enforce a minimum buffers-per-partition (or clamp npartitions to NBuffers) in BufferPartitionsCalculate().

---

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

Mis-attributed numBufferAllocs. With balance=false, ChooseClockSweep() skips the budget block and returns &StrategyControl->sweeps[clocksweep_partition_current], i.e. the redirected partition left over from the previous StrategyGetBuffer() call, not this backend's home partition (sweep computed from the current index). So numBufferAllocs (the per-partition count the bgwriter reads as recent_alloc) is credited to a stale/other partition rather than the one that actually serves this allocation (chosen by ChooseClockSweep(true) plus the scan-all fallback). The nearby XXX only mentions NUMA migration between the two calls, not this current-vs-home mismatch. Increment the count on the partition actually chosen for this allocation.

[github-actions]

This file is personal editor/clangd tooling and does not belong in a PostgreSQL contribution. Per upstream patch-hygiene, a patch must be a minimal diff containing only what the stated feature (HOT indexed updates) requires. Committing a clangd config to the repo root pollutes the tree, has no tests or docs, and would be rejected on -hackers. There is no precedent in the tree's .gitignore files for committing such per-developer config. Keep it out of the tree (local, untracked) or in a personal dotfiles setup.

[github-actions]

The fixed relative include -I../../../../src/include only resolves from one specific build-subdirectory depth, yet CompilationDatabase: build/ points at a build dir under the repo root. When clangd is invoked from the repo root these paths are inconsistent and will produce spurious diagnostics. (Secondary to the larger point that this file should not be committed at all.)

[github-actions]

This GDB breakpoint file is WIP debug cruft and should not be committed. Two concrete defects prove it is already stale and unmaintainable: (1) The hardcoded line numbers are wrong -- heapam.c:4019 is annotated "pure HOT (no indexed columns changed)" but the actual code at line 4019 is the "Rats, it doesn't fit anymore" free-space recheck in heap_update(); any edit silently invalidates every line-numbered breakpoint. (2) Several function-name breakpoints reference symbols that do not exist in this tree at all (e.g. heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_xlog_indexed_update), so GDB will fail to resolve them. This belongs in a personal, untracked setup, not in a PostgreSQL patch.

[github-actions]

This Nix flake is a personal development environment and does not belong in a PostgreSQL contribution. A patch posted to -hackers must be a minimal diff limited to the stated feature; committing flake.nix/shell.nix/pg-aliases.sh scaffolding to the repo root pollutes the tree, ships no tests or docs, and would be rejected. The feature itself (HOT indexed updates) lives in heapam.c/pruneheap.c/etc. and is not part of this review group, confirming these files are pure scaffolding.

[github-actions]

environment.localBinInPath is a NixOS module option, not a valid flake output attribute. Placed in the per-system output attrset produced by eachDefaultSystem, it is meaningless -- silently ignored at best, misleading at worst. It looks copy-pasted from a NixOS configuration and has no consumer in a flake output.

[github-actions]

Potential infinite spin. clocksweep_partition_budget is driven by sweep->balance[], which StrategySyncBalance()/ClockSweepSetWeights() rewrite lock-free from the reader's perspective (ChooseClockSweep reads balance[] with no spinlock). If every balance[] entry is transiently observed as 0 (possible during the read-modify-write weighted-average update, or if a manual set leaves all-zero weights), this while (budget == 0) loop never terminates. Guard against an all-zero weight vector (e.g. bound the number of wrap-arounds and fall back to the home partition).

[github-actions]

Duplicated semicolon.

Suggested change

	pid_t pid = MyProcPid;;
	pid_t pid = MyProcPid;

[github-actions]

Unchecked malloc of saved_info -> NULL deref. Unlike prev_cool_claims (which does check and ereports), this malloc has no NULL check, and saved_info[partition] is dereferenced immediately below in the partition loop. If malloc fails the bgwriter crashes with a NULL deref. Add the same ereport(ERROR, ...) guard used for prev_cool_claims.

Suggested change

	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	}
	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	if (saved_info == NULL)
	ereport(ERROR,
	(errcode(ERRCODE_OUT_OF_MEMORY),
	errmsg("out of memory")));
	}

[github-actions]

Integer-division truncation silently changes throttling. When bgwriter_lru_maxpages < num_partitions, bgwriter_lru_maxpages / num_partitions == 0, so the condition ++num_written >= 0 is true on the first written buffer in every partition, immediately counting a maxwritten_clean event and breaking the scan. This disables the bgwriter's write-cap behavior for small/default maxpages on many-partition systems (e.g. maxpages=100 with >100 partitions, or any value below num_partitions). Round up, or clamp to at least 1.

[github-actions]

TOCTOU / inaccurate BUF_COOLED accounting. This releases the header lock (UnlockBufHdrExt), then re-acquires it (LockBufHdr) and unconditionally sets BUF_COOLED. Between the two, another backend can PinBuffer() and re-promote the buffer to HOT, yet BUF_COOLED is still reported, and the demotion itself may be immediately undone. Since you already hold the header lock from the LockBufHdr(bufHdr) above, demote in place by clearing BUF_USAGECOUNT_MASK on buf_state and keep the lock, rather than unlock/relock -- that avoids the window entirely and the extra atomic round-trip.

[github-actions]

🔍 OCR found 63 issue(s).

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

---

📄 src/backend/port/sysv_shmem.c (L713-L714)

The comment claims the interleaving policy was set earlier via numa_set_membind(), but the code above actually sets it with numa_set_interleave_mask(). Fix the comment to reference the function that is actually used, otherwise the reasoning is misleading.

💡 Suggested change

Before:

		/* undo the policy set by numa_set_membind() earlier */
		numa_set_localalloc();

After:

		/* undo the interleave policy set by numa_set_interleave_mask() earlier */
		numa_set_localalloc();

---

📄 src/backend/port/sysv_shmem.c (L632-L633)

numa_set_interleave_mask() sets a process-wide allocation policy that persists until numa_set_localalloc() resets it below. If mmap() fails in between (CreateAnonymousSegment errors out / the function returns before reaching the reset), the backend is left with a global interleave policy affecting all subsequent allocations. Reset the policy on all exit paths (e.g. reset immediately after the mmap succeeds/fails, or in an error cleanup), not only on the success path.

---

📄 src/backend/port/sysv_shmem.c (L106-L107)

GUC-backed globals should follow the file's existing indentation style (tab-aligned, like the surrounding static declarations). These will be reflowed by pgindent; align the type/name as the rest of the file does.

💡 Suggested change

Before:

bool shmem_populate = false;	/* MAP_POPULATE */
bool shmem_interleave = false;	/* NUMA interleaving */

After:

bool		shmem_populate = false; /* MAP_POPULATE */
bool		shmem_interleave = false;	/* NUMA interleaving */

---

📄 src/backend/storage/buffer/freelist.c (L1082-L1085)

StrategySyncPrepare and StrategySyncBalance take the per-sweep clock_sweep_lock only to read/exchange atomics (numBufferAllocs, numRequestedAllocs, numTotalAllocs) -- the author's own comment admits "Do we need the lock, if we're only accessing atomics? Surely not." Taking a spinlock around pure atomic ops adds needless contention and the discipline is inconsistent (StrategySweepClaims reads the same kind of counter with no lock). Drop the spinlock for the atomic-only paths; keep it only where genuinely non-atomic shared state (balance[]) is modified.

---

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

Typo: "should't" -> "shouldn't".

💡 Suggested change

Before:

	 * We should't get unexpected NUMA nodes, not considered when setting up the

After:

	 * We shouldn't get unexpected NUMA nodes, not considered when setting up the

---

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

Typos in comments: "allocatios" -> "allocations", "intervaal" -> "interval".

💡 Suggested change

Before:

 * - numRequestedAllocs - number of allocatios requested in a partition

After:

 * - numRequestedAllocs - number of allocations requested in a partition

---

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

Typo: "exacly" -> "exactly".

💡 Suggested change

Before:

	 * We lock the partitions one by one, so not exacly in sync, but that

After:

	 * We lock the partitions one by one, so not exactly in sync, but that

---

📄 src/backend/storage/buffer/freelist.c (L1230-L1235)

The FIXME admits the sizing may be wrong when NBuffers is not a perfect multiple of partition count. buf_init.c (via BufferPartitionGet) already computes the correct per-partition num_buffers/first_buffer (with the remainder distributed), and this code reads exactly those values -- so the ClockSweep ranges are consistent with the registry as long as the buf_init.c remainder bug (NBuffers % buffers_per_partition) is fixed. Once that is fixed, update/remove this stale FIXME so it does not imply a nonexistent overflow. If it is not fixed, numBuffers here will indeed overshoot and nextVictimBuffer % numBuffers will index past the partition range.

---

📄 src/backend/storage/buffer/buf_init.c (L386-L386)

Typo: "higher thah" -> "higher than".

💡 Suggested change

Before:

 * Picks the smallest number of partitions higher thah MIN_BUFFER_PARTITIONS,

After:

 * Picks the smallest number of partitions higher than MIN_BUFFER_PARTITIONS,

---

📄 src/include/storage/buf_internals.h (L112-L113)

ABI/semantics break to a PGDLLIMPORT'd subsystem: removing BUF_USAGECOUNT_* and BM_MAX_USAGE_COUNT and repurposing flag bit 6 changes the meaning of buffer state as seen by extensions. In-tree pg_buffercache has been updated (it now maps the bit to a 0/1 usage_count), but any external extension that reads usagecount via the old macros or relies on the 0..5 range is silently broken. This is a large behavior change that needs a design-thread reference, tests, and documentation before it is commit-ready; flag for the mailing list.

---

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

Changing BUFFERDESC_PAD_TO_SIZE to always 64 (removing the 32-bit special case) is an ABI/layout change for BufferDescPadded on 32-bit builds and is justified in the comment by the partitioning requirement (page size must be a multiple of the descriptor size). The comment itself is ungrammatical ("the space wastage is be a bit more noticeable") -- clean it up. Confirm this layout change is really required by the partitioning math and not just incidental, since it inflates per-descriptor memory on 32-bit platforms.

---

📄 src/backend/storage/buffer/localbuf.c (L840-L841)

Behavior change for local (temp-table) buffers: previously usagecount could reach BM_MAX_USAGE_COUNT (5), letting a hot local buffer survive several clock passes; the single BM_SECOND_CHANCE bit now grants exactly one reprieve. For workloads that heavily reuse a small set of local buffers (large temp tables, hash spills) this can increase local-buffer eviction/thrashing. This is an intentional part of the second-chance redesign and the code is self-consistent, but it is a user-visible regression risk that needs a benchmark and a note in the design discussion.

---

📄 src/port/pg_numa.c (L241-L253)

[high confidence] Non-void function stubs fall off the end without returning a value. Assert(false) compiles to nothing in a non-assert (USE_ASSERT_CHECKING off) build, so both pg_numa_bind_to_node (returns int) and pg_numa_page_size (returns Size) return an indeterminate value -- undefined behavior, and a hard error under -Werror=return-type even in assert builds (the body is unconditionally non-void after a no-op). The existing non-libnuma stubs just above (pg_numa_init, pg_numa_query_pages, pg_numa_get_max_node) all return a value; follow that pattern. These are expected to be unreachable, so return a benign value after the assert.

💡 Suggested change

Before:

int
pg_numa_bind_to_node(char *startptr, char *endptr, int node)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
}

Size
pg_numa_page_size(void)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
}

After:

int
pg_numa_bind_to_node(char *startptr, char *endptr, int node)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return 0;
}

Size
pg_numa_page_size(void)
{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return 0;
}

---

📄 src/port/pg_numa.c (L143-L143)

[high confidence] Casting a pointer to signed int64 for the alignment check is a portability defect. On a 32-bit build a pointer is 32-bit, and the signed cast of a high-address pointer can yield a negative value, making % pg_numa_page_size() semantically wrong (C's % of a negative operand). The tree's convention is uintptr_t for pointer-to-integer casts (see c.h TYPEALIGN/PointerIsAligned macros). Use uintptr_t.

💡 Suggested change

Before:

	Assert((int64) startptr % pg_numa_page_size() == 0);

After:

	Assert((uintptr_t) startptr % pg_numa_page_size() == 0);

---

📄 src/port/pg_numa.c (L23-L23)

[high confidence] numa_flags (and the NUMA_BUFFERS macro declared in pg_numa.h) are dead scaffolding: a codebase-wide search finds numa_flags only at its declaration (pg_numa.h) and definition (here), never assigned or read, and NUMA_BUFFERS is only defined, never used. Exporting an unused PGDLLIMPORT global and an unused flag macro adds ABI surface for a path that isn't wired. Drop both per YAGNI, or wire up the actual consumer.

---

📄 src/port/pg_numa.c (L151-L152)

[moderate confidence] Inconsistent and silent error handling. pg_numa_bind_to_node reports failure via return value/errno (and the buf_init.c callers do check != 0), but pg_numa_move_to_node is void and silently discards any numa_tonode_memory failure, so NUMA mis-placement goes unnoticed -- a footgun given these two are near-interchangeable (the callers even carry a /* XXX or should we use pg_numa_move_to_node? */ comment). Also unlike bind_to_node, this path has no node < 0 guard. At minimum document that failures are intentionally ignored, or make the signature consistent with bind_to_node.

---

📄 src/port/pg_numa.c (L21-L21)

[moderate confidence] src/port/pg_numa.c is listed in both the backend and frontend-usable port object sets (src/port/Makefile, meson.build). Including backend-only storage/pg_shmem.h and referencing backend globals/functions (huge_pages_status, GetHugePageSize) pulls backend-only symbols into a port TU; a frontend program that links pg_numa.o and reaches this code would fail to resolve them. It is currently masked because these symbols sit inside #ifdef USE_LIBNUMA and no frontend caller invokes them, but it breaks the frontend/backend separation convention for port files. Consider isolating the huge-page lookup so the port file does not depend on backend-only headers.

---

📄 contrib/pg_buffercache/pg_buffercache_pages.c (L1095-L1096)

Crash on NULL input. pg_buffercache_set_partition is declared LANGUAGE C VOLATILE PARALLEL UNSAFE (not STRICT) in pg_buffercache--1.7--1.8.sql, so a call like pg_buffercache_set_partition(0, NULL) or pg_buffercache_set_partition(NULL, '{1}') reaches this C code and PG_GETARG_ARRAYTYPE_P(1) / PG_GETARG_INT32(0) dereference/read a NULL argument, crashing the backend. Either mark the SQL function STRICT, or add explicit PG_ARGISNULL(0)/PG_ARGISNULL(1) guards that raise a clean error. Confidence: high.

💡 Suggested change

Before:

	int			partition = PG_GETARG_INT32(0);
	ArrayType  *array = PG_GETARG_ARRAYTYPE_P(1);

After:

	int			partition;
	ArrayType  *array;

	if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
		ereport(ERROR,
				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
				 errmsg("partition and weights must not be NULL")));

	partition = PG_GETARG_INT32(0);
	array = PG_GETARG_ARRAYTYPE_P(1);

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L1-L2)

Two header inconsistencies versus every other pg_buffercache upgrade script. (1) This file is missing the leading path comment that all siblings carry, e.g. /* contrib/pg_buffercache/pg_buffercache--1.6--1.7.sql */. (2) An upgrade script is sourced via ALTER EXTENSION, not CREATE EXTENSION; the sibling scripts say "rather than via ALTER EXTENSION". Confidence: high.

💡 Suggested change

Before:

-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.8'" to load this file. \quit

After:

/* contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql */

-- complain if script is sourced in psql, rather than via ALTER EXTENSION
\echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.8'" to load this file. \quit

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L5-L5)

Use plain CREATE FUNCTION. This is a brand-new function introduced in 1.8, so there is nothing to replace; every other function definition in this extension's scripts uses CREATE FUNCTION. CREATE OR REPLACE here only masks accidental name collisions and is inconsistent with the subsystem's style. Confidence: high.

💡 Suggested change

Before:

CREATE OR REPLACE FUNCTION pg_buffercache_partitions()

After:

CREATE FUNCTION pg_buffercache_partitions()

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L11-L12)

Indentation here uses tabs, whereas the existing pg_buffercache--*.sql scripts indent with spaces. Keep the whitespace style consistent with the other extension scripts to avoid gratuitous churn and a mixed-style file. Confidence: medium.

---

📄 contrib/pg_buffercache/Makefile (L12-L13)

No regression test is added for the new 1.8 functionality. REGRESS is still pg_buffercache pg_buffercache_numa, so pg_buffercache_partitions() / pg_buffercache_set_partition() and their error/edge paths (NULL args, wrong weight count, out-of-range weights, multi-dim array) are untested. A user-visible SQL addition needs regression coverage before it is commit-ready. Confidence: high.

---

📄 src/port/pg_numa.c (L138-L143)

Dead scaffolding (YAGNI). pg_numa_move_to_node has no caller anywhere in the tree -- a codebase-wide search finds it only in its declaration (pg_numa.h), its two definitions here, and two XXX or should we use pg_numa_move_to_node? comments in buf_init.c. The only live NUMA-placement path is pg_numa_bind_to_node. An exported, PGDLLIMPORT'd function with no caller is speculative API surface that should not land. Remove pg_numa_move_to_node (both the libnuma and the stub definition) and its declaration until a caller actually exists; also drop the dangling XXX comments in buf_init.c that reference it.

---

📄 src/port/pg_numa.c (L194-L201)

Dead code: this #ifdef WIN32 branch can never be compiled. The entire enclosing block is under #ifdef USE_LIBNUMA, which is only ever defined when libnuma is found (Linux-only -- see the comment at the top of this file and configure.ac/meson.build). On Windows USE_LIBNUMA is never set, so GetSystemInfo/sysinfo.dwPageSize is unreachable. Drop the WIN32 branch and just use sysconf(_SC_PAGESIZE) here.

---

📄 src/port/pg_numa.c (L175-L179)

The comment claims mbind() uses MPOL_MF_MOVE to migrate pages already faulted in, but the design intent (and the buf_init.c callers) run this during startup before shared memory is faulted/touched, so MPOL_MF_MOVE has nothing to migrate -- the comment overstates what happens at the actual call site. Also, unlike pg_numa_move_to_node, this function has no header-comment block describing its contract (return value / errno semantics). Add a function header documenting that it returns the mbind() result (0 on success, -1 with errno set) and align the inline comment with the startup-time usage.

---

📄 src/backend/tcop/postgres.c (L117-L118)

debug_numa_node is never registered as a GUC. I searched guc_tables.c and all .c files for any DefineCustomIntVariable/table entry and found none, and there is no .sgml documentation. Since it is only ever set to its initializer value -1, process_debug_numa_node() always short-circuits on debug_numa_node < 0 and the pin path is dead/unreachable code. Either wire up the GUC entry (and include that file plus docs in this patch) or remove the knob. (high confidence)

---

📄 src/backend/tcop/postgres.c (L90-L92)

This includes <numa.h> and calls raw libnuma functions (numa_available, numa_max_node, numa_run_on_node, numa_run_on_node_mask) directly in backend code. The tree already provides a portability abstraction in src/port/pg_numa.c / src/include/port/pg_numa.h (e.g. pg_numa_init, pg_numa_get_max_node). Backend code should route through that layer rather than embedding libnuma directly, both for DRY and to keep the non-Linux/non-libnuma no-op handling in one place. (moderate confidence)

---

📄 src/backend/tcop/postgres.c (L1061-L1062)

Message-style issues: (1) user-facing text is not wrapped in _(); (2) embedding the GUC variable name debug_numa_node and the trailing ", ignoring" fragment in the primary errmsg is non-idiomatic — prefer a clean lowercase errmsg plus an errhint, e.g. errmsg("NUMA node %d does not exist", ...) with the max-node info moved to errdetail/errhint. The could not pin backend ... message below has the same missing-_() issue. (low confidence)

---

📄 src/backend/tcop/postgres.c (L1102-L1105)

This adds a per-query call on the hot query-execution path for a developer debug knob. Even though it short-circuits via the static applied_numa_node guard, mutating backend CPU affinity on every query is a footgun: a misconfigured node silently serializes all backends onto a single NUMA node. If kept, this needs regression/TAP coverage for the pin / unpin (debug_numa_node = -1) / out-of-range-WARNING paths, which are currently absent. (moderate confidence)

---

📄 src/include/miscadmin.h (L216-L218)

shmem_populate/shmem_interleave are defined in sysv_shmem.c and used there — but like debug_numa_node, none of the three are registered as GUCs anywhere in the tree, so they can only ever hold their default value (false/false/-1) and the branches guarded by them are effectively dead. These declarations tied to an unwired feature read as speculative scaffolding; wire up the GUC registration (and docs) or drop them to keep the patch minimal. (high confidence)

---

📄 src/include/miscadmin.h (L218-L218)

Double blank line after the new externs is unnecessary whitespace churn; keep a single blank line before the next comment block to stay pgindent-clean and minimal. (low confidence)

💡 Suggested change

Before:

+extern PGDLLIMPORT int debug_numa_node;
+
+

After:

+extern PGDLLIMPORT int debug_numa_node;
+

---

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

Unrelated change / scope creep. shellprog is SHELLPROG, which is $(SHELL) (normally /bin/sh) under Make and /bin/sh under meson -- i.e. an absolute path. For an absolute path execl and execlp are equivalent, so this change has no effect in the normal case. The only difference execlp introduces is a $PATH lookup when SHELLPROG contains no slash, which is a behavioral change (shell resolution now depends on the invoking environment's $PATH) that no part of this patch requires. This looks like a developer-environment convenience (Nix/env-based toolchains) and should be dropped from a patch headed for pgsql-hackers -- keep the diff minimal and the test harness's shell path deterministic.

💡 Suggested change

Before:

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

After:

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

---

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

This diverges from the tree-wide convention: every other Perl script in the repository (36+ files, e.g. src/backend/catalog/genbki.pl, src/tools/copyright.pl, src/backend/nodes/gen_node_support.pl) uses #!/usr/bin/perl. Changing only pgindent to #!/usr/bin/env perl is an inconsistent, unrelated edit -- a developer-environment convenience (e.g. Nix) that does not belong in this patch. Drop this hunk to keep the diff minimal and consistent with project style.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/backend/storage/buffer/freelist.c (L497-L506)

Potential un-interruptible infinite loop on the hottest allocation path. If the home partition's balance vector is all zeros, clocksweep_partition_budget is 0 on entry and this while loop keeps advancing clocksweep_partition_current reading sweep->balance[...] -- which never becomes non-zero -- forever. There is no CHECK_FOR_INTERRUPTS here. This is user-reachable: ClockSweepSetWeights() only validates each weight is 0..100, not that the sum is > 0 (see below), so an operator can set all weights of a partition to 0 and hang any backend whose home partition that is. Guard the loop (e.g. bound it by num_partitions and fall back to the home partition) and/or reject an all-zero weight vector in ClockSweepSetWeights(). [high confidence]

---

📄 src/backend/storage/buffer/freelist.c (L1693-L1701)

ClockSweepSetWeights() validates each weight individually (0..100) but never checks that the vector has at least one non-zero entry. An all-zero vector passes validation and then wedges ChooseClockSweep()'s budget loop (see above) into an infinite spin for any backend whose home partition this is. Reject a weight vector whose sum is 0 (and arguably require the home weight sweep->balance[partition] to be non-zero). [high confidence]

---

📄 src/backend/storage/buffer/bufmgr.c (L3882-L3884)

smoothed_alloc and smoothed_density are function-level statics, but BgBufferSyncPartition() is now invoked once per partition within a single BgBufferSync() cycle. In the original BgBufferSync these were per-pool state; here they are a single shared pair clobbered by every partition in turn, so the moving averages no longer track any one partition's history -- after the loop they reflect only the last partition processed. All other cross-call state was correctly made per-partition (saved_info[], prev_sweep_claims[]); these two were missed. Make them per-partition (e.g. fields in BufferSyncPartition) to preserve the intended smoothing semantics. [moderate confidence]

---

📄 src/backend/tcop/postgres.c (L1034-L1035)

Comment accuracy: this calls debug_numa_node a "GUC", but it is never registered via DefineCustomIntVariable/guc_tables.c, so it is not actually a GUC. The comment should describe what the variable actually is. Also the final sentence "-1 disables it (default)" is missing its terminating period (incomplete comment). (moderate confidence)

💡 Suggested change

Before:

 * If the debug_numa_node GUC is set (>= 0), pin this backend to run on the
 * CPUs of the requested NUMA node.  -1 disables it (default)

After:

 * If debug_numa_node is set (>= 0), pin this backend to run on the
 * CPUs of the requested NUMA node.  -1 disables it (default).

---

📄 src/backend/tcop/postgres.c (L1055-L1056)

Inconsistent error handling: the disable path ignores the return value of numa_run_on_node_mask(), while the two sibling branches (numa_run_on_node failure and the out-of-range node) both emit a WARNING on failure. If re-pinning to all nodes fails, the backend stays silently pinned and applied_numa_node is still set to -1, so the failure is never retried. Check the return value and warn on failure for consistency. (moderate confidence)

[github-actions]

This entire file is machine-specific developer scaffolding and should not be committed to the shared tree. It targets a "HOT indexed updates" patch series whose symbols do not exist in this repository: heap_hot_indexed_create_tuple, heap_xlog_indexed_update, HEAP_INDEXED_UPDATED, etc. all return no matches in the source. The function breakpoints will silently fail to bind, and the line-number breakpoints point at unrelated code. For a change destined for pgsql-hackers/commitfest, committing personal dotfiles violates the minimal-diff / one-logical-change discipline and will be rejected. Drop this file from the patch.

[github-actions]

These file:line breakpoints are stale and misdescribed. In the current tree heapam_indexscan.c:182 is lp = PageGetItemId(page, offnum);, :250 is skip = false;, and :297 is a plain return heapam_index_getnext_slot(...) -- none of them relate to "bitmap accumulator", "INDEXED_UPDATED tuple", or "stale entry detection". The comments describe behavior that was never applied here. Hard-coded source line numbers also rot on every edit; they do not belong in a committed artifact.

[github-actions]

Destructive data-loss footgun. These aliases run rm -rf on paths derived from PG_DATA_DIR, PG_BUILD_DIR, and PG_INSTALL_DIR, but those variables are exported only by the Nix commonHookTail in shell.nix, not by this file. When pg-aliases.sh is sourced standalone (outside the dev shell), the variables are unset and these commands become rm -rf "" / trash "", operating on the current directory or unexpected paths. At minimum guard with [ -n "$PG_DATA_DIR" ] before any removal -- but this file should not be in-tree at all.

[github-actions]

Host-wide, privilege-escalating side effect embedded in a sourced alias file. pg-enable-cores/pg-disable-cores write to the system-wide sysctl /proc/sys/kernel/core_pattern via sudo tee. While gated behind an explicit function call, shipping machine-mutating sudo commands in a committed dev-shell helper is a security-relevant footgun and reinforces that this file is personal tooling, not tree content.

[github-actions]

pg-compdb invokes compdb with no availability check or fallback, unlike other helpers in this file (which have || rm -rf style fallbacks). On a host without compdb the alias silently fails. This is a minor maintainability inconsistency; the broader issue is that this entire developer-tooling file should likely not be committed to the tree.

[github-actions]

Unchecked malloc(): if saved_info allocation fails, the next access saved_info[partition] is a NULL dereference / crash. This is inconsistent with prev_sweep_claims in BgBufferSyncPartition, which does check calloc() and ereports ERRCODE_OUT_OF_MEMORY. Add a NULL check and ereport, matching the sibling allocation.

Suggested change

	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	}
	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	if (saved_info == NULL)
	ereport(ERROR,
	(errcode(ERRCODE_OUT_OF_MEMORY),
	errmsg("out of memory")));
	}

[github-actions]

BufferGetNode(Buffer) is declared extern here ("in buf_init.c") but there is no definition anywhere in the tree (buf_init.c defines BufferPartitionGet/Count/Params but not BufferGetNode). This is dead scaffolding; either remove the declaration or provide the implementation it promises. A declared-but-undefined exported symbol is a link hazard for any caller.

[github-actions]

Typo in comment: "numa_nod" -> "numa_node".

Suggested change

	* numa_nod specifies node for this partition (-1 means allocated on any node)
	* numa_node specifies node for this partition (-1 means allocated on any node)

[github-actions]

Integer-division truncation changes bgwriter behavior versus the single-pool original. With recent_alloc < num_partitions, recent_alloc_partition truncates to 0, so every partition sees zero allocations: the density/alloc smoothing is suppressed and the LRU-scan trigger (strategy_delta > 0 && recent_alloc_partition > 0) never fires, so the bgwriter stops pre-cleaning under light-but-nonzero load. On a 32-partition pool this means up to 31 allocations/cycle are invisible. Consider distributing the remainder or using a per-partition count from StrategySyncPrepare rather than an averaged truncation.

[github-actions]

bgwriter_lru_maxpages / num_partitions truncates to 0 whenever bgwriter_lru_maxpages < num_partitions (e.g. the default 100 with 128 partitions, or any small setting). A per-partition write cap of 0 means the inner if (++num_written >= 0) breaks immediately after the first written buffer, effectively crippling bgwriter cleaning even though bgwriter_lru_maxpages > 0. The global cap should be apportioned without truncating to zero (clamp to at least 1, or track a global written count across partitions).

[github-actions]

🔍 OCR found 58 issue(s).

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

---

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

ChooseClockSweep() is called twice per buffer allocation on the hottest path in the server: once as ChooseClockSweep(false) to bump numBufferAllocs, then again as ChooseClockSweep(true). Each call runs ClockSweepPartitionIndex() (and, under shared_buffers_numa, periodically sched_getcpu()/numa_node_of_cpu()). The code's own XXX admits the two calls may select different partitions if the backend migrates, making the numBufferAllocs vs numRequestedAllocs accounting inconsistent. Compute the sweep once and reuse it.

---

📄 src/backend/storage/buffer/freelist.c (L43-L45)

Numerous unresolved WIP markers (XXX/TODO/FIXME) and open design questions remain in production code across this file (e.g. the MAX_BUFFER_PARTITIONS enforcement note, the FIXME in StrategyCtlShmemInit about numBuffers sizing, the "Surely not" lock question in StrategySyncPrepare, "TODO:: just guessing,heuristics"). Per project discipline these must be resolved before commit; comments should describe current behavior, not aspirations. This signals the patch is WIP, not commit-ready.

---

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

Typo in the struct comment: "numa_nod" should be "numa_node" to match the field name.

💡 Suggested change

Before:

 * numa_nod specifies node for this partition (-1 means allocated on any node)

After:

 * numa_node specifies node for this partition (-1 means allocated on any node)

---

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

Grammar error in the rewritten comment: "the space wastage is be a bit more noticeable" (stray "be").

💡 Suggested change

Before:

 * sized even on 32bit systems, where the space wastage is be a bit more

After:

 * sized even on 32bit systems, where the space wastage is a bit more

---

📄 src/include/storage/buf_internals.h (L143-L144)

The comment contradicts the macro value. BM_SECOND_CHANCE comment says "the 0..5 usage counter it replaces" and "accumulate up to five chances", but the old counter was 0..BM_MAX_USAGE_COUNT where BM_MAX_USAGE_COUNT was 5, i.e. 0..5 is correct for the range but "up to five chances" undercounts (six states, five increments). Minor, but since the whole justification hinges on this, state it precisely (the counter capped at BM_MAX_USAGE_COUNT=5).

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L37-L37)

pg_buffercache_set_partition() mutates shared clock-sweep state but is only REVOKEd from PUBLIC with no compensating GRANT, so after the upgrade only superusers can call it. The sibling pg_buffercache_partitions() is granted to pg_monitor. If the intent is for pg_monitor (or another role) to tune weights, a GRANT is missing; if superuser-only is intended, that asymmetry should be deliberate and documented. Please confirm the intended privilege model. (moderate confidence)

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L5-L5)

Use CREATE FUNCTION, not CREATE OR REPLACE FUNCTION, in an extension upgrade script. This function is brand new in 1.8, so OR REPLACE is unnecessary and, worse, silently masks an accidental pre-existing object with the same signature instead of erroring out. The existing upgrade scripts (e.g. 1.6--1.7) consistently use plain CREATE FUNCTION. (high confidence)

💡 Suggested change

Before:

CREATE OR REPLACE FUNCTION pg_buffercache_partitions()

After:

CREATE FUNCTION pg_buffercache_partitions()

---

📄 contrib/pg_buffercache/pg_buffercache_pages.c (L953-L953)

Obtaining the int4[] type OID via lookup_type_cache(INT4OID, 0)->typarray is a roundabout, allocation-heavy way to name a well-known catalog type. Use the INT4ARRAYOID constant directly for the column type and drop the type-cache lookup entirely. (high confidence)

---

📄 contrib/pg_buffercache/pg_buffercache_pages.c (L990-L991)

Correspondingly, use INT4ARRAYOID here instead of typentry->typarray so the weights column type is spelled with the direct constant and the lookup_type_cache call can be removed. (high confidence)

💡 Suggested change

Before:

		TupleDescInitEntry(tupledesc, (AttrNumber) 12, "weigths",
						   typentry->typarray, -1, 0);

After:

		TupleDescInitEntry(tupledesc, (AttrNumber) 12, "weights",
						   INT4ARRAYOID, -1, 0);

---

📄 src/backend/port/sysv_shmem.c (L632-L633)

These libnuma calls (numa_set_interleave_mask, numa_interleave_memory, numa_set_localalloc, and numa_all_nodes_ptr) are invoked without first checking numa_available(). Per numa(3), calling any libnuma routine before numa_available() returns a non-negative value has undefined behavior and numa_all_nodes_ptr is not guaranteed to be initialized; this can crash at startup on builds where libnuma loads but the kernel reports no NUMA support. Note the sibling path in postgres.c:process_debug_numa_node() correctly guards with numa_available() < 0. The tree already provides pg_numa_init() (src/port/pg_numa.c), which wraps numa_available() and even works around the libnuma EPERM bug; this path should gate on it (DRY). (high confidence)

---

📄 src/backend/port/sysv_shmem.c (L699-L700)

Comment drift: both this comment and the inner "undo the policy set by numa_set_membind() earlier" reference numa_set_membind(), but no numa_set_membind() call exists in this function. The policy set earlier is via numa_set_interleave_mask() (line 633). Fix the comments to name the actual API so they describe what the code does now. (high confidence)

---

📄 src/backend/port/sysv_shmem.c (L713-L714)

Same stale API reference here: this says numa_set_membind() but the policy was set via numa_set_interleave_mask(). Update to match. (high confidence)

---

📄 src/backend/tcop/postgres.c (L1102-L1105)

process_debug_numa_node() runs on every exec_simple_query/exec_execute_message (the hot query path). The static applied_numa_node guard only short-circuits when the value is unchanged; it is a function-local static, so pinning established by one query persists into later queries in the same backend even if those are for different sessions in a pooled/reused backend. A PGC_USERSET debug_ GUC that silently and persistently alters CPU/NUMA affinity for the backend is a footgun worth calling out, and placing this on the per-query path (rather than, say, a GUC assign_hook) is a questionable design for a debug-only knob. (moderate confidence)

---

📄 src/include/miscadmin.h (L218-L218)

Two blank lines after the new declarations is spurious vertical whitespace that pgindent / git diff --check style would flag; the surrounding declarations use a single blank-line separator. Also note: these NUMA/shmem globals (shmem_populate/shmem_interleave live in sysv_shmem.c; debug_numa_node in postgres.c) are an awkward fit for miscadmin.h, whose other externs are process/database identity globals. Consider declaring them in a NUMA/shmem-specific header (e.g. port/pg_numa.h or storage/pg_shmem.h) next to the code that owns them. (low confidence)

💡 Suggested change

Before:

extern PGDLLIMPORT int debug_numa_node;
+
+

After:

extern PGDLLIMPORT int debug_numa_node;

---

📄 .gdbinit (L1-L3)

This entire file is developer-local debugger scaffolding that does not belong in a PostgreSQL patch destined for pgsql-hackers/commitfest. Worse, it references a completely different patch series: the header and every breakpoint target the "HOT Indexed Updates" work (heap_update, heap_hot_indexed_create_tuple, heap_xlog_indexed_update, etc.), but the actual C change in this update is NUMA-aware clock-sweep buffer partitioning (freelist.c/buf_init.c/pg_buffercache). None of the referenced heap_hot_indexed_* functions exist anywhere in the tree (confirmed via search), and heapam.c/pruneheap.c are not part of this change at all. This bundles unrelated concerns into one commit and must be dropped from the patch.

---

📄 .gdbinit (L19-L20)

Hardcoded absolute source line-number breakpoints are extremely brittle: they drift the instant any line is inserted above them and silently land on the wrong statement. These are already stale -- the comment claims line 3210 is heap_update, but heap_update() actually starts at heapam.c:3269 in the tree. Prefer function-name breakpoints (break heap_update) over line numbers; the numeric breakpoints in this file (heapam.c:4019, pruneheap.c:1802, indexam.c:299, etc.) point at code that either moved or does not exist.

---

📄 pg-aliases.sh (L1-L1)

This file (658 lines of personal workflow aliases, valgrind/asan/rr wrappers, perf/flamegraph helpers, benchmark drivers) is developer-local scaffolding that does not belong in a PostgreSQL patch. It is unrelated to the actual change in this update (NUMA clock-sweep buffer partitioning) and violates the minimal-diff discipline; committing it bundles multiple unrelated concerns and would be rejected on -hackers. It should live in the author's dotfiles/home dir, not in the repo.

---

📄 pg-aliases.sh (L191-L194)

The generated wrapper interpolates $PG_SOURCE_DIR, $bindir and $PG_BENCH_DIR unquoted into the heredoc. If any of these paths contain spaces or shell metacharacters, the emitted 'postgres' wrapper will be syntactically broken or misexecute (e.g. --suppressions=$PG_SOURCE_DIR/... and $bindir/postgres split on whitespace). Since the wrapper dir is a world-accessible mktemp dir under /tmp that is later prepended to PATH, this is also a footgun on multi-user hosts. If this file is kept at all, quote the interpolated paths.

---

📄 pg-aliases.sh (L580-L580)

pg-enable-cores/pg-disable-cores silently invoke 'sudo tee' to overwrite the system-wide kernel.core_pattern sysctl. For a file that is auto-sourced on shell entry (via shell.nix's commonHookTail), having helpers that mutate a global kernel setting via sudo is a surprising, unguarded side effect. There is no save/restore of the prior core_pattern value either, so pg-disable-cores hardcodes 'core' rather than restoring whatever was there before.

---

📄 flake.nix (L2-L2)

This flake defines a personal multi-variant Nix dev environment (gcc/clang, glibc/musl, valgrind/asan). Like .gdbinit and pg-aliases.sh, it is developer-local tooling that does not belong in a PostgreSQL patch destined for pgsql-hackers/commitfest, and is unrelated to the actual change in this update (NUMA clock-sweep buffer partitioning). It should be kept outside the repo. (Note: shell.nix references ./glibc-no-fortify-warning.patch, which does exist at the repo root, so evaluation is not broken -- but that patch file is itself more unrelated scaffolding being pulled into the tree.)

---

📄 shell.nix (L6-L6)

This 745-line file defines personal Nix dev shells, a custom gdbinit, flamegraph/pgbench helper scripts and a patched glibc. It is developer-local environment scaffolding unrelated to the actual change in this update (NUMA clock-sweep buffer partitioning) and does not belong in a PostgreSQL patch; it should be maintained outside the repository.

---

📄 src/backend/port/sysv_shmem.c (L106-L107)

These global declarations are not pgindent-clean: the type and name are separated by a single space rather than the tab alignment pgindent produces (e.g. bool\t\tshmem_populate = false;). As written, git diff --check / pgindent will churn these lines. Align with tabs.

💡 Suggested change

Before:

bool shmem_populate = false;	/* MAP_POPULATE */
bool shmem_interleave = false;	/* NUMA interleaving */

After:

bool		shmem_populate = false;	/* MAP_POPULATE */
bool		shmem_interleave = false;	/* NUMA interleaving */

---

📄 src/backend/port/sysv_shmem.c (L713-L714)

Behavioral hazard (moderate confidence): numa_set_interleave_mask()/numa_set_localalloc() change the process-wide default memory allocation policy of the postmaster, not just a scope local to this segment. Restoring with numa_set_localalloc() does not restore whatever policy the postmaster was launched with (e.g. a policy inherited from a numactl/cpuset wrapper); it unconditionally forces MPOL_DEFAULT/localalloc. Any later allocation in the postmaster (and children that fork afterwards) now runs under localalloc instead of the operator's chosen policy. If the intent is to interleave only this segment, prefer applying the policy to the mapping (numa_interleave_memory / mbind on ptr) rather than mutating the global process policy, or save and restore the prior policy via get_mempolicy().

---

📄 src/backend/tcop/postgres.c (L1038-L1040)

POLA/footgun (moderate confidence): on a build without USE_LIBNUMA this function compiles to an empty body, so setting debug_numa_node has no effect and no feedback. The established PostgreSQL pattern for build-time-optional features is to reject the setting via a GUC check hook (cf. commands/variable.c: GUC_check_errmsg("SSL is not supported by this build")). Without that, operators silently get no pinning on non-NUMA builds. Consider a check_debug_numa_node hook that errors when !USE_LIBNUMA (and when numa_available() < 0) instead of only emitting a one-shot WARNING at query time.

---

📄 .clangd (L1-L2)

This .clangd file is developer-local IDE/editor configuration (clangd language-server tuning, inlay hints, personal compile-flag overrides). Like the already-flagged .gdbinit, flake.nix, shell.nix and pg-aliases.sh, it is personal tooling that does not belong in a PostgreSQL patch destined for pgsql-hackers/commitfest and is unrelated to the actual change in this update (NUMA clock-sweep buffer partitioning). Committing editor dotfiles into the tree is a top rejection reason (non-minimal diff). It should be removed from the patch and kept in a local, gitignored config instead. Confidence: high.

---

📄 src/backend/storage/buffer/bufmgr.c (L3882-L3884)

Regression: these moving-average accumulators are function-level static, but BgBufferSyncPartition() is now called once per partition inside the loop in BgBufferSync(). The single smoothed_alloc/smoothed_density state is therefore shared and overwritten across all partitions on every cycle, instead of being tracked per clock-sweep partition. The result is garbage smoothing: partition N's density estimate is clobbered by partition N+1 within the same BgBufferSync() call, so reusable_buffers_est/upcoming_alloc_est no longer reflect the partition being scanned. These must be stored per-partition (e.g. in BufferSyncPartition / saved) like next_to_clean and the other per-partition state. (high confidence)

---

📄 src/backend/storage/buffer/freelist.c (L497-L506)

Potential infinite loop on the hottest path in the server. This loop advances round-robin until it finds a partition with non-zero budget, reading sweep->balance[clocksweep_partition_current]. If every entry of sweep->balance[] is 0, the loop never terminates and spins forever with no CHECK_FOR_INTERRUPTS. An all-zero vector is reachable: (a) the hysteresis blend 0.5*old + 0.5*new truncates to uint8, so a self-weight of 1 that is not refreshed decays 1 -> 0 across rebalances; (b) ClockSweepSetWeights() validates each weight 0..100 individually but never rejects an all-zero vector. Guard the loop with a bounded counter (fall back to the home partition after a full pass) and reject an all-zero weight vector. (high confidence)

---

📄 src/backend/storage/buffer/freelist.c (L1693-L1701)

ClockSweepSetWeights() validates each weight in 0..100 individually but never checks that the vector sums to something non-zero. A user can set all weights of a partition to 0; a backend whose home partition is that one then enters the while (clocksweep_partition_budget == 0) loop in ChooseClockSweep() and hangs (see the comment there). Reject a weight vector whose sum is 0 (ERRCODE_INVALID_PARAMETER_VALUE) to close this footgun. (high confidence)

---

📄 src/include/storage/bufmgr.h (L403-L404)

Dangling declaration: BufferGetNode is declared here (comment says "in buf_init.c") but no definition exists anywhere in the tree -- buf_init.c defines no such function, and there are no callers. Either this is dead scaffolding that should be removed, or an implementation is missing and any future caller will fail to link. Per the minimalism/YAGNI discipline, drop the declaration until it is actually implemented and used. (high confidence)

---

📄 src/backend/storage/buffer/freelist.c (L1636-L1638)

idx is validated only by Assert(), which compiles out in production builds. The index is already used at line above to form sweep before the Assert runs, and this is an exported (PGDLLIMPORT) API callable from extensions such as pg_buffercache. The current in-tree caller bounds i itself, but an exported helper should not rely on that; promote the check to a runtime ereport(ERROR) with ERRCODE_INVALID_PARAMETER_VALUE and move it before the array access. (moderate confidence)

---

📄 .clangd (L42-L43)

Beyond not belonging in the tree, this include path is internally inconsistent: CompilationDatabase: build/ points clangd at the repo-root build/ directory (and -I. resolves to the repo root), yet -I../../../../src/include is a four-levels-up relative path that only resolves correctly from a deeply nested source file, not from the repo root. The two cannot both be right; for files opened at the top level this header path will miss src/include, defeating the stated purpose of silencing missing-include diagnostics. Confidence: moderate.

---

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

This breaks the tree-wide convention. Every other Perl script in the PostgreSQL source (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 others) uses #!/usr/bin/perl; this diff introduces the only #!/usr/bin/env perl in the entire codebase, making pgindent an inconsistent outlier.

This change is also unrelated to the buffer-management work this patch is about -- it is local-environment churn (matching the Nix shell scaffolding elsewhere in the change set) and would be flagged on -hackers as an out-of-scope modification. #!/usr/bin/env perl is additionally considered less secure/reproducible since it resolves perl via PATH rather than a fixed interpreter. Revert this hunk. (high confidence)

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

Switching from execl to execlp changes behavior and is unnecessary. shellprog is set from SHELLPROG, which is defined as $(SHELL) in src/test/regress/GNUmakefile (typically the absolute path /bin/sh) and hardcoded to "/bin/sh" in src/test/regress/meson.build. For an absolute path, execlp behaves identically to execl, so this change has no benefit in the normal build.

The only case where it differs is when SHELLPROG contains no slash -- then execlp performs a PATH search. That is a footgun: it makes the regression driver pick up whatever sh happens to be first on PATH instead of the shell chosen at configure/build time, which is both a reproducibility and a (minor) security concern. Since the build definitions that produce SHELLPROG are not changed in this patch, this hunk is unrelated out-of-scope churn and should be reverted. (high confidence)

💡 Suggested change

Before:

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

After:

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

[github-actions]

These stubs fall off the end of non-void functions. Assert(false) compiles to a no-op in production (non-assert) builds, so pg_numa_bind_to_node (returns int) and pg_numa_page_size (returns Size) return no value -- undefined behavior, and a guaranteed -Wreturn-type warning/error under -Werror. The existing pg_numa_get_max_node stub returns 0; follow that pattern. [high confidence]

Suggested change

	int
	pg_numa_bind_to_node(char *startptr, char *endptr, int node)
	{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	}
	int
	pg_numa_bind_to_node(char *startptr, char *endptr, int node)
	{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return -1;
	}

[github-actions]

Same missing-return defect: pg_numa_page_size returns Size but only calls Assert(false), which is a no-op in non-assert builds. Add a return 0; after the Assert. [high confidence]

Suggested change

	Size
	pg_numa_page_size(void)
	{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	}
	Size
	pg_numa_page_size(void)
	{
	/* we don't expect to ever get here in builds without libnuma */
	Assert(false);
	return 0;
	}

[github-actions]

Non-portable pointer-to-integer cast. Casting a pointer to signed int64 is implementation-defined and not the tree's idiom; c.h consistently uses uintptr_t for alignment checks (e.g. PointerIsAligned, TYPEALIGN). Use (uintptr_t) startptr (or the PointerIsAligned-style check) so the alignment test is correct across 32/64-bit targets. [high confidence]

Suggested change

	Assert((int64) startptr % pg_numa_page_size() == 0);
	Assert((uintptr_t) startptr % pg_numa_page_size() == 0);

[github-actions]

numa_flags is a newly exported global that is never assigned or read anywhere in this change set (the buffer/shmem code keys off shared_buffers_numa instead), and NUMA_BUFFERS has no users. This is speculative scaffolding -- dead code and an unused exported symbol. Drop numa_flags/NUMA_BUFFERS until there is a caller, or wire it in. [medium confidence]

[github-actions]

This WIN32 branch is unreachable dead code. The entire function sits inside the #ifdef USE_LIBNUMA block, which (per this file's own header comment) is Linux-only; libnuma is not available on Windows. On Windows the #else stub (Assert(false)) compiles instead, so GetSystemInfo here never runs. Remove the WIN32 branch and use sysconf(_SC_PAGESIZE) unconditionally within the libnuma section. [high confidence]

Suggested change

	#ifdef WIN32
	SYSTEM_INFO sysinfo;
	GetSystemInfo(&sysinfo);
	os_page_size = sysinfo.dwPageSize;
	#else
	os_page_size = sysconf(_SC_PAGESIZE);
	#endif
	os_page_size = sysconf(_SC_PAGESIZE);

[github-actions]

Spaces used for indentation instead of tabs in this block ( int cpu;, if (...), and the elog/cached_node lines). PostgreSQL uses tabs (width 4); this will not pass pgindent and signals hand-formatting.

[github-actions]

Unchecked malloc. saved_info is malloc'd but the return value is never NULL-checked; on allocation failure the partition loop below dereferences &saved_info[partition], crashing. prev_sweep_claims in BgBufferSyncPartition does check its calloc result and ereport(ERROR) on failure; do the same here. Note also this assumes num_partitions is identical on every call (the array is sized once); that invariant should be asserted since StrategySyncPrepare recomputes it each time.

Suggested change

	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	}
	saved_info = malloc(sizeof(BufferSyncPartition) * num_partitions);
	if (saved_info == NULL)
	ereport(ERROR,
	(errcode(ERRCODE_OUT_OF_MEMORY),
	errmsg("out of memory")));
	}

[github-actions]

bgwriter_lru_maxpages / num_partitions truncates to 0 whenever bgwriter_lru_maxpages < num_partitions (e.g. the default 100 with >100 partitions, or any small setting). The subsequent ++num_written >= 0 then breaks after writing a single buffer per partition, effectively disabling the bgwriter's LRU writeback. The per-partition cap needs a floor of at least 1, or the division needs rethinking.

[github-actions]

Dead/commented-out code and a self-contradicting WIP comment left in production. saved_info_valid is now set in the BgBufferSync caller after the partition loop, so this commented-out assignment and its XXX note are dead code. Remove them (comments must describe current behavior, and no commented-out code should ship).

[github-actions]

Spinlock discipline: StrategySyncBalance holds clock_sweep_lock across a full floating-point hysteresis loop (CLOCKSWEEP_HISTORY_COEFF * ... + ... over MAX_BUFFER_PARTITIONS entries). Spinlocks must be held only for a handful of instructions with no loops or FP math; this violates the "minimize work under contended locks" rule. Compute the new weights into a local array first, then take the lock only to copy them in. (Also, the whole recompute is O(num_partitions^2).)