Re: Unhappy about API changes in the no-fsm-for-small-rels patch

On Thu, May 2, 2019 at 12:39 PM John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> wrote:
>
> On Thu, May 2, 2019 at 2:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > @@ -776,7 +776,10 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
> > if ((rel->rd_smgr->smgr_fsm_nblocks == 0 ||
> > rel->rd_smgr->smgr_fsm_nblocks == InvalidBlockNumber) &&
> > !smgrexists(rel->rd_smgr, FSM_FORKNUM))
> > + {
> > smgrcreate(rel->rd_smgr, FSM_FORKNUM, false);
> > + fsm_clear_local_map(rel);
> > + }
> >
> > I think this won't be correct because when we call fsm_extend via
> > vacuum the local map won't be already existing, so it won't issue an
> > invalidation call. Isn't it better to directly call
> > CacheInvalidateRelcache here to notify other backends that their local
> > maps are invalid now?
>
> Yes, you're quite correct.
>

Okay, I have updated the patch to incorporate your changes and call
relcache invalidation at required places. I have updated comments at a
few places as well. The summarization of this patch is that (a) it
moves the local map to relation cache (b) performs the cache
invalidation whenever we create fsm (either via backend or vacuum),
when some space in a page is freed by vacuum (provided fsm doesn't
exist) or whenever the local map is cleared (c) additionally, we clear
the local map when we found a block from FSM, when we have already
tried all the blocks present in cache or when we are allowed to create
FSM.

If we agree on this, then we can update the README accordingly.

Can you please test/review?

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