Add test_bufmgr: algorithm-agnostic tests for the buffer manager - #50
Open
cooltodinesh wants to merge 5 commits into
Open
Add test_bufmgr: algorithm-agnostic tests for the buffer manager#50cooltodinesh wants to merge 5 commits into
cooltodinesh wants to merge 5 commits into
Conversation
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.
The buffer manager (src/backend/storage/buffer/) has no test module of
its own. It is covered only incidentally, as a side effect of the rest
of the regression suite, so a change to the replacement policy or to the
residency bookkeeping can regress without any test asserting the
affected property directly.
Add src/test/modules/test_bufmgr. It tests the buffer manager as a
subsystem, not any one replacement algorithm: the module is named for
the subsystem under test, which is stable, rather than for the current
eviction policy, which is not. Every property asserted is one that any
correct buffer manager must have, expressed only through observable
behavior -- page residency, page contents and buffer pins. Residency is
read as "does pg_buffercache have a row for this relation block",
contents as the page data itself, and pins as
pg_buffercache.pinning_backends; no test reads usagecount or any other
replacement-policy internal, and no test predicts which buffer is chosen
as a victim. The intent is that these keep passing across an algorithm
swap, including one to a pool with no usagecount field at all.
Seven regress tests and one isolation spec:
- eviction_reload: an evicted page reloads with correct contents, and
the manager makes progress under pressure (no "no unpinned buffers
available" error).
- dirty_persistence: an evicted dirty page reloads with its modified
contents; eviction itself forces the write-out, with no checkpoint
or restart needed.
- scan_no_retain: a one-touch page does not indefinitely retain its
buffer under pressure.
- truncate_stale / drop_recreate_stale: TRUNCATE and DROP+recreate
never let stale cached pages resurface.
- local_eviction: the local (temp-table) buffer pool reuses correctly,
sized from temp_buffers in bytes.
- churn: contents stay correct after many replacement cycles over a
working set several times the pool.
- pinned_safety (isolation): a buffer pinned via a suspended cursor in
one session survives a concurrent flood in another, with an unpinned
control probe confirming the flood actually exceeded the pool.
Three things keep the suite from being tied to one configuration or one
policy:
- Pressure is generated with pg_prewarm in 'buffer' mode, which pulls
blocks in through the normal replacement path with no
BufferAccessStrategy ring -- unlike a sequential scan or COPY, which
take a ring and so never cycle the whole pool.
- Fixed working-set sizes are derived from the relevant GUC in bytes
(shared_buffers or temp_buffers times block_size) rather than from a
hand-tuned row count, so they exceed the pool at any BLCKSZ.
- Where a fixed size will not do, the test loops, adding pressure
until the end state it is looking for is actually observed (a
tracked block goes non-resident), bounded by a generous round cap so
a real failure raises an error rather than hanging.
A scan-resistance test -- "a genuinely hot page survives a
greater-than-pool one-touch flood" -- was attempted and dropped. It is
not a property of every correct buffer manager, and even where it holds
it proved geometry-dependent and non-monotonic across BLCKSZ, so a
single post-flood residency check is not a fair test of it. It is
recorded as a TODO in the README, along with the other cases this first
iteration defers; a real version needs injection-point control over
buffer placement, or a comparative hit-ratio measure.
Verified green and deterministic over repeated runs on master and
REL_17_STABLE, and at BLCKSZ 4, 8, 16 and 32 kB. NOTICE output from
CREATE EXTENSION IF NOT EXISTS and DROP ... IF EXISTS is suppressed with
client_min_messages = warning in the module's test config, rather than
with per-statement SETs, so the expected output does not depend on
schedule order.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The buffer manager (
src/backend/storage/buffer/) has no test module of itsown. It is covered only incidentally, as a side effect of the rest of the
regression suite, so a change to the replacement policy or to the residency
bookkeeping can regress without any test asserting the affected property
directly.
This adds
src/test/modules/test_bufmgr: tests for the buffer manager as asubsystem, not for any one replacement algorithm. The module is named for the
subsystem under test, which is stable, rather than for the current eviction
policy, which is not.
Everything asserted is a property any correct buffer manager must have,
expressed only through observable behavior — page residency, page contents,
buffer pins. Residency is read as "does
pg_buffercachehave a row for thisrelation block", contents as the page data itself, pins as
pg_buffercache.pinning_backends. No test readsusagecountor any otherreplacement-policy internal, and no test predicts which buffer is chosen as a
victim, so the suite should keep passing across an algorithm swap — including
to a pool that has no
usagecountfield at all.What's here
Seven regress tests and one isolation spec:
manager makes progress under pressure (no "no unpinned buffers available"
error).
contents. No checkpoint or restart needed: eviction itself forces the
write-out. (Verified directly that
CHECKPOINTclears the dirty bit withoutevicting, so a background checkpoint stealing the write would make the
assertion pass for the wrong reason rather than fail — hence
checkpoint_timeout = 1hin the test config; see README for the mechanism.)under pressure.
never let stale cached pages resurface.
sized from
temp_buffersin bytes so it holds at any BLCKSZ.set several times the pool.
session survives a concurrent flood in another session, with an unpinned
control probe confirming the flood actually exceeded the pool (guards against
a vacuous pass).
What keeps it from being tied to one configuration
pg_prewarm('rel', 'buffer'), not plain scans. Prewarm'sbuffer mode pulls blocks in through the normal replacement path with no
BufferAccessStrategyring, unlike a sequential scan or COPY, which take aring and so never cycle the whole pool.
shared_buffers/temp_buffers×block_size), not a hand-tuned row count, so working sets exceed the pool atany BLCKSZ rather than at one config.
adding pressure until the observed end state occurs (a tracked block goes
non-resident), bounded by a generous round cap, instead of asserting that a
fixed size happened to be right on this machine.
Scan resistance: attempted and dropped
I tried a "hot page survives a one-touch flood" test and pulled it after
validating it across configurations:
greater-than-pool flood at the test pool size, because the flood's own bulk
read/write pressure churns the pool. It is a property of a scan-resistant
replacement policy, not of every correct buffer manager.
8MB pool the outcome flipped between 1024 buffers (8kB BLCKSZ), 256 (32kB) and
2048 (4kB), because whether a page survives depends on how many times a
greater-than-pool sweep revisits its slot, regardless of how hot it is.
It is recorded as a TODO in the README — a real version needs injection-point
control over buffer placement (or a comparative hit-ratio measure), not a single
post-flood residency check. The README lists the other deferred cases too
(crash durability across restart, bgwriter as a first-class subject, concurrent
reads of the same block, cleanup-lock acquisition, async/streaming reads, I/O
failure handling, checkpoint/writeback interactions).
Verified
masterandon
REL_17_STABLE.NOTICEoutput fromCREATE EXTENSION IF NOT EXISTS/DROP ... IF EXISTSis suppressed via
client_min_messages = warningin the test config(server-level, not per-statement
SET), so expected output isn't coupled toschedule order.
Happy to iterate on the scan-resistance tradeoff, on which properties are worth
asserting first, or on anything else.