Review report — CommitFest 61 Patch 7020: Do not flush BufFile for regular temp files on close

code-review · generated from canonical Markdown

Type
code-review
Source
postgresql-master
Slug
commitfest-7020-review
Tags
code-reviewcommitfest-7020patch-reviewpg-review-harnesspostgresqlreview-report-v2
Updated
2026-08-04T03:09:24.589779+00:00
Verdict / SummaryCommitFest 61 Patch 7020 (Do not flush BufFile for regular temp files on close) — ready-for-committer, 0 blockers, 2 verified findings (MINOR stats undercount + NIT comment wording), 100% changed-code coverage (2/2 lines, 2/2 branches), security CLEAN, single-message thread.
Front matter
schema: review-report-v2
target: "CommitFest 61 Patch 7020 — Do not flush BufFile for regular temp files on close"
author: Sergey Solovev
canonical_message_id: d01c9594-d3e5-42bb-8459-d91ab998966c@tantorlabs.ru
source: postgresql-master
base: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
diff_kind: uncommitted
diff_sha: 26ab1b25270e047bddd5e0d6b5b3745e888366409bd711258e687d94347bf30b
pg_verdict: ready-for-committer
generic_verdict: approve-with-nits
blockers_verified: 0
blockers_unverified: 0
model_fixed: glm-5.2:cloud

Review Report — CommitFest 61 Patch 7020

Title: Do not flush BufFile for regular temp files on close Author: Sergey Solovev <sergey.soloviev@tantorlabs.ru> Canonical Message-ID: d01c9594-d3e5-42bb-8459-d91ab998966c@tantorlabs.ru CommitFest: 61, Patch 7020 Source: postgresql-master Base = HEAD: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811 (master) Diff kind: uncommitted (working-tree change) Diff SHA: 26ab1b25270e047bddd5e0d6b5b3745e888366409bd711258e687d94347bf30b Changed files: src/backend/storage/file/buffile.c (+3/-2, 5 lines changed) Changed functions: BufFileClose Maintainers (3y): Michael Paquier (6), Peter Eisentraut (4), Bruce Momjian (3)

The patch

