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[email protected]

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);