From 11121f8ce57c9225c8bd3d4cf14cb89bead04af1 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 24 Oct 2022 12:18:18 -0700 Subject: [PATCH v7 08/14] Convert many uses of ReadBuffer[Extended](P_NEW) with ExtendBufferedRel() A few places are not converted. Some because they are tackled in later commits (e.g. hio.c, xlogutils.c), some because they are more complicated (e.g. brin_pageops.c). Having a few users of ReadBuffer(P_NEW) is good anyway, to ensure the backward compat path stays working. Discussion: https://postgr.es/m/20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de --- 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/hash/hashpage.c | 6 ++--- src/backend/access/nbtree/nbtpage.c | 34 +++++++------------------- src/backend/access/nbtree/nbtree.c | 3 +++ src/backend/access/spgist/spgutils.c | 13 ++-------- src/backend/commands/sequence.c | 3 ++- contrib/bloom/blutils.c | 12 ++------- 15 files changed, 53 insertions(+), 98 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 53e4721a54e..41bf950a4af 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -837,9 +837,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); @@ -904,9 +904,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 03fec1704e9..3c6f87236c5 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -299,7 +299,6 @@ Buffer GinNewBuffer(Relation index) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -327,16 +326,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 c3a3d49bca0..b5c1754e788 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -134,8 +134,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 a607464b979..70861884113 100644 --- a/src/backend/access/gist/gistutil.c +++ b/src/backend/access/gist/gistutil.c @@ -824,7 +824,6 @@ Buffer gistNewBuffer(Relation r, Relation heaprel) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -877,17 +876,8 @@ gistNewBuffer(Relation r, Relation heaprel) 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/hash/hashpage.c b/src/backend/access/hash/hashpage.c index 2d8fdec98ed..6d8af422609 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -206,14 +206,14 @@ _hash_getnewbuf(Relation rel, BlockNumber blkno, ForkNumber forkNum) elog(ERROR, "access to noncontiguous page in hash index \"%s\"", RelationGetRelationName(rel)); - /* smgr insists we use P_NEW to extend the relation */ + /* smgr insists we explicitly extend the relation */ if (blkno == nblocks) { - buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_NORMAL, NULL); + buf = ExtendBufferedRel(EB_REL(rel), forkNum, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); if (BufferGetBlockNumber(buf) != blkno) elog(ERROR, "unexpected hash relation size: %u, should be %u", BufferGetBlockNumber(buf), blkno); - LockBuffer(buf, HASH_WRITE); } else { diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 0144c3ab571..41aa1c4ccd1 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -882,7 +882,6 @@ _bt_getbuf(Relation rel, Relation heaprel, BlockNumber blkno, int access) } else { - bool needLock; Page page; Assert(access == BT_WRITE); @@ -963,31 +962,16 @@ _bt_getbuf(Relation rel, Relation heaprel, 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 409a2c12100..75b4a2cc364 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -970,6 +970,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 4e7ff1d1603..190e4f76a9e 100644 --- a/src/backend/access/spgist/spgutils.c +++ b/src/backend/access/spgist/spgutils.c @@ -366,7 +366,6 @@ Buffer SpGistNewBuffer(Relation index) { Buffer buffer; - bool needLock; /* First, try to get a page from FSM */ for (;;) @@ -406,16 +405,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/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index f3d1779655b..ef014496782 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -377,7 +377,8 @@ fill_seq_fork_with_data(Relation rel, HeapTuple tuple, ForkNumber forkNum) /* Initialize first page of relation with special magic number */ - buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_ZERO_AND_LOCK, NULL); + buf = ExtendBufferedRel(EB_REL(rel), forkNum, NULL, + EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); Assert(BufferGetBlockNumber(buf) == 0); page = BufferGetPage(buf); 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