Convert many uses of ReadBuffer[Extended](P_NEW) to 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

diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index a6d9f09f31585a2ee618d8868d9e7c5df0aad6ca..d935ed8fbdff4fbe79ee6bbff846dddd18970979 100644 (file)
--- 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;
 }

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 53e4721a54e2e683933534161eac7e692d17bba0..41bf950a4af4eff6d355c323e69376595b1e3544 100644 (file)
--- 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 ad5a89bd0516730b754f3e2b04d0419b7fdc3ef4..b578d2595451fa4cdbf45e1e3ba17ce714a49a79 100644 (file)
--- 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 7fc5226bf74702dbf8d620f3df363685f56a1bbc..f4271ba31c91659cb1ba67b56650f1446daa12d2 100644 (file)
--- 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 d5d748009ea09192948e068cae3fcd49b5e93893..be1841de7bfc00034271c95c6d3ab21606682d98 100644 (file)
--- 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 03fec1704e93917c5e0a720f99d64a1e8cccfb27..437f24753c03100beb7b3b07aaa3dc5a1de8f531 100644 (file)
--- 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 (;;)
@@ -328,15 +327,8 @@ GinNewBuffer(Relation index)
        }
 
        /* 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/gist/gist.c b/src/backend/access/gist/gist.c
index c3a3d49bca0311b5e89facb2d7e714f38c591a53..b5c1754e788c2f1b177456f135ea7760c29337f9 100644 (file)
--- 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 a607464b979a98501c3d0dd2654a9a25532566b5..f9f51152b8e18cb8299a0f2800779eb145a63d85 100644 (file)
--- 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 (;;)
@@ -878,16 +877,8 @@ gistNewBuffer(Relation r, Relation heaprel)
        }
 
        /* 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/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 2d8fdec98edc23cf74f654c30b1c5363242379b4..6d8af422609afb9cc666e9cb373c6cd94f4505eb 100644 (file)
--- 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 0144c3ab57108a2f48d22ded1f6b73b643d4986b..41aa1c4ccd1c1e8bbcccb647ef19302c6c8af47f 100644 (file)
--- 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.
-                */
-               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.
+                * 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.
                 */
-               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 409a2c1210040b9b472b3c6ee9b3ea4423a6922a..992f84834f836a51b7e330c4ef0b015f9ed5b81b 100644 (file)
--- 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.
         *
+        * XXX: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't
+        * think the use of the extension lock is still required.
+        *
         * 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 4e7ff1d16033e063cb8217d87e8b7e2b23e56834..190e4f76a9eec89da79b6edb9a460703245d3313 100644 (file)
--- 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 f3d1779655b9e35c71541a4675df636ea8d88dc8..ef014496782ad55cb3633820a266c994ba1e46f9 100644 (file)
--- 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);