schema: review-report-v2
source: postgresql-master
slug: commitfest-6728-review
target: CommitFest 6728 — md: measure just FileSync() for pgstat_io without FileClose()
head: 42e96cf2fe095c822f76bc94669ea875cb1e3351
base: 42e96cf2fe095c822f76bc94669ea875cb1e3351
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
coverage_requirement: required
coverage_status: measured
model: glm-5.2:cloud

CommitFest 6728 Review: md: measure just FileSync() for pgstat_io without FileClose()

Summary

Patch: Reorder FileClose() to execute after pgstat_count_io_op_time() in mdsyncfiletag() (src/backend/storage/smgr/md.c), so that pg_stat_io fsync timing reflects only FileSync() duration and excludes close()/FileClose() overhead.

Author: Jakub Wartak (EnterpriseDB) Verdict: ready-for-committer Findings: 0 (verified clean) Blockers: 0 verified / 0 unverified

The patch is a minimal, correct, low-risk reordering of two adjacent statements in a single function. It does not change error handling, control flow, errno preservation, or resource lifetime — it only changes which operations are included in the fsync IO-time metric reported to pg_stat_io.

Changed Code

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1937,12 +1937,12 @@ mdsyncfiletag(const FileTag *ftag, char *path)
 	result = FileSync(file, WAIT_EVENT_DATA_FILE_SYNC);
 	save_errno = errno;

-	if (need_to_close)
-		FileClose(file);
-
 	pgstat_count_io_op_time(IOOBJECT_RELATION, IOCONTEXT_NORMAL,
 							IOOP_FSYNC, io_start, 1, 0);

+	if (need_to_close)
+		FileClose(file);
+
 	errno = save_errno;
 	return result;
 }

Changed file: src/backend/storage/smgr/md.c (1 file, +3/-3) Changed function: mdsyncfiletag (lines 1905–1948)

Correctness Analysis

errno preservation (unchanged)

save_errno is captured immediately after FileSync() at line 1938 in both orderings. errno = save_errno at line 1946 restores the FileSync errno regardless of whether pgstat_count_io_op_time() or FileClose() runs first. Neither pgstat_count_io_op_time() nor FileClose() can corrupt the saved errno because it is captured before both. The reorder is errno-safe.

IO timing accuracy (the intent)

Before: pgstat_count_io_op_time() measured FileSync() + FileClose() elapsed time (since io_start was set before FileSync). After: pgstat_count_io_op_time() measures only FileSync() elapsed time.

This is the desired behavior: pg_stat_io.fsync_time should reflect only the actual fsync syscall duration, not file-descriptor teardown. FileClose() may invoke pgaio_closing_fd(), close(), and VFD bookkeeping whose cost is unrelated to fsync and would inflate the metric.

Resource lifetime (unchanged)

FileClose() is still called exactly once when need_to_close is true. The file descriptor is still released before mdsyncfiletag returns. No leak or double-close is introduced.

Control flow (unchanged)

No new branches, loops, or early returns are added. The if (need_to_close) guard and return result path are structurally identical; only statement order within the tail block changes.

Security Analysis

No security-relevant changes. The patch does not touch authentication, authorization, privilege checks, search paths, catalog writes, crypto, TLS, or any SQL-reachable input parsing. No new memory-safety risk: FileClose() and pgstat_count_io_op_time() are existing well-audited functions; their argument sets are unchanged. No SECURITY DEFINER or search_path exposure.

Verification Summary

Repair Validation Summary

No findings were raised; no repairs were required or validated. The patch is accepted as-is. No candidate edits were produced because no verified or cannot-verify findings exist.

Changed-Code Coverage

Requirement: required — src/backend/storage/smgr/md.c contains instrumentable C implementation code changed by the patch.

Execution lane

Test commands

initdb -D <data> --no-sync -U test
postgres -D <data> -p 55434   # fsync=on, track_io_timing=on
psql: CREATE TABLE cov4(a int, b text);
      INSERT INTO cov4 SELECT g, repeat('z',100) FROM generate_series(1,5000) g;
      CREATE INDEX ON cov4(a); CHECKPOINT;
psql: for i in 1..15: CREATE TABLE cov4_i(a int);
      INSERT INTO cov4_i SELECT g FROM generate_series(1,1000) g;
      CREATE INDEX ON cov4_i(a);
psql: CHECKPOINT;
psql: SELECT backend_type, fsyncs FROM pg_stat_io WHERE object='relation' AND fsyncs>0;
gcov -b -c build/src/backend/storage/smgr/md.gcno

Metrics

Per-line coverage (HEAD-side changed lines)

Touched functions

Uncovered regions

None. All changed executable lines and the single changed branch are covered.

Uncovered branches

None.

Non-instrumented changes

None — all changes are in instrumentable C implementation code.

pgsql-hackers Thread Ledger

Status: reconciled Ledger report slug: commitfest-6728-review-handoff-thread

Thread 645277: "md: measure just FileSync() for pgstat_io without FileClose()"

Finding links: none (no findings to link).

Reconstruction methods: amauta-mail BM25 search on "mdsyncfiletag FileClose" and "measure just FileSync for pgstat_io"; thread-id grouping (645277); full message-body inspection of both messages.

Deterministic Verdict

• Findings: 0 verified, 0 cannot-verify, 0 refuted.
• Blockers: 0 verified / 0 unverified.
• No surviving BLOCKER, MAJOR, or live thread objection.
• Verdict: ready-for-committer.

Provenance

Aggregated Test Plan

1. Existing coverage: The checkpointer fsync path (mdsyncfiletag) is

exercised by any workload that dirties relations and checkpoints. The src/test/regress/sql/stats.sql regress test exercises pg_stat_io and indirectly the fsync path.

1. Suggested follow-up (optional, non-blocking): A regress test that

enables track_io_timing, creates + dirties a relation, checkpoints, and asserts that pg_stat_io.fsync_time is non-zero for the checkpointer relation/normal context. This would guard against future regressions in the fsync-timing measurement path but is not required for this patch.

Residual Risk

Low. The patch is a pure statement reorder in a single function with unchanged semantics for errno, resource lifetime, and control flow. The only behavioral change is the intended one: pg_stat_io.fsync_time for relation fsyncs will no longer include FileClose() overhead, making the metric more accurate.