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 cross-reference returns nothing (expected — the scrape is noted as incomplete in the tool). I have enough to write the report.

🧵 Related discussion

• Adding basic NUMA awareness (Tomas Vondra, started 2025-07-01, thread 8657, 94+ messages) — the primary parent thread. It introduces NUMA-aware shared-buffer interleaving, per-node freelists, and a partitioned clock sweep. This is where almost every commit in this PR originates:

  • 4d2fa4f85f2 shmem_populate/shmem_interleave GUCs and ab69ff60fc4/5ad4f24d46f shared-buffer partitioning = Vondra's numa_buffers_interleave/numa_partition_freelist work.
  • 4782aa50ed8/ec279e0793d/9a8ab30d527/1f6a1db1ef0/16cd1cd9300 (partitioned clock sweep, scan-all-partitions, allocation balancing, cached CPU/NUMA node) map directly to the ClockSweep struct, ClockSweepPartitionIndex(), balance[], MAX_BUFFER_PARTITIONS, and sched_getcpu() discussion in Vondra's 2026-06-16 reply to Jakub Wartak (struct padding/false-sharing, bounds on balance[], caching sched_getcpu() to avoid syscall cost — the exact rationale behind the "cached CPU/NUMA node" commit).

• [PATCH] Batched clock sweep to reduce cross-socket atomic contention (Greg Burd, started 2026-04-25, thread 7412) — the "batched sweep" half of the PR title. Greg later withdrew this standalone thread and folded the keeper piece into the NUMA thread; he says so explicitly in his 2026-09-15 message: "I withdrew my batched clock sweep on its own thread. The piece worth keeping belongs here: replacing the 0..5 usage_count with a 1-bit HOT/COOL cooling state, on top of your partitioning." That is precisely PR commit 9634225d52 ("track one second chance per buffer, not a 0..5 count") and the HOT/COOL evictor language. Ants Aasma and Andres Freund reviewed the batching design (July 2026).

  • The batching-granularity debate (a backend claiming B[0..63] and starving others) is argued in Greg's 2026-04-28 reply to Andres — relevant to commits 6ebbba65aaf ("bound the work one allocation can do") and aab9afd2629 (bgwriter spends chances "when the sweep starves").

• [PATCH] Let's get rid of the freelist and the buffer_strategy_lock (Greg Burd, 2025-07-11, thread 18478) — the "strategy removal" part of the title. Andres' reply (2025-07-11) flags that autoprewarm's use of have_free_buffer() needs a replacement once the freelist is gone — directly relevant to the PR's freelist rework and the demand-loaded pages on probation commit (4fbc4b48847). Thomas Munro and Tomas Vondra also reviewed here.

🔗 Related commits / prior art

• NUMA shared memory interleaving (Jakub Wartak, thread 20394) — the lighter-weight competing approach (plain numa_interleave of the shm segment) that Vondra's series explicitly positions against. Prior art for the src/port/pg_numa.c / sysv_shmem.c changes.
• pg_shmem_allocations_numa view (committed, E1u1tr8-...@gemulon) — existing NUMA observability infra this builds on; relevant to the pg_buffercache 1.7→1.8 NUMA-node column additions in this PR.
• Historical clock-sweep lineage worth citing for reviewers: Lockless StrategyGetBuffer() clock sweep (Andres, 2014) and Scaling shared buffer eviction / bgreclaimer (Robert Haas, 2014) — a prior, rejected attempt to move eviction work off backends into a dedicated reclaimer, which the current freelist-removal / bgwriter-assist design supersedes.

📋 Commitfest

• No commitfest entry resolved via the thread cross-reference tool (returns empty — the mailing-list↔CF scrape is incomplete per the tool's own note), and none found under author greg@burd.me. Treat "no CF entry" as unconfirmed, not "no entry exists."

🧭 Context for reviewers

• This PR is a squash of two in-flight, still-WIP lines of work: Vondra's NUMA series (interleaving + partitioned freelist + partitioned clock sweep) and Burd's batched-sweep / HOT-COOL / freelist-removal work. Neither has landed upstream; the discussion was active through at least October 2026. Confidence: high that these are the source threads — commit titles, GUC names, and the ClockSweep/balance[]/sched_getcpu symbols match the thread text verbatim.
• Known open design concerns already raised on-list that reviewers should check this PR actually addresses:
  • balance[] bounds vs. MAX_BUFFER_PARTITIONS=32 on high-sub-NUMA-node hardware (Wartak/Vondra).
  • sched_getcpu() cost on non-x86 / non-VDSO platforms — the "cached CPU/NUMA node" commit (16cd1cd9) is the response, but caching staleness vs. correctness is the tradeoff.
  • ClockSweep struct bloat / false sharing (80 bytes, 2 cachelines; padding unresolved).
  • Removing the freelist breaks autoprewarm's have_free_buffer() stop condition — Andres flagged this needs a replacement, not just deletion.
  • Batch size vs. victim starvation for low-rate backends (Andres/Ants) — relates to commits 6ebbba65 and aab9afd2.
• The repo-hygiene files in the diff (.clangd, .gdbinit, flake.nix, shell.nix, pg-aliases.sh, glibc-no-fortify-warning.patch) are personal dev-setup (cf4f0c6ef60) and do not belong in an upstream submission.

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

[github-actions]

🔍 OCR found 12 issue(s).

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

[github-actions]

The \gset variables are still named stats_bulkreads_before/stats_bulkreads_after, but this query now deliberately measures the normal context (the comment above was rewritten to say the reads are no longer bulkreads). The names now contradict what they hold. Since these exact lines are already being touched, rename them to reflect the new semantics (e.g. stats_normal_reads_before) for both \gset sites and the comparison on the following line. This also requires the matching update in contrib/amcheck/expected/check_heap.out.

Suggested change

	SELECT sum(reads) AS stats_bulkreads_before
	FROM pg_stat_io WHERE context = 'bulkread' \gset
	FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset
	SELECT sum(reads) AS stats_normal_reads_before
	FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset

[github-actions]

This silently changes the user-visible semantics of the pg_buffercache.usagecount column. BUF_STATE_GET_USAGECOUNT returned the full clock-sweep count (0..BM_MAX_USAGE_COUNT, historically 0..5), whereas BUF_STATE_GET_COOLSTATE returns only bit 0 (0=COOL, 1=HOT). The column is still declared smallint and documented as "Clock-sweep access count", so users querying usagecount will now silently get only 0/1 with no doc or column-name update. This is a backward-incompatible behavior change that needs the documentation (pgbuffercache.sgml) updated and, arguably, the column renamed to reflect HOT/COOL semantics. (high confidence)

[github-actions]

usagecount_total now accumulates only 0/1 per buffer instead of 0..BM_MAX_USAGE_COUNT, so the derived usagecount_avg column changes range/meaning without any doc update. The SGML still labels it a clock-sweep average. Update the documentation to match the new HOT/COOL semantics. (high confidence)

[github-actions]

pg_buffercache_usage_counts() now buckets buffers only into indices 0 and 1 (COOL/HOT); the usage_count output column will never exceed 1. This is fine for array bounds since BM_MAX_USAGE_COUNT is redefined to BUF_COOLSTATE_HOT (=1), but the SQL column name usage_count and its doc ("A possible buffer usage count") are now misleading. Update pgbuffercache.sgml and the expected regression output to reflect the reduced value domain. (high confidence)

[github-actions]

The if body is now a single statement after removing the allow_strat assignment, but the braces are retained. PostgreSQL style (and pgindent-adjacent convention) omits braces around single-statement bodies. Since this block was directly modified by the diff, drop the braces to keep the code reading as if it had always been written this way:

	if (!RelationUsesLocalBuffers(scan->rs_base.rs_rd) &&
		scan->rs_nblocks > NBuffers / 4)
		allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
	else
		allow_sync = false;

Confidence: moderate.

Suggested change

	allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
	}
	else
	allow_strat = allow_sync = false;
	if (allow_strat)
	{
	/* During a rescan, keep the previous strategy object. */
	if (scan->rs_strategy == NULL)
	scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD);
	}
	else
	{
	if (scan->rs_strategy != NULL)
	FreeAccessStrategy(scan->rs_strategy);
	scan->rs_strategy = NULL;
	}
	allow_sync = false;
	allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
	else
	allow_sync = false;

[github-actions]

This comment is now dangling. It solely described the removed vac_strategy = bstrategy; assignment; the only other static in this file (anl_context) is set up in do_analyze_rel, not here. Leaving an empty comment block referring to nonexistent code violates the minimal-diff/comment-accuracy discipline. Remove the comment (and the now-doubled blank line).

[github-actions]

With strategy removed, read_stream_next_block() no longer has any distinguishing behavior: it is now an exported thin wrapper around read_stream_get_block(stream, NULL) and has no callers anywhere in the tree (verified: only the callback functions apw_read_stream_next_block and collect_corrupt_items_read_stream_next_block match by name; no invocation of read_stream_next_block(...) exists). Its sole prior reason to exist over calling the internal helper directly was reporting the strategy, which this patch deletes. Recommend removing the function (and its prototype in read_stream.h) as dead transitional scaffolding, or, if it is intentionally retained as public API, keep it clearly justified. Leaving a caller-less exported wrapper contradicts the minimal-diff / YAGNI discipline. (moderate confidence)

[github-actions]

