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

Hi,

On 2019-04-23 15:46:17 +0530, Amit Kapila wrote:
> On Mon, Apr 22, 2019 at 10:34 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2019-04-22 18:49:44 +0530, Amit Kapila wrote:
> > > /*
> > > @@ -1132,9 +1110,6 @@ fsm_local_set(Relation rel, BlockNumber cur_nblocks)
> > > BlockNumber blkno,
> > > cached_target_block;
> > >
> > > - /* The local map must not be set already. */
> > > - Assert(!FSM_LOCAL_MAP_EXISTS);
> > > -
> > > /*
> > > * Starting at the current last block in the relation and working
> > > * backwards, mark alternating blocks as available.
> > > @@ -1142,7 +1117,7 @@ fsm_local_set(Relation rel, BlockNumber cur_nblocks)
> > > blkno = cur_nblocks - 1;
> > > while (true)
> > > {
> > > - fsm_local_map.map[blkno] = FSM_LOCAL_AVAIL;
> > > + rel->fsm_local_map->map[blkno] = FSM_LOCAL_AVAIL;
> > > if (blkno >= 2)
> > > blkno -= 2;
> > > else
> > > @@ -1150,13 +1125,13 @@ fsm_local_set(Relation rel, BlockNumber cur_nblocks)
> > > }
> > >
> > > /* Cache the number of blocks. */
> > > - fsm_local_map.nblocks = cur_nblocks;
> > > + rel->fsm_local_map->nblocks = cur_nblocks;
> > >
> > > /* Set the status of the cached target block to 'unavailable'. */
> > > cached_target_block = RelationGetTargetBlock(rel);
> > > if (cached_target_block != InvalidBlockNumber &&
> > > cached_target_block < cur_nblocks)
> > > - fsm_local_map.map[cached_target_block] = FSM_LOCAL_NOT_AVAIL;
> > > + rel->fsm_local_map->map[cached_target_block] = FSM_LOCAL_NOT_AVAIL;
> > > }
> >
> > I think there shouldn't be any need for this anymore. After this change
> > we shouldn't invalidate the map until there's no space on it - thereby
> > addressing the cost overhead, and greatly reducing the likelihood that
> > the local FSM can lead to increased bloat.

> If we invalidate it only when there's no space on the page, then when
> should we set it back to available, because if we don't do that, then
> we might miss the space due to concurrent deletes.

Well, deletes don't traditionally (i.e. with an actual FSM) mark free
space as available (for heap). I think RecordPageWithFreeSpace() should
issue a invalidation if there's no FSM, and the block goes from full to
empty (as there's no half-full IIUC).

> > > /*
> > > @@ -1168,18 +1143,18 @@ fsm_local_set(Relation rel, BlockNumber cur_nblocks)
> > > * This function is used when there is no FSM.
> > > */
> > > static BlockNumber
> > > -fsm_local_search(void)
> > > +fsm_local_search(Relation rel)
> > > {
> > > BlockNumber target_block;
> > >
> > > /* Local map must be set by now. */
> > > - Assert(FSM_LOCAL_MAP_EXISTS);
> > > + Assert(FSM_LOCAL_MAP_EXISTS(rel));
> > >
> > > - target_block = fsm_local_map.nblocks;
> > > + target_block = rel->fsm_local_map->nblocks;
> > > do
> > > {
> > > target_block--;
> > > - if (fsm_local_map.map[target_block] == FSM_LOCAL_AVAIL)
> > > + if (rel->fsm_local_map->map[target_block] == FSM_LOCAL_AVAIL)
> > > return target_block;
> > > } while (target_block > 0);
> > >
> > > @@ -1189,7 +1164,22 @@ fsm_local_search(void)
> > > * first, which would otherwise lead to the same conclusion again and
> > > * again.
> > > */
> > > - FSMClearLocalMap();
> > > + fsm_clear_local_map(rel);
> >
> > I'm not sure I like this. My inclination would be that we should be able
> > to check the local fsm repeatedly even if there's no space in the
> > in-memory representation - otherwise the use of the local FSM increases
> > bloat.
> >
>
> Do you mean to say that we always check all the pages (say 4)
> irrespective of their state in the local map?

I was wondering that, yes. But I think just issuing invalidations is the
right approach instead, see above.

> I think we should first try to see in this new scheme (a) when to set
> the map, (b) when to clear it, (c) how to use. I have tried to
> summarize my thoughts about it, let me know what do you think about
> the same?
>
> When to set the map.
> At the beginning (the first time relation is used in the backend), the
> map will be clear. When the first time in the backend, we find that
> FSM doesn't exist and the number of blocks is lesser than
> HEAP_FSM_CREATION_THRESHOLD, we set the map for the total blocks that
> exist at that time and mark all or alternate blocks as available.

I think the alternate blocks scheme has to go. It's not defensible.

Greetings,

Andres Freund