The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: tested, passed
Hi, Contents & Purpose ================== This patch converts ProcessSingleRelationFork() in src/backend/postmaster/datachecksum_state.c from a plain per-block ReadBufferExtended() loop to the new ReadStream API. This is a pure internals/performance change: the external contract is unchanged. No documentation changes are included, which seems right since no user-visible behavior changes. Initial Run =========== The patch applies cleanly to HEAD and builds without warnings. I built it and ran it end-to-end by hand: initialized a cluster without checksums, created a ~200,000-row/5,883-page table, and called pg_enable_data_checksums(). Progress was visible via pg_stat_progress_data_checksums while the new stream-based scan was running, and data_checksums correctly flipped to "on" on completion. I then independently verified correctness with the offline pg_checksums --check tool: 9,434 blocks scanned across the cluster, 0 bad checksums. I also exercised the abort path (the "success = false; break;" case when checksums are disabled while an enable is in progress): I re-ran pg_enable_data_checksums() with a cost_delay/cost_limit slow enough to catch it mid-scan, then called pg_disable_data_checksums() concurrently. The worker logged "data checksums processing was aborted" and cleanly transitioned to disabled, and the server stayed healthy afterward. Performance =========== I benchmarked this against pre-patch HEAD using a ~7.5 GiB table (956,815 blocks) with debug_io_direct=data to force real synchronous reads, 5 repetitions per configuration, median reported: - io_method=sync: HEAD = 460003ms, 958482 reads, 299082ms io_time patch = 383069ms, 60248 reads, 264387ms io_time - io_method=worker: HEAD = 458885ms, 958482 reads patch = 352155ms, 60248 reads There's a ~16x drop in read syscalls (matching the default io_combine_limit). My test storage is SSD, so the clock time multiplier may understate the win on higher-latency disks. Nitpicking & Conclusion ======================= One nit: the new "BlockRangeReadStreamPrivate p;" declaration is not properly indented. Please run pgindent and then I'll be +1 for committer review. Best regards, Enrique.