completePasses can be under-counted under concurrency, corrupting bgwriter pacing. StrategySyncStart() relies on nextVictimBuffer staying below 2*NBuffers so its pending-wrap compensation (nextVictimBuffer / NBuffers) is at most 1. With batching, many concurrent backends can each land a fetch_add(batch_size) before any of them reaches the spinlock, so the shared counter can grow to NBuffers + MaxBackends*batch_size. When it exceeds 2*NBuffers, wrapped = current % NBuffers subtracts multiple NBuffers-multiples but completePasses is incremented only once, permanently losing pass counts. This is reachable in the small-NBuffers case (minimum 16, batch capped at NBuffers): 2*NBuffers is easily exceeded by concurrent claims. Consider incrementing completePasses by current / NBuffers instead of a flat +1, and/or bounding growth so the counter cannot exceed 2*NBuffers.

Suggested change

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

[github-actions]

A batch can straddle the NBuffers boundary without any wrap being registered on the shared counter. When start < NBuffers but start + batch_size > NBuffers, the wrap branch (start >= NBuffers) is skipped, yet victim = MyBatchPos % NBuffers wraps internally as MyBatchPos crosses NBuffers. The completePasses increment for that crossing is deferred until a later fetch-add happens to return start >= NBuffers. Between those events, StrategySyncStart() sees nextVictimBuffer already advanced past NBuffers and relies solely on its nextVictimBuffer / NBuffers term to compensate. That compensation is only correct while the counter stays below 2*NBuffers (see the related finding). This coupling is fragile and should be documented or handled explicitly, since a mid-batch wrap is now the common case (batch_size defaults to 32).

[github-actions]

Include is out of group/alphabetical order: port/pg_numa.h is placed after the storage/* block. Per the tree's convention (and pgindent grouping), it belongs with the other port/* headers, immediately after port/atomics.h.

Suggested change

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

[github-actions]

🔍 OCR found 43 issue(s).

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

---

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

The strategy argument was removed from this ReadBufferExtended call, but the comment above still claims "we want to use a nondefault buffer access strategy." This comment is now stale and misleading -- no strategy is passed anymore. Update the comment to reflect that the buffer access strategy is now handled elsewhere (or drop that sentence). (moderate confidence)

---

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

The /* Set up static variables */ comment is now stale: this diff removed the only statement it introduced (vac_strategy = bstrategy;), leaving the comment describing nothing. Remove the dangling comment so the code reads as if it had always been written this way (minimal-diff hygiene). (high confidence)

---

📄 src/backend/commands/vacuum.c (L178-L179)

This removal makes VACUUM (BUFFER_USAGE_LIMIT ...) and ANALYZE (BUFFER_USAGE_LIMIT ...) a hard error. The grammar (gram.y utility_option_list) still accepts any option name as a generic DefElem, so with this branch gone the option now falls through to the unrecognized "VACUUM"/"ANALYZE" option ereport below. This is a backward-compatibility break of a documented, user-visible SQL option, and it breaks existing dump/restore scripts and tooling that pass BUFFER_USAGE_LIMIT. If the intent is to fully retire the feature, the corresponding user-facing surfaces (SQL docs in doc/src/sgml/ref/{vacuum,analyze}.sgml and psql tab-completion in tab-complete.in.c, which still advertise BUFFER_USAGE_LIMIT) must be updated in the same change so the option isn't offered while silently erroring. Confidence: high.

---

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

completePasses accounting is inconsistent with StrategySyncStart's compensation term and can double-count / go backwards.

StrategySyncStart() still returns *complete_passes = completePasses + nextVictimBuffer / NBuffers. But now:

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

2. The if (start >= NBuffers) branch above wraps current % NBuffers but increments completePasses by only 1 even when current drifted to e.g. 2*NBuffers; the sync-start compensation term drops from 2 to 0 across that wrap, so the visible pass count decreases.

Because BgBufferSync does passes_delta = strategy_passes - prev_strategy_passes; strategy_delta += passes_delta*NBuffers; Assert(strategy_delta >= 0), a non-monotonic completePasses can trip that assertion and corrupts bgwriter pacing. Confidence: high. The batched wrap accounting needs to be made consistent with StrategySyncStart's nextVictimBuffer / NBuffers term (i.e. don't both pre-count the crossing here and let sync-start count it again).

---

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

This if (start >= NBuffers) wrap path can lose passes when the counter has drifted more than one NBuffers period. current may be >= 2*NBuffers under batching/contention, but wrapped = current % NBuffers collapses it and completePasses is bumped by only 1, while StrategySyncStart's nextVictimBuffer / NBuffers compensation was >1 before the wrap and becomes 0 after. Net effect: the reported completed-pass count can decrease across the wrap, which BgBufferSync's Assert(strategy_delta >= 0) does not tolerate. Confidence: high. Consider incrementing completePasses by current / NBuffers (the number of whole periods being discarded) so the (nextVictimBuffer, completePasses) pair stays monotonic.

---

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

Under force_cool, resetting trycounter = NBuffers on the ref-bit-clear transition (not just on the HOT->COOL demote) means a highly contended, all-HOT pool where PinBuffer keeps re-setting BUF_REFBIT (it does so on every pin, bufmgr.c:3302) can keep this backend clearing ref bits and resetting the counter without ever landing on a reclaimable COOL buffer. The intended "second unproductive full pass -> elog(ERROR, no unpinned buffers)" backstop can then be deferred well beyond the ~3 passes the comment claims. Consider only resetting trycounter on the actual HOT->COOL demotion (real forward progress toward a victim), and treating a bare ref-clear as non-progress so the escalation/backstop stays bounded. Confidence: moderate.

---

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

Dead flag: BUF_COOLED is defined here, documented in the SyncOneBuffer header, and set via result |= BUF_COOLED, but no caller ever inspects it. SyncOneBuffer's only cool_if_hot=true caller is BgBufferSync, which reacts only to BUF_WRITTEN and BUF_REUSABLE. Per the minimalism/YAGNI discipline, either wire BUF_COOLED into a consumer (e.g. bgwriter pacing/instrumentation) or drop the flag, its result |= assignment, and the doc line. (high confidence)

---

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

Comment-drift / misleading placement. This "Admit the newly loaded page COOL" comment sits directly above the unrelated BM_PERMANENT conditional, but the actual COOL admission is the removal of BUF_USAGECOUNT_ONE from the set_bits |= BM_TAG_VALID; line above (a freshly admitted buffer now has coolstate 0 == COOL). As written the comment misleads readers into thinking the BM_PERMANENT block performs the cooling admission. Move the comment to sit above the set_bits |= BM_TAG_VALID; line. (moderate confidence)

---

📄 src/backend/storage/buffer/bufmgr.c (L2964-L2968)

Same comment-placement issue as the other admission site: the "Admit COOL (probation)" comment is above the BM_PERMANENT conditional rather than the set_bits |= BM_TAG_VALID; line where BUF_USAGECOUNT_ONE was previously set. Relocate it above set_bits |= BM_TAG_VALID; so the comment describes the line that actually performs COOL admission. (moderate confidence)

---

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

This comment block is now stale/dangling. The if blocks it described (the B_CHECKPOINTER/B_BG_WRITER/autovac restrictions on the removed bulkread/bulkwrite/vacuum IOContexts) were deleted, leaving the comment sitting directly above return true; describing nothing. Per PG comment discipline (comments must describe what the code does now) and minimal-diff hygiene, remove the orphaned comment so the function reads as if it had always been written this way.

💡 Suggested change

Before:

	/*
	 * Some BackendTypes do not currently perform any IO in certain
	 * IOContexts, and, while it may not be inherently incorrect for them to
	 * do so, excluding those rows from the view makes the view easier to use.
	 */
 
 	return true;

After:

	return true;

---

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

This removes the user-visible --buffer-usage-limit option (getopt entry, handler, validation, and help text), but the corresponding documentation is not removed: doc/src/sgml/ref/vacuumdb.sgml still contains a full <varlistentry> for --buffer-usage-limit (around line 135). A user-visible removal must delete the matching doc entry in the same patch, otherwise the docs describe an option the binary no longer accepts. Please remove the SGML block as part of this change. (high confidence)

---

📄 src/bin/scripts/vacuuming.c (L267-L272)

This removes the --buffer-usage-limit handling from vacuumdb, a user-visible option shipped since PG16. Together with the coordinated removals in vacuumdb.c and vacuuming.h, this deletes an established CLI feature. This is a backward-compatibility break: existing scripts invoking vacuumdb --buffer-usage-limit=... will now fail with an unknown-option error. Such a removal needs extraordinary justification and a design discussion on -hackers; nothing in the diff explains why. Additionally, doc/src/sgml/ref/vacuumdb.sgml still documents --buffer-usage-limit and is not part of this change set, so the docs will describe an option that no longer exists. (high confidence)

---

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

Dead section header and whitespace churn. All prototypes that lived under "/* in freelist.c /" (GetAccessStrategy, FreeAccessStrategy) were removed, so this comment now labels nothing, and the removal left a stray double blank line before "/* inline functions /". This will trip git diff --check / pgindent style expectations. Remove the empty "/ in freelist.c */" section entirely and collapse to a single blank line.

💡 Suggested change

Before:

/* in freelist.c */


/* inline functions */

After:

/* inline functions */

---

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

