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

On Tue, Apr 30, 2019 at 11:42 AM John Naylor
<john.naylor@2ndquadrant.com> wrote:
>
> On Fri, Apr 26, 2019 at 11:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > As discussed above, we need to issue an
> > invalidation for following points:  (a) when vacuum finds there is no
> > FSM and page has more space now, I think you can detect this in
> > RecordPageWithFreeSpace
>
> I took a brief look and we'd have to know how much space was there
> before. That doesn't seem possible without first implementing the idea
> to save free space locally in the same way the FSM does. Even if we
> have consensus on that, there's no code for it, and we're running out
> of time.
>
> > (b) invalidation to notify the existence of
> > FSM, this can be done both by vacuum and backend.
>
> I still don't claim to be anything but naive in this area, but does
> the attached get us any closer?
>

@@ -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?

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