Subject: [PATCH v5 11/14] hio: Use ExtendBufferedRelBy()

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

---
 src/backend/access/heap/hio.c | 332 ++++++++++++++++++++--------------
 1 file changed, 194 insertions(+), 138 deletions(-)

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 65886839e70..40f53d7177e 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -186,89 +186,176 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
 }
 
 /*
- * Extend a relation by multiple blocks to avoid future contention on the
- * relation extension lock.  Our goal is to pre-extend the relation by an
- * amount which ramps up as the degree of contention ramps up, but limiting
- * the result to some sane overall value.
+ * Extend the relation. By multiple pages, if beneficial.
+ *
+ * If the caller needs multiple pages (num_pages > 1), we always try to extend
+ * by at least that much.
+ *
+ * If there is contention on the extension lock, we don't just extend "for
+ * ourselves", but we try to help others. We can do so by adding empty pages
+ * into the FSM. Typically there is no contention when we can't use the FSM.
+ *
+ * We do have to limit the number of pages to extend by to some value, as the
+ * buffers for all the extended pages need to, temporarily, be pinned. For now
+ * we define MAX_BUFFERS_TO_EXTEND_BY to be 64 buffers, it's hard to see
+ * benefits with higher numbers. This partially is because copyfrom.c's
+ * MAX_BUFFERED_TUPLES / MAX_BUFFERED_BYTES prevents larger multi_inserts.
  */
-static void
-RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
+static Buffer
+RelationAddBlocks(Relation relation, BulkInsertState bistate,
+				  int num_pages,  bool use_fsm)
 {
-	BlockNumber blockNum,
-				firstBlock = InvalidBlockNumber;
-	int			extraBlocks;
-	int			lockWaiters;
-
-	/* Use the length of the lock wait queue to judge how much to extend. */
-	lockWaiters = RelationExtensionLockWaiterCount(relation);
-	if (lockWaiters <= 0)
-		return;
+#define MAX_BUFFERS_TO_EXTEND_BY 64
+	Buffer		victim_buffers[MAX_BUFFERS_TO_EXTEND_BY];
+	BlockNumber firstBlock = InvalidBlockNumber;
+	BlockNumber firstBlockFSM = InvalidBlockNumber;
+	uint32		extend_by_pages;
+	uint32		not_in_fsm_pages;
+	BlockNumber curBlock;
+	uint32		waitcount;
+	Buffer		buffer;
 
 	/*
-	 * It might seem like multiplying the number of lock waiters by as much as
-	 * 20 is too aggressive, but benchmarking revealed that smaller numbers
-	 * were insufficient.  512 is just an arbitrary cap to prevent
-	 * pathological results.
+	 * Determine by how many pages to try to extend by.
 	 */
-	extraBlocks = Min(512, lockWaiters * 20);
-
-	do
+	if (bistate == NULL && !use_fsm)
 	{
-		Buffer		buffer;
-		Page		page;
-		Size		freespace;
-
 		/*
-		 * Extend by one page.  This should generally match the main-line
-		 * extension code in RelationGetBufferForTuple, except that we hold
-		 * the relation extension lock throughout, and we don't immediately
-		 * initialize the page (see below).
+		 * If we have neither bistate, nor can use the FSM, we can't bulk
+		 * extend - there'd be no way to find the additional pages.
 		 */
-		buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);
-		page = BufferGetPage(buffer);
-
-		if (!PageIsNew(page))
-			elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
-				 BufferGetBlockNumber(buffer),
-				 RelationGetRelationName(relation));
-
-		/*
-		 * Add the page to the FSM without initializing. If we were to
-		 * initialize here, the page would potentially get flushed out to disk
-		 * before we add any useful content. There's no guarantee that that'd
-		 * happen before a potential crash, so we need to deal with
-		 * uninitialized pages anyway, thus avoid the potential for
-		 * unnecessary writes.
-		 */
-
-		/* we'll need this info below */
-		blockNum = BufferGetBlockNumber(buffer);
-		freespace = BufferGetPageSize(buffer) - SizeOfPageHeaderData;
-
-		UnlockReleaseBuffer(buffer);
-
-		/* Remember first block number thus added. */
-		if (firstBlock == InvalidBlockNumber)
-			firstBlock = blockNum;
-
-		/*
-		 * Immediately update the bottom level of the FSM.  This has a good
-		 * chance of making this page visible to other concurrently inserting
-		 * backends, and we want that to happen without delay.
-		 */
-		RecordPageWithFreeSpace(relation, blockNum, freespace);
+		extend_by_pages = 1;
+	}
+	else
+	{
+		/*
+		 * Try to extend at least by the number of pages the caller needs. We
+		 * can remember the additional pages (either via FSM or bistate).
+		 */
+		extend_by_pages = num_pages;
+
+		if (!RELATION_IS_LOCAL(relation))
+			waitcount = RelationExtensionLockWaiterCount(relation);
+		else
+			waitcount = 0;
+
+		/*
+		 * Multiply the number of pages to extend by the number of waiters. Do
+		 * this even if we're not using the FSM, as it still relieves
+		 * contention, by deferring the next time this backend needs to
+		 * extend. In that case the extended pages will be found via
+		 * bistate->next_free.
+		 */
+		extend_by_pages += extend_by_pages * waitcount;
+
+		/*
+		 * Can't extend by more than MAX_BUFFERS, we need to pin them all
+		 * concurrently.
+		 */
+		extend_by_pages = Min(extend_by_pages, MAX_BUFFERS_TO_EXTEND_BY);
 	}
