Conversation
14db4ac to
bad9884
Compare
b481005 to
ac73e0f
Compare
3b1d505 to
7dda555
Compare
7e77e41 to
f9645af
Compare
15913cb to
06a2a69
Compare
3b49b2a to
0b3e646
Compare
📜 Change history & discussion (Agora / pg.ddx.io)The author-name lookup returned unrelated CF59 entries (substring match noise, not Greg Burd's). No commitfest entry is linked to this thread. Let me verify upstream merge status is not applicable (no PR URL given) and finalize. I have all confirmed facts. 🧵 Related discussion
Caveat: the thread as posted covers only the batched-sweep change (commit 🔗 Related commits / prior art
🧭 Context for reviewers
(Note: the Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| SELECT sum(reads) AS stats_bulkreads_before | ||
| FROM pg_stat_io WHERE context = 'bulkread' \gset | ||
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset |
There was a problem hiding this comment.
The \gset variables are still named stats_bulkreads_before/stats_bulkreads_after, but this query now deliberately measures the normal context (the comment above was rewritten to say the reads are no longer bulkreads). The names now contradict what they hold. Since these exact lines are already being touched, rename them to reflect the new semantics (e.g. stats_normal_reads_before) for both \gset sites and the comparison on the following line. This also requires the matching update in contrib/amcheck/expected/check_heap.out.
| SELECT sum(reads) AS stats_bulkreads_before | |
| FROM pg_stat_io WHERE context = 'bulkread' \gset | |
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset | |
| SELECT sum(reads) AS stats_normal_reads_before | |
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset |
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This silently changes the user-visible semantics of the pg_buffercache.usagecount column. BUF_STATE_GET_USAGECOUNT returned the full clock-sweep count (0..BM_MAX_USAGE_COUNT, historically 0..5), whereas BUF_STATE_GET_COOLSTATE returns only bit 0 (0=COOL, 1=HOT). The column is still declared smallint and documented as "Clock-sweep access count", so users querying usagecount will now silently get only 0/1 with no doc or column-name update. This is a backward-incompatible behavior change that needs the documentation (pgbuffercache.sgml) updated and, arguably, the column renamed to reflect HOT/COOL semantics. (high confidence)
| { | ||
| buffers_used++; | ||
| usagecount_total += BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount_total += BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
usagecount_total now accumulates only 0/1 per buffer instead of 0..BM_MAX_USAGE_COUNT, so the derived usagecount_avg column changes range/meaning without any doc update. The SGML still labels it a clock-sweep average. Update the documentation to match the new HOT/COOL semantics. (high confidence)
| CHECK_FOR_INTERRUPTS(); | ||
|
|
||
| usage_count = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usage_count = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
pg_buffercache_usage_counts() now buckets buffers only into indices 0 and 1 (COOL/HOT); the usage_count output column will never exceed 1. This is fine for array bounds since BM_MAX_USAGE_COUNT is redefined to BUF_COOLSTATE_HOT (=1), but the SQL column name usage_count and its doc ("A possible buffer usage count") are now misleading. Update pgbuffercache.sgml and the expected regression output to reflect the reduced value domain. (high confidence)
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | ||
| } | ||
| else | ||
| allow_strat = allow_sync = false; | ||
|
|
||
| if (allow_strat) | ||
| { | ||
| /* During a rescan, keep the previous strategy object. */ | ||
| if (scan->rs_strategy == NULL) | ||
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | ||
| } | ||
| else | ||
| { | ||
| if (scan->rs_strategy != NULL) | ||
| FreeAccessStrategy(scan->rs_strategy); | ||
| scan->rs_strategy = NULL; | ||
| } | ||
| allow_sync = false; |
There was a problem hiding this comment.
The if body is now a single statement after removing the allow_strat assignment, but the braces are retained. PostgreSQL style (and pgindent-adjacent convention) omits braces around single-statement bodies. Since this block was directly modified by the diff, drop the braces to keep the code reading as if it had always been written this way:
if (!RelationUsesLocalBuffers(scan->rs_base.rs_rd) &&
scan->rs_nblocks > NBuffers / 4)
allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
else
allow_sync = false;Confidence: moderate.
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | |
| } | |
| else | |
| allow_strat = allow_sync = false; | |
| if (allow_strat) | |
| { | |
| /* During a rescan, keep the previous strategy object. */ | |
| if (scan->rs_strategy == NULL) | |
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | |
| } | |
| else | |
| { | |
| if (scan->rs_strategy != NULL) | |
| FreeAccessStrategy(scan->rs_strategy); | |
| scan->rs_strategy = NULL; | |
| } | |
| allow_sync = false; | |
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | |
| else | |
| allow_sync = false; |
| else | ||
| elevel = DEBUG2; | ||
|
|
||
| /* Set up static variables */ |
There was a problem hiding this comment.
This comment is now dangling. It solely described the removed vac_strategy = bstrategy; assignment; the only other static in this file (anl_context) is set up in do_analyze_rel, not here. Leaving an empty comment block referring to nonexistent code violates the minimal-diff/comment-accuracy discipline. Remove the comment (and the now-doubled blank line).
| read_stream_next_block(ReadStream *stream) | ||
| { | ||
| *strategy = stream->ios[0].op.strategy; | ||
| return read_stream_get_block(stream, NULL); | ||
| } |
There was a problem hiding this comment.
With strategy removed, read_stream_next_block() no longer has any distinguishing behavior: it is now an exported thin wrapper around read_stream_get_block(stream, NULL) and has no callers anywhere in the tree (verified: only the callback functions apw_read_stream_next_block and collect_corrupt_items_read_stream_next_block match by name; no invocation of read_stream_next_block(...) exists). Its sole prior reason to exist over calling the internal helper directly was reporting the strategy, which this patch deletes. Recommend removing the function (and its prototype in read_stream.h) as dead transitional scaffolding, or, if it is intentionally retained as public API, keep it clearly justified. Leaving a caller-less exported wrapper contradicts the minimal-diff / YAGNI discipline. (moderate confidence)
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | ||
| if (current >= (uint32) NBuffers) | ||
| { | ||
| wrapped = current % NBuffers; | ||
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | ||
| ¤t, wrapped)) | ||
| StrategyControl->completePasses++; |
There was a problem hiding this comment.
completePasses can be under-counted under concurrency, corrupting bgwriter pacing. StrategySyncStart() relies on nextVictimBuffer staying below 2*NBuffers so its pending-wrap compensation (nextVictimBuffer / NBuffers) is at most 1. With batching, many concurrent backends can each land a fetch_add(batch_size) before any of them reaches the spinlock, so the shared counter can grow to NBuffers + MaxBackends*batch_size. When it exceeds 2*NBuffers, wrapped = current % NBuffers subtracts multiple NBuffers-multiples but completePasses is incremented only once, permanently losing pass counts. This is reachable in the small-NBuffers case (minimum 16, batch capped at NBuffers): 2*NBuffers is easily exceeded by concurrent claims. Consider incrementing completePasses by current / NBuffers instead of a flat +1, and/or bounding growth so the counter cannot exceed 2*NBuffers.
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | |
| if (current >= (uint32) NBuffers) | |
| { | |
| wrapped = current % NBuffers; | |
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | |
| ¤t, wrapped)) | |
| StrategyControl->completePasses++; | |
| current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); | |
| if (current >= (uint32) NBuffers) | |
| { | |
| wrapped = current % NBuffers; | |
| if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, | |
| ¤t, wrapped)) | |
| StrategyControl->completePasses += current / NBuffers; |
| MyBatchPos = start; | ||
| MyBatchEnd = start + batch_size; |
There was a problem hiding this comment.
A batch can straddle the NBuffers boundary without any wrap being registered on the shared counter. When start < NBuffers but start + batch_size > NBuffers, the wrap branch (start >= NBuffers) is skipped, yet victim = MyBatchPos % NBuffers wraps internally as MyBatchPos crosses NBuffers. The completePasses increment for that crossing is deferred until a later fetch-add happens to return start >= NBuffers. Between those events, StrategySyncStart() sees nextVictimBuffer already advanced past NBuffers and relies solely on its nextVictimBuffer / NBuffers term to compensate. That compensation is only correct while the counter stays below 2*NBuffers (see the related finding). This coupling is fragile and should be documented or handled explicitly, since a mid-batch wrap is now the common case (batch_size defaults to 32).
| #include "storage/subsystems.h" | ||
| #include "port/pg_numa.h" |
There was a problem hiding this comment.
Include is out of group/alphabetical order: port/pg_numa.h is placed after the storage/* block. Per the tree's convention (and pgindent grouping), it belongs with the other port/* headers, immediately after port/atomics.h.
| #include "storage/subsystems.h" | |
| #include "port/pg_numa.h" | |
| #include "storage/subsystems.h" |
0b3e646 to
3ed29c4
Compare
There was a problem hiding this comment.
🔍 OCR found 43 issue(s).
- 25 inline, 18 in summary (inline capped at 25)
📄 src/backend/access/nbtree/nbtree.c (L1732-L1732)
The strategy argument was removed from this ReadBufferExtended call, but the comment above still claims "we want to use a nondefault buffer access strategy." This comment is now stale and misleading -- no strategy is passed anymore. Update the comment to reflect that the buffer access strategy is now handled elsewhere (or drop that sentence). (moderate confidence)
📄 src/backend/commands/analyze.c (L124-L124)
The /* Set up static variables */ comment is now stale: this diff removed the only statement it introduced (vac_strategy = bstrategy;), leaving the comment describing nothing. Remove the dangling comment so the code reads as if it had always been written this way (minimal-diff hygiene). (high confidence)
📄 src/backend/commands/vacuum.c (L178-L179)
This removal makes VACUUM (BUFFER_USAGE_LIMIT ...) and ANALYZE (BUFFER_USAGE_LIMIT ...) a hard error. The grammar (gram.y utility_option_list) still accepts any option name as a generic DefElem, so with this branch gone the option now falls through to the unrecognized "VACUUM"/"ANALYZE" option ereport below. This is a backward-compatibility break of a documented, user-visible SQL option, and it breaks existing dump/restore scripts and tooling that pass BUFFER_USAGE_LIMIT. If the intent is to fully retire the feature, the corresponding user-facing surfaces (SQL docs in doc/src/sgml/ref/{vacuum,analyze}.sgml and psql tab-completion in tab-complete.in.c, which still advertise BUFFER_USAGE_LIMIT) must be updated in the same change so the option isn't offered while silently erroring. Confidence: high.
📄 src/backend/storage/buffer/freelist.c (L161-L163)
completePasses accounting is inconsistent with StrategySyncStart's compensation term and can double-count / go backwards.
StrategySyncStart() still returns *complete_passes = completePasses + nextVictimBuffer / NBuffers. But now:
-
This
else ifincrements completePasses for a batch that crosses the wrap point without wrapping the shared counter. The counter is then >= NBuffers, so a subsequent StrategySyncStart addsnextVictimBuffer / NBuffers(>=1) on top of the increment already applied here -> the same pass is counted twice. -
The
if (start >= NBuffers)branch above wrapscurrent % NBuffersbut increments completePasses by only 1 even whencurrentdrifted to e.g.2*NBuffers; the sync-start compensation term drops from 2 to 0 across that wrap, so the visible pass count decreases.
Because BgBufferSync does passes_delta = strategy_passes - prev_strategy_passes; strategy_delta += passes_delta*NBuffers; Assert(strategy_delta >= 0), a non-monotonic completePasses can trip that assertion and corrupts bgwriter pacing. Confidence: high. The batched wrap accounting needs to be made consistent with StrategySyncStart's nextVictimBuffer / NBuffers term (i.e. don't both pre-count the crossing here and let sync-start count it again).
📄 src/backend/storage/buffer/freelist.c (L137-L142)
This if (start >= NBuffers) wrap path can lose passes when the counter has drifted more than one NBuffers period. current may be >= 2*NBuffers under batching/contention, but wrapped = current % NBuffers collapses it and completePasses is bumped by only 1, while StrategySyncStart's nextVictimBuffer / NBuffers compensation was >1 before the wrap and becomes 0 after. Net effect: the reported completed-pass count can decrease across the wrap, which BgBufferSync's Assert(strategy_delta >= 0) does not tolerate. Confidence: high. Consider incrementing completePasses by current / NBuffers (the number of whole periods being discarded) so the (nextVictimBuffer, completePasses) pair stays monotonic.
📄 src/backend/storage/buffer/freelist.c (L324-L330)
Under force_cool, resetting trycounter = NBuffers on the ref-bit-clear transition (not just on the HOT->COOL demote) means a highly contended, all-HOT pool where PinBuffer keeps re-setting BUF_REFBIT (it does so on every pin, bufmgr.c:3302) can keep this backend clearing ref bits and resetting the counter without ever landing on a reclaimable COOL buffer. The intended "second unproductive full pass -> elog(ERROR, no unpinned buffers)" backstop can then be deferred well beyond the ~3 passes the comment claims. Consider only resetting trycounter on the actual HOT->COOL demotion (real forward progress toward a victim), and treating a bare ref-clear as non-progress so the escalation/backstop stays bounded. Confidence: moderate.
📄 src/backend/storage/buffer/bufmgr.c (L86-L86)
Dead flag: BUF_COOLED is defined here, documented in the SyncOneBuffer header, and set via result |= BUF_COOLED, but no caller ever inspects it. SyncOneBuffer's only cool_if_hot=true caller is BgBufferSync, which reacts only to BUF_WRITTEN and BUF_REUSABLE. Per the minimalism/YAGNI discipline, either wire BUF_COOLED into a consumer (e.g. bgwriter pacing/instrumentation) or drop the flag, its result |= assignment, and the doc line. (high confidence)
📄 src/backend/storage/buffer/bufmgr.c (L2320-L2325)
Comment-drift / misleading placement. This "Admit the newly loaded page COOL" comment sits directly above the unrelated BM_PERMANENT conditional, but the actual COOL admission is the removal of BUF_USAGECOUNT_ONE from the set_bits |= BM_TAG_VALID; line above (a freshly admitted buffer now has coolstate 0 == COOL). As written the comment misleads readers into thinking the BM_PERMANENT block performs the cooling admission. Move the comment to sit above the set_bits |= BM_TAG_VALID; line. (moderate confidence)
📄 src/backend/storage/buffer/bufmgr.c (L2964-L2968)
Same comment-placement issue as the other admission site: the "Admit COOL (probation)" comment is above the BM_PERMANENT conditional rather than the set_bits |= BM_TAG_VALID; line where BUF_USAGECOUNT_ONE was previously set. Relocate it above set_bits |= BM_TAG_VALID; so the comment describes the line that actually performs COOL admission. (moderate confidence)
📄 src/backend/utils/activity/pgstat_io.c (L443-L449)
This comment block is now stale/dangling. The if blocks it described (the B_CHECKPOINTER/B_BG_WRITER/autovac restrictions on the removed bulkread/bulkwrite/vacuum IOContexts) were deleted, leaving the comment sitting directly above return true; describing nothing. Per PG comment discipline (comments must describe what the code does now) and minimal-diff hygiene, remove the orphaned comment so the function reads as if it had always been written this way.
💡 Suggested change
Before:
/*
* Some BackendTypes do not currently perform any IO in certain
* IOContexts, and, while it may not be inherently incorrect for them to
* do so, excluding those rows from the view makes the view easier to use.
*/
return true;
After:
return true;
📄 src/bin/scripts/vacuumdb.c (L355-L355)
This removes the user-visible --buffer-usage-limit option (getopt entry, handler, validation, and help text), but the corresponding documentation is not removed: doc/src/sgml/ref/vacuumdb.sgml still contains a full <varlistentry> for --buffer-usage-limit (around line 135). A user-visible removal must delete the matching doc entry in the same patch, otherwise the docs describe an option the binary no longer accepts. Please remove the SGML block as part of this change. (high confidence)
📄 src/bin/scripts/vacuuming.c (L267-L272)
This removes the --buffer-usage-limit handling from vacuumdb, a user-visible option shipped since PG16. Together with the coordinated removals in vacuumdb.c and vacuuming.h, this deletes an established CLI feature. This is a backward-compatibility break: existing scripts invoking vacuumdb --buffer-usage-limit=... will now fail with an unknown-option error. Such a removal needs extraordinary justification and a design discussion on -hackers; nothing in the diff explains why. Additionally, doc/src/sgml/ref/vacuumdb.sgml still documents --buffer-usage-limit and is not part of this change set, so the docs will describe an option that no longer exists. (high confidence)
📄 src/include/storage/bufmgr.h (L357-L360)
Dead section header and whitespace churn. All prototypes that lived under "/* in freelist.c /" (GetAccessStrategy, FreeAccessStrategy) were removed, so this comment now labels nothing, and the removal left a stray double blank line before "/* inline functions /". This will trip git diff --check / pgindent style expectations. Remove the empty "/ in freelist.c */" section entirely and collapse to a single blank line.
💡 Suggested change
Before:
/* in freelist.c */
/* inline functions */
After:
/* inline functions */
📄 src/include/storage/bufmgr.h (L221-L222)
API/ABI break on widely-used exported entry points (moderate confidence on impact). ReadBufferExtended(), ReadBufferWithoutRelcache(), and the ExtendBufferedRel*() family drop their BufferAccessStrategy parameter, while GetAccessStrategy()/GetAccessStrategyWithSize()/GetAccessStrategyBufferCount()/GetAccessStrategyPinLimit()/FreeAccessStrategy() are removed outright. These are core buffer-manager APIs used pervasively by out-of-tree extensions; every such extension will fail to compile against this header. On HEAD a signature change is allowed, but wholesale removal of the strategy concept with no replacement is a substantial, non-obvious semantic change (ring buffers gone; scan/VACUUM no longer bounded to a small buffer ring). For pgsql-hackers this needs (a) a reference to the design thread / commitfest entry justifying dropping ring buffers, and (b) confirmation it is a single logical change -- as submitted it bundles at least three independent things (remove BufferAccessStrategy, replace usage_count clock sweep with HOT/COOL cooling state, add NUMA-batched clock sweep), which should be split into separately-committable, individually-bisectable patches.
📄 src/include/pgstat.h (L294-L294)
This change reduces IOCONTEXT_NUM_TYPES from 6 to 2, which shrinks the fixed-stats structs PgStat_BktypeIO/PgStat_PendingIO/PgStat_IO (dimensioned [IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES], lines 328-337). PgStat_IO is a fixed stats kind that is serialized to the persisted stats file. The comment above PGSTAT_FILE_FORMAT_ID (line 216-217) explicitly requires bumping the format ID "whenever any of these data structures change", and on startup pgstat_read_statsfile() only discards a stale file when format_id != PGSTAT_FILE_FORMAT_ID. Since PGSTAT_FILE_FORMAT_ID (0x01A5BCBC) is unchanged in this patch, a stats file written by a pre-patch server will be accepted and read with the new, smaller layout, corrupting the fixed-stats block (and desynchronizing subsequent parsing). Bump PGSTAT_FILE_FORMAT_ID. Unlike catversion.h this stamp is the author's responsibility. Confidence: high.
📄 src/include/storage/buf_internals.h (L136-L137)
BUF_STATE_GET_USAGECOUNT now has no callers anywhere in the tree -- every consumer was switched to BUF_STATE_GET_COOLSTATE. Leaving the old accessor behind is dead code and a footgun: the field it reads now mixes the cooling bit (bit 0) and the ref bit (bit 1), so any new caller that reaches for the familiar name gets a 0..3 value conflating two orthogonal flags. Remove it. (high confidence)
📄 src/include/storage/buf_internals.h (L139-L140)
pgindent-nonconforming multi-line comment: the block opens with text on the same line as /* and closes with an inline */, unlike every other multi-line comment in this header (which puts /* on its own line with asterisk-aligned continuation). This will not survive pgindent cleanly. (moderate confidence)
💡 Suggested change
Before:
/* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
* the second-chance ref bit (bit 1) does not perturb the HOT/COOL test. */
After:
/*
* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so
* the second-chance ref bit (bit 1) does not perturb the HOT/COOL test.
*/
📄 src/include/storage/buf_internals.h (L191-L191)
BM_MAX_USAGE_COUNT now means "the HOT cool-state value" (=1), not a usage-count maximum, yet it is still consumed as a numeric range bound -- e.g. pg_buffercache sizes usage_counts[BM_MAX_USAGE_COUNT + 1] and iterates over it. Retaining a count-named macro for a boolean cool-state violates POLA and invites a future reader to treat it as a real max count. Prefer a name that reflects the new semantics (e.g. BM_MAX_COOLSTATE) and update call sites, rather than aliasing the old name for the single expression that happens to read naturally. (moderate confidence)
| @@ -0,0 +1,41 @@ | |||
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |||
There was a problem hiding this comment.
Non-ASCII em-dash character (—) in a source/config comment. The project's contribution standards require ASCII-only in source and diffs (no smart quotes, em-dashes, or ellipsis characters). Replace with an ASCII - (or --).
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |
| # LiteLLM proxy config - bridges Open Code Review (OpenAI protocol) to AWS Bedrock. |
| aws_region_name: os.environ/AWS_REGION | ||
|
|
||
| # "High effort" review. Claude Opus 4.8 on Bedrock uses *adaptive* thinking | ||
| # controlled by output_config.effort. Set it DIRECTLY here — NOT via |
There was a problem hiding this comment.
Non-ASCII em-dash character (—) in this comment. Replace with an ASCII - to comply with the ASCII-only rule for source and diffs.
| # controlled by output_config.effort. Set it DIRECTLY here — NOT via | |
| # controlled by output_config.effort. Set it DIRECTLY here - NOT via |
| import boto3 | ||
| from botocore.config import Config |
There was a problem hiding this comment.
The module docstring promises the script "exits 0 even on soft failures (writes a note)", and every other failure path here honors that (MCP unreachable at line 168, no tools at line 172, Bedrock error at line 212 all write a note to OUT and return). But import boto3 / from botocore.config import Config are not guarded: if the SDK is missing/unimportable this raises an uncaught ImportError, exits non-zero, and writes NO note to PG_HISTORY_OUT, leaving the downstream "Upsert PR comment" step with empty output. The workflow's || true masks the exit code but not the missing note. Wrap the import in the same try/except-and-write-note pattern used for the MCP setup for consistency with the documented contract. (moderate confidence)
| import boto3 | |
| from botocore.config import Config | |
| try: | |
| import boto3 | |
| from botocore.config import Config | |
| except Exception as e: | |
| open(OUT, "w").write(f"_pg-history: boto3/botocore unavailable: {e}_\n") | |
| print(f"boto3 unavailable: {e}") | |
| return |
| open(OUT, "w").write(body) | ||
| print(body) |
There was a problem hiding this comment.
open(OUT, "w").write(...) leaves the file handle to be closed by refcount-based GC, so written bytes are only flushed at interpreter cleanup. This pattern is repeated at every write site (lines 158, 168, 172, 220). On CPython in this short-lived script it usually works, but it is not guaranteed on other interpreters and violates the tree's resource-management/portability expectations. Prefer with open(OUT, "w") as f: f.write(...). (low confidence, minor)
| check-model: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
This job has no timeout-minutes, unlike every job in .github/workflows/pg-ci.yml (which sets it on all jobs). The OIDC credential step and the inline Python subprocess invoking the AWS CLI can hang on network/throttling/OIDC issues, letting the job run to the default 6-hour runner limit. Add a short explicit timeout for this scheduled maintenance job.
| check-model: | |
| runs-on: ubuntu-latest | |
| steps: | |
| check-model: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: |
| sync: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| issues: write |
There was a problem hiding this comment.
The sync job has no timeout-minutes. git fetch upstream master against the full-history PostgreSQL repository plus a rebase can hang on network problems, and with an hourly schedule stalled runs can pile up and burn runner minutes indefinitely. Other workflows in this repo (pg-ci.yml) set timeout-minutes; add an explicit timeout here for consistency and safety.
| sync: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write | |
| issues: write | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 | |
| permissions: | |
| contents: write | |
| issues: write |
| echo "⚠️ Local master has $DIVERGED commits not in upstream" | ||
|
|
||
| # Check commit messages for "dev setup" or "dev v" pattern | ||
| DEV_SETUP_COMMITS=$(git log --format=%s upstream/master..origin/master | grep -iE "^dev (setup|v[0-9])" | wc -l) |
There was a problem hiding this comment.
The decision to rebase and force-push over master rests on brittle string matching: grep -iE "^dev (setup|v[0-9])" on commit subjects and grep -v "^\.github/" on changed paths. A squashed commit that mixes .github/ and non-.github/ changes, a merge commit, or a commit whose subject coincidentally starts with "dev v1" can be misclassified. A false positive here permits git rebase upstream/master followed by --force-with-lease, which can silently drop legitimate local commits on master. This is a data-loss footgun — the guard protecting destructive history rewriting should be more robust than subject-line pattern matching (e.g., fail closed on any non-.github/ change rather than trusting the commit subject).
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
|
|
||
| if git rebase upstream/master; then |
There was a problem hiding this comment.
git config user.name/user.email is already set in the earlier "Configure Git" step, and git config persists across steps within a job on the runner. Re-setting it here is redundant and can be removed.
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| if git rebase upstream/master; then | |
| if git rebase upstream/master; then |
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This change alters the user-visible semantics of the usage_count column exposed by all three functions (pg_buffercache_pages, pg_buffercache_summary, pg_buffercache_usage_counts). BUF_STATE_GET_USAGECOUNT returned the full 0..N usage count, whereas BUF_STATE_GET_COOLSTATE returns only the 0/1 cooling state (bit 0 of the field). The documentation in doc/src/sgml/pgbuffercache.sgml still describes and shows a multi-valued "usage count" (e.g. the sample output table and the column descriptions "A possible buffer usage count"), and is not updated in this change. A user-visible SQL-interface change without matching docs is incomplete for a pgsql-hackers patch. Update pgbuffercache.sgml to reflect the new HOT/COOL cooling-state semantics (and consider whether the exposed column/variable naming should still be usage_count). (high confidence)
| allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0; | ||
| } | ||
| else | ||
| allow_strat = allow_sync = false; | ||
|
|
||
| if (allow_strat) | ||
| { | ||
| /* During a rescan, keep the previous strategy object. */ | ||
| if (scan->rs_strategy == NULL) | ||
| scan->rs_strategy = GetAccessStrategy(BAS_BULKREAD); | ||
| } | ||
| else | ||
| { | ||
| if (scan->rs_strategy != NULL) | ||
| FreeAccessStrategy(scan->rs_strategy); | ||
| scan->rs_strategy = NULL; | ||
| } | ||
| allow_sync = false; |
There was a problem hiding this comment.
This diff removes the bulk-read access strategy from initscan(), but the comment above this block (lines 383-390) still reads "use a bulk-read access strategy and enable synchronized scanning ... only two behaviors to tune rather than four ... some callers need to be able to disable one or both of these behaviors". After this change there is only one behavior left here (sync scan); the bulk-read strategy no longer exists. The comment now describes code that is gone and should be updated to mention only synchronized scanning, otherwise it will mislead readers into looking for strategy handling that isn't there. (moderate confidence)
e495986 to
d7cd5d6
Compare
There was a problem hiding this comment.
🔍 OCR found 32 issue(s).
- 25 inline, 7 in summary (inline capped at 25)
📄 src/backend/utils/activity/pgstat_io.c (L537-L540)
The double-hyphen '--' is used here as an em-dash substitute, which is inconsistent with PostgreSQL comment style (plain sentences). Rewrite as separate sentences. e.g. "IOOP_REUSE was only relevant when a BufferAccessStrategy was in use. Buffer access strategies (ring buffers) have been removed, so IOOP_REUSE never occurs." Also, the "scan resistance is now intrinsic to the cooling-stage clock sweep" clause is design narrative that belongs in the buffer manager, not in the pg_stat_io validity table; drop it here.
Confidence: high (style), moderate (scope of the design claim).
💡 Suggested change
Before:
* IOOP_REUSE was only relevant when a BufferAccessStrategy was in use.
* Buffer access strategies (ring buffers) have been removed -- scan
* resistance is now intrinsic to the cooling-stage clock sweep -- so
* IOOP_REUSE never occurs.
After:
* IOOP_REUSE was only relevant when a BufferAccessStrategy was in use.
* Buffer access strategies (ring buffers) have been removed, so
* IOOP_REUSE never occurs.
📄 src/include/access/tableam.h (L691-L692)
This changes the signature of the relation_vacuum callback in TableAmRoutine, which is the public, extension-facing table access method (TAM) API. Any out-of-tree TAM implementing relation_vacuum will fail to compile (and breaks ABI) against this. That is acceptable on HEAD, but this is part of a large, multi-purpose change that removes the entire BufferAccessStrategy mechanism (dropping the BUFFER_USAGE_LIMIT VACUUM/ANALYZE option and the VacuumBufferUsageLimit GUC -- both user-visible, documented behavior) in favor of a new cooling-stage/NUMA-batched clock sweep. The commit message / diff should clearly justify the removal and reference the -hackers design thread, and the removal of a documented GUC and SQL option needs the corresponding doc and test updates. Confidence: high that this is an API/ABI break; the justification/design-reference concern applies to the patch as a whole.
📄 src/include/pgstat.h (L294-L294)
Confidence: high. Shrinking IOCONTEXT_NUM_TYPES from 5 to 2 changes the on-disk/shared-memory layout of PgStat_BktypeIO (its bytes/counts/times arrays are dimensioned [IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES], see lines 328-330). The header comment above PGSTAT_FILE_FORMAT_ID (line 216) says it "should be changed whenever any of these data structures change." This patch does not bump PGSTAT_FILE_FORMAT_ID (still 0x01A5BCBC), so a server reading a stats file written by a prior binary will misinterpret the PgStat_IO blob (wrong element count / offsets), risking garbage stats or a read failure. Bump PGSTAT_FILE_FORMAT_ID in this patch. Unlike catversion.h, the stats format ID is author-owned and must accompany this change.
📄 src/include/storage/bufmgr.h (L357-L360)
This diff removed the only prototypes that lived under /* in freelist.c */ (GetAccessStrategy*/FreeAccessStrategy). The section comment is now orphaned, and the removal also leaves two consecutive blank lines before /* inline functions */. Drop the now-empty /* in freelist.c */ header and the extra blank line so the header reads as if it had always been written this way (minimal-diff / no dead section markers). (low confidence: cosmetic, but committers flag exactly this.)
💡 Suggested change
Before:
/* in freelist.c */
/* inline functions */
After:
/* inline functions */
📄 src/include/storage/buf_internals.h (L139-L140)
This multi-line block comment opens with text on the same line as /*, which is inconsistent with the surrounding convention in this file (see the other block comments at the top of the file that place /* on its own line) and will not survive pgindent cleanly for a multi-line comment. Reformat to the standard style.
💡 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/test/regress/pg_regress.c (L1246-L1246)
This change is a no-op in every current build configuration and should be dropped to keep the diff minimal. shellprog is initialized solely from SHELLPROG and never reassigned; both build systems define it as an absolute path containing a slash ($(SHELL), which defaults to /bin/sh, in src/test/regress/GNUmakefile, and /bin/sh in src/test/regress/meson.build). 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 line produces no behavioral change today.
If the intent is to allow a bare shell name resolved via PATH, that is a behavioral change that (a) is not wired up (nothing sets SHELLPROG to a bare name), and (b) is an undesirable footgun in a process-spawn path, since it makes shell resolution depend on the ambient PATH. Either revert this hunk or land it together with the change that actually requires PATH resolution, with justification. 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 out of scope for this patch and diverges from the tree-wide convention. Every other Perl script in the PostgreSQL tree uses #!/usr/bin/perl (e.g. copyright.pl, genbki.pl, gen_node_support.pl); this diff would make pgindent the sole file using #!/usr/bin/env perl. It is also unrelated to the rest of this change set (buffer/vacuum/storage work), so it violates the one-logical-change-per-commit rule. Drop this hunk (or split it into its own patch with a justification/-hackers reference).
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
| - -g | ||
| - -std=c11 | ||
| - -I. | ||
| - -I../../../../src/include |
There was a problem hiding this comment.
This include path is almost certainly wrong for a .clangd placed at the repository root. Relative -I flags added via CompileFlags.Add are resolved relative to the compile command's directory (the build/ dir set above) or the source file's directory, not the repo root. Going up four levels (../../../../) from anywhere at/under the repo root lands outside the tree and never resolves to <repo>/src/include. The ../../../../ prefix looks copy-pasted from a deeply nested subdirectory's build flags. Since src/include sits directly under the root, use a root-relative path instead. (Impact is limited to the author's own clangd header resolution, and MissingIncludes: None suppresses the resulting diagnostics, hence low severity.)
| - -I../../../../src/include | |
| - -Isrc/include |
| FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset | ||
| BEGIN; |
There was a problem hiding this comment.
This changes the measurement from the isolated bulkread context to the cluster-wide normal/relation context, but the surrounding \gset variables are still named stats_bulkreads_before/stats_bulkreads_after (and likewise stats_bulkreads_after below). The names now misrepresent what they hold (normal-context relation reads, not bulkreads), which will confuse future readers. Since the query semantics changed, rename the variables (e.g. stats_reads_before/stats_reads_after) and update the expected output accordingly. The after > before assertion itself remains sound: verify_heapam guarantees at least one relation read after the eviction, and reads is a monotonic counter, so concurrent normal-context reads cannot make it fail.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
This entire file (.gdbinit), together with the sibling additions .clangd and pg-aliases.sh, is a personal developer debugging/environment artifact and does not belong in a PostgreSQL patch. Patches destined for pgsql-hackers/commitfest must be minimal and contain only the changes required by the stated purpose; per-developer scratch files are a top rejection reason and cause needless merge conflicts. The project's .gitignore explicitly states such auxiliary files 'should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude', not committed. Additionally, a .gdbinit in the repo root is a footgun: GDB auto-loads ./.gdbinit from the working directory, so any developer launching gdb in the checkout silently executes these commands. Please drop this file (and its siblings) from the patch.
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These breakpoints reference symbols that do not exist anywhere in this change: heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_tuple_size, heap_hot_indexed_bitmap_raw_size, heap_hot_indexed_read_bitmap, heap_hot_indexed_bitmap_overlaps_raw, heap_hot_indexed_merge_bitmaps_raw, heap_hot_indexed_deserialize_bitmap, and heap_xlog_indexed_update (there is no XLOG_HEAP2_INDEXED_UPDATE WAL record either). A search of the tree returns matches only inside .gdbinit itself. When sourced, GDB will error on every unresolvable symbol, and the file documents a code structure (new helper functions, a new WAL record, a HEAP_INDEXED_UPDATED flag) that is absent from the actual diff. This indicates the debug file is stale/aspirational relative to the patch content.
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Line-number breakpoints are inherently fragile: they silently drift the moment any adjacent code changes. In fact heapam.c:3210 is not heap_update at all — in the current tree line 3210 is case TM_Ok: inside simple_heap_update's result switch. The embedded per-line annotations (e.g. 'Line 4019: pure HOT') are self-invalidating documentation of exactly the kind the project discourages. If this file were to be kept at all, breakpoints should be set by function name only.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
The header (and several other comment lines) use a non-ASCII em-dash character ('—'), which violates the project's ASCII-only rule for source and diffs. Use a plain ASCII hyphen/dash instead.
| # HOT Indexed Updates — GDB breakpoints for code review | |
| +# HOT Indexed Updates - GDB breakpoints for code review |
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This silently repurposes the user-visible usage_count column: it previously exposed the 0..BM_MAX_USAGE_COUNT usage counter but now returns only the 1-bit cooling state (0=COOL, 1=HOT). The SQL column name/type are unchanged, so existing queries and dashboards keep working but will now only ever see 0 or 1 -- a POLA/backward-compatibility surprise. This patch must be accompanied by an update to the pg_buffercache documentation (and expected regression output) describing the new semantics; consider whether the column should be renamed or the state exposed as text (HOT/COOL) instead of silently overloading usage_count. Confidence: high.
| @@ -0,0 +1,658 @@ | |||
| # PostgreSQL Development Aliases | |||
There was a problem hiding this comment.
This entire file does not belong in the PostgreSQL source tree. pg-aliases.sh is a collection of personal developer-environment aliases/functions, and it is bundled together with other developer-local artifacts (.clangd, .gdbinit) in a change set whose substantive work is unrelated vacuum/buffer/read-stream code. Note the accompanying .gdbinit even documents a different patch series ("HOT Indexed Updates"), confirming these are machine-specific tooling files.
This violates the minimal/atomic/single-purpose patch discipline required for a pgsql-hackers/commitfest submission: it bundles unrelated concerns, adds tree noise, and will cause needless merge conflicts and near-certain rejection. Keep this file local and untracked (e.g. via .git/info/exclude) rather than committing it. (high confidence)
| if [ "$last_compiler" != "$current_compiler" ] && [ "$last_compiler" != "unknown" ]; then | ||
| echo "Detected compiler change from $last_compiler to $current_compiler" | ||
| echo "Cleaning build directory..." | ||
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" |
There was a problem hiding this comment.
Destructive footgun: rm -rf "$build_dir" (and the trash before it) run against a variable that may be empty or unset. If PG_BUILD_DIR is unset, build_dir collapses to an empty string and the fallbacks operate on an unintended path. There is no guard (set -u, ${build_dir:?}, or an explicit non-empty check) before the destructive operation, risking silent, irreversible deletion. Guard with e.g. [ -n "$build_dir" ] || return 1 or ${build_dir:?build_dir is empty}. (high confidence)
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" | |
| [ -n "$build_dir" ] || { echo "build_dir is empty; refusing to delete" >&2; return 1; } | |
| trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" |
| alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' | ||
|
|
||
| # Database management | ||
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
There was a problem hiding this comment.
Same destructive footgun: rm -rf "$PG_DATA_DIR" with no check that PG_DATA_DIR is non-empty. If the variable is unset/empty, this can target unintended paths. These variables (PG_DATA_DIR, PG_INSTALL_DIR, PG_BUILD_DIR, etc.) are never defined or documented anywhere in this file, so operating on empty values is a realistic hazard. Guard the value before any rm -rf. (high confidence)
| --suppressions=$PG_SOURCE_DIR/src/tools/valgrind.supp \\ | ||
| --time-stamp=yes \\ | ||
| --log-file=$PG_BENCH_DIR/valgrind-%p.log \\ | ||
| $bindir/postgres "\$@" |
There was a problem hiding this comment.
The generated wrapper interpolates $PG_SOURCE_DIR, $PG_BENCH_DIR, and $bindir unquoted into a script written under a world-readable /tmp mkdtemp directory. Any path containing spaces or shell metacharacters will corrupt the generated wrapper (broken --suppressions=/--log-file= arguments, or unintended expansion when the wrapper runs). Quote the interpolated paths in the heredoc (e.g. "--suppressions=$PG_SOURCE_DIR/...", "$bindir/postgres"). (moderate confidence)
| pg_clean_for_compiler "$PG_BUILD_DIR" | ||
|
|
||
| echo "=== PostgreSQL Build Configuration ===" |
There was a problem hiding this comment.
This file depends on many environment variables (PG_BUILD_DIR, PG_BUILD_DIR_VALGRIND, PG_BUILD_DIR_ASAN, PG_INSTALL_DIR, PG_SOURCE_DIR, PG_DATA_DIR, PG_BENCH_DIR, PG_FLAME_DIR, PERL_CORE_DIR, CC, GDBINIT) that are never defined or documented in the file. Only PERL_CORE_DIR is checked, and inconsistently. Without a bootstrap/defaults section, the commands silently operate on empty/wrong paths, which directly compounds the rm -rf hazards above. The file is not self-contained. (moderate confidence)
|
|
||
| alias pg-start='ulimit -c unlimited && "$PG_INSTALL_DIR/bin/postgres" -D "$PG_DATA_DIR" -k "$PG_DATA_DIR"' | ||
|
|
||
| alias pg-stop='pkill -f "postgres.*-D.*$PG_DATA_DIR" || true' |
There was a problem hiding this comment.
pkill -f "postgres.*-D.*$PG_DATA_DIR" builds a regex from $PG_DATA_DIR. If that variable is empty, the pattern degrades to postgres.*-D.* and can match/kill unrelated postgres processes; if it contains regex-special characters, the match is unpredictable. Anchor/escape the pattern and guard against an empty PG_DATA_DIR before killing. (moderate confidence)
| Buffer wbuf, IndexTuple *itups, OffsetNumber *itup_offsets, | ||
| Size *tups_size, uint16 nitups, | ||
| BufferAccessStrategy bstrategy) | ||
| Size *tups_size, uint16 nitups) |
There was a problem hiding this comment.
Stale doc comment: the header comment above still says "Since this function is invoked in VACUUM, we provide an access strategy parameter that controls fetches of the bucket pages." This diff removes the bstrategy parameter, so that sentence no longer matches the signature. Remove the now-inaccurate access-strategy note. (high confidence)
| BlockNumber bucket_blkno, | ||
| Buffer bucket_buf, | ||
| BufferAccessStrategy bstrategy) | ||
| Buffer bucket_buf) |
There was a problem hiding this comment.
Two stale comments in this function are now inaccurate after removing the bstrategy parameter: (1) the header comment "Since this function is invoked in VACUUM, we provide an access strategy parameter that controls fetches of the bucket pages." and (2) the inline note at the "Find the last page in the bucket chain" loop: "Note: we assume that a hash bucket chain is usually smaller than the buffer ring being used by VACUUM, else using the access strategy here would be counterproductive." Neither an access-strategy parameter nor a VACUUM buffer ring exists here anymore. Update/remove both to reflect the current code. (high confidence)
| */ | ||
| buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, | ||
| info->strategy); | ||
| buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL); |
There was a problem hiding this comment.
This change removes the info->strategy argument, so ReadBufferExtended no longer uses a nondefault buffer access strategy here. The immediately preceding comment ("Also, we want to use a nondefault buffer access strategy.") is now stale and contradicts the code. Since the buffer access strategy is now handled elsewhere by this refactoring, remove or update that sentence so the comment describes what the code does now.
| else | ||
| elevel = DEBUG2; | ||
|
|
||
| /* Set up static variables */ |
There was a problem hiding this comment.
This comment is now orphaned: vac_strategy = bstrategy; was the only "static variable" it introduced, and that assignment was removed just above. A comment that describes nothing is stale and should go, per minimal-diff/comment-accuracy discipline. Remove the /* Set up static variables */ line (and the now-doubled blank line) so the code reads as if it had always been written this way.
Confidence: high.
| /* | ||
| * Admit the newly loaded page COOL (probation); a second access via | ||
| * PinBuffer promotes it to HOT. This is what makes a one-touch scan | ||
| * self-evicting -- see the cooling-state notes in buf_internals.h. | ||
| */ | ||
| if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM) |
There was a problem hiding this comment.
This comment sits directly above the BM_PERMANENT logic, but describes the COOL admission policy. The actual change that admits the page COOL is the removal of BUF_USAGECOUNT_ONE on the set_bits |= BM_TAG_VALID; line above (a COOL page is now the absence of the promotion). As placed, the comment annotates unrelated permanence handling and will mislead future readers. Move it to immediately above set_bits |= BM_TAG_VALID;, or reword to explain that COOL admission is implied by not setting the cooling bit. (confidence: high)
| /* | ||
| * Admit COOL (probation); see the comment at the other admission | ||
| * site and the cooling-state notes in buf_internals.h. | ||
| */ | ||
| if (bmr.relpersistence == RELPERSISTENCE_PERMANENT || fork == INIT_FORKNUM) |
There was a problem hiding this comment.
Same comment-placement problem as in BufferAlloc: this "Admit COOL" note sits above the BM_PERMANENT conditional rather than above the set_bits |= BM_TAG_VALID; line that actually performs the COOL admission (by omitting the old BUF_USAGECOUNT_ONE). Reposition so the comment annotates the code it describes. (confidence: high)
| write_limit = bgwriter_lru_maxpages; | ||
| if (upcoming_alloc_est > write_limit) | ||
| write_limit = upcoming_alloc_est; |
There was a problem hiding this comment.
This silently overrides the operator-configured bgwriter_lru_maxpages upper bound whenever upcoming_alloc_est exceeds it. bgwriter_lru_maxpages is documented as a hard cap on the number of buffers the bgwriter writes per round; exceeding it is a POLA violation and a potential I/O footgun on constrained systems. This needs (a) a documentation update to config.sgml describing the new demand-driven behavior, and (b) a reproducible benchmark justifying the change per pgsql-hackers norms. Note the maxwritten_clean statistic below now also increments against write_limit rather than the configured cap, changing its meaning. (confidence: high)
| if (BUF_STATE_GET_REFBIT(buf_state)) | ||
| { | ||
| /* second chance: consume the ref bit, stay HOT */ | ||
| UnlockBufHdrExt(bufHdr, buf_state, 0, BUF_REFBIT, 0); | ||
| buf_state = LockBufHdr(bufHdr); | ||
| } | ||
| else | ||
| { | ||
| /* not re-accessed since last pass: demote to COOL */ | ||
| UnlockBufHdrExt(bufHdr, buf_state, 0, BUF_USAGECOUNT_MASK, 0); | ||
| buf_state = LockBufHdr(bufHdr); | ||
| result |= BUF_COOLED; | ||
| } |
There was a problem hiding this comment.
The pre-cooling block acquires the header lock, then in each branch calls UnlockBufHdrExt(...) (which releases the lock via its CAS) and immediately re-acquires it with LockBufHdr(bufHdr) to continue the dirty-write inspection. This adds a second full header-spinlock acquire/release round-trip per unpinned HOT buffer on the bgwriter's hot LRU scan, roughly doubling header-lock traffic for every cooled/reprieved buffer. Since the lock is already held here, the ref-bit clear or HOT->COOL demotion can be folded into the existing single UnlockBufHdr path at the end of the reusable/clean checks (or the local buf_state can be mutated and applied once), avoiding the extra lock churn. (confidence: high)
| int autovacuum_max_parallel_workers = 0; | ||
| int MaxBackends = 0; | ||
|
|
||
| /* GUC parameters for vacuum */ |
There was a problem hiding this comment.
Removing VacuumBufferUsageLimit leaves the /* GUC parameters for vacuum */ comment followed by a stray blank line before VacuumCostPageHit. Drop the now-empty line so the comment sits directly above the declarations it describes and the diff stays minimal/pgindent-clean.
Confidence: high.
| /* GUC parameters for vacuum */ | |
| /* GUC parameters for vacuum */ | |
| int VacuumCostPageHit = 1; |
| #include "storage/proc.h" | ||
| #include "storage/shmem.h" | ||
| #include "storage/subsystems.h" | ||
| #include "port/pg_numa.h" |
There was a problem hiding this comment.
Include out of order. PostgreSQL sorts includes alphabetically within a group; port/pg_numa.h is placed after the storage/* block instead of in the port/* group right after port/atomics.h. Move it up so git diff/pgindent conventions and the surrounding block stay sorted.
| * memory rather than a backend-local static so that EXEC_BACKEND children | ||
| * (which do not inherit the postmaster's statics) see the same value; a | ||
| * backend-local copy would silently reset to 1 there, disabling batching | ||
| * on Windows. |
There was a problem hiding this comment.
This rationale is inaccurate and will mislead. StrategyCtlShmemInit is registered as the subsystem .init_fn (StrategyCtlShmemCallbacks) and runs once during shared-memory initialization, not per backend. A backend-local static would not "silently reset to 1 [on Windows]" as a consequence of EXEC_BACKEND missing statics -- it would simply never be initialized by the postmaster's run of the init function. The real reason to keep batchSize in shared memory is that it is computed once at init and read by all backends. Please correct the comment to state that, and drop the EXEC_BACKEND/Windows claim.
|
📊 OCR posted 24/25 inline comment(s). 1 could not be posted
|
Replace the 0..5 usage_count buffer-replacement policy with a cooling-stage
clock (the LeanStore / 2Q-A1 model): a buffer is simply HOT (recently used)
or COOL (an eviction candidate), with "pinned" being the existing refcount.
There is no per-buffer access counter.
- A demand-loaded page is admitted COOL (probationary), not HOT. A second
access via PinBuffer promotes it COOL -> HOT (the rescue). So a page
touched once -- a sequential scan -- fills and drains the COOL stage and
is evicted from it without ever displacing the HOT working set. Scan
resistance is intrinsic to the replacement algorithm, which is what lets a
later commit remove the BufferAccessStrategy ring buffers entirely.
- The foreground sweep in StrategyGetBuffer() reclaims an already-COOL,
unpinned buffer, pinning it with a CAS so a racing PinBuffer always wins.
Promotion (COOL -> HOT) and demotion (HOT -> COOL) are single-bit
transitions; only the eviction claim is a CAS.
- The background writer maintains the supply of COOL victims. As its LRU
scan runs ahead of the clock hand it demotes HOT buffers to COOL, so the
foreground finds a victim in a single pass rather than having to cool
buffers itself. The demotion is demand-driven -- bounded by the predicted
allocation for the next cycle -- so it stages just enough COOL buffers
without cooling the whole pool, and it is done under the buffer header
lock the scan already holds. A single second-chance reference bit
(set by PinBuffer, cleared on the bgwriter's first pass over a HOT buffer)
spares a recently-accessed buffer one cooling pass, keeping the genuinely
hot set out of the COOL stage under scan pressure. BgBufferSync also
tracks the reusable-buffer density it directly observes on a shorter
smoothing window, so a burst of probationary/scan COOL pages is followed
promptly rather than averaged away. Under a bulk-dirtying workload the
per-cycle clean-write cap is raised from bgwriter_lru_maxpages to predicted
demand so the bgwriter keeps supplying clean victims, rather than the
foreground sweep having to flush dirty victims inline; normal workloads,
where demand is below the cap, are unaffected.
The 4-bit usage_count field is reinterpreted in place: bit 0 is the HOT/COOL
state (BUF_COOLSTATE_ONE), bit 1 the reference bit (BUF_REFBIT). The 64-bit
buffer-state layout -- refcount, flag and lock offsets and their StaticAsserts
-- is unchanged; only the meaning of the field and the instructions that touch
it change. A StaticAssert requires the field to be at least 2 bits wide so a
future width change cannot push the reference bit into the flag bits.
BM_MAX_USAGE_COUNT becomes BUF_COOLSTATE_HOT (1), so the pin fast path
saturates at HOT. Local (temp-table) buffers get the same two-state
treatment; being single-backend they need no background cooler.
Because the reference bit shares the field, the full 4-bit value can be 0..3;
readers that mean "the cooling state" must use BUF_STATE_GET_COOLSTATE(), which
masks to bit 0 and returns only 0 (COOL) or 1 (HOT), never the raw field.
contrib/pg_buffercache is updated accordingly: its usagecount column and the
pg_buffercache_summary average report the cooling state (0 = COOL, 1 = HOT),
and pg_buffercache_usage_counts() buckets on it. Using the raw 4-bit getter
there would index its BM_MAX_USAGE_COUNT+1 = 2-element arrays with values up
to 3 and overrun the stack; masking to the cooling bit keeps the index in
range. The reference bit is deliberately not exposed as usagecount.
Depends on the batched clock sweep from the previous commit.
There was a problem hiding this comment.
🔍 OCR found 32 issue(s).
- 25 inline, 7 in summary (inline capped at 25)
📄 src/backend/storage/buffer/freelist.c (L345-L345)
These tab-aligned trailing comments on the two if/else assignment statements are long and hand-aligned; pgindent will re-flow trailing comments and this is likely to change on the next pgindent run, producing whitespace churn. Verify pgindent/git diff --check is clean, or move the explanations into the block comment above rather than aligning them at end-of-line.
📄 src/include/storage/buf_internals.h (L91-L100)
This comment attributes the second-chance ref-bit handling to "The bgwriter's pre-cooling", but that path is not wired in the shipped code: both callers of SyncOneBuffer() pass cool_if_hot=false (bufmgr.c:3790 and :4112), so the bgwriter never sets/clears the ref bit or demotes HOT->COOL. The ref bit is actually consumed by the foreground clock sweep in StrategyGetBuffer() (freelist.c). Rewrite the comment to describe the mechanism that actually runs (the foreground sweep), otherwise it describes behavior that does not exist. (high confidence)
📄 src/include/storage/buf_internals.h (L80-L84)
This describes a prefer-then-demote sweep that demotes HOT "only when a full pass finds no COOL victim", but the shipped foreground sweep in StrategyGetBuffer() (freelist.c) is COOL-IN-PLACE: it demotes a HOT buffer (respecting the ref bit) on every tick it passes one, and does not claim it the same tick. The header comment therefore contradicts the actual algorithm and the freelist.c comment. Please make the two descriptions consistent. (high confidence)
📄 src/include/storage/buf_internals.h (L191-L191)
Reusing the name BM_MAX_USAGE_COUNT for what is now a single HOT/COOL bit (value 1, not a 0..5 count) is a maintainability/POLA footgun. The name still reads as a usage-count ceiling, misleading future readers and any out-of-tree/extension code that references it, and it is now (mis)used to size arrays in pg_buffercache. Consider renaming to something the reflects the new meaning (e.g. BUF_COOLSTATE_HOT everywhere) rather than keeping the misleading alias. (moderate confidence)
📄 src/include/storage/buf_internals.h (L139-L140)
Comment style: this block comment starts descriptive text on the opening "/" line, which is inconsistent with the PostgreSQL multi-line comment convention used throughout this file (open "/" on its own line, text on subsequent lines). Reformat to match so it pgindents/reads cleanly. (low 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/test/regress/pg_regress.c (L1246-L1246)
This execl -> execlp change does not belong in this patch. The rest of the change set is a buffer-manager (cooling-stage clock sweep) change; this hunk is unrelated and should be dropped to keep the diff minimal (unrelated churn is a top rejection reason on -hackers).
Substantively it is also either a no-op or a footgun, not an improvement. shellprog is SHELLPROG, defined as /bin/sh (meson) or $(SHELL) (make, normally /bin/sh) -- both contain a slash. execlp() performs a PATH search only when its filename argument contains no slash, so for the paths actually used here it behaves identically to execl(). The only case in which it differs is if SHELLPROG were ever a bare name, where execlp() would resolve the test shell via the environment PATH -- a non-deterministic, unintended behavior change for the regression harness. Revert to execl(). (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)
This is the only Perl script in the entire tree that would use #!/usr/bin/env perl; every other Perl script (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.) uses the canonical #!/usr/bin/perl. This one-line divergence breaks project convention and reads as unrelated churn that doesn't belong in the accompanying buffer-manager change set. If the intent is to run under a non-system Perl, that is normally handled via PROVE/PERL build variables rather than editing the shebang of a single script. Revert to #!/usr/bin/perl to keep the diff minimal and consistent, or submit this as a separate, tree-wide, justified change with its own -hackers discussion.
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
| forknum = BufTagGetForkNum(&bufHdr->tag); | ||
| blocknum = bufHdr->tag.blockNum; | ||
| usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
This is a user-visible SQL behavior change that is not documented in this patch. The usagecount column of the pg_buffercache view is defined and documented as a 0..5 clock-sweep usage count. After this change it silently reports only 0 or 1 (the new HOT/COOL cooling bit), because BM_MAX_USAGE_COUNT is now BUF_COOLSTATE_HOT (1). The column name and its documentation (doc/src/sgml/pgbuffercache.sgml) still describe the old 0..5 semantics, so existing user queries/dashboards that interpret usagecount as a 0..5 popularity score will now be wrong with no warning. This must be accompanied by a doc update, and per PostgreSQL convention a change to the meaning of an existing extension-exposed column warrants a new pg_buffercache extension version rather than an in-place semantic redefinition. (moderate confidence)
| { | ||
| buffers_used++; | ||
| usagecount_total += BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usagecount_total += BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
Same user-visible contract break here: usagecount_avg in pg_buffercache_summary() was the average of a 0..5 usage count; it now averages a 0/1 cooling bit, so its value range and meaning change silently. The summary function's documentation is not updated in this patch to reflect that the field is now effectively the fraction of HOT buffers. Update the docs (and consider whether the column should be renamed / versioned) so callers are not misled. (moderate confidence)
| CHECK_FOR_INTERRUPTS(); | ||
|
|
||
| usage_count = BUF_STATE_GET_USAGECOUNT(buf_state); | ||
| usage_count = BUF_STATE_GET_COOLSTATE(buf_state); |
There was a problem hiding this comment.
pg_buffercache_usage_counts() is documented to aggregate buffers "over the possible usage count values" (0..5). With this change it can only ever emit two rows (usage_count 0 and 1), which is a user-visible change in the result set and its meaning. The array sizing is safe (BM_MAX_USAGE_COUNT is now 1 and BUF_STATE_GET_COOLSTATE returns only 0/1, so usage_counts[usage_count] stays in bounds), but the SQL/doc contract for this function needs updating to describe the new HOT/COOL semantics. (moderate confidence)
| - -I. | ||
| - -I../../../../src/include |
There was a problem hiding this comment.
These -I paths are resolved by clangd relative to each translation unit's own directory, not the repo root. -I../../../../src/include only points at <repo>/src/include for source files that happen to sit exactly four directories deep (e.g. src/backend/storage/buffer/*.c). For files at other depths — e.g. contrib/pg_buffercache/*.c (two deep) or top-level files — it resolves to a non-existent directory, so clangd will fail to find core headers and produce spurious diagnostics for those TUs. Since CompilationDatabase: build/ already supplies correct per-file -I flags from compile_commands.json, these hardcoded relative includes are both fragile and redundant; consider dropping them (or using an absolute/${workspaceFolder}-style path) so the fallback works uniformly across the tree.
| # HOT Indexed Updates — GDB breakpoints for code review | ||
| # | ||
| # Usage: gdb -x .gdbinit <postgres-binary> |
There was a problem hiding this comment.
This entire file is a personal debugging artifact and must not be committed to the PostgreSQL tree. GDB auto-loads .gdbinit from the working directory, so committing it is also a footgun (auto-executed setup for anyone building in this dir). Along with the sibling .clangd and pg-aliases.sh, this violates the minimal-diff rule: a patch destined for pgsql-hackers/commitfest must contain only the changes required by the stated feature. This file (and its siblings) should be moved to a personal/global ignore list or removed from the change set entirely.
Confidence: high.
| /* second chance: consume the ref bit, stay HOT */ | ||
| UnlockBufHdrExt(bufHdr, buf_state, 0, BUF_REFBIT, 0); | ||
| buf_state = LockBufHdr(bufHdr); |
There was a problem hiding this comment.
This pre-cool path takes the header lock, unlocks via UnlockBufHdrExt, then immediately re-locks with LockBufHdr solely to continue the dirty/reusable inspection - two extra lock round-trips per bgwriter tick over a HOT buffer. Since you already hold the lock and hold the old buf_state, prefer a lock-preserving update (or fold the ref-bit clear / HOT->COOL demotion into the single UnlockBufHdrExt already done later on the write path) to avoid the extra CAS-loop and lock churn on this scan hot path. The eligibility decision below correctly re-reads the relocked buf_state, so there is no TOCTOU bug, but the churn is avoidable. (moderate confidence)
| else if (start + batch_size > (uint32) NBuffers) | ||
| { |
There was a problem hiding this comment.
The comment's rationale is inaccurate: it claims completePasses would only "drift low" without this branch, but this else-if path can actually cause it to drift high (over-count). Interleaving: backend A's fetch-add returns start=NBuffers-8 (< NBuffers, batch spans wrap) and increments completePasses here; the counter is now past NBuffers, so backend B's fetch-add returns >= NBuffers and the wrap-CAS path (line ~176) increments completePasses again for the same pool-boundary crossing. Meanwhile StrategySyncStart also adds nextVictimBuffer/NBuffers before the CAS resets the counter, so the crossing is briefly triple-counted. This only affects bgwriter pacing (not victim-selection correctness), but the stated reasoning is wrong and should be corrected, or the two spinlock sections should coordinate on who owns the pass boundary.
| #include "storage/proc.h" | ||
| #include "storage/shmem.h" | ||
| #include "storage/subsystems.h" | ||
| #include "port/pg_numa.h" |
There was a problem hiding this comment.
Include ordering: PostgreSQL groups includes alphabetically within a block. "port/pg_numa.h" is appended after the storage/* headers; it belongs immediately after "port/atomics.h". pgindent/headerscheck won't fix ordering, so move it to keep the block sorted.
| /* HOT: give it a second chance, cool it and keep scanning. */ | ||
| buf_state -= BUF_COOLSTATE_ONE; |
There was a problem hiding this comment.
Nit (low, maintainability): the parallel change in freelist.c performs the HOT->COOL demotion with a masked clear (local_buf_state &= ~BUF_USAGECOUNT_MASK), whereas here it is done arithmetically with -= BUF_COOLSTATE_ONE. Both are correct in this branch (the != BUF_COOLSTATE_COOL guard guarantees bit 0 is set, so the subtraction clears bit 0 without borrowing), but using two different idioms for the same conceptual operation across the two sweep implementations invites confusion for future readers. Consider matching freelist.c's masked clear for consistency.
| /* 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; |
| StrategyControl->batchSize = Min(PG_CACHE_LINE_SIZE / (uint32) sizeof(uint32), | ||
| (uint32) NBuffers); |
There was a problem hiding this comment.
This freelist.c change bundles two orthogonal features in one hunk: (a) NUMA-gated clock-hand batching (batchSize/ClockSweepTick), and (b) a full replacement of the usage_count second-chance algorithm with the cooling-state model. These are independently reviewable and independently committable; per pgsql-hackers patch hygiene they should be split into separate, atomic, bisectable commits (each must build and pass tests on its own). Also, the comment asserts a performance win ("Benchmarking showed the win holds with or without huge pages") without a reproducible benchmark or a design-thread Message-Id; the batching gate is a non-trivial change that needs that reference.
1 participant
Cooling-stage clock sweep for the buffer manager — CI check.
This PR is against gburd/postgres:master (my fork), not upstream, purely
to exercise CI on the three-patch series before posting to pgsql-hackers.
Net +510/-1642 over 83 files (almost all deletion is patch 3).
Each commit builds -Werror + cassert + injection_points and passes
regress/regress(245 tests) independently; verified on both the fork baseand a clean upstream/master rebase. See the draft cover letter for the
design reasoning and the m6i / r8i (6-node NUMA) / huge-pages / local-NVMe
real-IO benchmark data.
Not for merge — this is the review/CI vehicle for the -hackers submission.