From 09e03c3750f70da7e37fb6575c97511588b162d3 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 17 Oct 2024 14:14:35 -0400 Subject: [PATCH v2 12/15] bufmgr: Don't copy pages while writing out After the series of preceding commits introducing and using BufferPrepareToSetHintBits() hint bits cannot be set while IO is going on. Therefore we do not need to copy pages while they are being written out anymore. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/include/storage/bufpage.h | 3 +- src/backend/access/hash/hashpage.c | 2 +- src/backend/access/transam/xloginsert.c | 28 +++++---------- src/backend/storage/buffer/bufmgr.c | 23 +++++------- src/backend/storage/buffer/localbuf.c | 2 +- src/backend/storage/page/bufpage.c | 48 ++++--------------------- src/backend/storage/smgr/bulk_write.c | 2 +- 7 files changed, 26 insertions(+), 82 deletions(-) diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h index 6222d46e535..e8cadc73aad 100644 --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -507,7 +507,6 @@ extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems); extern void PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum); extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum, Item newtup, Size newsize); -extern char *PageSetChecksumCopy(Page page, BlockNumber blkno); -extern void PageSetChecksumInplace(Page page, BlockNumber blkno); +extern void PageSetChecksum(Page page, BlockNumber blkno); #endif /* BUFPAGE_H */ diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index d09c349e28f..2becf48bf06 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -1029,7 +1029,7 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks) zerobuf.data, true); - PageSetChecksumInplace(page, lastblock); + PageSetChecksum(page, lastblock); smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data, false); diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index f92d0626082..6fecbb0fe09 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -1052,6 +1052,8 @@ XLogCheckBufferNeedsBackup(Buffer buffer) * failures. So instead we copy the page and insert the copied data as normal * record data. * + * FIXME: outdated + * * We only need to do something if page has not yet been full page written in * this checkpoint round. The LSN of the inserted wal record is returned if we * had to write, InvalidXLogRecPtr otherwise. @@ -1089,37 +1091,23 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std) if (lsn <= RedoRecPtr) { int flags = 0; - PGAlignedBlock copied_buffer; char *origdata = (char *) BufferGetBlock(buffer); RelFileLocator rlocator; ForkNumber forkno; BlockNumber blkno; - /* - * Copy buffer so we don't have to worry about concurrent hint bit or - * lsn updates. We assume pd_lower/upper cannot be changed without an - * exclusive lock, so the contents bkp are not racy. - */ - if (buffer_std) - { - /* Assume we can omit data between pd_lower and pd_upper */ - Page page = BufferGetPage(buffer); - uint16 lower = ((PageHeader) page)->pd_lower; - uint16 upper = ((PageHeader) page)->pd_upper; - - memcpy(copied_buffer.data, origdata, lower); - memcpy(copied_buffer.data + upper, origdata + upper, BLCKSZ - upper); - } - else - memcpy(copied_buffer.data, origdata, BLCKSZ); - XLogBeginInsert(); if (buffer_std) flags |= REGBUF_STANDARD; BufferGetTag(buffer, &rlocator, &forkno, &blkno); - XLogRegisterBlock(0, &rlocator, forkno, blkno, copied_buffer.data, flags); + + /* + * XXX: Should probably be XLogRegisterBuffer(), but assertions make + * that non-viable right now. + */ + XLogRegisterBlock(0, &rlocator, forkno, blkno, origdata, flags); recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI_FOR_HINT); } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index dd94b9c16b0..6f39cc14596 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1996,9 +1996,8 @@ again: /* * If the buffer was dirty, try to write it out. There is a race * condition here, in that someone might dirty it after we released the - * buffer header lock above, or even while we are writing it out (since - * our share-lock won't prevent hint-bit updates). We will recheck the - * dirty bit after re-locking the buffer header. + * buffer header lock above. We will recheck the dirty bit after + * re-locking the buffer header. */ if (buf_state & BM_DIRTY) { @@ -3786,9 +3785,8 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum, * * The caller must hold a pin on the buffer and have share-locked the * buffer contents. (Note: a share-lock does not prevent updates of - * hint bits in the buffer, so the page could change while the write - * is in progress, but we assume that that will not invalidate the data - * written.) + * hint bits in the buffer, but hint bits are not allowed to be set after + * StartBufferIO() succeeded.) * * If the caller has an smgr reference for the buffer's relation, pass it * as the second parameter. If not, pass NULL. @@ -3801,7 +3799,6 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, ErrorContextCallback errcallback; instr_time io_start; Block bufBlock; - char *bufToWrite; uint32 buf_state; /* @@ -3867,12 +3864,8 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, */ bufBlock = BufHdrGetBlock(buf); - /* - * Update page checksum if desired. Since we have only shared lock on the - * buffer, other processes might be updating hint bits in it, so we must - * copy the page to private storage if we do checksumming. - */ - bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum); + /* Update page checksum if desired. */ + PageSetChecksum((Page) bufBlock, buf->tag.blockNum); io_start = pgstat_prepare_io_time(track_io_timing); @@ -3882,7 +3875,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, smgrwrite(reln, BufTagGetForkNum(&buf->tag), buf->tag.blockNum, - bufToWrite, + bufBlock, false); /* @@ -4531,7 +4524,7 @@ FlushRelationBuffers(Relation rel) errcallback.previous = error_context_stack; error_context_stack = &errcallback; - PageSetChecksumInplace(localpage, bufHdr->tag.blockNum); + PageSetChecksum(localpage, bufHdr->tag.blockNum); io_start = pgstat_prepare_io_time(track_io_timing); diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 6fd1a6418d2..b7cbbc57554 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -242,7 +242,7 @@ GetLocalVictimBuffer(void) /* Find smgr relation for buffer */ oreln = smgropen(BufTagGetRelFileLocator(&bufHdr->tag), MyProcNumber); - PageSetChecksumInplace(localpage, bufHdr->tag.blockNum); + PageSetChecksum(localpage, bufHdr->tag.blockNum); io_start = pgstat_prepare_io_time(track_io_timing); diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c index aa264f61b9c..f4822811331 100644 --- a/src/backend/storage/page/bufpage.c +++ b/src/backend/storage/page/bufpage.c @@ -1484,51 +1484,15 @@ PageIndexTupleOverwrite(Page page, OffsetNumber offnum, /* * Set checksum for a page in shared buffers. * - * If checksums are disabled, or if the page is not initialized, just return - * the input. Otherwise, we must make a copy of the page before calculating - * the checksum, to prevent concurrent modifications (e.g. setting hint bits) - * from making the final checksum invalid. It doesn't matter if we include or - * exclude hints during the copy, as long as we write a valid page and - * associated checksum. + * If checksums are disabled, or if the page is not initialized, just + * return. Otherwise compute and set the checksum. * - * Returns a pointer to the block-sized data that needs to be written. Uses - * statically-allocated memory, so the caller must immediately write the - * returned page and not refer to it again. - */ -char * -PageSetChecksumCopy(Page page, BlockNumber blkno) -{ - static char *pageCopy = NULL; - - /* If we don't need a checksum, just return the passed-in data */ - if (PageIsNew(page) || !DataChecksumsEnabled()) - return (char *) page; - - /* - * We allocate the copy space once and use it over on each subsequent - * call. The point of palloc'ing here, rather than having a static char - * array, is first to ensure adequate alignment for the checksumming code - * and second to avoid wasting space in processes that never call this. - */ - if (pageCopy == NULL) - pageCopy = MemoryContextAllocAligned(TopMemoryContext, - BLCKSZ, - PG_IO_ALIGN_SIZE, - 0); - - memcpy(pageCopy, (char *) page, BLCKSZ); - ((PageHeader) pageCopy)->pd_checksum = pg_checksum_page(pageCopy, blkno); - return pageCopy; -} - -/* - * Set checksum for a page in private memory. - * - * This must only be used when we know that no other process can be modifying - * the page buffer. + * In the past this needed to be done on a copy of the page, due to the + * possibility of e.g. hint bits being set concurrently. However, this is not + * necessary anymore as hint bits won't be set while IO is going on. */ void -PageSetChecksumInplace(Page page, BlockNumber blkno) +PageSetChecksum(Page page, BlockNumber blkno) { /* If we don't need a checksum, just return */ if (PageIsNew(page) || !DataChecksumsEnabled()) diff --git a/src/backend/storage/smgr/bulk_write.c b/src/backend/storage/smgr/bulk_write.c index f0a65bfe242..2c294ddab1f 100644 --- a/src/backend/storage/smgr/bulk_write.c +++ b/src/backend/storage/smgr/bulk_write.c @@ -277,7 +277,7 @@ smgr_bulk_flush(BulkWriteState *bulkstate) BlockNumber blkno = pending_writes[i].blkno; Page page = pending_writes[i].buf->data; - PageSetChecksumInplace(page, blkno); + PageSetChecksum(page, blkno); if (blkno >= bulkstate->pages_written) { -- 2.45.2.746.g06e570c0df.dirty