From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Unhappy about API changes in the no-fsm-for-small-rels patch |
Date: | 2019-04-22 17:04:12 |
Message-ID: | 20190422170412.ndbhtxhf2flp464q@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2019-04-22 17:08:05 | Re: block-level incremental backup |
Previous Message | Daniel Verite | 2019-04-22 17:03:37 | Trouble with FETCH_COUNT and combined queries in psql |