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