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

Hi,

On 2019-04-22 18:49:44 +0530, Amit Kapila wrote:
> Attached is a hacky and work-in-progress patch to move fsm map to
> relcache.  This will give you some idea.  I think we need to see if
> there is a need to invalidate the relcache due to this patch.  I think
> some other comments of Andres also need to be addressed, see if you
> can attempt to fix some of them.



>  /*
> @@ -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.


>  /*
> @@ -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.

Greetings,

Andres Freund