schema: review-report-v2
target: CommitFest 61, Patch 7038 — Add vacuum_delay_point() to GiST empty-page deletion pass
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
diff_sha: b0b91611e2e892d4e0e69b20aed9041fababbe4b69274cb8136396dd249371cc
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
model: glm-5.2:cloud

Review — Add vacuum_delay_point() to GiST empty-page deletion pass

• CommitFest: 61, Patch 7038
• Author: Paul Kim (mok03127@gmail.com)
• Thread: 652229 (pgsql-hackers)
• Canonical Message-ID: 178462815730.66373.14808873588374085998@mail.gmail.com
• HEAD SHA: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
• Diff SHA: b0b91611e2e892d4e0e69b20aed9041fababbe4b69274cb8136396dd249371cc
• Changed file: src/backend/access/gist/gistvacuum.c (+3 / -0)
• Changed function: gistvacuum_delete_empty_pages (line 528)

Patch summary

The patch adds a single vacuum_delay_point(false) call at the top of the outer while-loop in gistvacuum_delete_empty_pages(), before ReadBufferExtended and before any buffer lock is acquired. This matches the established convention in the same file:

• gistvacuumscan() line 248: vacuum_delay_point(false) before read_stream_next_buffer
• gistvacuumpage() line 492: vacuum_delay_point(false) before ReadBufferExtended in the tail-recursion loop

The function gistvacuum_delete_empty_pages() (introduced by commit 7df159a620b, 2019) is the only GiST vacuum loop that lacked this call. The bool is_analyze parameter was added by e5b0b0ce150 (PG 18+); false is correct for a VACUUM path.

Findings

Zero findings. Both the patch-reviewer and security-reviewer seats found no blockers, no changes-requested, no majors, no minors, and no nits.

Patch-reviewer seat

• Consistency: Verified — vacuum_delay_point(false) is the established throttle point across all index AMs (btvacuumscan, btvacuumpage, ginbulkdelete, ginvacuumcleanup, ginVacuumPostingTreeLeaves, hashbucketcleanup, spgvacuumscan, spgprocesspending, blbulkdelete, blvacuumcleanup, lazy_scan_heap, lazy_vacuum_heap_rel). All use the same pre-buffer-lock placement.
• Placement correctness: Verified — the call is at the outer-loop top, before ReadBufferExtended at line 530, while no buffer lock is held. The inner leaf-deletion loop correctly does *not* call the delay point (it holds leafbuf exclusively).
• Argument correctness: Verified — false is correct for a VACUUM path (not ANALYZE). Routes delay timing to PROGRESS_VACUUM_DELAY_TIME.
• Semantics: Verified — vacuum_delay_point does CHECK_FOR_INTERRUPTS, cost-based delay sleep, config reload, and parallel vacuum balance updates. All four are desirable in this long-running pass.
• Memory-context filter: No memory-context findings apply. No allocation, no context switching, no catalog access.
• Non-findings: No catversion bump needed, no docs change needed, no pg_dump impact, no OID assignment, no error-style change, no pgindent concern.

Security-reviewer seat

• Privilege/role escalation: No impact. Reached only via standard VACUUM/autovacuum entry path with MAINTAIN privilege already enforced upstream.
• SECURITY DEFINER / search_path: Not applicable. Static C function, no SQL surface.
• Catalog write race / TOCTOU: No widened window. The delay occurs entirely before the critical read-check-delete sequence. Concurrent writers are observed through the existing lock-then-recheck logic.
• SQL injection: Not applicable. No string handling.
• Crypto/TLS: Not applicable.
• Superuser guards: Not applicable.
• Interrupt handling: CHECK_FOR_INTERRUPTS() is correctly located outside any buffer critical section, following the required convention.
• Memory-safety: No C memory-safety surface. Single function call with no pointer/length/buffer arguments.

Verification Summary

Both specialist seats (patch-reviewer and security-reviewer) independently verified the patch as clean. No findings were produced, so no finding-level verification (pg-finding-verifier) was required. The zero-finding set was confirmed by merging and de-duplicating the two seats' outputs by file:line + mechanism — the merged set is empty.

Mailing-list thread reconciliation

Thread 652229 (pgsql-hackers) — reconciled with 5 messages:

1. Paul Kim (v1 patch, 2026-07-21): Identified the oversight by cross-AM comparison of vacuum loops. Correct diagnosis.
2. Andrey Borodin (x4mmm, 2026-07-21): Confirmed "Yup, this looks like an oversight." Independent GiST maintainer agreement.
3. Michael Paquier (2026-07-21): CC'd Heikki Linnakangas (original committer). Argued backpatch not strictly required (7 years to notice).
4. Paul Kim (2026-07-22): Clarified GiST differs from the related GIN thread — GIN was a regression (call was dropped), GiST never had the call. Master-only fix is natural.
5. Solai (solai.cdac, 2026-07-27): Reviewed and tested v1 — reproduced the execution path with a large GiST index, confirmed vacuum_delay_point(false) is invoked, no functional issues. "The patch looks good to me."

