From: Andres Freund Date: Wed, 26 Oct 2022 14:14:11 -0700 Subject: [PATCH v5 12/15] hio: Use ExtendBufferedRelBy() --- 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; }