From dd75c4b1fec6ed30eb7aa88152b3479396855d51 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 1 Mar 2023 13:08:17 -0800 Subject: [PATCH v6 16/17] Convert a few places to ExtendBufferedRelTo --- src/backend/access/heap/visibilitymap.c | 80 +++++++--------------- src/backend/access/transam/xlogutils.c | 29 ++------ src/backend/storage/freespace/freespace.c | 83 +++++++---------------- 3 files changed, 55 insertions(+), 137 deletions(-) diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 709213d0d97..60b96f5df80 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -126,7 +126,7 @@ /* prototypes for internal routines */ static Buffer vm_readbuf(Relation rel, BlockNumber blkno, bool extend); -static void vm_extend(Relation rel, BlockNumber vm_nblocks); +static Buffer vm_extend(Relation rel, BlockNumber vm_nblocks); /* @@ -575,22 +575,29 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend) reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0; } - /* Handle requests beyond EOF */ + /* + * For reading we use ZERO_ON_ERROR mode, and initialize the page if + * necessary. It's always safe to clear bits, so it's better to clear + * corrupt pages than error out. + * + * We use the same path below to initialize pages when extending the + * relation, as a concurrent extension can end up with vm_extend() + * returning an already-initialized page. + */ if (blkno >= reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM]) { if (extend) - vm_extend(rel, blkno + 1); + buf = vm_extend(rel, blkno + 1); else return InvalidBuffer; } + else + buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno, + RBM_ZERO_ON_ERROR, NULL); /* - * Use ZERO_ON_ERROR mode, and initialize the page if necessary. It's - * always safe to clear bits, so it's better to clear corrupt pages than - * error out. - * - * The initialize-the-page part is trickier than it looks, because of the - * possibility of multiple backends doing this concurrently, and our + * Initializing the page when needed is trickier than it looks, because of + * the possibility of multiple backends doing this concurrently, and our * desire to not uselessly take the buffer lock in the normal path where * the page is OK. We must take the lock to initialize the page, so * recheck page newness after we have the lock, in case someone else @@ -603,8 +610,6 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend) * long as it doesn't depend on the page header having correct contents. * Current usage is safe because PageGetContents() does not require that. */ - buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno, - RBM_ZERO_ON_ERROR, NULL); if (PageIsNew(BufferGetPage(buf))) { LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); @@ -619,51 +624,16 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend) * Ensure that the visibility map fork is at least vm_nblocks long, extending * it if necessary with zeroed pages. */ -static void +static Buffer vm_extend(Relation rel, BlockNumber vm_nblocks) { - BlockNumber vm_nblocks_now; - PGAlignedBlock pg = {0}; - SMgrRelation reln; + Buffer buf; - /* - * We use the relation extension lock to lock out other backends trying to - * extend the visibility map at the same time. It also locks out extension - * of the main fork, unnecessarily, but extending the visibility map - * happens seldom enough that it doesn't seem worthwhile to have a - * separate lock tag type for it. - * - * Note that another backend might have extended or created the relation - * by the time we get the lock. - */ - LockRelationForExtension(rel, ExclusiveLock); - - /* - * Caution: re-using this smgr pointer could fail if the relcache entry - * gets closed. It's safe as long as we only do smgr-level operations - * between here and the last use of the pointer. - */ - reln = RelationGetSmgr(rel); - - /* - * Create the file first if it doesn't exist. If smgr_vm_nblocks is - * positive then it must exist, no need for an smgrexists call. - */ - if ((reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 || - reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) && - !smgrexists(reln, VISIBILITYMAP_FORKNUM)) - smgrcreate(reln, VISIBILITYMAP_FORKNUM, false); - - /* Invalidate cache so that smgrnblocks() asks the kernel. */ - reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber; - vm_nblocks_now = smgrnblocks(reln, VISIBILITYMAP_FORKNUM); - - /* Now extend the file */ - while (vm_nblocks_now < vm_nblocks) - { - smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false); - vm_nblocks_now++; - } + buf = ExtendBufferedRelTo(EB_REL(rel), VISIBILITYMAP_FORKNUM, NULL, + EB_CREATE_FORK_IF_NEEDED | + EB_CLEAR_SIZE_CACHE, + vm_nblocks, + RBM_ZERO_ON_ERROR); /* * Send a shared-inval message to force other backends to close any smgr @@ -672,7 +642,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks) * to keep checking for creation or extension of the file, which happens * infrequently. */ - CacheInvalidateSmgr(reln->smgr_rlocator); + CacheInvalidateSmgr(RelationGetSmgr(rel)->smgr_rlocator); - UnlockRelationForExtension(rel, ExclusiveLock); + return buf; } diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 2c28956b1aa..e174a2a8919 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -524,28 +524,13 @@ XLogReadBufferExtended(RelFileLocator rlocator, ForkNumber forknum, /* OK to extend the file */ /* we do this in recovery only - no rel-extension lock needed */ Assert(InRecovery); - buffer = InvalidBuffer; - do - { - if (buffer != InvalidBuffer) - { - if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK) - LockBuffer(buffer, BUFFER_LOCK_UNLOCK); - ReleaseBuffer(buffer); - } - buffer = ReadBufferWithoutRelcache(rlocator, forknum, - P_NEW, mode, NULL, true); - } - while (BufferGetBlockNumber(buffer) < blkno); - /* Handle the corner case that P_NEW returns non-consecutive pages */ - if (BufferGetBlockNumber(buffer) != blkno) - { - if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK) - LockBuffer(buffer, BUFFER_LOCK_UNLOCK); - ReleaseBuffer(buffer); - buffer = ReadBufferWithoutRelcache(rlocator, forknum, blkno, - mode, NULL, true); - } + buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT), + forknum, + NULL, + EB_PERFORMING_RECOVERY | + EB_SKIP_EXTENSION_LOCK, + blkno + 1, + mode); } recent_buffer_fast_path: diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c index 90c529958e7..2face615d07 100644 --- a/src/backend/storage/freespace/freespace.c +++ b/src/backend/storage/freespace/freespace.c @@ -98,7 +98,7 @@ static BlockNumber fsm_get_heap_blk(FSMAddress addr, uint16 slot); static BlockNumber fsm_logical_to_physical(FSMAddress addr); static Buffer fsm_readbuf(Relation rel, FSMAddress addr, bool extend); -static void fsm_extend(Relation rel, BlockNumber fsm_nblocks); +static Buffer fsm_extend(Relation rel, BlockNumber fsm_nblocks); /* functions to convert amount of free space to a FSM category */ static uint8 fsm_space_avail_to_cat(Size avail); @@ -558,24 +558,30 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend) reln->smgr_cached_nblocks[FSM_FORKNUM] = 0; } - /* Handle requests beyond EOF */ + /* + * For reading we use ZERO_ON_ERROR mode, and initialize the page if + * necessary. The FSM information is not accurate anyway, so it's better + * to clear corrupt pages than error out. Since the FSM changes are not + * WAL-logged, the so-called torn page problem on crash can lead to pages + * with corrupt headers, for example. + * + * We use the same path below to initialize pages when extending the + * relation, as a concurrent extension can end up with vm_extend() + * returning an already-initialized page. + */ if (blkno >= reln->smgr_cached_nblocks[FSM_FORKNUM]) { if (extend) - fsm_extend(rel, blkno + 1); + buf = fsm_extend(rel, blkno + 1); else return InvalidBuffer; } + else + buf = ReadBufferExtended(rel, FSM_FORKNUM, blkno, RBM_ZERO_ON_ERROR, NULL); /* - * Use ZERO_ON_ERROR mode, and initialize the page if necessary. The FSM - * information is not accurate anyway, so it's better to clear corrupt - * pages than error out. Since the FSM changes are not WAL-logged, the - * so-called torn page problem on crash can lead to pages with corrupt - * headers, for example. - * - * The initialize-the-page part is trickier than it looks, because of the - * possibility of multiple backends doing this concurrently, and our + * Initializing the page when needed is trickier than it looks, because of + * the possibility of multiple backends doing this concurrently, and our * desire to not uselessly take the buffer lock in the normal path where * the page is OK. We must take the lock to initialize the page, so * recheck page newness after we have the lock, in case someone else @@ -588,7 +594,6 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend) * long as it doesn't depend on the page header having correct contents. * Current usage is safe because PageGetContents() does not require that. */ - buf = ReadBufferExtended(rel, FSM_FORKNUM, blkno, RBM_ZERO_ON_ERROR, NULL); if (PageIsNew(BufferGetPage(buf))) { LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); @@ -604,56 +609,14 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend) * it if necessary with empty pages. And by empty, I mean pages filled * with zeros, meaning there's no free space. */ -static void +static Buffer fsm_extend(Relation rel, BlockNumber fsm_nblocks) { - BlockNumber fsm_nblocks_now; - PGAlignedBlock pg = {0}; - SMgrRelation reln; - - - /* - * We use the relation extension lock to lock out other backends trying to - * extend the FSM at the same time. It also locks out extension of the - * main fork, unnecessarily, but extending the FSM happens seldom enough - * that it doesn't seem worthwhile to have a separate lock tag type for - * it. - * - * Note that another backend might have extended or created the relation - * by the time we get the lock. - */ - LockRelationForExtension(rel, ExclusiveLock); - - /* - * Caution: re-using this smgr pointer could fail if the relcache entry - * gets closed. It's safe as long as we only do smgr-level operations - * between here and the last use of the pointer. - */ - reln = RelationGetSmgr(rel); - - /* - * Create the FSM file first if it doesn't exist. If - * smgr_cached_nblocks[FSM_FORKNUM] is positive then it must exist, no - * need for an smgrexists call. - */ - if ((reln->smgr_cached_nblocks[FSM_FORKNUM] == 0 || - reln->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber) && - !smgrexists(reln, FSM_FORKNUM)) - smgrcreate(reln, FSM_FORKNUM, false); - - /* Invalidate cache so that smgrnblocks() asks the kernel. */ - reln->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber; - fsm_nblocks_now = smgrnblocks(reln, FSM_FORKNUM); - - /* Extend as needed. */ - while (fsm_nblocks_now < fsm_nblocks) - { - smgrextend(reln, FSM_FORKNUM, fsm_nblocks_now, - pg.data, false); - fsm_nblocks_now++; - } - - UnlockRelationForExtension(rel, ExclusiveLock); + return ExtendBufferedRelTo(EB_REL(rel), FSM_FORKNUM, NULL, + EB_CREATE_FORK_IF_NEEDED | + EB_CLEAR_SIZE_CACHE, + fsm_nblocks, + RBM_ZERO_ON_ERROR); } /* -- 2.38.0