Thread consensus: No unresolved objections, no rejection, no prior counter-argument. Reviewer sign-off received. Backpatch question resolved as master-only (Paul Kim's reasoning accepted by the thread).

Ledger report: commitfest-7038-review-handoff-thread-ledger (source postgresql-master).

Changed-Code Coverage

Coverage requirement

• Status: required
• Changed instrumentable paths: src/backend/access/gist/gistvacuum.c

Execution

• Selected lane: host-disposable
• Attempt 1 (host-disposable, passed): Coverage-instrumented build of PostgreSQL 20devel with --coverage CFLAGS in disposable scratch. initdb + pg_ctl started on port 54979. Ran functional GiST vacuum tests exercising gistvacuum_delete_empty_pages (50000-row table with 90% deletion + second VACUUM truncating to trigger empty-page deletion: 407 pages newly deleted) plus regress tests gist and vacuum via pg_regress. gcov collected from gistvacuum.gcda with 33 runs.

Test commands

make -C src/backend/access/gist clean && make -C src/backend/access/gist CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage'
make -j4 CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage' LDFLAGS='--coverage'
initdb -D <pgdata> --no-locale -E UTF8
pg_ctl -D <pgdata> start -o '-p 54979'
psql -p 54979 -d postgres  # functional GiST vacuum tests
pg_regress --inputdir=src/test/regress --host=localhost --port=54979 gist vacuum
gcov -b -f -o src/backend/access/gist/ src/backend/access/gist/gistvacuum.c

Metrics

Changed line evidence

The single changed executable line is line 528: vacuum_delay_point(false);

gcov evidence: 8: 528: vacuum_delay_point(false) — executed 8 times during the test run. The call returned 100% (call 0 returned 100%).

The comment on line 527 (/* call vacuum_delay_point while not holding any buffer lock */) is structural-only text and is not an executable line — no gcov coordinate is generated for it.

Touched functions

The 2 uncovered lines (542–543: Assert(false) + UnlockReleaseBuffer) are the "shouldn't happen" panic path for a page that was an internal page earlier but is now something else. This is an Assert(false) guard — infeasible to execute in a production build (compiled out when NDEBUG is set) and deliberately unreachable by design. Not a coverage gap requiring a test.

Uncovered regions

None. All changed executable lines are covered.

Uncovered branches

None. The changed line has no branches (it's a straight function call).

Non-instrumented changes

None. The only changed file is src/backend/access/gist/gistvacuum.c, which is fully instrumentable C source.

Residual limitations

• The full make check regress suite was run with a non-coverage build (245/245 tests passed, including gist, vacuum, index_including_gist, create_index). Coverage was measured on a separate coverage-instrumented build with targeted GiST vacuum functional tests + regress tests gist and vacuum.
• The Assert(false) panic path (lines 542–543) is infeasible by design and not counted as an uncovered changed line (it is outside the changed hunk).

Repair Validation Summary

No findings were produced by either review seat, so no candidate repairs were generated, validated, or tested. The finding+repair verification seat (pg-finding-verifier) was not invoked because there were zero findings to verify. The patch as submitted is correct and requires no modifications.

Aggregated Test Plan

No test patches are required. The existing regress suite (gist, vacuum, index_including_gist) exercises the changed code path. The thread records that Solai independently reproduced the execution path with a large GiST index and confirmed the call fires.

Vacuum-delay timing is not deterministically regress-testable (it depends on vacuum_cost_delay/vacuum_cost_limit GUCs and system load), which is consistent with how all prior vacuum_delay_point additions across the codebase were merged — no dedicated regress test exists for any of the 20+ call sites.

Deterministic verdict

• Verdict: ready-for-committer
• Blocker counts: verified: 0, unverified: 0
• Reasoning: Zero findings from both patch-reviewer and security-reviewer. No correctness/security MAJOR findings. No live thread objections. No test-only MAJORs. The patch is correct, minimal, consistent with codebase conventions, and verified by coverage measurement.

Model routes

• patch-reviewer: glm-5.2:cloud (user-fixed override; policy recommended sonnet-5/opus-4.8)
• security-reviewer: glm-5.2:cloud (user-fixed override; policy recommended sonnet-5/opus-4.8)
• finding-verifier: not invoked (zero findings)
• change-coverage-reviewer: not invoked as nested agent (coverage executed by harness directly in host-disposable lane)
• policy_version: 1
• policy_hash: 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe

Provenance

• Patch-reviewer handoff: commitfest-7038-review-handoff-patch-reviewer (markdown_sha256: 397892dc58dc6193d614a69dcdf591958c98db54d6139e96949b2e6f47b0cb12)
• Security-reviewer handoff: commitfest-7038-review-handoff-security-reviewer (markdown_sha256: 6dc18a7c5eb8ea24212beaa6e3e52147c2c986486378b34fb3b8fbdefad6c5fa)
• Thread ledger: commitfest-7038-review-handoff-thread-ledger (markdown_sha256: bfee8f31a7f6d5fed794110c8f1665598146f26da5684a021c69a530f6ae879e)

Maintainer CC

Top authors of src/backend/access/gist/gistvacuum.c (3-year window): Bruce Momjian, Melanie Plageman, Peter Geoghegan. Original function introduced by Heikki Linnakangas (commit 7df159a620b, 2019) — already CC'd by Michael Paquier on the thread.

Residual risk

Minimal. The patch adds a single function call that is already used in 20+ sites across the codebase. The call is placed where no buffer lock is held. No new behavior is introduced — the cost-based vacuum delay and interrupt checking are now applied to a pass that should have had them since 2019. The change is master-only with no backpatch (per thread consensus).