From 188abd0e2f1158be92c92b735f9d0ddbc08dc798 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 24 Oct 2022 12:18:18 -0700 Subject: [PATCH v4 10/15] Convert a few places to ExtendBufferedRel Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/access/brin/brin.c | 9 +++---- src/backend/access/brin/brin_pageops.c | 4 +++ src/backend/access/brin/brin_revmap.c | 15 +++--------- src/backend/access/gin/gininsert.c | 10 +++----- src/backend/access/gin/ginutil.c | 13 ++-------- src/backend/access/gin/ginvacuum.c | 8 ++++++ src/backend/access/gist/gist.c | 4 +-- src/backend/access/gist/gistutil.c | 14 ++--------- src/backend/access/gist/gistvacuum.c | 3 +++ src/backend/access/nbtree/nbtpage.c | 34 +++++++------------------- src/backend/access/nbtree/nbtree.c | 3 +++ src/backend/access/spgist/spgutils.c | 13 ++-------- contrib/bloom/blutils.c | 12 ++------- 13 files changed, 48 insertions(+), 94 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index b5a5fa7b334..b283e987152 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -836,9 +836,9 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo) * whole relation will be rolled back. */ - meta = ReadBuffer(index, P_NEW); + meta = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); Assert(BufferGetBlockNumber(meta) == BRIN_METAPAGE_BLKNO); - LockBuffer(meta, BUFFER_LOCK_EXCLUSIVE); brin_metapage_init(BufferGetPage(meta), BrinGetPagesPerRange(index), BRIN_CURRENT_VERSION); @@ -903,9 +903,8 @@ 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 = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); /* 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 ad5a89bd051..b578d259545 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 7fc5226bf74..f4271ba31c9 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,8 @@ revmap_physical_extend(BrinRevmap *revmap) } else { - if (needLock) - LockRelationForExtension(irel, ExclusiveLock); - - buf = ReadBuffer(irel, P_NEW); + buf = ExtendBufferedRel(EB_REL(irel), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); if (BufferGetBlockNumber(buf) != mapBlk) { /* @@ -582,17 +579,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 d5d748009ea..be1841de7bf 100644 --- a/src/backend/access/gin/gininsert.c +++ b/src/backend/access/gin/gininsert.c @@ -440,12 +440,10 @@ 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 = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); + RootBuffer = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); /* 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 f05128ecf50..be2cf78d0d4 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,8 @@ 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 = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); return buffer; } diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index e5d310d8362..13251d7e07d 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 ba394f08f61..3865955c394 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -133,8 +133,8 @@ 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 = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL, + EB_SKIP_EXTENSION_LOCK | EB_LOCK_FIRST); /* Initialize and xlog buffer */ START_CRIT_SECTION(); diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c index b4d843a0ff1..446fcb3cc5e 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,8 @@ 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 = ExtendBufferedRel(EB_REL(r), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); return buffer; } diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 3f60d3274d2..cc711b04986 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 3feee28d197..c4f16b3d2b7 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,16 @@ _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 = ExtendBufferedRel(EB_REL(rel), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); + 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 3f7b541e9d2..bc184813a70 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 3761f2c193b..0775b7d2618 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,8 @@ 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 = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); return buffer; } diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index a6d9f09f315..d935ed8fbdf 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,8 @@ 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 = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL, + EB_LOCK_FIRST); return buffer; } -- 2.38.0