API/ABI break on widely-used exported entry points (moderate confidence on impact). ReadBufferExtended(), ReadBufferWithoutRelcache(), and the ExtendBufferedRel*() family drop their BufferAccessStrategy parameter, while GetAccessStrategy()/GetAccessStrategyWithSize()/GetAccessStrategyBufferCount()/GetAccessStrategyPinLimit()/FreeAccessStrategy() are removed outright. These are core buffer-manager APIs used pervasively by out-of-tree extensions; every such extension will fail to compile against this header. On HEAD a signature change is allowed, but wholesale removal of the strategy concept with no replacement is a substantial, non-obvious semantic change (ring buffers gone; scan/VACUUM no longer bounded to a small buffer ring). For pgsql-hackers this needs (a) a reference to the design thread / commitfest entry justifying dropping ring buffers, and (b) confirmation it is a single logical change -- as submitted it bundles at least three independent things (remove BufferAccessStrategy, replace usage_count clock sweep with HOT/COOL cooling state, add NUMA-batched clock sweep), which should be split into separately-committable, individually-bisectable patches.

---

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

This change reduces IOCONTEXT_NUM_TYPES from 6 to 2, which shrinks the fixed-stats structs PgStat_BktypeIO/PgStat_PendingIO/PgStat_IO (dimensioned [IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES], lines 328-337). PgStat_IO is a fixed stats kind that is serialized to the persisted stats file. The comment above PGSTAT_FILE_FORMAT_ID (line 216-217) explicitly requires bumping the format ID "whenever any of these data structures change", and on startup pgstat_read_statsfile() only discards a stale file when format_id != PGSTAT_FILE_FORMAT_ID. Since PGSTAT_FILE_FORMAT_ID (0x01A5BCBC) is unchanged in this patch, a stats file written by a pre-patch server will be accepted and read with the new, smaller layout, corrupting the fixed-stats block (and desynchronizing subsequent parsing). Bump PGSTAT_FILE_FORMAT_ID. Unlike catversion.h this stamp is the author's responsibility. Confidence: high.

---

📄 src/include/storage/buf_internals.h (L136-L137)

BUF_STATE_GET_USAGECOUNT now has no callers anywhere in the tree -- every consumer was switched to BUF_STATE_GET_COOLSTATE. Leaving the old accessor behind is dead code and a footgun: the field it reads now mixes the cooling bit (bit 0) and the ref bit (bit 1), so any new caller that reaches for the familiar name gets a 0..3 value conflating two orthogonal flags. Remove it. (high confidence)

---

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

