Hi, On Thu, Sep 24, 2026 at 10:07 PM shihao zhong <[email protected]> wrote: > > I reviewed v2. It fixes both regressions I raised on v1. With one > block, mdzeroextend() takes the same FileZero() path as the old > mdextend() call, so nothing changes at runtime there.
Thanks for reviewing. > One request. smgrzeroextend(..., nblocks - 1, 1, ...) looks like a bug > unless you know why, and changing it to (0, nblocks) brings the 2x > regression back. Please add a one line comment in bufmgr.c and md.c > saying only the last block is written on purpose. It can take a while to understand that part when first looking at it. Still, I would prefer not to add the comment, since it would mostly end up restating what the code already does, unless anyone thinks otherwise. > Nit, the "don't set checksum" comment in bulk_write.c no longer > applies, since no page is passed. Reworded it to match an existing comment around smgrzeroextend(). > Note that v2 does not move toward the zero page detection idea. The > blocks before the last one are still holes, which read as zeros and > never pass through smgrzeroextend(). Making them non-zero means writing > them, which is the 2x cost Bharath measured. Right, I don't think this patch was meant to solve that, though I may be missing something. What it does is keep the responsibilities of smgrextend() and smgrzeroextend() separate, as mentioned upthread, and that alone is worth having IMHO. I think the zero page detection part would need changes to the API itself. Peipei, appreciate any thoughts on this. I attached the v3 patch. Please have a look. I don't think this needs to be back-patched, since it is not fixing a bug. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 2d1d53198f60a2e1da92d4f1b3a5fd9035d9c73e Mon Sep 17 00:00:00 2001 From: Peipei Yin <[email protected]> Date: Wed, 12 Aug 2026 09:52:31 -0700 Subject: [PATCH v3] Use smgrzeroextend() in a few more places. Previously, the bulk write path (smgr_bulk_flush()), the database copy (RelationCopyStorageUsingBuffer()) and the segment padding in SMGR (_mdfd_getseg()) each kept a zero-filled buffer of their own and passed it to smgrextend(), or mdextend() inside SMGR, one block per call. The bulk write path did that in a loop, to fill the gap left by a write that was not sequential. This commit changes them to use smgrzeroextend() (added by commit 4d330a61bb19), which is meant for exactly this and can extend several blocks in one call. The bulk write path now fills a whole gap at once. Note that the database copy and the segment padding still zero-extend only the last block as they did before. Zero-extending the whole range instead would regress CREATE DATABASE, both in bytes written and in execution time, where extension falls back to writing zeros, and would write or reserve a whole segment in the segment padding where a single block is enough. Author: Peipei Yin <[email protected]> Reviewed-by: Bharath Rupireddy <[email protected]> Reviewed-by: shihao zhong <[email protected]> Discussion: https://postgr.es/m/CADkZ2Ka8Ky4bEPJu_BA5zZT-6K_bf58uT_zXWMDz-iC4Tqu9cA@mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 6 ++---- src/backend/storage/smgr/bulk_write.c | 16 +++++++--------- src/backend/storage/smgr/md.c | 10 +++------- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 5c82865a084..2f466344876 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -5379,7 +5379,6 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, bool use_wal; BlockNumber nblocks; BlockNumber blkno; - PGIOAlignedBlock buf; BufferAccessStrategy bstrategy_src; BufferAccessStrategy bstrategy_dst; BlockRangeReadStreamPrivate p; @@ -5405,9 +5404,8 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, * Bulk extend the destination relation of the same size as the source * relation before starting to copy block by block. */ - memset(buf.data, 0, BLCKSZ); - smgrextend(smgropen(dstlocator, INVALID_PROC_NUMBER), forkNum, nblocks - 1, - buf.data, true); + smgrzeroextend(smgropen(dstlocator, INVALID_PROC_NUMBER), forkNum, + nblocks - 1, 1, true); /* This is a bulk operation, so use buffer access strategies. */ bstrategy_src = GetAccessStrategy(BAS_BULKREAD); diff --git a/src/backend/storage/smgr/bulk_write.c b/src/backend/storage/smgr/bulk_write.c index f3c24082a69..c115801fad1 100644 --- a/src/backend/storage/smgr/bulk_write.c +++ b/src/backend/storage/smgr/bulk_write.c @@ -46,8 +46,6 @@ #define MAX_PENDING_WRITES XLR_MAX_BLOCK_ID -static const PGIOAlignedBlock zero_buffer = {0}; /* worth BLCKSZ */ - typedef struct PendingWrite { BulkWriteBuffer buf; @@ -290,14 +288,14 @@ smgr_bulk_flush(BulkWriteState *bulkstate) * space will read as zeroes anyway), but it should help to avoid * fragmentation. The dummy pages aren't WAL-logged though. */ - while (blkno > bulkstate->relsize) + if (blkno > bulkstate->relsize) { - /* don't set checksum for all-zero page */ - smgrextend(bulkstate->smgr, bulkstate->forknum, - bulkstate->relsize, - &zero_buffer, - true); - bulkstate->relsize++; + /* We don't need to set checksum for all-zero pages. */ + smgrzeroextend(bulkstate->smgr, bulkstate->forknum, + bulkstate->relsize, + blkno - bulkstate->relsize, + true); + bulkstate->relsize = blkno; } smgrextend(bulkstate->smgr, bulkstate->forknum, blkno, page, true); diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 780c88c0630..0687c800d80 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -1823,13 +1823,9 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, */ if (nblocks < ((BlockNumber) RELSEG_SIZE)) { - char *zerobuf = palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, - MCXT_ALLOC_ZERO); - - mdextend(reln, forknum, - nextsegno * ((BlockNumber) RELSEG_SIZE) - 1, - zerobuf, skipFsync); - pfree(zerobuf); + mdzeroextend(reln, forknum, + nextsegno * ((BlockNumber) RELSEG_SIZE) - 1, + 1, skipFsync); } flags = O_CREAT; } -- 2.47.3
