Re: WIP: Avoid creation of the free space map for small tables

On 10/15/18, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few comments on your latest patch:
> -
> +static bool
> +allow_write_to_fsm(Relation rel, BlockNumber heapBlk)
> +{
> + BlockNumber heap_nblocks;
> +
> + if (heapBlk > HEAP_FSM_EXTENSION_THRESHOLD ||
> + rel->rd_rel->relkind != RELKIND_RELATION)
> + return true;
> +
> + /* XXX is this value cached? */
> + heap_nblocks = RelationGetNumberOfBlocks(rel);
> +
> + if (heap_nblocks > HEAP_FSM_EXTENSION_THRESHOLD)
> + return true;
> + else
> + {
> + RelationOpenSmgr(rel);
> + return smgrexists(rel->rd_smgr, FSM_FORKNUM);
> + }
> +}
>
> I think you can avoid calling RelationGetNumberOfBlocks, if you call
> smgrexists before

Okay, I didn't know which was cheaper, but I'll check smgrexists
first. Thanks for the info.

> and for the purpose of vacuum, we can get that as an
> input parameter.  I think one can argue for not changing the interface
> functions like RecordPageWithFreeSpace to avoid calling
> RelationGetNumberOfBlocks, but to me, it appears worth to save the
> additional system call.

I agree, and that should be fairly straightforward. I'll include that
in the next patch.

> -
> targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace);
> -
> - /*
> - * If the FSM knows nothing of the rel, try the last page before we
> - * give up and extend.  This avoids one-tuple-per-page syndrome during
> - * bootstrapping or in a recently-started system.
> - */
>   if (targetBlock == InvalidBlockNumber)
> - {
> - BlockNumber nblocks = RelationGetNumberOfBlocks(relation);
> -
> - if (nblocks > 0)
> - targetBlock = nblocks - 1;
> - }
> + targetBlock = get_page_no_fsm(relation, InvalidBlockNumber,
> +   &try_every_page);
>
>
> Is it possible to hide the magic of trying each block within
> GetPageWithFreeSpace?  It will simplify the code and in future, if
> another storage API has a different function for
> RelationGetBufferForTuple, it will work seamlessly, provided they are
> using same FSM.  One such user is zheap.

Hmm, here I'm a bit more skeptical about the trade offs. That would
mean, in effect, to put a function called get_page_no_fsm() in the FSM
code. ;-)  I'm willing to be convinced otherwise, of course, but
here's my reasoning:

For one, we'd have to pass prevBlockNumber and &try_every_block to
GetPageWithFreeSpace() (and RecordAndGetPageWithFreeSpace() by
extension), which are irrelevant to some callers. In addition, in
hio.c, there is a call where we don't want to try any blocks that we
have already, much less all of them:

/*
 * Check if some other backend has extended a block for us while
 * we were waiting on the lock.
 */
targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace);

By the time we get to this call, we likely wouldn't trigger the logic
to try every block, but I don't think we can guarantee that. We could
add a boolean parameter that means "consider trying every block", but
I don't think the FSM code should have so much state passed to it.

Thanks for reviewing,
-John Naylor