pgindent-nonconforming multi-line comment: the block opens with text on the same line as /* and closes with an inline */, unlike every other multi-line comment in this header (which puts /* on its own line with asterisk-aligned continuation). This will not survive pgindent cleanly. (moderate confidence)

💡 Suggested change

Before:

/* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
 * the second-chance ref bit (bit 1) does not perturb the HOT/COOL test. */

After:

/*
 * Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
 * the second-chance ref bit (bit 1) does not perturb the HOT/COOL test.
 */

---

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

BM_MAX_USAGE_COUNT now means "the HOT cool-state value" (=1), not a usage-count maximum, yet it is still consumed as a numeric range bound -- e.g. pg_buffercache sizes usage_counts[BM_MAX_USAGE_COUNT + 1] and iterates over it. Retaining a count-named macro for a boolean cool-state violates POLA and invites a future reader to treat it as a real max count. Prefer a name that reflects the new semantics (e.g. BM_MAX_COOLSTATE) and update call sites, rather than aliasing the old name for the single expression that happens to read naturally. (moderate confidence)

[github-actions]

Non-ASCII em-dash character (—) in a source/config comment. The project's contribution standards require ASCII-only in source and diffs (no smart quotes, em-dashes, or ellipsis characters). Replace with an ASCII - (or --).

Suggested change

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

[github-actions]

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

Suggested change

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

[github-actions]

The module docstring promises the script "exits 0 even on soft failures (writes a note)", and every other failure path here honors that (MCP unreachable at line 168, no tools at line 172, Bedrock error at line 212 all write a note to OUT and return). But import boto3 / from botocore.config import Config are not guarded: if the SDK is missing/unimportable this raises an uncaught ImportError, exits non-zero, and writes NO note to PG_HISTORY_OUT, leaving the downstream "Upsert PR comment" step with empty output. The workflow's || true masks the exit code but not the missing note. Wrap the import in the same try/except-and-write-note pattern used for the MCP setup for consistency with the documented contract. (moderate confidence)

Suggested change

	import boto3
	from botocore.config import Config
	try:
	import boto3
	from botocore.config import Config
	except Exception as e:
	open(OUT, "w").write(f"_pg-history: boto3/botocore unavailable: {e}_\n")
	print(f"boto3 unavailable: {e}")
	return

[github-actions]

open(OUT, "w").write(...) leaves the file handle to be closed by refcount-based GC, so written bytes are only flushed at interpreter cleanup. This pattern is repeated at every write site (lines 158, 168, 172, 220). On CPython in this short-lived script it usually works, but it is not guaranteed on other interpreters and violates the tree's resource-management/portability expectations. Prefer with open(OUT, "w") as f: f.write(...). (low confidence, minor)

[github-actions]

This job has no timeout-minutes, unlike every job in .github/workflows/pg-ci.yml (which sets it on all jobs). The OIDC credential step and the inline Python subprocess invoking the AWS CLI can hang on network/throttling/OIDC issues, letting the job run to the default 6-hour runner limit. Add a short explicit timeout for this scheduled maintenance job.

Suggested change

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

[github-actions]

The sync job has no timeout-minutes. git fetch upstream master against the full-history PostgreSQL repository plus a rebase can hang on network problems, and with an hourly schedule stalled runs can pile up and burn runner minutes indefinitely. Other workflows in this repo (pg-ci.yml) set timeout-minutes; add an explicit timeout here for consistency and safety.

Suggested change

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

[github-actions]

The decision to rebase and force-push over master rests on brittle string matching: grep -iE "^dev (setup|v[0-9])" on commit subjects and grep -v "^\.github/" on changed paths. A squashed commit that mixes .github/ and non-.github/ changes, a merge commit, or a commit whose subject coincidentally starts with "dev v1" can be misclassified. A false positive here permits git rebase upstream/master followed by --force-with-lease, which can silently drop legitimate local commits on master. This is a data-loss footgun — the guard protecting destructive history rewriting should be more robust than subject-line pattern matching (e.g., fail closed on any non-.github/ change rather than trusting the commit subject).

[github-actions]

git config user.name/user.email is already set in the earlier "Configure Git" step, and git config persists across steps within a job on the runner. Re-setting it here is redundant and can be removed.

Suggested change

	git config user.name "github-actions[bot]"
	git config user.email "github-actions[bot]@users.noreply.github.com"
	if git rebase upstream/master; then
	if git rebase upstream/master; then

[github-actions]

This change alters the user-visible semantics of the usage_count column exposed by all three functions (pg_buffercache_pages, pg_buffercache_summary, pg_buffercache_usage_counts). BUF_STATE_GET_USAGECOUNT returned the full 0..N usage count, whereas BUF_STATE_GET_COOLSTATE returns only the 0/1 cooling state (bit 0 of the field). The documentation in doc/src/sgml/pgbuffercache.sgml still describes and shows a multi-valued "usage count" (e.g. the sample output table and the column descriptions "A possible buffer usage count"), and is not updated in this change. A user-visible SQL-interface change without matching docs is incomplete for a pgsql-hackers patch. Update pgbuffercache.sgml to reflect the new HOT/COOL cooling-state semantics (and consider whether the exposed column/variable naming should still be usage_count). (high confidence)

[github-actions]

This diff removes the bulk-read access strategy from initscan(), but the comment above this block (lines 383-390) still reads "use a bulk-read access strategy and enable synchronized scanning ... only two behaviors to tune rather than four ... some callers need to be able to disable one or both of these behaviors". After this change there is only one behavior left here (sync scan); the bulk-read strategy no longer exists. The comment now describes code that is gone and should be updated to mention only synchronized scanning, otherwise it will mislead readers into looking for strategy handling that isn't there. (moderate confidence)

[github-actions]

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

[github-actions]

🔍 OCR found 56 issue(s).

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

---

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

With clocksweep_scan_all_partitions=off, a home partition full of pinned buffers makes StrategyGetBufferPartition return NULL, after which StrategyGetBuffer immediately raises "no unpinned buffers available" even though other partitions may have free buffers -- a regression versus the old global sweep which considered all NBuffers before erroring. The old code only erred after NBuffers consecutive pinned buffers across the whole pool. Confirm this failure mode is acceptable, or scan other partitions as a last resort before erroring regardless of the GUC. (moderate confidence)

---

📄 contrib/pg_buffercache/pg_buffercache--1.7--1.8.sql (L29-L32)

pg_buffercache_set_partition is not declared STRICT, but the C implementation dereferences the array argument unconditionally (ARR_NDIM(array) in pg_buffercache_set_partition, with no PG_ARGISNULL check). Calling e.g. SELECT pg_buffercache_set_partition(0, NULL) passes a NULL ArrayType* to ARR_NDIM() and crashes the backend. All other argument-taking C functions in this extension are declared STRICT (see 1.4--1.5, 1.5--1.6, 1.6--1.7). Add STRICT here (or add explicit PG_ARGISNULL guards in the C function).

💡 Suggested change

Before:

CREATE FUNCTION pg_buffercache_set_partition(IN partition int, IN weights int[])
+RETURNS void
+AS 'MODULE_PATHNAME', 'pg_buffercache_set_partition'
+LANGUAGE C VOLATILE PARALLEL UNSAFE;

After:

CREATE FUNCTION pg_buffercache_set_partition(IN partition int, IN weights int[])
RETURNS void
AS 'MODULE_PATHNAME', 'pg_buffercache_set_partition'
LANGUAGE C VOLATILE STRICT PARALLEL UNSAFE;

---

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

Looking up the int4 array type via lookup_type_cache(INT4OID, 0)->typarray is a roundabout way to obtain a well-known static OID. Use INT4ARRAYOID directly and drop the TypeCacheEntry lookup; it is simpler and avoids including utils/typcache.h just for this.

---

📄 src/include/port/pg_numa.h (L24-L26)

Dead scaffolding (YAGNI). numa_flags, the NUMA_BUFFERS flag bit, and the three new functions pg_numa_page_size/pg_numa_move_to_node/pg_numa_bind_to_node have no readers, writers, or callers anywhere in the tree (verified via full-tree search of .c/.h and contrib/). This review group adds the primitives but nothing consumes them. A commit that only compiles but is never exercised is not independently committable and will draw a split-this-patch rejection on -hackers. Either wire the consumers in the same commit, or drop this until the buffer-manager change that uses them lands. (high confidence)

---

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

Casting a pointer to int64 for alignment math is a portability deviation. The established tree convention for pointer-to-integer modulo/mask is (uintptr_t) (see src/port/pg_crc32c_*.c, pg_popcount_x86.c). On 32-bit and under MSVC, int64 is the wrong width for a pointer. 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 (L21-L23)

src/port modules are meant to be linkable by frontend programs, but this file now pulls in storage/pg_shmem.h and references the backend-only globals huge_pages_status and GetHugePageSize() inside pg_numa_page_size. These symbols are defined in the backend only; any frontend/common link of this TU would fail to resolve them. If this helper is intended to be backend-only, it does not belong in src/port. Confirm the layering before accepting the new storage/pg_shmem.h dependency here. (moderate confidence)

---

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

huge_pages_status can be HUGE_PAGES_UNKNOWN early in startup (before shared-memory setup decides huge-page usage), and this function is explicitly documented as being used "during startup". In that window the != HUGE_PAGES_OFF test is true, so GetHugePageSize is consulted and pg_numa_page_size returns the huge-page size even when huge pages were not actually mapped -- making the startptr % page_size == 0 assertion in pg_numa_move_to_node over-strict, and skewing any alignment the callers compute. Verify huge_pages_status is reliably resolved before this runs. (moderate confidence)

---

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

pg_numa_move_to_node is declared void and silently discards the return value of numa_tonode_memory, so any placement failure is invisible to callers -- NUMA placement silently no-ops. Given there are currently no callers, decide the contract now: either propagate the result (as pg_numa_bind_to_node does) or document explicitly why failure is intentionally ignored. (moderate confidence)

---

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

This XXX tag marks a limitation/caveat but reads as a plain explanatory note. Per tree convention XXX/TODO/FIXME flag issues to revisit; if this is just describing expected usage, drop the XXX prefix. (low confidence)

---

📄 src/port/pg_numa.c (L252-L255)

The non-libnuma stubs return 0/-1 after Assert(false). In a production (non-assert) build the Assert compiles out and pg_numa_page_size returns 0; if pg_numa_move_to_node were ever reached in such a build its own % pg_numa_page_size() would divide by zero. These stubs are only safe if every call site is gated by USE_LIBNUMA so they are truly unreachable. Since no call sites exist yet, ensure that gating is in place when consumers are added. (low confidence)

---

📄 src/backend/port/sysv_shmem.c (L628-L630)

These libnuma policy calls run whenever shmem_interleave is set, but there is no numa_available() guard and pg_numa_init() is never invoked on this path. The sibling path process_debug_numa_node() in postgres.c explicitly bails out when numa_available() < 0 before touching libnuma, precisely because calling these routines when the kernel/library has no NUMA support (or get_mempolicy is blocked by a seccomp profile, as the pg_numa_init() comment warns) is unsafe. Here, a user can set the debug_shmem_interleave GUC on a USE_LIBNUMA build running on a non-NUMA kernel or restricted container and numa_set_interleave_mask() / numa_interleave_memory() will be called unconditionally. Guard with numa_available() (and/or numa_num_configured_nodes() > 1) as the postgres.c path does. Confidence: high.

---

📄 src/backend/port/sysv_shmem.c (L707-L710)

Comment references numa_set_membind(), but no such call exists in this function; the interleave policy above was set with numa_set_interleave_mask(). Both the block comment ("If set the policy to interleaving by numa_set_membind()") and the inline comment ("undo the policy set by numa_set_membind() earlier") name the wrong API and will mislead readers. Fix to reference numa_set_interleave_mask(). Confidence: high.

💡 Suggested change

Before:

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

After:

	if (shmem_interleave)
	{
		/* undo the policy set by numa_set_interleave_mask() earlier */
		numa_set_localalloc();

---

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

Double blank line after the new declarations; pgindent/project style uses a single blank line between declaration groups. Drop one blank line. Confidence: high.

💡 Suggested change

Before:

extern PGDLLIMPORT int debug_numa_node;

After:

extern PGDLLIMPORT int debug_numa_node;

---

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

The #ifdef WIN32 branch is unreachable. pg_numa_page_size lives inside #ifdef USE_LIBNUMA, and USE_LIBNUMA is only defined when libnuma (Linux-only) is available (see configure.ac AC_CHECK_LIB(numa,...) and meson.build). This file's own header comment states support is "only for Linux thanks to libnuma". So on Windows GetSystemInfo can never be reached from here -- this is dead/misleading scaffolding (YAGNI). Drop the WIN32 branch and just use sysconf(_SC_PAGESIZE). (moderate confidence)

💡 Suggested change

Before:

#ifdef WIN32
	SYSTEM_INFO sysinfo;

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

After:

	os_page_size = sysconf(_SC_PAGESIZE);

---

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

Grammar: "the expensive part happen later" -> "the expensive part happens later". (low confidence)

💡 Suggested change

Before:

	 * means the expensive part happen later, on the first access.

After:

	 * means the expensive part happens later, on the first access.

---

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

pg_numa_bind_to_node returns mbind()'s raw result; mbind returns -1 on failure with errno set, 0 on success. The comment says to "report the failure", but no caller exists yet to interpret this contract. More importantly, the two comment lines exceed the project's ~78-column width and will reflow under pgindent; wrap them. (low confidence)

💡 Suggested change

Before:

	 * MPOL_BIND places the pages strictly on the node, and MPOL_MF_MOVE migrates
	 * pages already faulted in to that node. If mbind() fails, leave the default
	 * placement in effect, and report the failure.

After:

	 * MPOL_BIND places the pages strictly on the node, and MPOL_MF_MOVE
	 * migrates pages already faulted in to that node. If mbind() fails, leave
	 * the default placement in effect, and report the failure.

---

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

pgindent will not keep this declaration alignment. With struct bitmask *nodemask as the widest declarator in the block, pgindent aligns names to that column; the hand-widened tabs after int here will be rewritten. Run pgindent before posting. (low confidence)

---

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

smoothed_alloc and smoothed_density are function-local static variables, but BgBufferSyncPartition() is now invoked once per partition inside a loop in BgBufferSync(). These two moving averages are therefore shared across every partition and get overwritten N times per bgwriter cycle, so each partition's density/alloc estimate stomps the previous one's. All the other per-cycle state was correctly moved into the per-partition BufferSyncPartition struct (saved_info[]); these two were left behind, which defeats the whole point of per-partition tracking and skews reusable_buffers_est/upcoming_alloc_est for every partition. They should be per-partition fields in BufferSyncPartition (or an equivalent per-partition array). (high confidence)

---

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

Infinite loop / CPU-bound hang reachable from SQL. ChooseClockSweep() scans sweep->balance[] (the home partition's weight row) in while (clocksweep_partition_budget == 0) until it finds a non-zero entry. If the entire row is zero the loop never terminates. ClockSweepSetWeights() validates each individual weight is 0..100 but never checks that the row has at least one non-zero weight, so a user with debug_clocksweep_balance_recalc=off can set all weights of a partition to 0 and wedge every backend whose home partition is that one, inside buffer allocation. Reject an all-zero weight vector in ClockSweepSetWeights() (and/or guard the loop to fall back to the home partition). (high confidence)

---

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

Weight-vector integrity can be destroyed by the hysteresis step. The new weights are combined as 0.5*old + 0.5*new and stored back into a uint8, truncating toward zero. A floor weight of balance[i] = 1 (set to avoid negative balances) combined with an old value of 0 yields 0.5, which truncates to 0. Over successive rebalances an entire row can decay to all zeros, which then hangs ChooseClockSweep() (see the budget loop). Round instead of truncate, and/or guarantee at least one non-zero entry survives the combine. (moderate confidence)

---

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

ClockSweepPartitionGetInfo() palloc's *weights into the current memory context on every call, but the function provides no way to know the array length and the caller must free/track it; since the length equals StrategyControl->num_partitions, this is undocumented and easy to misuse (the out-param comment doesn't state the length or ownership). Document the returned array length and ownership in the header/comment. (low confidence)

---

📄 src/backend/port/sysv_shmem.c (L709-L710)

numa_set_localalloc() does not restore the postmaster's prior NUMA memory policy; it unconditionally forces a localalloc policy. If the postmaster was started under an external policy (e.g. numactl --interleave=all), enabling debug_shmem_interleave silently clobbers that policy for the remainder of the process's lifetime, not just for this segment. Consider saving and restoring the previous policy (numa_get_* / set_mempolicy) instead of hard-coding localalloc. Confidence: moderate.

---

📄 src/backend/port/sysv_shmem.c (L628-L630)

The process-wide interleave mask set here affects every allocation made before it is reset by numa_set_localalloc() further down, including allocations inside GetHugePageSize(), SetConfigOption(), and the ereport(FATAL, ...) error path (which allocates error-message buffers). Setting a process-global policy that far ahead of the mmap it is meant for is error-prone; prefer applying the policy to the segment only (via numa_interleave_memory() / numa_tonode_memory() after the mmap) rather than a global mask spanning the huge-page fallback logic. Confidence: moderate.

---

📄 src/backend/utils/misc/guc_tables.c (L537-L541)

This comment's rationale is misleading. win32_shmem.c never references shmem_populate/shmem_interleave (only sysv_shmem.c does), so the variables are not needed "because Windows links win32_shmem.c instead." The real reason the definitions must live in an always-linked module is that the GUC entries in guc_parameters.dat are declared unconditionally (no ifdef), so the backing symbols must exist on every platform regardless of which shmem backend is linked. Suggest rewording to state that reason. Confidence: moderate.

---

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

Inconsistent with the rest of the tree and out of scope. Every other Perl script in the repository uses #!/usr/bin/perl (e.g. genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 others). Switching only pgindent to #!/usr/bin/env perl breaks that convention and changes which perl is picked up (first on $PATH vs. the system perl the project standardizes on), which is a reproducibility/portability concern for a developer tool. This hunk is also unrelated to the rest of this change set and would draw an out-of-scope-churn objection on -hackers. Revert unless there is a tree-wide decision to change all shebangs. [moderate confidence]

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

Out-of-scope behavior change. shellprog is SHELLPROG, which is set from $(SHELL) (GNUmakefile) or /bin/sh (meson) and is normally an absolute path. For an absolute path, execl and execlp behave identically, so the only effect of this switch is that if SHELLPROG is ever a bare command name it will now be resolved through $PATH instead of failing. That makes the shell used by the regression harness depend on the invoking environment's $PATH -- a subtle, environment-dependent change that nobody in the stated scope requested, and it is unrelated to the rest of this change set. If this is needed for a specific environment (e.g. Nix), justify it explicitly and reference the discussion; otherwise revert to keep the diff minimal. [moderate confidence]

---

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

Coverage gap vs. documented behavior. The GUC long_desc / this comment claim the backend is pinned "at query start", but process_debug_numa_node() is only invoked from exec_simple_query and exec_execute_message. The extended-protocol parse/bind paths (exec_parse_message, exec_bind_message at the other two pgstat_report_activity(STATE_RUNNING, ...) sites) are not covered, and neither are parallel workers, autovacuum, or other backends. For a debug knob whose entire purpose is reproducible NUMA placement, pinning only on execute is a footgun: a client that binds+executes a prepared statement will have planning and parse-time work run unpinned, and the pin leaks into subsequent commands on the same backend until the GUC changes. Consider a single lower-level hook (or at least documenting precisely which paths are affected) rather than two ad-hoc insertion points. [moderate confidence]

---

📄 src/backend/tcop/postgres.c (L1058-L1063)

Caching inconsistency in the out-of-range branch. When debug_numa_node > numa_max_node(), no affinity change is applied, yet applied_numa_node = debug_numa_node is still set at the end of the function. If the backend was previously pinned to a valid node and the GUC is then set to an out-of-range value, the WARNING fires once and the backend silently remains pinned to the old node, while applied_numa_node records the invalid value and suppresses any further reaction. The resulting state (pinned to a stale node but reporting the new value as "applied") contradicts the invalid-input WARNING. Consider resetting to numa_all_nodes_ptr in this branch, or not updating applied_numa_node so the condition is re-evaluated. [moderate confidence]

---

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

numa_run_on_node_mask() return value is ignored here, unlike the numa_run_on_node() call below which is checked and warns on failure. For consistency (and because unpinning can also fail), check the return and warn, or document why failure is deliberately ignored on this path. [low confidence]

---

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

Comment style: the house convention closes multi-line block comments with a complete sentence ending in a period. "-1 disables it (default)" is missing its trailing period. Minor, but pgsql-hackers reviewers will flag it. [low confidence]

---

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

Include ordering violates the project convention and is inconsistent with the rest of this very patch. System headers (<...>) must come before the PostgreSQL headers ("..."). Here <numa.h> is placed after all the "..." includes. The sibling file in this same change set (src/backend/port/sysv_shmem.c) correctly places the identical #ifdef USE_LIBNUMA / #include <numa.h> block up with the other system headers, before "miscadmin.h". Move this block up to match, so pgindent/convention checks and the rest of the patch stay consistent. [moderate confidence]

[github-actions]

This file (along with .gdbinit, pg-aliases.sh, flake.nix, shell.nix) is developer-local tooling and should not be committed to the PostgreSQL tree. The repository's own root .gitignore states the policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing editor/dev-environment config pollutes the tree, creates merge conflicts, and violates the minimal-diff discipline for a patch destined for pgsql-hackers/commitfest. These files belong in a personal local-excludes setup, not in the patch.

[github-actions]

These breakpoints reference an unrelated "HOT indexed updates" patch series that is not present in this tree. The symbols broken on here -- heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_read_bitmap, heap_xlog_indexed_update, etc. -- do not exist anywhere in the codebase (verified via search), so GDB will reject them. Combined with the hardcoded absolute line-number breakpoints below (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1287/1802/1836/1863/2936), which drift with any edit, this file is immediately stale and misleading. It is developer-local debug scaffolding that should not be committed.

[github-actions]

This 658-line shell-alias helper is developer-local tooling that should not be committed to the PostgreSQL tree (see root .gitignore policy on local-workflow files). Beyond the in-tree concern, many aliases depend on non-standard external tools without presence checks -- trash, compdb, rr, bpftrace, codespell, clang-tidy, iostat/vmstat -- so they silently fail or error out when a tool is absent. It also mixes build systems (pg-clean uses ninja directly while pg-build/pg-setup use meson), which is fragile. This belongs in personal dotfiles, not the repository.

[github-actions]

This flake defines a developer-local Nix environment that should not be committed to the PostgreSQL tree; the repo's root .gitignore documents that local-workflow files belong in local excludes. Additionally, environment.localBinInPath = true; is a NixOS module option, not a valid flake per-system output -- placed here inside the eachDefaultSystem result attrset it is dead config that does nothing. The gcc-musl/clang-musl devShells are also advertised for all default systems while shell.nix hardcodes an x86_64 musl target, so they will not build on aarch64/darwin.

[github-actions]

Entering the dev shell silently mutates host/repo state on every invocation: ulimit -c unlimited changes the process core-dump limit, and git config core.excludesFile .local-gitignore rewrites this repository's git config (note .local-gitignore is itself untracked and not provided, so until the user creates it the repo's excludes point at a missing file). Side-effecting the user's environment and repo config as an unavoidable part of shell entry is a footgun; these should at minimum be opt-in rather than unconditional.

[github-actions]

This block uses 2-space indentation instead of the project's tabs (width 4); it will not pass pgindent. The elog(ERROR, "sched_getcpu failed: %m") also runs inside buffer allocation (a hot, possibly critical path) -- a hard error here on an otherwise recoverable condition is a footgun. (low confidence on the error-handling nit; high confidence on the indentation)

[github-actions]

Commented-out code left in place. The saved_info_valid write is disabled here and moved to BgBufferSync after the partition loop; leaving the dead line plus an "XXX this needs to happen only after all partitions" note is patch-hygiene noise. Remove the commented-out line (the real assignment already lives in BgBufferSync). (high confidence)

[github-actions]

Typo "numa_nod" -> "numa_node" in the struct field documentation. (high confidence)

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]

