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]

pg-history: Bedrock call failed: HTTP Error 502: Bad Gateway

[github-actions]

🔍 OCR found 12 issue(s).

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

[github-actions]

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

Suggested change

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

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

Confidence: moderate.

Suggested change

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

Suggested change

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

[github-actions]

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

[github-actions]

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

Suggested change

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

[github-actions]

🔍 OCR found 43 issue(s).

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

---

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

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

---

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

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

---

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

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

---

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

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

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

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

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

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

---

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

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

---

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

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

---

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

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

---

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

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

---

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

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

---

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

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

💡 Suggested change

Before:

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

After:

	return true;

---

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

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

---

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

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

---

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

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

💡 Suggested change

Before:

/* in freelist.c */


/* inline functions */

After:

/* inline functions */

---

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

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

---

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

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

---

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

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

---

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

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

💡 Suggested change

Before:

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

After:

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

---

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

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

[github-actions]

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

Suggested change

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

[github-actions]

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

Suggested change

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

[github-actions]

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

Suggested change

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

[github-actions]

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

[github-actions]

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

Suggested change

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

[github-actions]

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

Suggested change

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

[github-actions]

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

[github-actions]

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

Suggested change

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

[github-actions]

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

[github-actions]

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

[github-actions]

This entire file is a personal developer-tooling artifact (a GDB breakpoint script) and must not be committed to the PostgreSQL tree. It is added alongside two other local scratch files in this same change (.clangd, pg-aliases.sh), so the patch does more than one thing and pollutes the repository root -- an instant rejection on pgsql-hackers under the minimal-diff rule. Confirmed: .gdbinit is not listed in any .gitignore, so it was actively tracked rather than ignored. Worse, none of the code this file targets exists in the tree: a search for the referenced helpers (heap_hot_indexed_*) and the HEAP_INDEXED_UPDATED / XLOG_HEAP2_INDEXED_UPDATE symbols finds no matches anywhere except inside this file, and none of the files it references (heapam.c, heapam_indexscan.c, indexam.c, execIndexing.c, pruneheap.c, heapam_xlog.c) are part of this change. The file should be dropped from the commit entirely; if it must exist locally, put it in a personal global gitignore, not the repo. (high confidence)

[github-actions]

Line-number breakpoints (e.g. heapam.c:3210, heapam.c:4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1287/1802/1836/1863/2936) are inherently fragile: any edit to those files silently shifts every line, so the comments asserting specific semantics ("Line 4019: pure HOT", "Line 4024: HOT indexed path") are unverifiable and become stale immediately. Since the target source files are not in this change and the referenced functions do not exist in the tree, these breakpoints cannot even be confirmed to be meaningful today. This reinforces that the file does not belong in version control. (high confidence)

[github-actions]

Non-ASCII em-dash (U+2014) characters appear throughout the section headers (e.g. "HOT Indexed Updates — GDB breakpoints", "UPDATE DECISION — heap_update()"). PostgreSQL sources and diffs are required to be ASCII-only; use a plain hyphen/dash instead. Minor relative to the file not belonging in the tree at all, but noted for completeness. (high confidence)

Suggested change

	# 1. UPDATE DECISION — heap_update() HOT/HOT-indexed/non-HOT choice
	+# 1. UPDATE DECISION -- heap_update() HOT/HOT-indexed/non-HOT choice

[github-actions]