diff
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -414,8 +414,9 @@ BufFileClose(BufFile *file)
 {
 	int			i;

-	/* flush any unwritten data */
-	BufFileFlush(file);
+	/* flush any unwritten data if it will not be deleted on close */
+	if (file->fileset != NULL)
+		BufFileFlush(file);
 	/* close and delete the underlying file(s) */
 	for (i = 0; i < file->numFiles; i++)
 		FileClose(file->files[i]);

The patch gates BufFileFlush in BufFileClose on file->fileset != NULL. Regular temp files (BufFileCreateTemp, fileset == NULL) are created with FD_DELETE_AT_CLOSE and unlinked by FileClose, so flushing their final dirty buffer before unlink is wasted I/O. FileSet-backed BufFiles (BufFileCreateFileSet/BufFileOpenFileSet, fileset != NULL) persist after close and may be re-opened by name; the patch preserves their flush.

Deterministic verdict

Rationale: No BLOCKER or MAJOR findings. No live thread objections (single-message thread, no replies). Two verified findings are MINOR (stats-accounting observation) and NIT (comment wording). Per the deterministic verdict rules: no blocker → not waiting-on-author; no correctness/security MAJOR or live objection → not needs-review; no test-only MAJORs → ready-for-committer.

Findings

[MINOR] PR-1 — src/backend/storage/file/buffile.c:417 — temp I/O statistics undercount

What: With the patch, the final BufFileFlush of a BufFileCreateTemp-based BufFile is skipped, so BufFileDumpBuffer (buffile.c:497) is not called for the closing dirty buffer. That function increments pgBufferUsage.temp_blks_written (buffile.c:578) and accumulates pgBufferUsage.temp_blk_write_time (buffile.c:572). After this patch, those counters no longer include the final write of a doomed temp file.

Why: EXPLAIN (BUFFERS) and pg_stat_statements temp_blks_written/temp_blk_write_time will report marginally lower I/O for queries that spill to disk (hashjoin batches, tuplestore overflow, GiST buffered build, backup manifest, serial sort). The effect is at most one BLCKSZ write per BufFile closed with a dirty buffer. Arguably the new numbers are more accurate — the skipped write was wasted I/O on a file about to be unlink()'d — but it is an observable behavioral change in instrumentation.

Verification: verified by inspection. BufFileFlush (buffile.c:724-731) calls BufFileDumpBuffer only when dirty; BufFileDumpBuffer (buffile.c:497-585, lines 553-557 and 562/578) performs the write and counter increments. The patch's if (file->fileset != NULL) gate (line 418) skips this for fileset == NULL temp files.

Repair (suggested): No code change required for correctness. The suggested repair is a comment expansion documenting the intentional stats undercount (combined with PR-2's comment fix). A concrete HEAD-bound unified diff was validated in disposable scratch: apply-check OK, diff-check OK, compile of buffile.o OK with zero warnings.

Suggested code (exact-postimage excerpt):

c
/*
 * Flush any unwritten data, but only for fileset-backed BufFiles whose
 * underlying physical files survive FileClose().  Plain temp files
 * (fileset == NULL) are created with FD_DELETE_AT_CLOSE, so FileClose()
 * unlinks them regardless of whether the final buffer is written;
 * skipping the flush therefore avoids a pointless write to a doomed
 * file.  As a consequence, pgBufferUsage.temp_blks_written and
 * temp_blk_write_time no longer account for that final write of a
 * doomed temp file; this is intentional.
 */
if (file->fileset != NULL)
    BufFileFlush(file);

Test example: waived (not-instrumentable) — comment-only change with no behavioral delta; a regression test cannot distinguish a comment change. The stats delta is documented in the comment rather than enforced by test.

[NIT] PR-2 — src/backend/storage/file/buffile.c:417 — comment wording imprecise

What: The comment says "flush any unwritten data if it will not be deleted on close" but the code predicate is file->fileset != NULL, which is a proxy for "the underlying physical files will not be deleted on close" (because FileSet-based files are created without FD_DELETE_AT_CLOSE, while OpenTemporaryFile sets it). The equivalence holds today but is an implicit invariant.

Why: A future reader or a future code path that creates a BufFile with fileset != NULL but FD_DELETE_AT_CLOSE set (or vice versa) would make the comment misleading.

Verification: verified by inspection. OpenTemporaryFile (fd.c:1754) sets FD_DELETE_AT_CLOSE; PathNameCreateTemporaryFile (fd.c:1872) sets only FD_TEMP_FILE_LIMIT (no FD_DELETE_AT_CLOSE); FileClose (fd.c:2022) unlinks only when FD_DELETE_AT_CLOSE is set.

Repair (suggested): Comment rewording making the fileset predicate and FD_DELETE_AT_CLOSE invariant explicit. Subsumed by the combined repair patch for PR-1.

Test example: waived (not-instrumentable) — comment-only style change with no behavioral delta.

Verified clean / non-findings

Security review

The security reviewer found NO findings (CLEAN). The fileset != NULL gate correctly preserves the flush for persisted/shareable fileset files and skips it only for FD_DELETE_AT_CLOSE temp files that are unlinked on close and have no possible reader. No memory-safety, authorization, race, or FD-lifecycle defect is introduced. No SQL-reachable C memory-safety surface (the change neither allocates, frees, nor indexes anything; it only gates a function call).

Test adequacy

No new test-gap findings. The change has two branches (fileset != NULL → flush; fileset == NULL → skip), both exercised by existing indirect tests: regress suite (sort/hashjoin/tuplestore/gist/backup-manifest paths for temp; logical replication/shared tuplestore paths for fileset) and isolation suite. No new error paths, no observable behavior change for correct usage. No pg-test-author delegation was required.

pgsql-hackers thread ledger

Submission review

Single-file, 5-line change. No catversion bump needed (no catalog/storage/WAL format change). No docs change needed (internal storage optimization, no user-visible GUC or behavior). No pg_dump impact. No OID allocation. No Discussion: link needed (not a feature). The mailing-list thread has no replies yet.

Performance review

Eliminates one potentially-unnecessary BLCKSZ-sized FileWrite (8KB) per BufFileClose of a temp BufFile that has a dirty buffer at close time. Savings occur when a temp BufFile is written to but never read back (tuplestore overflow discarded, hashjoin batch files skipped, GiST build buffers, backup manifest). Magnitude is small: at most one 8KB write per BufFile close, only when buffer is dirty. No regression possible — the flush is only skipped for files being deleted. No benchmark run in this review (read-only worktree).

Changed-Code Coverage

Coverage requirement: required (changed instrumentable path: src/backend/storage/file/buffile.c)

Measurement: measured in a fresh disposable coverage epoch at frozen HEAD 7cb9060dcde08ca63df1f0d701b0674e0e8ae811 with the scope diff applied.

Execution lane: host-disposable (passed). Meson coverage build with -Db_coverage=true -Dcassert=true -Dinjection_points=true -Dc_args=-fprofile-update=atomic -fprofile-arcs -ftest-coverage. Counters reset to zero before test runs.

Tests executed:

Metrics:

Uncovered regions: none Uncovered branches: none

Excluded changed coordinates:

Touched function: BufFileClose (buffile.c:413-426) — entered, 8/8 whole-function lines covered, 2/2 whole-function branches covered.

Non-instrumented changes: none

**COV-* findings:** none (no coverable uncovered regions or branches)

Verification Summary

findingseverityclassverification_statusmethodrepair.validation
PR-1MINORinstrumentationverifiedinspectionpassed
PR-2NITstyleverifiedinspectionpassed

Both findings are static/inspection findings confirmed by quoting relevant code paths. A single combined repair patch (comment expansion) addresses both findings and was validated in disposable scratch: apply-check OK, diff-check OK, compile of buffile.o with zero warnings, postimage uniqueness verified.

Repair Validation Summary

gateresultevidence
apply-check (clean HEAD)PASSgit apply --check returned 0 on a fresh clone at frozen HEAD
applyPASSgit apply returned 0; postimage matches expected code
diff-checkPASSgit diff --check returned 0; no whitespace errors
compile (buffile.o)PASSmake -C src/backend/storage/file buffile.o compiled with zero warnings; buffile.o produced
static-check (postimage uniqueness)PASSThe added comment block appears exactly once; the if-guard and BufFileFlush call are unchanged from the scope patch

The combined repair patch is a comment-only expansion; the behavioral code (the if (file->fileset != NULL) guard and BufFileFlush call) is unchanged from the scope patch. The repair is a proposal governed by the findings' root mechanism and intended postcondition; applying it remains user-controlled.

Aggregated Test Plan

No durable regression/TAP/isolation test artifacts are contributed. Both findings are documentation/comment findings with no behavioral delta beyond the scope patch's intended optimization. The test_example for both findings is a typed waiver (not-instrumentable: comment-only change). The stats-accounting delta (PR-1) is documented in the code comment rather than enforced by test, as a stable oracle for a one-BLCKSZ-write undercount is impractical.

Residual risk

Provenance / model routes

Durable handoff reports

Commands run (coordinator)

CC

Maintainers of src/backend/storage/file/buffile.c (top authors 3y): Michael Paquier, Peter Eisentraut, Bruce Momjian.