Behavioral/performance change with no tests or benchmark reference in the diff. Freshly admitted pages now get BM_TAG_VALID only (no second chance), relying on a subsequent PinBuffer to grant BM_SECOND_CHANCE. For the admitting access the pin on the victim buffer is already held (via GetVictimBuffer), so it does NOT go through PinBuffer's CAS path that sets the bit here -- a page read in and used exactly once can be reclaimed on the very next hand pass. This changes replacement quality for single-use-then-reaccess patterns. For a change to the core clock-sweep this needs regression/TAP coverage and a reproducible benchmark reference (and a -hackers thread Message-Id) before it is commit-ready. (moderate confidence)

[github-actions]

saved_info is malloc'd once and sized by num_partitions from StrategySyncPrepare, then never freed and never re-checked. This is only correct because num_partitions is immutable after shared-memory init (set from BufferPartitionsParams); there is no guard asserting that. If the count ever differed on a later call the array would be undersized -> out-of-bounds in the partition loop. Add an Assert(num_partitions is stable) or allocate for MAX_BUFFER_PARTITIONS, and note why the lifetime leak (process-lifetime, bgwriter only) is acceptable rather than leaving it as an "XXX ... how else" comment. (moderate confidence)

[github-actions]

🔍 OCR found 49 issue(s).

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

---

📄 src/include/port/pg_numa.h (L21-L21)

pg_numa_move_to_node is exported here but has no caller in the tree — the only reference is an "XXX or should we use pg_numa_move_to_node?" comment in buf_init.c; the actual code calls pg_numa_bind_to_node. A new PGDLLIMPORT-exported function with no caller is dead code that prematurely freezes ABI. Remove it (and its two definitions in pg_numa.c) until it is actually used.

[high confidence]

---

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