Scope / patch-hygiene: this entire file is personal developer tooling and is unrelated to the actual change in this set (the cooling-stage buffer-manager rewrite in src/backend/storage/buffer/*). Bundling personal helper scripts, .clangd, and .gdbinit into a PostgreSQL patch destined for pgsql-hackers/commitfest is a top rejection reason ("minimal diff" / "does more than one thing"). These files carry machine-specific paths and non-portable assumptions and belong in the developer's own dotfiles, not the tree. Recommend dropping this file (and .clangd/.gdbinit) from the patch, or add them to a local (untracked) location / .gitignore.

[github-actions]

Destructive-command footgun: PG_BUILD_DIR is never validated as non-empty and this file never sets it or uses set -u. If it is unset/empty, rm -rf "$build_dir" expands to rm -rf "", which resolves to the current directory contents in many shells -- silent data loss. Guard with a non-empty check (e.g. [ -n "$build_dir" ] || return 1) before any rm -rf/trash.

[github-actions]

Same destructive footgun: PG_BUILD_DIR/PG_INSTALL_DIR are never defined or validated. If either is empty, this expands to rm -rf "" (and trash with an empty arg), risking deletion of unintended paths. Validate the variables are non-empty before running the destructive command.

[github-actions]

Same issue for PG_DATA_DIR: unset/empty expands to rm -rf "", a data-loss hazard. Validate PG_DATA_DIR is non-empty before deleting.

[github-actions]

$CC is unquoted inside the command substitution: basename $CC breaks (word-splitting) if the compiler path contains spaces, and mis-parses if it contains multiple words. Quote it: local current_compiler="$(basename "$CC")".

[github-actions]

Unquoted iteration over a whitespace-delimited variable: for file in $modified_files word-splits and glob-expands filenames, so paths containing spaces or glob metacharacters are mis-handled or silently skipped. Prefer a NUL-safe read loop (e.g. git diff -z --name-only ... | while IFS= read -r -d '' file). The same pattern recurs in pg-tidy (for f in $files) and pg-spell.

[github-actions]

Privileged system-wide side effect in a repo-tracked helper: this uses sudo tee to mutate the global sysctl kernel.core_pattern, affecting the whole machine (all users/processes), not just this dev session. A tracked helper silently escalating privileges to change a system-wide setting is a footgun. This (along with the Linux-only /proc/sys/... writes and iostat/vmstat/perf/rr/trash/compdb/codespell dependencies elsewhere) reinforces that the file is Linux-specific developer tooling that doesn't belong in the cross-platform tree.

[github-actions]

completePasses is double-counted across concurrent batches, over-reporting the number of clock-sweep passes to StrategySyncStart()/BgBufferSync.

This else if branch increments completePasses but, unlike the wrap branch above, does NOT wrap nextVictimBuffer back below NBuffers -- the counter stays at start + batch_size (>= NBuffers). A later backend whose fetch-add returns that value >= NBuffers then enters the first branch, re-reads current (still >= NBuffers), wraps it and increments completePasses again for the very same wrap crossing.

Concrete trace (NBuffers=100, batch=16): counter=90. Backend A: start=90, counter->106; 90<100 but 90+16=106>100 -> this branch, completePasses++ (counter left at 106). Backend B: start=106, counter->122; 106>=100 -> first branch, current=122 wraps to 22, completePasses++ again. Two increments, one real wrap. StrategySyncStart then reports 2 complete passes when only 1 occurred, skewing bgwriter LRU pacing.

The stock invariant was that a completed pass corresponds to the counter being wrapped down exactly once. This branch breaks that by incrementing the pass count without advancing/wrapping the shared counter, so the same crossing is accounted twice.

[github-actions]

When start >= NBuffers, the local batch window is computed from the wrapped start (start % NBuffers) while the shared counter was advanced by the unwrapped amount. If (start % NBuffers) + batch_size > NBuffers, the local iteration victim = MyBatchPos % NBuffers spans the wrap point a second time, but that crossing is counted by neither this wrap branch nor the else if branch below -- an undercount that partially and unpredictably cancels the double-count. The net effect is that completePasses drift is nondeterministic under concurrency. Consider deriving MyBatchEnd from the unwrapped absolute start (or explicitly accounting for a local-window wrap) so the pass count stays consistent.

[github-actions]

Dead code: the entire cool_if_hot mechanism added to SyncOneBuffer is unreachable. Both call sites pass false (checkpointer at the BufferSync call and the bgwriter LRU scan below), and the bgwriter comment explicitly states cooling is left entirely to the foreground sweep ("no background pre-cooling"). The new parameter, the pre-cool block (ref-bit second-chance / HOT->COOL demotion), and the BUF_COOLED flag are never exercised: BUF_COOLED is only ever set here, never read by any caller. Per the minimalism/YAGNI discipline, unused parameters, unreachable branches, and unconsumed flags should not be added; this scaffolding also cannot be covered by tests. Drop cool_if_hot, BUF_COOLED, and the pre-cool block, or wire up a caller that actually uses them in this same patch.

[github-actions]

BUF_COOLED is defined but never consumed anywhere (only set in SyncOneBuffer, never tested by a caller). Dead flag; remove it together with the unused cooling path.

[github-actions]

write_limit silently raises the effective per-round cap above bgwriter_lru_maxpages, which is documented as a hard limit. This is a user-visible behavior change with no doc update (config.sgml) and no test. It also changes the meaning of the maxwritten_clean counter, which will now only increment at the higher limit. The comment claim "normal workloads are unaffected because there upcoming_alloc_est <= bgwriter_lru_maxpages" is inaccurate: upcoming_alloc_est = smoothed_alloc * bgwriter_lru_multiplier (default 2.0), so it can exceed the default maxpages (100) on ordinary moderately-busy workloads, not only under bulk-dirtying. If overriding the GUC is intended, it needs documentation, a benchmark justifying the change, and a corrected comment.

[github-actions]

Second hardcoded smoothing constant. smoothing_samples (16) is still used for the strategy-density and smoothed_alloc updates, while this new cleaner_smoothing_samples (4) is used only for the cleaner-density update. Two coexisting magic constants make the smoothing behavior harder to reason about, and the value is asserted purely by a long narrative comment with no reproducible benchmark. Consider whether a distinct value is actually warranted versus reusing smoothing_samples, and note that neither is tunable.

[github-actions]

Non-standard block-comment format: the comment text starts on the opening /* line. PostgreSQL style puts the text on subsequent lines (/* alone, then * text). pgindent will not fix this. Same issue in the analogous comment added in ExtendBufferedRelShared. Reformat to the standard block-comment style.

[github-actions]

Aspirational/future-tense comment: "The whole ring mechanism is removed in a later patch" describes behavior of an unshipped change. Comments must describe what the code does now. This also signals a non-atomic patch series (this commit leaves GetBufferFromRing in a transitional state whose rationale only makes sense given a follow-up commit), which conflicts with the bisectable-commit requirement. Reword to explain the current invariant (a ring element is reusable whenever unpinned under the cooling-state model) without referencing future patches.

[github-actions]

Demoting HOT->COOL by subtraction (buf_state -= BUF_COOLSTATE_ONE) is correct only because local buffers never set BUF_REFBIT (bit 1 of the same field). It silently depends on that cross-module invariant: the coolstate field is only ever 0 or 1 here, so the subtract can't underflow or touch the ref bit. The equivalent shared-buffer sweep in freelist.c deliberately uses a masked clear (local_buf_state &= ~BUF_USAGECOUNT_MASK) instead, which is robust regardless of the ref bit. If the local path ever starts setting BUF_REFBIT (e.g. to unify with the shared code), this subtraction becomes a silent corruption footgun. Prefer the masked-clear form here too for robustness and consistency: buf_state &= ~BUF_USAGECOUNT_MASK;.

Suggested change

	/* HOT: give it a second chance, cool it and keep scanning. */
	buf_state -= BUF_COOLSTATE_ONE;
	/* HOT: give it a second chance, cool it and keep scanning. */
	buf_state &= ~BUF_USAGECOUNT_MASK;

[github-actions]

This change is unrelated to the patch (whose changed files are all buffer-management related) and appears to be a no-op. shellprog comes from SHELLPROG, which is an absolute path in both build systems (GNUmakefile: -DSHELLPROG="$(SHELL)", typically /bin/sh; meson.build: -DSHELLPROG="/bin/sh"). Per POSIX, execlp only performs a PATH search when the program name contains no slash; for a path containing a slash it behaves identically to execl. So this switch changes nothing for any shipped configuration while introducing PATH-search semantics (a subtle footgun for security/portability). Drop this hunk to keep the patch minimal and single-purpose. (high confidence)

Suggested change

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

[github-actions]

📊 OCR posted 24/25 inline comment(s).

1 could not be posted

src/backend/storage/buffer/freelist.c L353: Validation Failed: {"resource":"PullRequestReviewComment","code":"custom","field":"pull_request_review_thread.line","message":"could not be resolved"} - https://docs.github.com/rest/pulls/comments#create-a-review-comment-for-a-pull-request

[github-actions]

🔍 OCR found 21 issue(s).

• 21 inline, 0 in summary

[github-actions]

This .clangd file, along with .gdbinit and pg-aliases.sh, is a personal developer-environment artifact and does not belong in a PostgreSQL patch. Committing editor/LSP configuration into the source tree violates the minimal-diff discipline: the patch should contain only the changes required for the stated purpose (the HOT indexed updates work). These files will cause needless conflicts, pollute every developer's tree, and would be immediately rejected on pgsql-hackers. Remove it from the commit (and add it to your local git excludes / .git/info/exclude instead).

[github-actions]

The include path -I../../../../src/include is hardcoded relative to a specific nested subdirectory depth. Combined with -I., this only resolves correctly from a directory exactly four levels deep in the tree, so it is broken for a repo-root .clangd and is inherently non-portable across the tree layout. This reinforces that the file is a machine-specific artifact that should not be committed.

[github-actions]

This entire .gdbinit is a personal debugging artifact and must not be committed. For a patch destined for pgsql-hackers/commitfest, the minimal-diff rule is a hard gate: the tree should contain only changes required for the stated functional purpose. A debugger config in the repository root is a top rejection reason and pollutes the root for every other developer. I confirmed .gitignore does not ignore it, so it would actually be committed. The co-added .clangd and pg-aliases.sh are the same class of personal tooling and should likewise be dropped (gitignore them locally instead). (high confidence)

[github-actions]

These breakpoints reference HOT-indexed logic (heapam.c HOT-indexed decision, the heap_hot_indexed_* helpers, heapam_indexscan.c, pruneheap.c redirect-with-data) that is not present in this changeset. I searched the tree and the heap_hot_indexed_* helpers do not exist. Combined with the fact that the files actually modified here are buffer-manager/regress/pgindent files, the debug config is inconsistent with the change's real scope: it points at code that isn't in the diff. (high confidence)

[github-actions]

All break <file>:<line> directives depend on hardcoded absolute line numbers (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936). These go stale the instant any referenced file shifts by a line, making the file misleading even as a private debugging aid. Prefer function-name breakpoints (as done elsewhere in this file) over line numbers. This is moot if the file is dropped, which it should be. (high confidence)

[github-actions]

This shebang change is unrelated to the buffer-manager changes that make up the rest of this patch, and it should be dropped (or split into its own separately-discussed change). Beyond the minimal-diff concern, it also breaks a tree-wide convention: every other Perl script in the tree uses #!/usr/bin/perl (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 others). Making pgindent the sole outlier with #!/usr/bin/env perl is inconsistent; if env perl were desired it would need a deliberate, tree-wide change agreed on -hackers. Confidence: high.

Suggested change

	#!/usr/bin/env perl
	#!/usr/bin/perl

[github-actions]

This execl -> execlp change is unmotivated and, at best, a no-op; at worst a footgun. shellprog derives from SHELLPROG, which is "/bin/sh" (meson) or $(SHELL) (GNUmakefile/ecpg) — none of which changed in this PR. execlp only performs a $PATH search when the program argument contains no slash; since these values always contain a slash, execlp here behaves identically to execl, so the change accomplishes nothing in the shipped configuration.

The only situation where it differs is if shellprog were ever a bare name, in which case execlp would resolve the shell via $PATH — a classic security footgun for a process-spawning harness (an attacker-controlled PATH could select a different shell binary). Absent a stated rationale, a -hackers thread/Message-Id, and any test demonstrating the intended new behavior, this is unrelated churn that widens the diff and should be dropped, or justified explicitly. (high confidence)

Suggested change

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

[github-actions]

The COOL/HOT test masks only bit 0, but the field is manipulated as a 2-bit unit elsewhere (freelist.c and SyncOneBuffer clear the whole field via &= ~BUF_USAGECOUNT_MASK; localbuf.c uses -= BUF_COOLSTATE_ONE). Meanwhile the old BUF_STATE_GET_USAGECOUNT macro right above (unchanged) still masks the full 2-bit field, so the two accessors of the same field now disagree: a stale caller of BUF_STATE_GET_USAGECOUNT sees 0..3 (cooling bit + ref bit), while BUF_STATE_GET_COOLSTATE sees a clean 0/1. This is a footgun. Since BUF_STATE_GET_USAGECOUNT now has zero live callers (all migrated to COOLSTATE), consider removing it in this patch so the field has exactly one, ref-bit-safe accessor — otherwise a future reader may reintroduce a bug by using the wrong one. (moderate confidence)

[github-actions]

Non-standard block-comment opening: PostgreSQL/pgindent formats multi-line block comments with /* alone on the first line and the text starting on the following line. Here the text starts on the same line as the leading /*. pgindent will not leave this untouched, producing whitespace churn on commit. Reformat to the tree convention.

Suggested change

	/* 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. */
	#define BUF_STATE_GET_COOLSTATE(state) \
	/*
	* 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.
	*/
	#define BUF_STATE_GET_COOLSTATE(state) \

[github-actions]

Retaining the name BM_MAX_USAGE_COUNT for a value that is no longer a usage count (it is now a HOT/COOL flag) is misleading: the identifier will mislead future readers and defeat grep-based reasoning about "usage count". The comment itself concedes the name is kept only because one call site reads naturally. The remaining callers are the local-buffer pin fast path (localbuf.c, migrated to BUF_STATE_GET_COOLSTATE < BUF_COOLSTATE_HOT and thus no longer even referencing this macro) and pg_buffercache_pages.c, which uses BM_MAX_USAGE_COUNT + 1 to size histogram arrays that now only ever hold indices 0/1. Prefer renaming to something honest (e.g. reuse BUF_COOLSTATE_HOT directly at the call sites) rather than preserving a drifted name. (moderate confidence)

[github-actions]

🔍 OCR found 42 issue(s).

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

---

📄 pg-aliases.sh (L16-L16)

Silent data-loss footgun (high confidence). $build_dir defaults to $PG_BUILD_DIR, which is never validated. If PG_BUILD_DIR is unset/empty (nothing in this file guards it), rm -rf "$build_dir" expands to rm -rf "" and, worse, the subsequent mkdir -p "$build_dir" also acts on an empty path. Guard against empty/unset expansion before any destructive operation, e.g. [ -n "$build_dir" ] || { echo 'build_dir empty' >&2; return 1; }.

---

📄 pg-aliases.sh (L79-L79)

Destructive rm -rf on unvalidated variables (high confidence). PG_BUILD_DIR and PG_INSTALL_DIR are used without any check that they are set and non-empty. If either is unset (e.g. the file is sourced outside the intended nix shell), this deletes unintended paths. Validate both variables are non-empty before the removal.

---

📄 pg-aliases.sh (L82-L82)

Destructive rm -rf "$PG_DATA_DIR" with no guard (high confidence). If PG_DATA_DIR is unset or empty this becomes rm -rf "". Because this is an alias that runs unconditionally on invocation, an unset variable silently causes data loss. Validate the variable is non-empty first.

---

📄 pg-aliases.sh (L422-L422)

Broken/dead commands (high confidence). These aliases call pg-flame-generate, which is defined nowhere in this file or anywhere else in the repository (verified by search). Every flame-graph command will fail with 'command not found'. Either add the missing definition or remove the aliases that depend on it.

---

📄 pg-aliases.sh (L434-L434)

Broken/dead commands (high confidence). These aliases and functions call pg-bench-run, which is not defined in this file or anywhere else in the repository (verified by search). All benchmark commands (and pg-bench-flame) will fail with 'command not found'. Add the missing definition or remove the dependent commands.

---

📄 pg-aliases.sh (L515-L515)

Word-splitting hazard (medium confidence). Iterating with for file in $modified_files over unquoted git diff --name-only output breaks on paths containing spaces or newlines, silently skipping files or operating on wrong tokens. Prefer a NUL-delimited read loop, e.g. git diff --name-only -z ... | while IFS= read -r -d '' file; do ....

---

📄 pg-aliases.sh (L580-L580)

Privilege escalation / host-wide mutation footgun (medium confidence). This silently invokes root (sudo tee) to mutate a system-wide sysctl (kernel.core_pattern) that affects every process on the host, not just PostgreSQL, from within a sourced shell function. Shipping this in a project repo is unsafe; at minimum it should be an explicit opt-in with a clear warning, and ideally not part of the tree at all.

---

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

Dead code: cool_if_hot is never passed true. Both call sites in this file pass false (BufferSync at ~L3779 and BgBufferSync at ~L4101, whose own comment states "the bgwriter does writeback only and leaves all cooling to the sweep (no background pre-cooling)"). Consequently this entire pre-cooling block, the cool_if_hot parameter, and the BUF_COOLED return flag are unreachable. Per the minimalism/YAGNI discipline, remove the unused parameter, this block, and BUF_COOLED (0x04) rather than shipping speculative scaffolding for a path that is not wired.

---

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

The new BUF_COOLED flag has no consumer anywhere in the tree (searched src/backend/storage/buffer and src/include/storage: only the definition, this set site, and the doc comment match). Since it is only set on the dead cool_if_hot path, it is dead flexibility and should be removed along with that path.

---

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

This silently overrides the long-standing bgwriter_lru_maxpages cap: under sustained bulk-dirtying, write_limit becomes upcoming_alloc_est and the bgwriter can write far more per cycle than the documented GUC allows, changing operator-visible behavior. maxwritten_clean will also stop reflecting the configured limit. This is a behavioral change to a documented tuning knob with no doc update and no reproducible benchmark; on -hackers this needs both. Confirm whether overriding a user-set GUC is intended, and document it.

---

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

cleaner_smoothing_samples = 4 is a hardcoded magic constant diverging from the existing smoothing_samples = 16, with no GUC, test, or reproducible benchmark backing the choice -- only in-code prose. Per the performance/minimalism rules a hidden tuning constant like this is a footgun and will draw list objections. Either justify with a benchmark reference (Message-Id) or make it a named/derived value with rationale.

---

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

Non-standard block-comment opening: text starts on the same line as /*. PostgreSQL convention (and pgindent) puts the first line of a multi-line block comment on the line after the bare /*. Reformat so the opening /* is alone on its line.

---

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

The comment is inaccurate for the local-buffer path. There is no "second chance" here: local buffers never set BUF_REFBIT (PinLocalBuffer does not set it, and no other code in localbuf.c touches it), so a HOT local buffer is demoted straight to COOL on this first pass and becomes an eviction candidate on the next sweep. The "second chance" ref-bit reprieve exists only in the shared-buffer bgwriter path (SyncOneBuffer). The comment should describe what this code actually does, e.g. "HOT: demote to COOL and keep scanning; a re-pin promotes it back to HOT."

💡 Suggested change

Before:

				/* HOT: give it a second chance, cool it and keep scanning. */

After:

				/* HOT: demote to COOL and keep scanning; a re-pin re-promotes it. */

---

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

This crossing-branch increments completePasses without wrapping nextVictimBuffer, but StrategySyncStart() still computes *complete_passes = completePasses + nextVictimBuffer / NBuffers (line 416). When a batch crosses the wrap with the fetch-add return still < NBuffers, the counter is left at start + batch_size (>= NBuffers), so StrategySyncStart's nextVictimBuffer / NBuffers term already accounts for that pass. Bumping completePasses here on top of that double-counts the pass.

Example: NBuffers=100, batch_size=32, start=90. This branch does completePasses++, and the counter is now 122; the next StrategySyncStart adds 122/100=1 as well -> the same wrap is counted twice. The stated intent (fix a low drift) is inverted into an over-count. This feeds bgwriter pacing, so it is a real (non-correctness) regression. Either drop this branch (StrategySyncStart's division already covers the crossing), or wrap the counter here as the primary path does.

(high confidence)

---

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

This shebang change is out of step with the rest of the tree and appears unrelated to any functional change. Every other Perl script in the PostgreSQL source (e.g. src/tools/copyright.pl, src/backend/catalog/genbki.pl, src/tools/mark_pgdllimport.pl, and all *.pl/*.pm files) uses #!/usr/bin/perl; this would make pgindent the sole exception, hurting consistency. It's also a footgun: env perl resolves to whatever perl happens to be first in PATH, which can silently invoke an unexpected interpreter. Unless there is a documented reason on -hackers, drop this unrelated change and keep the fixed path. Confidence: high.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

This execl -> execlp change is a no-op for every shipped configuration and is unjustified. shellprog comes from SHELLPROG, which is defined as /bin/sh (meson) or $(SHELL) (make) — both absolute paths. Per POSIX, execlp only performs a PATH search when the file argument contains no slash; with an absolute path it behaves identically to execl. So this changes nothing for the standard build.

The only scenario where it alters behavior is if SHELLPROG were ever a bare name (no slash), in which case the shell would be resolved via PATH instead of failing — a subtle footgun (running whichever sh happens to be first on PATH). Either revert to execl to keep the minimal, intended behavior, or, if resolving the shell via PATH is genuinely desired, the patch needs a rationale (commit message / -hackers reference) and ideally a test demonstrating the case it fixes. As-is this is an unexplained one-line behavioral change with no test or docs. (high confidence)

💡 Suggested change

Before:

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

After:

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

---

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

Minor style nit: this block comment opens with text on the same line as /*, which is inconsistent with the surrounding block-comment style in this file (e.g. line 133 and the multi-line blocks above, which put /* on its own line). pgindent leaves this as-is, but for consistency the opening /* should be on its own line.

(Separately, note that BUF_STATE_GET_USAGECOUNT on line 136 now has zero callers in the whole tree after all consumers moved to BUF_STATE_GET_COOLSTATE -- it is dead and could be removed, though its definition line is outside this diff.)

💡 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. */
#define BUF_STATE_GET_COOLSTATE(state) \

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.
 */
#define BUF_STATE_GET_COOLSTATE(state) \

[github-actions]

This entire file should not be committed. It is a personal editor/LSP configuration, not part of PostgreSQL's build or source. The project's own top-level .gitignore documents this policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." A patch destined for pgsql-hackers/commitfest must be a minimal diff; unrelated tooling/config files like this are a top rejection reason. Please drop this file from the patch and ignore it locally instead. (high confidence)

[github-actions]

Non-ASCII em-dash characters (U+2014) appear in these comments, violating the project's "ASCII only in source and diffs" rule. Two occurrences: line 1 ("config — bridges") and line 24 ("here — NOT via"). Replace each em-dash with an ASCII hyphen (surrounded by spaces) so git diff --check and the ASCII gate stay clean.

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]

Second em-dash (U+2014) here; same ASCII-only violation. Use an ASCII hyphen instead.

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]

This comment asserts you must NOT use reasoning_effort because LiteLLM maps it to the legacy thinking.type.enabled. But the companion workflow (ocr-review.yml) pins a specific LiteLLM commit precisely because it "maps reasoning_effort -> output_config.effort, incl. xhigh" - the opposite claim. One of these is stale/wrong. Reconcile the comment with the actual behavior of the pinned LiteLLM version so the config's stated rationale isn't misleading (moderate confidence - I can't verify LiteLLM internals from the diff).

[github-actions]

This repository-root .gdbinit is a personal debugging artifact and does not belong in a patch destined for pgsql-hackers/commitfest (minimal-diff / YAGNI hygiene). Beyond the unrelated-tooling problem (it is bundled with pg-aliases.sh), it is a real footgun: gdb auto-sources a .gdbinit in the current directory when launched from the project root (subject to auto-load safe-path settings), so committing this can silently alter another developer's debugging session by setting 30+ breakpoints they never asked for. Remove it from the patch and add .gdbinit to .gitignore instead.

[github-actions]

The 'Attempt merge' step mixes continue-on-error: true with explicit exit 1 and set -e (GitHub's default bash runs with -eo pipefail). If any command fails on a path that does NOT set merge_status first — e.g. git merge --ff-only upstream/master (line 115) failing, or git rebase --abort (line 138) itself failing before the exit 1 — the step aborts with merge_status unset. Both the Push step (== 'success') and the issue-creation step (== 'conflict') would then be skipped, silently swallowing the failure with no alert. Set merge_status before the fallible git merge --ff-only completes, or drop continue-on-error and handle failures explicitly so no path leaves merge_status empty.

[github-actions]

This job has no timeout-minutes and no concurrency control. On an hourly cron, a hung git fetch/git rebase can run indefinitely, and overlapping runs (a long run overlapping the next hourly trigger, or schedule + workflow_dispatch) can race on the same master force-push and clobber each other. Other workflows in this repo (e.g. ocr-review.yml, pg-ci.yml) already declare concurrency. Add a concurrency group with cancel-in-progress and a timeout-minutes to serialize and bound sync runs.

Suggested change

	sync:
	runs-on: ubuntu-latest
	permissions:
	contents: write
	issues: write
	sync:
	runs-on: ubuntu-latest
	timeout-minutes: 15
	concurrency:
	group: sync-upstream
	cancel-in-progress: false
	permissions:
	contents: write
	issues: write

[github-actions]

Comment/identity drift: the workflow is named 'Automatic' and this comment says 'Run hourly every day' (self-contradictory), while the cron is 0 * * * * (hourly) yet the issue/summary bodies elsewhere refer to a 'daily sync'. Align the schedule comment, the workflow copy, and the issue text so operators reading a created incident issue aren't misled about the cadence.

Suggested change

	# Run hourly every day
	- cron: '0 * * * *'
	# Run hourly
	- cron: '0 * * * *'

[github-actions]

This silently repurposes the SQL-visible usagecount column. The pg_buffercache view / pg_buffercache_pages() still exposes a column named usagecount (and pg_buffercache_summary.usagecount_avg) documented in pgbuffercache.sgml as a 0..5 clock-sweep usage count. After this change it carries a 0/1 cooling-state bit instead, while the column name, type, and docs are unchanged. That is a user-visible behavior change (POLA violation) shipped without any documentation update or column-semantics adjustment, and without an entry in the extension's test/expected output to lock in the new range. Either rename/redocument the exposed field to reflect the cooling state, or expose the value in a way that preserves the documented meaning. (high confidence)

[github-actions]

Scope/patch-hygiene violation (high confidence). This entire file is a personal developer-convenience script that is unrelated to the actual change in this PR (a buffer-manager clock-sweep rework in bufmgr.c/freelist.c/localbuf.c/buf_internals.h). A PostgreSQL patch destined for pgsql-hackers/commitfest must be minimal and do one thing; a shell aliases file at the repo root does not belong in the PostgreSQL source tree and is not referenced by any Makefile or meson.build (only sourced from shell.nix, which is itself out-of-tree tooling). Bundling it will draw immediate rejection and waste reviewer time. Remove pg-aliases.sh (and the sibling .clangd/.gdbinit/.github/ocr tooling) from the patch.

[github-actions]

🔍 OCR found 47 issue(s).

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

---

📄 .github/workflows/sync-upstream.yml (L42-L45)

The multi-line run: scripts do not enable strict bash options, and command outputs are consumed unquoted in integer tests further down (e.g. COMMITS_AHEAD=${{ ... }} then [ "$COMMITS_AHEAD" -gt 0 ]). If any git rev-list/grep | wc -l produces empty or unexpected output, or a check_commits output is empty, these [ -eq ]/[ -gt ] tests error out or silently mis-evaluate the guards that gate the force-push. Add set -euo pipefail at the top of the merge/check scripts (or bash shopt -s inherit_errexit) and guard against empty outputs so a classification failure fails loudly rather than bypassing the guard that protects master.

💡 Suggested change

Before:

      - name: Check for local commits
        id: check_commits
        run: |
          git checkout master

After:

      - name: Check for local commits
        id: check_commits
        run: |
          set -euo pipefail
          git checkout master

---

📄 .github/workflows/sync-upstream.yml (L10-L14)

This job has no timeout-minutes. A hung git fetch (network stall against github.com) or a stuck rebase can leave the job running until the default 6-hour limit, wasting runner minutes and overlapping the next hourly schedule. Set a reasonable timeout-minutes on the job.

💡 Suggested change

Before:

  sync:
    runs-on: ubuntu-latest
    permissions:
      contents: write
      issues: write

After:

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

---

📄 .github/workflows/sync-upstream.yml (L5-L6)

Cadence is described inconsistently: the cron '0 * * * *' runs hourly, but this comment says "Run hourly every day", while the workflow name ("Automatic"), the issue body ("The daily sync from ..."), and the Summary header ("Daily Sync Summary") all call it a daily sync. Align the comment and the user-facing text with the actual hourly cadence to avoid confusing maintainers.

💡 Suggested change

Before:

    # Run hourly every day
    - cron: '0 * * * *'

After:

    # Run hourly
    - cron: '0 * * * *'

---

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

This silently changes the user-visible meaning of the usagecount column (pg_buffercache_pages) from the documented 0..5 clock-sweep access count to a 0/1 HOT/COOL cooling state, while keeping the SQL column name usagecount and its docs (pgbuffercache.sgml still says "Clock-sweep access count"). This is a backward-incompatible behavior change for a stable extension view with no accompanying documentation or extension-version update. Either update pgbuffercache.sgml (and consider whether the column should be renamed / a new extension version added) or reconsider exposing the raw cooling bit here. Confidence: high.

---

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

usagecount_total now accumulates 0/1 cooling-state values, so usagecount_avg (exposed via pg_buffercache_summary()) becomes the fraction of HOT buffers rather than the documented average clock-sweep usage count (range 0..5). The docs (pgbuffercache.sgml, usagecount_avg) are now stale. Update the documentation to reflect the new semantics. Confidence: high.

---

📄 pg-aliases.sh (L1-L1)

Patch-hygiene defect (leading concern): this entire file is personal developer tooling unrelated to the substantive change in this PR (the buffer-manager cooling-state clock sweep in bufmgr.c/freelist.c/localbuf.c/buf_internals.h). It is confirmed to be sourced only from shell.nix and is documented in .github/docs/pristine-master-policy.md as "Personal shell aliases". Bundling it violates the minimal-diff and atomic-commit disciplines expected for a pgsql-hackers/commitfest patch and will draw immediate rejection. Split it out of the patch entirely; it does not belong in the source tree.

---

📄 pg-aliases.sh (L16-L17)

Destructive rm -rf/trash on an unvalidated variable. $PG_BUILD_DIR (and the $1 argument) is never checked for being non-empty before deletion. If PG_BUILD_DIR is unset (e.g. sourced outside the nix shell that exports it), this expands to trash "" || rm -rf "" and mkdir -p "", and the earlier [ -f "/compile_commands.json" ] guard does not protect the argument-supplied path. Guard with a non-empty check (e.g. [ -n "$build_dir" ] || return 1) before any deletion.

---

📄 pg-aliases.sh (L7-L7)

Unquoted $CC in the command substitution. If CC contains arguments or spaces (e.g. ccache gcc or a path with spaces), basename $CC receives multiple words and returns the wrong value. Quote it: local current_compiler="$(basename "$CC")".

---

📄 pg-aliases.sh (L82-L82)

Destructive deletion of $PG_DATA_DIR/$PG_INSTALL_DIR/$PG_BUILD_DIR without any non-empty guard. If these variables are unset, rm -rf ""/trash "" runs against unexpected targets. For a data directory in particular this is a silent data-loss footgun. Validate the variables are set and point to the expected location before deleting.

---

📄 pg-aliases.sh (L191-L194)

The generated postgres wrapper interpolates $PG_SOURCE_DIR, $PG_BENCH_DIR, and $bindir unquoted into the heredoc body. If any of these paths contains a space or shell-special character, the produced wrapper script is broken (word-split arguments) or injectable. Quote the interpolated paths inside the generated script (e.g. --suppressions="$PG_SOURCE_DIR/src/tools/valgrind.supp", --log-file="$PG_BENCH_DIR/valgrind-%p.log", and "$bindir/postgres").

---

📄 pg-aliases.sh (L515-L515)

Unquoted for file in $modified_files word-splits on whitespace and glob-expands filenames, so paths containing spaces or glob characters are mishandled. Iterate safely, e.g. read the list into an array or use git diff -z --name-only ... | while IFS= read -r -d '' file.

---

📄 pg-aliases.sh (L580-L580)

Silent privilege escalation and system-wide state mutation: this writes /proc/sys/kernel/core_pattern via sudo tee, changing core-dump behavior for the entire host (all processes), not just PostgreSQL. A user who merely sourced these aliases and ran pg-enable-cores may not expect a global kernel sysctl change under sudo. This should be clearly scoped/opt-in rather than silently invoking sudo.

---

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

Dead code: cool_if_hot is never passed true. All three call sites -- the checkpointer path (SyncOneBuffer(buf_id, false, false, ...)), the bgwriter LRU scan (SyncOneBuffer(next_to_clean, true, false, ...)), and the declaration -- pass false. Consequently this entire pre-cooling block, the new cool_if_hot parameter, and the BUF_COOLED result bit (defined at line 86, set here, and read nowhere) are speculative scaffolding for a path that is not wired up. The bgwriter comment at line 4107 even explains it deliberately passes false and "leaves all cooling to the sweep." Per YAGNI/minimalism, drop the unused parameter, the BUF_COOLED flag, and this block; the second-chance/HOT->COOL demotion already lives in StrategyGetBuffer (freelist.c). Confidence: high.

---

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

This silently overrides the bgwriter_lru_maxpages GUC, which the documentation defines as a hard per-round cap: "In each round, no more than this many buffers will be written by the background writer" and "no more than bgwriter_lru_maxpages buffers will be written per round" (doc/src/sgml/config.sgml). Raising write_limit to upcoming_alloc_est breaks that documented contract, changes the meaning of PendingBgWriterStats.maxwritten_clean, and can trigger large I/O bursts under bulk workloads -- all with no GUC, no doc change, and no benchmark. This is a user-visible behavior change bundled into the cooling-state patch; it should be split out and justified separately, or gated behind a knob with matching docs. Confidence: high.

---

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

The claim "normal workloads are unaffected because there upcoming_alloc_est <= bgwriter_lru_maxpages" is false. upcoming_alloc_est is derived from smoothed_alloc * bgwriter_lru_multiplier and is then floored to min_scan_buffers + reusable_buffers_est (lines 4069-4076), where reusable_buffers_est = bufs_ahead / smoothed_density can approach NBuffers scale. None of those terms are bounded by bgwriter_lru_maxpages, so upcoming_alloc_est can exceed the GUC even on ordinary workloads, meaning write_limit is raised well beyond the "bulk-dirtying" scenario the comment describes. Fix the comment to match actual behavior. Confidence: high.

---

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

Behavior change: the strategy argument is now ignored on the new-pin path. The removed branch enforced the ring-buffer anti-pollution invariant ("Ring buffers shouldn't evict others from pool. Thus we don't make usagecount more than 1."). The new code unconditionally promotes to HOT (BUF_COOLSTATE_ONE) and sets BUF_REFBIT regardless of strategy, so a bulk read/VACUUM ring access now promotes buffers into the HOT working set and marks them recently-used. That partly defeats the purpose of BufferAccessStrategy. Confirm this is intentional and that GetBufferFromRing's COOL check fully compensates; if not, the strategy case must still avoid HOT promotion. Confidence: moderate.

---

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

Non-standard block-comment layout: comment text starts on the /* line. PostgreSQL block comments put /* on its own line; pgindent will reflow this. Same issue at the ExtendBufferedRelShared admission site. Confidence: high.

---

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

Include ordering: port/pg_numa.h is placed after the storage/* block, breaking the alphabetical include-sort convention. It should be grouped with the other port/ header (port/atomics.h), i.e. moved up between pgstat.h and port/atomics.h. pgindent/headerscheck will flag this. (low confidence on tooling failure, high confidence on convention.)

💡 Suggested change

Before:

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

After:

#include "storage/subsystems.h"

---

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

This execl -> execlp change is functionally a no-op and does not belong in this patch.

1. No behavioral change: execlp only performs a PATH search when its file argument contains no slash. shellprog is SHELLPROG, which is always an absolute path with a slash (/bin/sh in meson; make's $(SHELL) in GNUmakefile, also absolute). The PATH-search branch is therefore never exercised, so execlp behaves identically to execl here. (high confidence)

2. Unrelated to the changeset: the rest of this PR modifies the buffer manager (bufmgr.c/freelist.c/localbuf.c/buf_internals.h/pg_buffercache). A shell-exec tweak in the regression driver is an unrelated change and violates the minimal/atomic-diff discipline. If it is genuinely wanted, it should be a separate patch with its own justification. (high confidence)

3. Direction is a mild footgun: resolving the shell via PATH is less predictable/secure than an explicit absolute path. Since the value is already absolute, prefer keeping execl.

Recommend reverting this hunk.

💡 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 inconsistent with the rest of the tree and is an unrelated, non-minimal edit. Every other Perl script in PostgreSQL (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~24 others) uses #!/usr/bin/perl. Making pgindent the sole env perl script deviates from the established convention without justification. Unless there is a discussed, tree-wide reason to switch, this hunk should be dropped to keep the diff minimal and consistent.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

The comment claims the ref bit "stays a plain masked store under the header lock," but PinBuffer sets BUF_REFBIT in the lock-free CAS fast path (bufmgr.c:3351, buf_state |= BUF_REFBIT; inside the pg_atomic_compare_exchange_u64 loop), not under the header spinlock. The bit is set via CAS, not a header-lock-protected store. Fix the comment to describe the actual mechanism (masked into buf_state and published via the atomic CAS on buf->state), otherwise a future reader will assume header-lock protection that does not exist. (high confidence)

💡 Suggested change

Before:

 * count) so it stays a plain masked store under the header lock.

After:

 * count) so it is a plain masked bit updated together with buf_state and
 * published via the atomic compare-exchange on buf->state.

---

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

The new block comment asserts "scan resistance intrinsic to the replacement algorithm" -- a performance/behavior claim for a change to the buffer replacement policy on the hottest path in the backend. This is a substantive, user-visible algorithm change with no regression/TAP tests, no documentation, and no reproducible benchmark in the changeset, and it is bundled with unrelated tooling additions (.clangd, .gdbinit, pg-aliases.sh, .github OCR workflows). For pgsql-hackers this is WIP, not commit-ready: split out the unrelated files, add tests+docs, and cite the design thread (Message-Id). Comments should explain why, not make unbenchmarked performance claims. (moderate confidence)

[github-actions]

This .clangd file is per-developer editor/LSP tooling and should not be committed to the tree. Upstream PostgreSQL does not carry .clangd, and this same PR git-ignores .vscode/ and .idea/ in .github/.gitignore for exactly this reason. A patch posted to pgsql-hackers carrying personal editor config will be rejected. Add .clangd to .gitignore instead of committing it. (high confidence)

[github-actions]

Hardcoded relative include path -I../../../../src/include only resolves for source files nested exactly four directories deep (e.g. contrib/foo/); it is wrong for top-level src/backend/... files and breaks clangd indexing there. Since CompilationDatabase: build/ already supplies per-file compile flags from compile_commands.json, this fragile manual -I is both redundant and incorrect and should be dropped. (moderate confidence)

[github-actions]

Non-ASCII em-dash (U+2014) character used in a comment. PostgreSQL sources and diffs must be ASCII only — no smart quotes, em-dashes, or ellipsis characters. Replace with an ASCII hyphen or -.

Suggested change

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

[github-actions]

Non-ASCII em-dash (U+2014) character used in a comment. Keep source/diffs ASCII only per project convention; replace the em-dash with an ASCII hyphen.

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]

This .gdbinit is a personal developer debugging artifact and should not be committed. It violates the minimal-diff discipline (unrelated scaffolding with no functional value to the patch), and it is a footgun: GDB auto-loads a .gdbinit from the current working directory, so shipping one in the repo root can silently alter debugging behavior for anyone who runs gdb from the checkout. Note that .github/docs/pristine-master-policy.md and pg-aliases.sh already treat .gdbinit as local debugger config, yet it is not in the root .gitignore. Remove it from the patch and keep it local/untracked (add to .gitignore if needed).

[github-actions]

The pg-history job also lacks timeout-minutes. It runs a Bedrock tool-use loop (MAX_ROUNDS = 14, each converse() call up to a 900s read timeout) against an external MCP server (pg.ddx.io). A stuck round can run the job for a very long time on runner minutes. Add timeout-minutes consistent with pg-ci.yml.

[github-actions]

All four git fetch ... || true calls swallow failures. If the origin/${DEFAULT_BRANCH} fetch fails, control still reaches the git show origin/${DEFAULT_BRANCH}:... commands below, which will fail with a confusing error unrelated to the real cause (the missing fetch). Consider letting the default-branch fetch fail loudly (drop || true on it), or validate the ref exists before git show, so failures point at the true root cause instead of masking it.

[github-actions]

aws-actions/configure-aws-credentials@v6 is a third-party action pinned to a mutable major tag, and it runs in the same job that mints AWS OIDC credentials. For actions that touch credentials, pin to a full commit SHA (e.g. aws-actions/configure-aws-credentials@<sha>) to prevent a hijacked tag from exfiltrating the assumed-role session.

[github-actions]

open-code-review is installed unpinned (@latest implicitly), so a compromised or breaking npm publish executes in a job holding AWS OIDC credentials and pull-requests: write. LiteLLM is at least pinned to a SHA (good); pin the npm package to an exact version too (e.g. @alibaba-group/open-code-review@<version>) for reproducibility and supply-chain safety.

[github-actions]

This workflow force-pushes master (git push origin master --force-with-lease) but declares no concurrency control while triggering hourly on schedule plus workflow_dispatch. In addition, .github/workflows/sync-upstream-manual.yml also force-pushes master and has no concurrency group either. Two overlapping runs (e.g. an hourly run overlapping a manual sync, or a manual dispatch during an hourly run) can both fetch, rebase, and force-push master at the same time, creating a race that can roll back or corrupt the default branch. --force-with-lease only guards against a stale local ref, not against two concurrent jobs each holding a fresh lease. Add a shared concurrency group (e.g. group: sync-upstream, cancel-in-progress: false to serialize) across both sync workflows.

Suggested change

	jobs:
	sync:
	runs-on: ubuntu-latest
	permissions:
	contents: write
	issues: write
	concurrency:
	group: sync-upstream
	cancel-in-progress: false
	jobs:
	sync:
	runs-on: ubuntu-latest
	timeout-minutes: 15
	permissions:
	contents: write
	issues: write