-	while (--extraBlocks > 0);
 
 	/*
-	 * Updating the upper levels of the free space map is too expensive to do
-	 * for every block, but it's worth doing once at the end to make sure that
-	 * subsequent insertion activity sees all of those nifty free pages we
-	 * just inserted.
+	 * How many of the extended pages should be entered into the FSM?
+	 *
+	 * If we have a bistate, 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. If we don't have a bistate, we can't avoid the FSM.
+	 *
+	 * Never enter the page returned into the FSM, we'll immediately use it.
 	 */
-	FreeSpaceMapVacuumRange(relation, firstBlock, blockNum + 1);
+	if (num_pages > 1 && bistate == NULL)
+		not_in_fsm_pages = 1;
+	else
+		not_in_fsm_pages = num_pages;
+
+	/* prepare to put another buffer into the bistate */
+	if (bistate && bistate->current_buf != InvalidBuffer)
+	{
+		ReleaseBuffer(bistate->current_buf);
+		bistate->current_buf = InvalidBuffer;
+	}
+
+	/*
+	 * Extend the relation. We ask for the first returned page to be locked,
+	 * so that we are sure that nobody has inserted into the page
+	 * concurrently.
+	 *
+	 * With the current MAX_BUFFERS_TO_EXTEND_BY there's no danger of
+	 * [auto]vacuum trying to truncate later pages as REL_TRUNCATE_MINIMUM is
+	 * way larger.
+	 */
+	firstBlock = ExtendBufferedRelBy(EB_REL(relation), MAIN_FORKNUM,
+									 bistate ? bistate->strategy : NULL,
+									 EB_LOCK_FIRST,
+									 extend_by_pages,
+									 victim_buffers,
+									 &extend_by_pages);
+	/* the buffer the function will return */
+	buffer = victim_buffers[0];
+
+	/*
+	 * Relation is now extended. Release pins on all buffers, except for the
+	 * first (which we'll return).  If we decided to put pages into the FSM,
+	 * we can do that as part of the same loop.
+	 *
+	 * FIXME: Figure out how to better deal with doing this operation while
+	 * holding a buffer lock. Likely we can just release the buffer lock (to
+	 * reacquire it later) after initializing the page - the target page isn't
+	 * in the FSM, so it's going to be very rare that it's found.
+	 */
+	curBlock = firstBlock;
+	for (uint32 i = 0; i < extend_by_pages; i++, curBlock++)
+	{
+		Assert(curBlock == BufferGetBlockNumber(victim_buffers[i]));
+		Assert(BlockNumberIsValid(curBlock));
+
+		/* don't release the pin on the page returned by this function */
+		if (i > 0)
+			ReleaseBuffer(victim_buffers[i]);
+
+		if (i >= not_in_fsm_pages && use_fsm)
+		{
+			if (firstBlockFSM == InvalidBlockNumber)
+				firstBlockFSM = curBlock;
+
+			RecordPageWithFreeSpace(relation,
+									curBlock,
+									BufferGetPageSize(victim_buffers[i]) - SizeOfPageHeaderData);
+		}
+	}
+
+	if (use_fsm && firstBlockFSM != InvalidBlockNumber)
+		FreeSpaceMapVacuumRange(relation, firstBlockFSM, firstBlock + num_pages);
+
+	if (bistate)
+	{
+		/*
+		 * Remember the pages we extended by so we can use them without
+		 * looking into the FSM.
+		 */
+		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;
+		}
+
+		/* maintain bistate->current_buf */
+		IncrBufferRefCount(buffer);
+		bistate->current_buf = buffer;
+	}
+
+	return buffer;
+#undef MAX_BUFFERS_TO_EXTEND_BY
 }
 