pg_numa_page_size() references huge_pages_status and GetHugePageSize(), both of which are backend-only symbols (defined in guc_tables.c and sysv_shmem.c/win32_shmem.c, none of which are built for the frontend). But pg_numa.c is compiled with -DFRONTEND into libpgport (see src/port/Makefile), so this introduces unresolved backend symbols into a port-layer object. Note the pre-existing code is careful to guard backend-only usage (CHECK_FOR_INTERRUPTS() is behind #ifndef FRONTEND). This new function breaks that discipline and risks a frontend link failure for any libpgport consumer. Either guard the huge-page logic with #ifndef FRONTEND or move this helper to the backend.

[medium-high confidence]

---

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

Casting a pointer to int64 for the modulo is not portable: on an ILP32 target the pointer and int64 widths differ, and the signedness of the cast result is implementation-defined. Use uintptr_t for pointer-to-integer alignment arithmetic. Separately, note this alignment contract is only enforced in assert-enabled builds; in production an unaligned startptr silently proceeds (the caller in buf_init.c does TYPEALIGN_DOWN before calling, so it's currently satisfied, but the exported API offers no guard).

[high confidence on the cast]

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

XXX reads as a WIP/TODO tag and the sentence "But it also means the expensive part happen later" is ungrammatical ("part happen"). Per comment-hygiene conventions, drop the XXX and fix to e.g. "the expensive part happens later, on the first access."

[low confidence]

💡 Suggested change

Before:

	 * means the expensive part happen later, on the first access.

After:

	 * means the expensive part happens later, on the first access.

---

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

pg_buffercache_set_partition dereferences the array argument via PG_GETARG_ARRAYTYPE_P(1) with no NULL check, and the SQL definition in pg_buffercache--1.7--1.8.sql is not declared STRICT. Since non-STRICT C functions are invoked even with NULL arguments, pg_buffercache_set_partition(0, NULL) passes a NULL datum to PG_GETARG_ARRAYTYPE_P, which detoasts/dereferences garbage and crashes the backend. Either mark the SQL function STRICT or guard with PG_ARGISNULL. 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_pages.c (L1037-L1041)

The weights array returned by ClockSweepPartitionGetInfo() is sized by StrategyControl->num_partitions, but this loop/construct_array uses funcctx->max_calls (= BufferPartitionCount(), i.e. BufferPartitionsRegistry->npartitions) as its length. The two counts come from independent structs and are only equal by an implicit invariant; if they ever diverge this reads past the end of weights. Prefer having ClockSweepPartitionGetInfo return the array length explicitly and iterate on that, rather than reusing max_calls. Confidence: medium.

---

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

These two moving averages are static locals of BgBufferSyncPartition(), but this function is now invoked once per partition inside the for (partition ...) loop in BgBufferSync(). A single pair of statics is therefore shared across all partitions in every cycle: each partition's update clobbers the previous partition's value, so by the time the next cycle runs the figures reflect only whichever partition ran last. The per-partition density/alloc smoothing the rest of the function relies on (reusable_buffers_est, upcoming_alloc_est, the upcoming_alloc_est == 0 reset) is meaningless. In the original single-sweep BgBufferSync one static was correct; after partitioning these must live in BufferSyncPartition (saved_info[partition]) like prev_strategy_passes et al. (high confidence)

---

📄 src/backend/storage/buffer/bufmgr.c (L4212-L4213)

Integer-division truncation discards the signal. With up to MAX_BUFFER_PARTITIONS=32 partitions, any cycle where recent_alloc < num_partitions yields recent_alloc_partition == 0 for every partition. That both (a) suppresses the density estimate update in BgBufferSyncPartition (guarded by recent_alloc_partition > 0) and (b) makes every partition report it can hibernate (recent_alloc_partition == 0), so the bgwriter stops cleaning and goes to low-power mode despite real allocation traffic. On a many-node system with a steady but modest allocation rate this permanently under-cleans. Consider distributing the remainder (or tracking per-partition alloc counts directly rather than averaging a global sum). (high confidence)

---

📄 src/backend/port/sysv_shmem.c (L628-L629)

Missing numa_available() guard before using libnuma. The libnuma manual requires numa_available() to be called (and return >= 0) before any other libnuma routine; otherwise numa_all_nodes_ptr may be uninitialized/unreliable and the behavior is undefined. The rest of this tree follows this contract (see process_debug_numa_node() in src/backend/tcop/postgres.c, which checks numa_available() < 0 before touching numa_all_nodes_ptr). Here numa_set_interleave_mask() and later numa_interleave_memory()/numa_set_localalloc() run with no such check. Guard the whole shmem_interleave path on numa_available() >= 0.

---

📄 src/backend/port/sysv_shmem.c (L695-L696)

Comment references the wrong function. The interleave policy was set above with numa_set_interleave_mask(), not numa_set_membind(). Both the block comment and the inline comment on line 709 cite numa_set_membind(), which is never called here. The opening sentence is also grammatically broken ("If set the policy to interleaving by ..."). Fix the function name and sentence so the comment matches the code.

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

	 * The interleave policy was set by numa_set_interleave_mask() above; undo
	 * it now by setting the policy to localalloc. With MAP_POPULATE, all the
	 * pages were

---

📄 src/backend/port/sysv_shmem.c (L709-L709)

Same wrong function name here: the earlier policy was set by numa_set_interleave_mask(), not numa_set_membind().

💡 Suggested change

Before:

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

After:

		/* undo the policy set by numa_set_interleave_mask() earlier */

---

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

pg_numa_bind_to_node is missing a header comment, unlike every other function in this file (pg_numa_init, pg_numa_query_pages, pg_numa_move_to_node all have one). It is a newly PGDLLIMPORT-exported function whose contract (returns 0 on success / -1 with errno set on failure; startptr/endptr expected page-aligned for mbind to succeed; MPOL_MF_MOVE only migrates pages mapped by the calling process, hence startup-only usage) is non-obvious and should be documented for consistency with the subsystem. [moderate confidence]

---

📄 src/include/storage/buf_internals.h (L387-L391)

Grammar errors in the reworded comment: "As this is primarily matters" (drop "is"), "the space wastage is be a bit more noticeable" ("is be"), and "requires the memory page be a multiple of buffer descriptor" (missing "size"). Suggest tightening to a complete sentence. (low confidence)

💡 Suggested change

Before:

 * XXX: As this is primarily matters in highly concurrent workloads which
 * probably all are 64bit these days. We force the stride to be cache line
 * sized even on 32bit systems, where the space wastage is be a bit more
 * noticeable, to allow partitioning of shared buffers (which requires the
 * memory page be a multiple of buffer descriptor).

After:

 * XXX: As this primarily matters in highly concurrent workloads, which
 * probably all are 64bit these days, we force the stride to be cache line
 * sized even on 32bit systems, where the space wastage is a bit more
 * noticeable, to allow partitioning of shared buffers (which requires the
 * memory page size to be a multiple of the buffer descriptor size).

---

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

avg_allocs - allocs[j] is unsigned (uint32) arithmetic evaluated here, before the if (allocs[j] > avg_allocs) continue; guard below. When allocs[j] > avg_allocs this underflows to a near-UINT32_MAX value, and the double product is then converted back into the uint32 receive_allocs (out-of-range float-to-unsigned conversion is undefined). The result is discarded by the guard today, but computing it unconditionally is a latent footgun. Move this assignment after the two continue guards. (low confidence)

---

📄 src/backend/port/sysv_shmem.c (L628-L629)

Process-wide interleave policy leaks on the error path. numa_set_interleave_mask() changes the task's default allocation policy for the whole process. If both mmap() attempts fail, control jumps to the ereport(FATAL) at line 681-690 and the undo (numa_set_localalloc() at line 710) is never reached, so every allocation in between (SetConfigOption, the ereport machinery itself) runs under the interleave policy. In the postmaster FATAL path this is benign because the process exits, but the asymmetry is a footgun: the policy change and its undo are not paired on all exits. Consider setting the mask only after a successful mmap, or using PG_TRY/PG_FINALLY-style pairing, so the global policy is always restored. (low confidence on practical impact, high confidence the pairing is asymmetric)

---

📄 src/backend/port/sysv_shmem.c (L707-L710)

This second libnuma block has the same missing numa_available() guard as the earlier numa_set_interleave_mask() block: numa_set_localalloc() and numa_interleave_memory() are libnuma routines that require numa_available() to have been called and returned >= 0 first; numa_all_nodes_ptr may otherwise be uninitialized. The rest of the tree (e.g. process_debug_numa_node() in postgres.c) guards every libnuma call this way. The guard added for the first block must also cover this block.

---

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

This execl -> execlp change is unrelated to the NUMA buffer-management work this patch set is about, and should be split out (minimal-diff discipline on -hackers).

More importantly, it is a behavioral change with a subtle footgun. shellprog is set from SHELLPROG, which is $(SHELL) (/bin/sh) from the Makefiles and /bin/sh in meson -- always an absolute path. For an absolute path execlp is identical to execl, so the change buys nothing in the normal build. The only case it affects is a SHELLPROG with no slash, where execlp would resolve the shell via $PATH -- i.e. it makes which shell runs the regression commands depend on the caller's PATH. That is a surprising, non-deterministic/security-relevant change for a test harness and should not be made without justification. Recommend reverting to execl.

Confidence: high.

💡 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 shebang change is unrelated to the patch's purpose and breaks tree-wide consistency. Every other Perl script in the tree (~35 of them, e.g. src/backend/catalog/genbki.pl, src/tools/mark_pgdllimport.pl, src/tools/copyright.pl) uses #!/usr/bin/perl; this edit makes pgindent the lone outlier. Unrelated dev-environment tweaks like this are a common rejection reason on -hackers -- drop this hunk and keep the established #!/usr/bin/perl convention.

Confidence: high.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

These three externs back GUCs (shmem_populate/shmem_interleave are defined in guc_tables.c, debug_numa_node in postgres.c). They do not belong in miscadmin.h's database-identity block alongside MyDatabaseId etc. Follow the existing convention and declare each next to its subsystem (debug_numa_node with the other tcop/GUC externs; the shmem flags near shared-memory declarations). Also drop the extra blank line - two consecutive blank lines here is pgindent/whitespace churn.

💡 Suggested change

Before:

+extern PGDLLIMPORT bool shmem_populate;
+extern PGDLLIMPORT bool shmem_interleave;
+extern PGDLLIMPORT int debug_numa_node;
+
+

After:

+extern PGDLLIMPORT bool shmem_populate;
+extern PGDLLIMPORT bool shmem_interleave;
+extern PGDLLIMPORT int debug_numa_node;

---

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

POLA/footgun: the debug_numa_node GUC is declared and settable unconditionally (boot_val -1, max INT_MAX in guc_parameters.dat), but process_debug_numa_node() is fully wrapped in #ifdef USE_LIBNUMA. On a build without libnuma, setting this GUC - to any value, including an out-of-range one - silently does nothing, with no WARNING. At minimum the no-op-without-NUMA behavior should be documented in the GUC's long_desc, or the setter should emit a one-time notice when libnuma is absent.

---

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

process_debug_numa_node() is invoked on every exec_simple_query and exec_execute_message - the query hot path. The static applied_numa_node short-circuit avoids repeated syscalls once the GUC stabilizes, but when debug_numa_node != applied_numa_node (e.g. the common default-build case where libnuma is absent: the first call hits numa_available() < 0 then caches -1 == -1 thereafter, OK) it is fine; however with libnuma present and the GUC left at default -1, the very first query still pays numa_available(). More importantly, confirm this per-query placement is intended vs. a hook on GUC assign/StartTransaction - re-evaluating CPU affinity mid-session on every statement is a surprising place for a process-lifetime affinity change.

---

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

By PostgreSQL convention, GUC parameter names are quoted in user-facing messages (e.g. errmsg("parameter \"%s\" ...") throughout guc.c). Quote the parameter name here and in the "could not pin backend" message for consistency, and consider using errcode(ERRCODE_INVALID_PARAMETER_VALUE) rather than a bare WARNING. (low confidence)

💡 Suggested change

Before:

		ereport(WARNING,
				(errmsg("debug_numa_node %d exceeds the highest available NUMA node %d, ignoring",
						debug_numa_node, numa_max_node())));

After:

		ereport(WARNING,
				(errmsg("parameter \"debug_numa_node\" value %d exceeds the highest available NUMA node %d, ignoring",
						debug_numa_node, numa_max_node())));

---

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

The un-pinning path ignores the return value of numa_run_on_node_mask(), while the pinning path below checks numa_run_on_node() and warns on failure. For symmetry and to avoid silently failing to lift a prior pin, check the return value here too. (low confidence)

---

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

Double blank line introduced after the new externs; the surrounding block uses single-blank-line separation. Remove the extra blank line (git diff --check / pgindent will flag the churn). (low confidence)

💡 Suggested change

Before:

extern PGDLLIMPORT int debug_numa_node;
+
+

After:

extern PGDLLIMPORT int debug_numa_node;
+

[github-actions]

This file does not belong in a patch destined for pgsql-hackers/commitfest. It is a local, personal debugging aid and violates the minimal-diff / one-logical-change discipline. Beyond that, it is factually stale and out of scope for this buffer/NUMA review group:

• Its header and breakpoints target the "HOT indexed updates" feature, which is unrelated to the changes in this group.
• The symbols it breaks on (e.g. heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_xlog_indexed_update) do not exist anywhere in the committed C source (verified by search) -- these breakpoints are dead on arrival.
• The hardcoded absolute source line numbers (heapam.c:3210, heapam.c:4019, heapam.c:4024, heapam.c:4033, heapam.c:4101, heapam.c:4147, and later pruneheap.c:1802, indexam.c:299, heapam_indexscan.c:182/250/297) are extremely brittle: any edit to those files silently shifts every breakpoint to the wrong line, producing misleading debugging. heapam_indexscan.c exists but its contents do not match these line references.
• The comments also use non-ASCII em-dash characters (--), violating the ASCII-only rule for source/diffs.

Recommend dropping this file from the patch entirely.

[github-actions]

Editor-local clangd configuration should not be committed to the PostgreSQL tree. It is not part of the logical change, pollutes the repo for other contributors, and the hardcoded relative include path -I../../../../src/include plus -DPGDLLIMPORT= are specific to one person's directory layout and will confuse other contributors' tooling. Recommend keeping this in a personal (gitignored) location rather than in the patch.

[github-actions]

This large personal shell helper library does not belong in a patch for pgsql-hackers/commitfest. It is local developer tooling, unrelated to the stated code change, and bundling it violates the minimal-diff / one-logical-change discipline (a reliable rejection trigger on -hackers). Recommend removing it from the patch and keeping it in a personal dotfiles location.

[github-actions]

This Nix dev-environment flake is personal local tooling and should not be part of a PostgreSQL patch headed for commitfest. It is unrelated to the actual change in this series and bundling it violates the minimal-diff discipline. (Note: the referenced glibc-no-fortify-warning.patch and flake.lock do exist in the tree and the inputs are pinned by rev/narHash, so evaluation/reproducibility is not itself the problem -- the problem is that these files are out of scope.) Recommend removing from the patch.

[github-actions]

Like flake.nix, this is a personal Nix dev-shell definition and does not belong in a PostgreSQL patch for commitfest. It is unrelated local tooling and should be removed from the change set. The referenced ./glibc-no-fortify-warning.patch is present in the tree, so this will evaluate, but the file itself is out of scope for the feature patch.

[github-actions]

Typo: "numa_nod" should be "numa_node". (low confidence)

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]

Typo: "thah" should be "than". (low confidence)

Suggested change

	* Picks the smallest number of partitions higher thah MIN_BUFFER_PARTITIONS,
	* Picks the smallest number of partitions higher than MIN_BUFFER_PARTITIONS,

[github-actions]

The adjacent FIXME claims numBuffers may be set too high for the last partition when NBuffers isn't a multiple of npartitions. That is stale/incorrect: BufferPartitionGet() returns part->num_buffers, which already accounts for the remainder per-partition (first_buffer..last_buffer inclusive). ClockSweepTick wraps on this per-partition numBuffers, so it stays within range. Remove or correct the misleading FIXME. Note this correctness depends on the buffers_remaining bug in buf_init.c being fixed. (medium confidence)

[github-actions]

Commented-out code (/* saved_info_valid = true; */) plus an unresolved XXX. Commented-out code is disallowed by project convention; the actual set now happens in BgBufferSync after the loop, so this dead line and the XXX should be removed. (medium confidence)

[github-actions]

numa_flags and NUMA_BUFFERS are dead scaffolding. A tree-wide search shows numa_flags is only defined (pg_numa.c) and declared here, and NUMA_BUFFERS is only defined here — neither is ever written or tested anywhere. The buffer-manager NUMA gating in the companion files uses a separate shared_buffers_numa bool (buf_init.c), not this flag. This is a YAGNI/minimalism violation, and worse, exporting numa_flags as PGDLLIMPORT freezes ABI for an unused global. Drop both until there is a consumer.

[high confidence]

[github-actions]

🔍 OCR found 44 issue(s).

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

---

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

Grammar error: "the space wastage is be a bit more noticeable" -- drop "is" (or reword). Also the surrounding comment reads as an unfinished edit of the original. [high confidence]

💡 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/backend/storage/buffer/buf_init.c (L386-L386)

Typo: "higher thah" -> "higher than". [high confidence]

💡 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/backend/storage/buffer/buf_init.c (L332-L333)

Per-node log spam risk: these WARNINGs fire once per NUMA node during shared-memory init whenever partition boundaries are not page-aligned, which is the common case when NBuffers is not a clean multiple of (page_size * npartitions). On a many-node box this produces one warning per node on every startup. Consider demoting to DEBUG or emitting a single summary, since the code then best-effort aligns the pointers and proceeds anyway. [moderate confidence]

---

📄 src/backend/port/sysv_shmem.c (L695-L697)

Comment is inconsistent with the code. The interleave policy earlier in this function is set via numa_set_interleave_mask() (not numa_set_membind()), so both these comments mis-name the API. Fix the references to describe the actual call that is being undone. [high confidence]

💡 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
	 * faulted and are now interleaved on the available nodes.

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 faulted and are now interleaved on the available
	 * nodes.

---

📄 src/backend/port/sysv_shmem.c (L628-L629)

The process-wide interleave policy set by numa_set_interleave_mask() before the first mmap() is only reset (numa_set_localalloc) after a successful allocation. On the HUGE_PAGES_ON failure path this function ereport(FATAL)s at the "could not map anonymous shared memory" check before the reset. While FATAL tears the process down so the leaked policy is harmless in practice, the asymmetry is fragile: any future early return between setting the mask and the reset would silently leave the interleave policy applied to all subsequent allocations in this process. Consider resetting the policy right after the mmap() calls regardless of success. [moderate confidence]

---

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

When built without libnuma, process_debug_numa_node() compiles to an empty body, yet debug_numa_node is still a fully settable PGC_USERSET GUC (registered in guc_parameters.dat with no ifdef). A user can set it, get no error, and silently get no pinning — a footgun with no feedback. Compare debug_raw_expression_coverage_test, which gates its GUC entry with ifdef => 'DEBUG_NODE_TESTS_ENABLED'. Either gate the GUC entry on USE_LIBNUMA or emit a one-time WARNING when the GUC is set but libnuma is unavailable. (high confidence)

---

📄 src/backend/tcop/postgres.c (L1046-L1051)

These call numa_available() and numa_max_node() directly instead of the existing portability wrappers pg_numa_init() and pg_numa_get_max_node() in src/port/pg_numa.c (included via port/pg_numa.h). This duplicates NUMA-API usage and bypasses the wrapper's handling (e.g. pg_numa_init() works around the pre-2.0.19 EPERM bug). numa_run_on_node[_mask]() have no wrapper so some direct use is unavoidable, but mixing direct <numa.h> calls with the abstraction is inconsistent; prefer routing through pg_numa.h, adding a wrapper for the run-on-node calls if needed. (moderate confidence)

---

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

errmsg conventions: the trailing fragment ", ignoring" is not a complete clause and reads awkwardly for a user-facing WARNING; and none of these three messages are wrapped in errmsg(...)'s translatable form consistently with the rest (errmsg already marks for translation, but the fragment style is the issue). Prefer phrasing like errmsg("NUMA node %d does not exist, maximum is %d", ...) plus an errhint, without a trailing ", ignoring". (moderate confidence)

---

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

This user-visible GUC has no documentation (doc/src/sgml/config.sgml) and no regression/TAP test coverage anywhere in the change set (confirmed by search). A user-visible GUC without docs and tests is WIP, not commit-ready per pgsql-hackers standards. (high confidence)

---

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

Comment is missing its terminating period: -1 disables it (default) should end with a period. (low confidence)

---

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

Two consecutive blank lines after the new externs; keep a single blank line to avoid needless whitespace churn and keep the file pgindent-clean. (high confidence)

💡 Suggested change

Before:

extern PGDLLIMPORT int debug_numa_node;

After:

extern PGDLLIMPORT int debug_numa_node;

---

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

Unnecessary indirection: lookup_type_cache(INT4OID, 0) is used solely to fetch typentry->typarray for the weights column. The array type OID for int4 is the well-known constant INT4ARRAYOID (pg_type_d.h), already used throughout the backend. Drop the type-cache lookup and pass INT4ARRAYOID directly to TupleDescInitEntry; it avoids a cache lookup and makes the intent explicit. (low confidence this is a defect; it works, but is non-idiomatic.)

---

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

Unnecessary behavioral change. shellprog is SHELLPROG, which is always an absolute path ($(SHELL), e.g. /bin/sh, or /bin/sh under meson). With an absolute path, execl() and execlp() behave identically, so this edit is not required for any functional purpose. Worse, execlp() adds a PATH-based lookup fallback: if SHELLPROG ever became a bare name, the test harness would resolve the shell against $PATH, making test execution non-deterministic and a potential security footgun. This hunk is unrelated to the rest of the change and should be dropped to keep the diff minimal. (high confidence)

💡 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, convention-breaking change. Every other Perl script in the tree uses #!/usr/bin/perl (e.g. src/tools/copyright.pl, src/tools/git_changelog, src/tools/mark_pgdllimport.pl). Switching only pgindent to #!/usr/bin/env perl makes it the sole outlier and diverges from the project's established shebang convention. It also has no bearing on the stated purpose of this change set, so it is scope creep in the diff. Revert to #!/usr/bin/perl for consistency. (high confidence)

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

Inconsistent error handling: the two numa_run_on_node() calls below check their return value and emit a WARNING on failure, but this numa_run_on_node_mask() call ignores its return value. numa_run_on_node_mask() also returns -1 on error; if restoring the full node mask fails, the backend silently remains pinned to the previously-set node with no feedback. Check and warn here too for symmetry. (moderate confidence)

💡 Suggested change

Before:

		/* Pinning disabled: allow running on all nodes again. */
		numa_run_on_node_mask(numa_all_nodes_ptr);

After:

		/* Pinning disabled: allow running on all nodes again. */
		if (numa_run_on_node_mask(numa_all_nodes_ptr) != 0)
			ereport(WARNING,
					(errmsg("could not unpin backend from NUMA node: %m")));

---

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

Integer division truncates to zero: with the default bgwriter_lru_maxpages=100 and enough partitions (e.g. a many-node box producing >100 partitions, or when an operator lowers the GUC), bgwriter_lru_maxpages / num_partitions is 0. The loop then breaks as soon as a single buffer is written (++num_written >= 0), effectively capping the background writer to one clean buffer per partition per cycle regardless of the configured limit. Round up or clamp to a minimum of 1 so the per-partition cap is never zero. [high confidence]

---

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

smoothed_alloc and smoothed_density are function-local statics, but BgBufferSyncPartition() is now called once per partition inside the BgBufferSync() loop. All per-partition clock-hand state was moved into the per-partition saved struct, yet these two moving averages were left shared: within a single BgBufferSync() call, partition N's update overwrites partition N-1's, so each partition's density/alloc estimate is corrupted by the others. Either move these into BufferSyncPartition (one per partition) or document and justify a single global estimate; the current state is inconsistent with the rest of the per-partition refactor. [high confidence]

---

📄 src/backend/storage/buffer/bufmgr.c (L4212-L4213)

recent_alloc_partition truncates to 0 whenever recent_alloc < num_partitions. Since this same averaged value feeds both the per-partition density estimation (scans_per_alloc, smoothed_alloc) and the hibernate decision (recent_alloc_partition == 0), low-but-nonzero allocation activity spread across partitions is invisible to the bgwriter: it may hibernate and stop pre-cleaning even though real allocations are occurring. Consider distributing the remainder or using the per-partition requested-alloc counters already tracked in freelist.c instead of a flat average. [moderate confidence]

---

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

numSweepClaims is incremented before the CAS that actually claims the buffer. If the compare_exchange fails (another backend changed the state), the inner loop retries with the refreshed old_buf_state, but the counter was already bumped - so numSweepClaims over-counts under contention. Since this counter drives the bgwriter's spend_chance heuristic (StrategySweepClaims), the inflated rate can make the sweep think it is starving when it is not. Move the increment after a successful CAS. [moderate confidence]

[github-actions]

This entire file is developer-local debugging scaffolding that must not be committed to a PostgreSQL tree. The root .gitignore explicitly directs such files to $GIT_DIR/info/exclude or ~/.gitexclude: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally". Including .gdbinit (along with .clangd, pg-aliases.sh, flake.nix, shell.nix) in a PR violates the minimal-diff discipline and will be rejected on -hackers. Remove it from the patch.

Independently, the breakpoints are broken: I verified that heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_xlog_indexed_update, and the other heap_hot_indexed_* functions referenced here do not exist anywhere in this codebase, and break heapam.c:3210 lands on the closing } of heap_update, not its entry. The file debugs code that was never committed, so its claim to aid "code review" is misleading.

