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)No dedicated on-list thread for a "cooling-stage / HOT-COOL evictor" redesign. That confirms the cooling-stage evictor (commit 898e100) is the PR author's own extension beyond what's on the mailing list — the list thread is specifically about the batched-sweep contention fix. I have enough to write the report. 🧵 Related discussion
🔗 Related commits / prior art
📋 Commitfest
🧭 Context for reviewers
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
| 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 throttle: whenever upcoming_alloc_est exceeds it, the per-cycle write cap is raised to demand. That changes the documented meaning of the GUC (a hard cap on bgwriter LRU writes) and can drive large write-I/O bursts under bulk-dirtying workloads (POLA/back-compat violation). No GUC/doc change accompanies it and no benchmark is cited. Additionally, the justifying comment's claim that "normal workloads are unaffected because there upcoming_alloc_est <= bgwriter_lru_maxpages" is incorrect: the floor just above (upcoming_alloc_est = min_scan_buffers + reusable_buffers_est) is computed independently of bgwriter_lru_maxpages, so upcoming_alloc_est can exceed the cap even on an idle/normal pool. Confidence: high.
| * a shorter window than the strategy proxy, tracking a burst of | ||
| * probationary/scan COOL pages within a cycle or two instead of lagging it. | ||
| */ | ||
| float cleaner_smoothing_samples = 4; |
There was a problem hiding this comment.
Hard-coded tuning constant (4) that halves-and-then-some the smoothing window only for the cleaner-density path, with no GUC and no benchmark. A much shorter smoothing window makes smoothed_density oscillate and can cause bgwriter over/under-provisioning; the community will require a reproducible benchmark or a configurable value before accepting this. Confidence: medium.
| /* 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. */ |
There was a problem hiding this comment.
Comment block style deviates from PostgreSQL convention (text on the /* opening line and dangling */), which will not survive pgindent cleanly. Use the standard multi-line block form with the leading /* on its own line. Same issue at the ExtendBufferedRelShared admission site below. Confidence: low.
| /* 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. */ | |
| /* | |
| * 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. | |
| */ |
| * makes cooling progress, the sweep is self-staging: it never needs a | ||
| * background pre-cooler to keep COOL victims available, and it cannot get | ||
| * stuck in a "no COOL victim anywhere" state that forces a foreground | ||
| * escalation pass. Newly loaded pages are admitted COOL (see |
There was a problem hiding this comment.
This comment contradicts the code as actually implemented. It asserts the sweep "is self-staging: it never needs a background pre-cooler to keep COOL victims available." But bufmgr.c does implement a bgwriter pre-cooler (the cool_if_hot / BUF_COOLED path in the sync-buffer logic), and buf_internals.h's own comments describe the second-chance ref bit as being consumed by "the bgwriter's pre-cooling" / "the bgwriter's next cooling pass." The foreground sweep and the bgwriter both act on the ref bit, so the pre-cooler is part of the design, not unnecessary. Reconcile these comments so they describe one consistent design; as written they will mislead reviewers about whether the bgwriter cooling path is required.
| #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 violates the project convention (alphabetical within the block). "port/atomics.h" is already at the top of the block; "port/pg_numa.h" should be grouped with it, not appended after the storage/* includes. Move it up next to port/atomics.h.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This change violates the project-wide convention and is unrelated to the rest of the patch. Every other Perl script in the PostgreSQL tree (dozens of them across contrib/, doc/, src/backend/, src/common/, src/include/, src/interfaces/, src/pl/, and src/tools/) uses the fixed #!/usr/bin/perl shebang. Switching this single file to #!/usr/bin/env perl makes it the lone outlier, breaking consistency for no stated reason. It is also unrelated churn to a patch whose other files concern the buffer manager/freelist; unrelated, non-minimal changes like this should be dropped so the diff stays focused. Revert to #!/usr/bin/perl.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| * max") reads naturally. | ||
| */ | ||
| #define BM_MAX_USAGE_COUNT 5 | ||
| #define BM_MAX_USAGE_COUNT BUF_COOLSTATE_HOT |
There was a problem hiding this comment.
Footgun: BM_MAX_USAGE_COUNT is retained as an alias for BUF_COOLSTATE_HOT, but the name literally claims a "max usage count" that no longer exists — the field is now a 1-bit HOT/COOL state. This is misleading for future readers and for out-of-tree code that still references the exported name. The retained name also leaks into contrib/pg_buffercache where it sizes the usage_counts[BM_MAX_USAGE_COUNT + 1] arrays, so the confusion propagates. Prefer renaming to something that reflects the new semantics (e.g. drop the alias and use BUF_COOLSTATE_HOT directly at the pin fast path, or introduce BM_MAX_COOLSTATE) rather than keeping a stale name purely because it "reads naturally".
| * A demand-loaded page is admitted COOL (probation); a second access promotes | ||
| * it to HOT (the rescue). The sweep prefers COOL victims and demotes HOT to | ||
| * COOL only when a full pass finds no COOL victim. So a one-touch scan fills | ||
| * and drains the COOL stage without displacing the HOT working set -- scan | ||
| * resistance intrinsic to the replacement algorithm. |
There was a problem hiding this comment.
These comment blocks read as design-thread / commit-message narrative rather than terse header documentation: prose like "the rescue", "scan resistance intrinsic to the replacement algorithm", and the detailed bgwriter pre-cooling walkthrough describes the algorithm's behavior and motivation at length. PostgreSQL header comments should explain the field/macro tersely (the why), and the algorithm rationale belongs in the commit message and/or the implementation site (bufmgr.c/freelist.c). Consider trimming this to a short description of what the bit encodes and pointing to the sweep code for the full rationale.
|
📊 OCR posted 20/21 inline comment(s). 1 could not be posted
|
| - -I. | ||
| - -I../../../../src/include |
There was a problem hiding this comment.
This hardcoded relative include path is fragile. Since .clangd lives at the repository root and CompilationDatabase: build/ is set, real include paths should come from build/compile_commands.json. -I../../../../src/include only resolves for a source file located exactly four directory levels below the repo root; for files at any other depth it points outside the tree (or nowhere), so it either does nothing or resolves to an unintended directory. Likewise -I. is resolved relative to each translation unit's directory, not the repo root, so it does not reliably add the source root. Recommend dropping both hardcoded -I entries and relying on the compilation database, or using an absolute path.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
This entire file should not be committed. .gdbinit at the repo root is a personal developer debugging artifact, not part of a PostgreSQL patch. It is also a footgun/security hazard: GDB auto-loads .gdbinit from the current working directory (subject to auto-load safe-path), so committing it can silently execute debugger commands for anyone who runs gdb in this checkout. Keep this local (developer's global gdbinit or a gitignored file) and drop it from the patch. This will otherwise draw an immediate rejection on -hackers and violates the minimal-diff discipline.
| break heap_hot_indexed_tuple_size | ||
|
|
||
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These symbols do not exist anywhere in the tree. A codebase search for heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_read_bitmap, heap_hot_indexed_bitmap_raw_size, heap_hot_indexed_merge_bitmaps_raw, heap_xlog_indexed_update, and the HEAP_INDEXED_UPDATED / XLOG_HEAP2_INDEXED_UPDATE identifiers returns matches only inside this .gdbinit. The referenced core commits are absent from this change, so the file documents behavior that is not present in the branch (breaking the atomic/bisectable-commit rule). Confidence: high.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Hardcoded source line-number breakpoints (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) are extremely brittle: they become wrong the moment any line above them is added or removed, and the accompanying comments ('pure HOT', 'HEAP_INDEXED_UPDATED flag', 'redirect-with-data') are unverifiable against the tree since the corresponding feature code is not present. This reinforces that the file should be dropped rather than maintained.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
Non-ASCII em-dash character ('—') violates the ASCII-only-in-source discipline. This appears in the header line and repeatedly throughout the file (e.g., subsystem headers). Since the file should be removed entirely this is moot, but if any comparable file were kept it must use ASCII ('-' or '--').
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This execl -> execlp change is unrelated to the buffer-manager cooling-state work that is the subject of this patch series (bufmgr.c/freelist.c/localbuf.c/buf_internals.h). Unrelated changes bundled into a patch are a top rejection reason on pgsql-hackers -- split this out or drop it. (high confidence)
Beyond being off-topic, the change is a functional no-op that introduces a latent footgun. shellprog is SHELLPROG, defined as /bin/sh (src/test/regress/meson.build) or $(SHELL) (src/test/regress/GNUmakefile) -- always an absolute path containing a slash. execlp only performs a PATH search when its file argument contains no slash, so with an absolute path it behaves identically to execl; nothing is gained. If SHELLPROG were ever a bare name, execlp would silently search PATH and could exec an attacker-controlled shell -- a security regression that the original execl prevented. Revert to execl. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This changes the shebang from #!/usr/bin/perl (the convention used by every other Perl script in the tree) to #!/usr/bin/env perl, making pgindent the sole outlier. This is an unrelated, non-minimal change with no stated rationale or -hackers discussion, and it breaks project-wide consistency for the tool that itself enforces uniform style. Unless there is a documented reason to switch, keep #!/usr/bin/perl to match the rest of the source tree.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| StaticAssertDecl(BM_MAX_USAGE_COUNT < (UINT64CONST(1) << BUF_USAGECOUNT_BITS), | ||
| "BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits"); | ||
| "cooling state doesn't fit in BUF_USAGECOUNT_BITS bits"); |
There was a problem hiding this comment.
This StaticAssert is now nearly vacuous and its message is misleading. With BM_MAX_USAGE_COUNT == BUF_COOLSTATE_HOT == 1, the test degenerates to 1 < (1 << BUF_USAGECOUNT_BITS), i.e. it only requires BUF_USAGECOUNT_BITS >= 1. But the field now stores TWO bits (cooling state at bit 0, BUF_REFBIT at bit 1), so the real invariant is BUF_USAGECOUNT_BITS >= 2, which is already asserted above. This assert no longer guards against BUF_REFBIT colliding with the flag bits and its message ("cooling state doesn't fit") overstates what it checks. Consider dropping it (the new >= 2 assert supersedes it) or rewording it to reflect that only the cooling-state bit is verified here. (moderate confidence)
| /* 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. */ |
There was a problem hiding this comment.
Block-comment style deviates from the prevailing convention in this file. Every other block comment here (e.g. the Cooling state and Second-chance reference bit blocks just above, and the BM_MAX_USAGE_COUNT block below) opens with /* on its own line. Start the comment text on the following line for pgindent/style consistency. (high confidence)
| /* 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. */ | |
| /* | |
| * 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. | |
| */ |
| * max") reads naturally. | ||
| */ | ||
| #define BM_MAX_USAGE_COUNT 5 | ||
| #define BM_MAX_USAGE_COUNT BUF_COOLSTATE_HOT |
There was a problem hiding this comment.
Retaining the name BM_MAX_USAGE_COUNT for what is now a boolean HOT flag (value 1) is a POLA/maintainability hazard. The comment concedes this, but the name still leaks: e.g. pg_buffercache_pages.c sizes histogram arrays as usage_counts[BM_MAX_USAGE_COUNT + 1] and UnlockBufHdrExt's comment (line 520-521) still speaks of "cap the usagecount at BM_MAX_USAGE_COUNT", both of which now read as stale artifacts of the 0..5 count. A clearer name (e.g. BM_COOLSTATE_HOT, already available as BUF_COOLSTATE_HOT) would make callers self-documenting and avoid future readers assuming a multi-value range. (moderate confidence)
70421e4 to
576fa38
Compare
Keeps gburd/postgres rebased hourly on postgres/postgres master with only .github/ changes on top (sync-upstream automatic + manual). Drops the bespoke Windows dependency-builder workflow: Windows is already built and tested in CI by upstream's pg-ci.yml (Visual Studio + MinGW meson jobs), so a separate dependency prebuild that nothing consumed was redundant.
The Open Code Review system: ocr-review and ocr-model-check workflows plus .github/ocr config (LiteLLM->Bedrock Claude Opus 4.8, rule.json, context.md, pg-history.py).
The default GITHUB_TOKEN is blocked by GitHub from pushing commits that touch .github/workflows/, which broke the auto-sync push step once the fork carried its own workflow files. Use a PAT with repo+workflow scope (SYNC_TOKEN secret), falling back to GITHUB_TOKEN when unset.
Silences the Node 20 deprecation warning on the auto-sync runs.
This isn't a commit that will be submitting for review, it is purely for local developer tooling while developing this patch set. Ignore it.
StrategyGetBuffer() advances the shared clock hand, nextVictimBuffer, with a pg_atomic_fetch_add_u32(..., 1) on every tick. On a multi-socket system the cache line holding that counter has to travel over the interconnect on each operation, pushing a sweep tick from ~20ns (same-socket, line warm in L1/L2) into the ~100-200ns range. Under eviction pressure with hundreds of backends in StrategyGetBuffer() concurrently, that single cache line becomes the dominant cost of the sweep, visible as elevated bus-cycles and cache-misses in a perf profile. Have each backend claim a run of consecutive buffer IDs from the shared hand with a single fetch-add and then iterate through them privately. The sweep still advances through the pool in order, each buffer is still visited exactly once per complete pass, and the meaning of the clock state is unchanged; only the temporal ordering of visits within a pass changes, which the algorithm does not depend on. The contended atomic now fires roughly once per batch rather than once per buffer. The batch is one cache line's worth of clock-hand values -- PG_CACHE_LINE_SIZE / sizeof(uint32) -- capped at NBuffers so a claim can never wrap the pool more than once. Batching only helps when the counter's cache line actually bounces between sockets, so it is enabled only on multi-node NUMA hardware (pg_numa_get_max_node() >= 1); on a single socket, or where libnuma is unavailable, the batch size stays 1 and the code path is byte-identical to the stock clock sweep. Wraparound handling is adjusted: with batching, several backends can each see a fetch-add return a value past NBuffers within the same pass. Any such backend takes buffer_strategy_lock, re-reads the counter, and if it is still out of range wraps it with a single CAS and increments completePasses. StrategySyncStart() continues to see a consistent (nextVictimBuffer, completePasses) pair. This is the batched-clock-sweep idea from Jim Mlodgenski's pgsql-hackers thread, adapted to derive the batch size from the platform cache-line size. Co-authored-by: Jim Mlodgenski <mlodj@amazon.com>
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 26 issue(s).
- 25 inline, 1 in summary (inline capped at 25)
📄 src/backend/storage/buffer/freelist.c (L24-L25)
Include is out of alphabetical order. PostgreSQL sorts includes alphabetically within a block; port/pg_numa.h belongs immediately after port/atomics.h (line 19), before the storage/* headers, not appended at the end. Reviewers on -hackers routinely flag this.
💡 Suggested change
Before:
#include "storage/subsystems.h"
#include "port/pg_numa.h"
After:
#include "storage/subsystems.h"
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This entire file should not be part of the patch. PostgreSQL's own top-level .gitignore states the policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." A .clangd file is precisely such an editor/tooling artifact (as are the companion .gdbinit and pg-aliases.sh files in this changeset). Committing personal IDE configuration is unrelated churn that will draw an immediate rejection on pgsql-hackers. Keep it in a local exclude instead of tracking it. [high confidence]
| 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.
Correctness / backward-compat (high confidence): this silently changes the semantics of the user-visible usage_count column of pg_buffercache_pages. Previously it reported 0..BM_MAX_USAGE_COUNT (0..5); it now reports only the HOT/COOL bit (0 or 1), because BUF_STATE_GET_COOLSTATE masks bit 0 only and discards the second-chance ref bit (bit 1). Existing queries/monitoring that interpret usage_count as the old clock-sweep counter break with no error and no cue. A user-visible interface change like this needs the column renamed (e.g. cool_state) or at least documented in pg_buffercache.sgml, plus regression-test updates; none are in this patch.
| { | ||
| 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.
Correctness (high confidence): usage_count_avg in pg_buffercache_summary now averages a 0/1 cooling bit instead of the 0..5 usage counter, so the column silently becomes "fraction of HOT buffers" while keeping its old name and documented meaning. This is a POLA violation for anyone consuming this column. Rename/redefine the column and update the docs, or keep the reported quantity consistent with its name.
| 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.
Consistency (moderate confidence): pg_buffercache_usage_counts now only ever emits rows for counts 0 and 1 (BM_MAX_USAGE_COUNT is redefined to BUF_COOLSTATE_HOT = 1), whereas the documented/expected behavior is one row per usage count 0..5. Array indexing is safe (2-element arrays sized by BM_MAX_USAGE_COUNT+1), but the SQL result shape changes silently. Update the function's docs and its regression test expected output, and consider whether the usage_count column here should be renamed to reflect the cooling-state semantics.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
This entire file is a personal developer debugging artifact, not part of any feature. A patch destined for pgsql-hackers/commitfest must be minimal and contain only what the change requires. Committing a top-level .gdbinit (alongside .clangd and pg-aliases.sh) pollutes the tree, makes the patch "do more than one thing," and is an immediate rejection reason. It also has security implications: a .gdbinit in a working directory auto-executes GDB commands, and shipping one in-tree is a footgun. Remove it from the patch and keep it local (e.g. via a personal, un-tracked gitignore).
| SpinLockAcquire(&StrategyControl->buffer_strategy_lock); | ||
| StrategyControl->completePasses++; | ||
| SpinLockRelease(&StrategyControl->buffer_strategy_lock); |
There was a problem hiding this comment.
completePasses is double-counted when batch_size > 1. When a fetch-add lands with start < NBuffers but the batch straddles the boundary (start + batch_size > NBuffers), this branch increments completePasses. But that same fetch-add pushes nextVictimBuffer past NBuffers, so the next backend to fetch-add gets start >= NBuffers and takes the wrap-CAS branch above, which increments completePasses again for the same physical wrap. Trace (NBuffers=100, batch=16): backend A start=90 -> straddle branch -> completePasses++ (counter now 106); backend B start=106 -> wrap branch -> CAS 106->6, completePasses++. One physical wrap, two increments. StrategySyncStart returns this to BgBufferSync, where an inflated passes_delta inflates strategy_delta (Assert(strategy_delta >= 0) and lap arithmetic), mispacing the LRU writer. The straddle-crossing is already implicitly accounted for by nextVictimBuffer / NBuffers in StrategySyncStart plus the eventual wrap-CAS, so this extra increment is redundant/wrong.
|
|
||
| cmdline2 = psprintf("exec %s", cmdline); | ||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | ||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
There was a problem hiding this comment.
This one-line change (execl -> execlp) is unrelated to the rest of the patch, which is a buffer-manager cooling-stage change, and does not belong in this commit. On -hackers a patch that bundles an unrelated change is a top rejection reason; drop this hunk (and the .clangd/.gdbinit/pg-aliases.sh dev artifacts) so the change reads as one logical unit.
Beyond patch hygiene, the switch is a behavioral loosening with no benefit for the in-tree use. shellprog is the compiled-in SHELLPROG, which is $(SHELL) from make or /bin/sh from meson -- an absolute path. execl invokes exactly that binary. execlp additionally performs a PATH search when the name contains no slash, so if SHELLPROG is ever a bare name the test harness will exec whatever sh sits earliest in the caller's PATH instead of the intended shell -- a reproducibility/security footgun in a test runner. There is no accompanying rationale, test, or commit-message justification. Revert to execl unless there is a documented need, in which case it should be a standalone patch. (high confidence)
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| * max") reads naturally. | ||
| */ | ||
| #define BM_MAX_USAGE_COUNT 5 | ||
| #define BM_MAX_USAGE_COUNT BUF_COOLSTATE_HOT |
There was a problem hiding this comment.
BM_MAX_USAGE_COUNT is redefined from 5 to 1 while keeping the historical name. This is a backward-compatibility footgun: the macro is visible to extensions (it lives in buf_internals.h and is used by contrib/pg_buffercache, which sizes usage_counts[BM_MAX_USAGE_COUNT + 1] on it). Any out-of-tree consumer that still assumes the 0..5 range now silently gets a 2-element domain, with no compile-time signal. The macro name no longer describes anything ("max usage count" when there is no usage count), which the comment itself concedes ("reads naturally"). Prefer renaming to something that reflects the new meaning (e.g. reference the cooling state directly, BUF_COOLSTATE_HOT) at the call sites and dropping the misleading alias, rather than overloading a semi-public name with new semantics.
| /* 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. */ |
There was a problem hiding this comment.
PostgreSQL multi-line comment style puts the opening /* on its own line; here comment text starts on the same line as /*. This will not survive pgindent unchanged and is inconsistent with the surrounding comments in this file. s//* Cooling state...//*\n * Cooling 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. */ | |
| /* | |
| * 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. | |
| */ |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This shebang change breaks tree-wide consistency and is not minimal. 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). Changing only pgindent to /usr/bin/env perl is an unrelated, standalone edit with no functional change requiring it, so it will be flagged as churn on pgsql-hackers. It also weakens reproducibility: /usr/bin/env perl picks up whatever perl is first on PATH (e.g. a perlbrew/plenv build) rather than the system perl the tree standardizes on, which can change pgindent's output and is a POLA violation for a formatting tool. Revert to the canonical shebang.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
2746528 to
3f6fff3
Compare
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.