From 7a6abff9eb31cf812fdfa4804a853091a27c38e7 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 24 Oct 2022 12:18:18 -0700 Subject: [PATCH v1 09/12] Convert a few places to ExtendRelationBuffered Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/access/brin/brin.c | 13 ++++++---- src/backend/access/brin/brin_pageops.c | 4 +++ src/backend/access/brin/brin_revmap.c | 17 ++++-------- src/backend/access/gin/gininsert.c | 14 +++++----- src/backend/access/gin/ginutil.c | 15 +++-------- src/backend/access/gin/ginvacuum.c | 8 ++++++ src/backend/access/gist/gist.c | 6 +++-- src/backend/access/gist/gistutil.c | 16 +++--------- src/backend/access/gist/gistvacuum.c | 3 +++ src/backend/access/nbtree/nbtpage.c | 36 ++++++++------------------ src/backend/access/nbtree/nbtree.c | 3 +++ src/backend/access/spgist/spgutils.c | 15 +++-------- contrib/bloom/blutils.c | 14 +++------- 13 files changed, 70 insertions(+), 94 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 20b7d65b948..27266b84b0f 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -829,9 +829,11 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo) * whole relation will be rolled back. */ - meta = ReadBuffer(index, P_NEW); + meta = ExtendRelationBuffered(index, NULL, true, + index->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); Assert(BufferGetBlockNumber(meta) == BRIN_METAPAGE_BLKNO); - LockBuffer(meta, BUFFER_LOCK_EXCLUSIVE); brin_metapage_init(BufferGetPage(meta), BrinGetPagesPerRange(index), BRIN_CURRENT_VERSION); @@ -896,9 +898,10 @@ brinbuildempty(Relation index) Buffer metabuf; /* An empty BRIN index has a metapage only. */ - metabuf = - ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE); + metabuf = ExtendRelationBuffered(index, NULL, true, + index->rd_rel->relpersistence, + INIT_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); /* Initialize and xlog metabuffer. */ START_CRIT_SECTION(); diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c index f17aad51b63..8525d6be918 100644 --- a/src/backend/access/brin/brin_pageops.c +++ b/src/backend/access/brin/brin_pageops.c @@ -730,6 +730,10 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz, * There's not enough free space in any existing index page, * according to the FSM: extend the relation to obtain a shiny new * page. + * + * XXX: It's likely possible to use RBM_ZERO_AND_LOCK here, + * which'd avoid the need to hold the extension lock during buffer + * reclaim. */ if (!RELATION_IS_LOCAL(irel)) { diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.c index 6e392a551ad..8b7c72f38d3 100644 --- a/src/backend/access/brin/brin_revmap.c +++ b/src/backend/access/brin/brin_revmap.c @@ -538,7 +538,6 @@ revmap_physical_extend(BrinRevmap *revmap) BlockNumber mapBlk; BlockNumber nblocks; Relation irel = revmap->rm_irel; - bool needLock = !RELATION_IS_LOCAL(irel); /* * Lock the metapage. This locks out concurrent extensions of the revmap, @@ -570,10 +569,10 @@ revmap_physical_extend(BrinRevmap *revmap) } else { - if (needLock) - LockRelationForExtension(irel, ExclusiveLock); - - buf = ReadBuffer(irel, P_NEW); + buf = ExtendRelationBuffered(irel, NULL, false, + irel->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); if (BufferGetBlockNumber(buf) != mapBlk) { /* @@ -582,17 +581,11 @@ revmap_physical_extend(BrinRevmap *revmap) * up and have caller start over. We will have to evacuate that * page from under whoever is using it. */ - if (needLock) - UnlockRelationForExtension(irel, ExclusiveLock); LockBuffer(revmap->rm_metaBuf, BUFFER_LOCK_UNLOCK); - ReleaseBuffer(buf); + UnlockReleaseBuffer(buf); return; } - LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); page = BufferGetPage(buf); - - if (needLock) - UnlockRelationForExtension(irel, ExclusiveLock); } /* Check that it's a regular block (or an empty page) */ diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c index ea1c4184fbf..795efa199b0 100644 --- a/src/backend/access/gin/gininsert.c +++ b/src/backend/access/gin/gininsert.c @@ -440,12 +440,14 @@ ginbuildempty(Relation index) MetaBuffer; /* An empty GIN index has two pages. */ - MetaBuffer = - ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(MetaBuffer, BUFFER_LOCK_EXCLUSIVE); - RootBuffer = - ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(RootBuffer, BUFFER_LOCK_EXCLUSIVE); + MetaBuffer = ExtendRelationBuffered(index, NULL, true, + index->rd_rel->relpersistence, + INIT_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); + RootBuffer = ExtendRelationBuffered(index, NULL, true, + index->rd_rel->relpersistence, + INIT_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); /* Initialize and xlog metabuffer and root buffer. */ START_CRIT_SECTION(); diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c index 6df7f2eaeba..12a7b6d03e1 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -298,7 +298,6 @@ Buffer GinNewBuffer(Relation index) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -326,16 +325,10 @@ GinNewBuffer(Relation index) ReleaseBuffer(buffer); } - /* Must extend the file */ - needLock = !RELATION_IS_LOCAL(index); - if (needLock) - LockRelationForExtension(index, ExclusiveLock); - - buffer = ReadBuffer(index, P_NEW); - LockBuffer(buffer, GIN_EXCLUSIVE); - - if (needLock) - UnlockRelationForExtension(index, ExclusiveLock); + buffer = ExtendRelationBuffered(index, NULL, false, + index->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); return buffer; } diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index b4fa5f6bf81..8ec7b399287 100644 --- a/src/backend/access/gin/ginvacuum.c +++ b/src/backend/access/gin/ginvacuum.c @@ -736,6 +736,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats) */ needLock = !RELATION_IS_LOCAL(index); + /* + * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't + * think this is still required? + */ if (needLock) LockRelationForExtension(index, ExclusiveLock); npages = RelationGetNumberOfBlocks(index); @@ -786,6 +790,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats) stats->pages_free = totFreePages; + /* + * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't + * think this is still required? + */ if (needLock) LockRelationForExtension(index, ExclusiveLock); stats->num_pages = RelationGetNumberOfBlocks(index); diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 30069f139c7..014abdc53c0 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -133,8 +133,10 @@ gistbuildempty(Relation index) Buffer buffer; /* Initialize the root page */ - buffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); + buffer = ExtendRelationBuffered(index, NULL, true, + index->rd_rel->relpersistence, + INIT_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); /* Initialize and xlog buffer */ START_CRIT_SECTION(); diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c index 1532462317b..22d8c49b3f7 100644 --- a/src/backend/access/gist/gistutil.c +++ b/src/backend/access/gist/gistutil.c @@ -824,7 +824,6 @@ Buffer gistNewBuffer(Relation r) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -877,17 +876,10 @@ gistNewBuffer(Relation r) ReleaseBuffer(buffer); } - /* Must extend the file */ - needLock = !RELATION_IS_LOCAL(r); - - if (needLock) - LockRelationForExtension(r, ExclusiveLock); - - buffer = ReadBuffer(r, P_NEW); - LockBuffer(buffer, GIST_EXCLUSIVE); - - if (needLock) - UnlockRelationForExtension(r, ExclusiveLock); + buffer = ExtendRelationBuffered(r, NULL, false, + r->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); return buffer; } diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 0aa6e58a62f..d19afc1e4b1 100644 --- a/src/backend/access/gist/gistvacuum.c +++ b/src/backend/access/gist/gistvacuum.c @@ -203,6 +203,9 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, * we must already have processed any tuples due to be moved into such a * page. * + * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't + * think this issue still exists? + * * We can skip locking for new or temp relations, however, since no one * else could be accessing them. */ diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 8b96708b3ea..b4367b73630 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -881,7 +881,6 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access) } else { - bool needLock; Page page; Assert(access == BT_WRITE); @@ -962,31 +961,18 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access) } /* - * Extend the relation by one page. - * - * We have to use a lock to ensure no one else is extending the rel at - * the same time, else we will both try to initialize the same new - * page. We can skip locking for new or temp relations, however, - * since no one else could be accessing them. + * Extend the relation by one page. Need to use RBM_ZERO_AND_LOCK or + * we risk a race condition against btvacuumscan --- see comments + * therein. This forces us to repeat the valgrind request that + * _bt_lockbuf() otherwise would make, as we can't use _bt_lockbuf() + * without introducing a race. */ - needLock = !RELATION_IS_LOCAL(rel); - - if (needLock) - LockRelationForExtension(rel, ExclusiveLock); - - buf = ReadBuffer(rel, P_NEW); - - /* Acquire buffer lock on new page */ - _bt_lockbuf(rel, buf, BT_WRITE); - - /* - * Release the file-extension lock; it's now OK for someone else to - * extend the relation some more. Note that we cannot release this - * lock before we have buffer lock on the new page, or we risk a race - * condition against btvacuumscan --- see comments therein. - */ - if (needLock) - UnlockRelationForExtension(rel, ExclusiveLock); + buf = ExtendRelationBuffered(rel, NULL, false, + rel->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); + if (!RelationUsesLocalBuffers(rel)) + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(buf), BLCKSZ); /* Initialize the new page before returning it */ page = BufferGetPage(buf); diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index b52eca8f38b..904ec32354f 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -969,6 +969,9 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, * write-lock on the left page before it adds a right page, so we must * already have processed any tuples due to be moved into such a page. * + * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't + * think this issue still exists? + * * We can skip locking for new or temp relations, however, since no one * else could be accessing them. */ diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c index 2c661fcf96f..bd0993a0501 100644 --- a/src/backend/access/spgist/spgutils.c +++ b/src/backend/access/spgist/spgutils.c @@ -365,7 +365,6 @@ Buffer SpGistNewBuffer(Relation index) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -405,16 +404,10 @@ SpGistNewBuffer(Relation index) ReleaseBuffer(buffer); } - /* Must extend the file */ - needLock = !RELATION_IS_LOCAL(index); - if (needLock) - LockRelationForExtension(index, ExclusiveLock); - - buffer = ReadBuffer(index, P_NEW); - LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); - - if (needLock) - UnlockRelationForExtension(index, ExclusiveLock); + buffer = ExtendRelationBuffered(index, NULL, false, + index->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); return buffer; } diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index a434cf93efd..39b5ad47e18 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -353,7 +353,6 @@ Buffer BloomNewBuffer(Relation index) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -387,15 +386,10 @@ BloomNewBuffer(Relation index) } /* Must extend the file */ - needLock = !RELATION_IS_LOCAL(index); - if (needLock) - LockRelationForExtension(index, ExclusiveLock); - - buffer = ReadBuffer(index, P_NEW); - LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); - - if (needLock) - UnlockRelationForExtension(index, ExclusiveLock); + buffer = ExtendRelationBuffered(index, NULL, false, + index->rd_rel->relpersistence, + MAIN_FORKNUM, RBM_ZERO_AND_LOCK, + NULL); return buffer; } -- 2.38.0