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

On Mon, Oct 15, 2018 at 4:09 PM John Naylor <jcnaylor(at)gmail(dot)com> wrote:
> On 10/15/18, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > Few comments on your latest 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.
>

I think we can avoid using prevBlockNumber and try_every_block, if we
maintain a small cache which tells whether the particular block is
tried or not. What I am envisioning is that while finding the block
with free space, if we came to know that the relation in question is
small enough that it doesn't have FSM, we can perform a local_search.
In local_seach, we can enquire the cache for any block that we can try
and if we find any block, we can try inserting in that block,
otherwise, we need to extend the relation. One simple way to imagine
such a cache would be an array of structure and structure has blkno
and status fields. After we get the usable block, we need to clear
the cache, if exists.

> 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.
>

I think the current code as proposed has that limitation, but if we
have a cache, then we can check if the relation has actually extended
after taking the lock and if so we can try only newly added block/'s.

I am not completely sure if the idea described above is certainly
better, but it seems that it will be clean and can handle some of the
cases with ease.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com