public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
Subject: [PATCH v5 12/15] hio: Use ExtendBufferedRelBy()
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;
}
view thread (29+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v5 12/15] hio: Use ExtendBufferedRelBy()
In-Reply-To: <no-message-id-1858017@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox