Subject: [PATCH v5 12/15] hio: Use ExtendBufferedRelBy()

From: Andres Freund <[email-masked]>
Date: Wed, 26 Oct 2022 14:14:11 -0700

---
 src/backend/access/heap/hio.c | 285 +++++++++++++++++-----------------
 1 file changed, 146 insertions(+), 139 deletions(-)

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 65886839e7..48cfcff975 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -354,6 +270,9 @@ RelationGetBufferForTuple(Relation relation, Size len,

so in RelationGetBufferForTuple() up above where your changes start,
there is this code

    /*
    * We first try to put the tuple on the same page we last inserted a tuple
    * on, as cached in the BulkInsertState or relcache entry.  If that
    * doesn't work, we ask the Free Space Map to locate a suitable page.
    * Since the FSM's info might be out of date, we have to be prepared to
    * loop around and retry multiple times. (To insure this isn't an infinite
    * loop, we must update the FSM with the correct amount of free space on
    * each page that proves not to be suitable.)  If the FSM has no record of
    * a page with enough free space, we give up and extend the relation.
    *
    * When use_fsm is false, we either put the tuple onto the existing target
    * page or extend the relation.
    */
    if (bistate && bistate->current_buf != InvalidBuffer)
    {
        targetBlock = BufferGetBlockNumber(bistate->current_buf);
    }
    else
        targetBlock = RelationGetTargetBlock(relation);

    if (targetBlock == InvalidBlockNumber && use_fsm)
    {
        /*
        * We have no cached target page, so ask the FSM for an initial
        * target.
        */
        targetBlock = GetPageWithFreeSpace(relation, targetFreeSpace);
    }

And, I was thinking how, ReadBufferBI() only has one caller now
(RelationGetBufferForTuple()) and, this caller basically already has
checked for the case in the inside of ReadBufferBI() (the code I pasted
above)

    /* If we have the desired block already pinned, re-pin and return it */
    if (bistate->current_buf != InvalidBuffer)
    {
        if (BufferGetBlockNumber(bistate->current_buf) == targetBlock)
        {
            /*
            * Currently the LOCK variants are only used for extending
            * relation, which should never reach this branch.
            */
            Assert(mode != RBM_ZERO_AND_LOCK &&
                  mode != RBM_ZERO_AND_CLEANUP_LOCK);

            IncrBufferRefCount(bistate->current_buf);
            return bistate->current_buf;
        }
        /* ... else drop the old buffer */

So, I was thinking maybe there is some way to inline the logic for
ReadBufferBI(), because I think it would feel more streamlined to me.

@@ -558,18 +477,46 @@ loop:
             ReleaseBuffer(buffer);
         }

Oh, and I forget which commit introduced BulkInsertState->next_free and
last_free, but I remember thinking that it didn't seem to fit with the
other parts of that commit.

-        /* Without FSM, always fall out of the loop and extend */
-        if (!use_fsm)
-            break;
+        if (bistate
+            && bistate->next_free != InvalidBlockNumber
+            && bistate->next_free <= bistate->last_free)
+        {
+            /*
+            * We bulk extended the relation before, and there are still some
+            * unused pages from that extension, so we don't need to look in
+            * the FSM for a new page. But do record the free space from the
+            * last page, somebody might insert narrower tuples later.
+            */

Why couldn't we have found out that we bulk-extended before and get the
block from there up above the while loop?

+            if (use_fsm)
+                RecordPageWithFreeSpace(relation, targetBlock, pageFreeSpace);

-        /*
-        * Update FSM as to condition of this page, and ask for another page
-        * to try.
-        */
-        targetBlock = RecordAndGetPageWithFreeSpace(relation,
-                                                    targetBlock,
-                                                    pageFreeSpace,
-                                                    targetFreeSpace);
+            Assert(bistate->last_free != InvalidBlockNumber &&

You don't need the below half of the assert.

+                  bistate->next_free <= bistate->last_free);
+            targetBlock = bistate->next_free;
+            if (bistate->next_free >= bistate->last_free)

they can only be equal at this point

+            {
+                bistate->next_free = InvalidBlockNumber;
+                bistate->last_free = InvalidBlockNumber;
+            }
+            else
+                bistate->next_free++;
+        }
+        else if (!use_fsm)
+        {
+            /* Without FSM, always fall out of the loop and extend */
+            break;
+        }

It would be nice to have a comment explaining why this is in its own
else if instead of breaking earlier (i.e. !use_fsm is still a valid case
in the if branch above it)

+        else
+        {
+            /*
+            * Update FSM as to condition of this page, and ask for another
+            * page to try.
+            */
+            targetBlock = RecordAndGetPageWithFreeSpace(relation,
+                                                        targetBlock,
+                                                        pageFreeSpace,
+                                                        targetFreeSpace);
+        }

we can get rid of needLock and waitcount variables like this

+#define MAX_BUFFERS 64
+        Buffer        victim_buffers[MAX_BUFFERS];
+        BlockNumber firstBlock = InvalidBlockNumber;
+        BlockNumber firstBlockFSM = InvalidBlockNumber;
+        BlockNumber curBlock;
+        uint32        extend_by_pages;
+        uint32        no_fsm_pages;
+        uint32        waitcount;
+
+        extend_by_pages = num_pages;
+
+        /*
+        * Multiply the number of pages to extend by the number of waiters. Do
+        * this even if we're not using the FSM, as it does relieve
+        * contention. Pages will be found via bistate->next_free.
+        */
+        if (needLock)
+            waitcount = RelationExtensionLockWaiterCount(relation);
+        else
+            waitcount = 0;
+        extend_by_pages += extend_by_pages * waitcount;

        if (!RELATION_IS_LOCAL(relation))
            extend_by_pages += extend_by_pages *
RelationExtensionLockWaiterCount(relation);

+
+        /*
+        * can't extend by more than MAX_BUFFERS, we need to pin them all
+        * concurrently. FIXME: Need an NBuffers / MaxBackends type limit
+        * here.
+        */
+        extend_by_pages = Min(extend_by_pages, MAX_BUFFERS);
+
+        /*
+        * How many of the extended pages not to enter into the FSM.
+        *
+        * Only enter pages that we don't need ourselves into the FSM.
+        * Otherwise every other backend will immediately try to use the pages
+        * this backend neds itself, causing unnecessary contention.
+        *
+        * Bulk extended pages are remembered in bistate->next_free_buffer. So
+        * without a bistate we can't directly make use of them.
+        *
+        * Never enter the page returned into the FSM, we'll immediately use
+        * it.
+        */
+        if (num_pages > 1 && bistate == NULL)
+            no_fsm_pages = 1;
+        else
+            no_fsm_pages = num_pages;

this is more clearly this:
  no_fsm_pages = bistate == NULL ? 1 : num_pages;

-    /*
-    * Release the file-extension lock; it's now OK for someone else to extend
-    * the relation some more.
-    */
-    if (needLock)
-        UnlockRelationForExtension(relation, ExclusiveLock);
+        if (bistate)
+        {
+            if (extend_by_pages > 1)
+            {
+                bistate->next_free = firstBlock + 1;
+                bistate->last_free = firstBlock + extend_by_pages - 1;
+            }
+            else
+            {
+                bistate->next_free = InvalidBlockNumber;
+                bistate->last_free = InvalidBlockNumber;
+            }
+        }
+
+        buffer = victim_buffers[0];

If we move buffer = up, we can have only one if (bistate)

+        if (bistate)
+        {
+            IncrBufferRefCount(buffer);
+            bistate->current_buf = buffer;
+        }
+    }

like this:

        buffer = victim_buffers[0];

        if (bistate)
        {
            if (extend_by_pages > 1)
            {
                bistate->next_free = firstBlock + 1;
                bistate->last_free = firstBlock + extend_by_pages - 1;
            }
            else
            {
                bistate->next_free = InvalidBlockNumber;
                bistate->last_free = InvalidBlockNumber;
            }

            IncrBufferRefCount(buffer);
            bistate->current_buf = buffer;
        }