+
 /*
  * RelationGetBufferForTuple
  *
@@ -350,10 +437,12 @@ RelationGetBufferForTuple(Relation relation, Size len,
 				targetFreeSpace = 0;
 	BlockNumber targetBlock,
 				otherBlock;
-	bool		needLock;
 
 	len = MAXALIGN(len);		/* be conservative */
 
+	if (num_pages <= 0)
+		num_pages = 1;
+
 	/* Bulk insert is not supported for updates, only inserts. */
 	Assert(otherBuffer == InvalidBuffer || !bistate);
 
@@ -558,83 +647,50 @@ loop:
 			ReleaseBuffer(buffer);
 		}
 
-		/* Without FSM, always fall out of the loop and extend */
-		if (!use_fsm)
-			break;
-
-		/*
-		 * Update FSM as to condition of this page, and ask for another page
-		 * to try.
-		 */
-		targetBlock = RecordAndGetPageWithFreeSpace(relation,
-													targetBlock,
-													pageFreeSpace,
-													targetFreeSpace);
-	}
-
-	/*
-	 * Have to extend the relation.
-	 *
-	 * 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(relation);
-
-	/*
-	 * If we need the lock but are not able to acquire it immediately, we'll
-	 * consider extending the relation by multiple blocks at a time to manage
-	 * contention on the relation extension lock.  However, this only makes
-	 * sense if we're using the FSM; otherwise, there's no point.
-	 */
-	if (needLock)
-	{
-		if (!use_fsm)
-			LockRelationForExtension(relation, ExclusiveLock);
-		else if (!ConditionalLockRelationForExtension(relation, ExclusiveLock))
+		if (bistate
+			&& bistate->next_free != InvalidBlockNumber
+			&& bistate->next_free <= bistate->last_free)
 		{
-			/* Couldn't get the lock immediately; wait for it. */
-			LockRelationForExtension(relation, ExclusiveLock);
-
 			/*
-			 * Check if some other backend has extended a block for us while
-			 * we were waiting on the lock.
+			 * 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.
 			 */
-			targetBlock = GetPageWithFreeSpace(relation, targetFreeSpace);
+			if (use_fsm)
+				RecordPageWithFreeSpace(relation, targetBlock, pageFreeSpace);
 
-			/*
-			 * If some other waiter has already extended the relation, we
-			 * don't need to do so; just use the existing freespace.
-			 */
-			if (targetBlock != InvalidBlockNumber)
+			Assert(bistate->last_free != InvalidBlockNumber &&
+				   bistate->next_free <= bistate->last_free);
+			targetBlock = bistate->next_free;
+			if (bistate->next_free >= bistate->last_free)
 			{
-				UnlockRelationForExtension(relation, ExclusiveLock);
-				goto loop;
+				bistate->next_free = InvalidBlockNumber;
+				bistate->last_free = InvalidBlockNumber;
 			}
-
-			/* Time to bulk-extend. */
-			RelationAddExtraBlocks(relation, bistate);
+			else
+				bistate->next_free++;
+		}
+		else if (!use_fsm)
+		{
+			/* Without FSM, always fall out of the loop and extend */
+			break;
+		}
+		else
+		{
+			/*
+			 * Update FSM as to condition of this page, and ask for another
+			 * page to try.
+			 */
+			targetBlock = RecordAndGetPageWithFreeSpace(relation,
+														targetBlock,
+														pageFreeSpace,
+														targetFreeSpace);
 		}
 	}
 
-	/*
-	 * In addition to whatever extension we performed above, we always add at
-	 * least one block to satisfy our own request.
-	 *
-	 * XXX This does an lseek - rather expensive - but at the moment it is the
-	 * only way to accurately determine how many blocks are in a relation.  Is
-	 * it worth keeping an accurate file length in shared memory someplace,
-	 * rather than relying on the kernel to do it for us?
-	 */
-	buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);
-
-	/*
-	 * Release the file-extension lock; it's now OK for someone else to extend
-	 * the relation some more.
-	 */
-	if (needLock)
-		UnlockRelationForExtension(relation, ExclusiveLock);
+	/* Have to extend the relation */
+	buffer = RelationAddBlocks(relation, bistate, num_pages, use_fsm);
 
 	/*
 	 * We need to initialize the empty new page.  Double-check that it really
-- 
2.38.0

--wwosng5vofi3dmcs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v5-0012-WIP-Don-t-initialize-page-in-vm-fsm-_extend-not-n.patch"