[github-actions]

Absolute line-number breakpoints (e.g. heapam.c:4019, heapam_indexscan.c:182, indexam.c:299, execIndexing.c:370, pruneheap.c:1802) are extremely fragile and silently drift the instant any surrounding code changes, so they cannot be trusted for review. I confirmed heapam.c:3210 already points at the wrong location. Function-name breakpoints (as used elsewhere in this file) are the only robust form.

[github-actions]

Developer-local editor/LSP configuration that should not be committed to the PostgreSQL tree. Per the root .gitignore, local-workflow files belong in $GIT_DIR/info/exclude or ~/.gitexclude, not in the patch. Including it violates minimal-diff discipline and will draw rejection on -hackers.

[github-actions]

This 658-line personal shell helper library is developer-local tooling that must not be part of a PostgreSQL patch. It belongs in a local, untracked location (per the root .gitignore guidance), not the committed tree.

[github-actions]

Data-loss footgun: these destructive helpers run rm -rf/trash on paths that are purely env-var expansions with no non-empty guard. If $PG_DATA_DIR, $PG_BUILD_DIR, or $PG_INSTALL_DIR is unset or empty (e.g. the script is sourced outside the Nix dev-shell that exports them), the expansions collapse and the command can target the current directory or /. Add [ -n "$PG_DATA_DIR" ] || return 1 style guards (and ideally set -u) before any destruction.

[github-actions]

This block is indented with two spaces instead of tabs; it will not survive pgindent and breaks the tree's tab/width-4 convention. [high confidence]

[github-actions]

Dead exported API. pg_numa_move_to_node is defined (both the libnuma and the no-op branch) and PGDLLIMPORT-exported in pg_numa.h, but it has no callers anywhere -- it is only mentioned in XXX or should we use pg_numa_move_to_node? comments in buf_init.c. Per YAGNI, drop the unused function (and its header declaration) until a caller exists, or wire it in. [high confidence]

[github-actions]

Casting a pointer to int64 for the modulus is a portability smell (pointer-to-integer width is not int64 on all platforms). Use uintptr_t for pointer arithmetic/alignment checks. [moderate confidence]

Suggested change

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

[github-actions]

BufferGetNode is declared and PGDLLIMPORT-exported here ("in buf_init.c") but no definition exists anywhere in the tree. If any caller references it (e.g. pg_buffercache additions) the link fails; if nothing calls it, it is a dead exported declaration. Either add the definition in buf_init.c or remove this extern. [high confidence]

[github-actions]

Typo: "numa_nod" should be "numa_node" in this declaration-site comment. [high confidence]

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)