On Wed, Jan 16, 2019 at 8:41 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Fri, Jan 11, 2019 at 3:54 AM John Naylor <john.nay...@2ndquadrant.com>
> wrote:
> 1.
> Commit message:
> > Any pages with wasted free space become visible at next relation extension,
> > so we still control table bloat.
>
> I think the free space will be available after the next pass of
> vacuum, no? How can relation extension make it available?
To explain, this diagram shows the map as it looks for different small
table sizes:
0123
A
NA
ANA
NANA
So for a 3-block table, the alternating strategy never checks block 1.
Any free space block 1 has acquired via delete-and-vacuum will become
visible if it extends to 4 blocks. We are accepting a small amount of
bloat for improved performance, as discussed. Would it help to include
this diagram in the README?
> 2.
> +2. For very small heap relations, the FSM would be relatively large and
> +wasteful, so as of PostgreSQL 12 we refrain from creating the FSM for
> +heaps with HEAP_FSM_CREATION_THRESHOLD pages or fewer, both to save space
> +and to improve performance. To locate free space in this case, we simply
> +iterate over the heap, trying alternating pages in turn. There may be some
> +wasted free space in this case, but it becomes visible again upon next
> +relation extension.
>
> a. Again, how space becomes available at next relation extension.
> b. I think there is no use of mentioning the version number in the
> above comment, this code will be present from PG-12, so one can find
> out from which version this optimization is added.
It fits with the reference to PG 8.4 earlier in the document. I chose
to be consistent, but to be honest, I'm not much in favor of a lot of
version references in code/READMEs.
> 3.
> BlockNumber
> RecordAndGetPageWithFreeSpace(Relation rel, BlockNumber oldPage,
> Size oldSpaceAvail, Size spaceNeeded)
> {
> ..
> + /* First try the local map, if it exists. */
> + if (oldPage < fsm_local_map.nblocks)
> + {
> ..
> }
>
> The comment doesn't appear to be completely in sync with the code.
> Can't we just check whether "fsm_local_map.nblocks > 0", if so, we
> can use a macro for the same? I have changed this in the attached
> patch, see what you think about it. I have used it at a few other
> places as well.
The macro adds clarity, so I'm in favor of using it.
> 4.
> + * When we initialize the map, the whole heap is potentially available to
> + * try. If a caller wanted to reset the map after another backend extends
> + * the relation, this will only flag new blocks as available. No callers
> + * do this currently, however.
> + */
> +static void
> +fsm_local_set(Relation rel, BlockNumber curr_nblocks)
> {
> ..
> + if (blkno >= fsm_local_map.nblocks + 2)
> ..
> }
>
>
> The way you have tried to support the case as quoted in the comment
> "If a caller wanted to reset the map after another backend extends .."
> doesn't appear to be solid and I am not sure if it is correct either.
I removed this case in v9 and you objected to that as unnecessary, so
I reverted it for v10.
> We don't have any way to test the same, so I suggest let's try to
> simplify the case w.r.t current requirement of this API. I think we
> should
> some simple logic to try every other block like:
>
> + blkno = cur_nblocks - 1;
> + while (true)
> + {
> + fsm_local_map.map[blkno] = FSM_LOCAL_AVAIL;
> + if (blkno >= 2)
> + blkno -= 2;
> + else
> + break;
> + }
>
> I have changed this in the attached patch.
Fine by me.
> 5.
> +/*
> + * Search the local map for an available block to try, in descending order.
> + *
> + * For use when there is no FSM.
> + */
> +static BlockNumber
> +fsm_local_search(void)
>
> We should give a brief explanation as to why we try in descending
> order. I have added some explanation in the attached patch, see what
> you think about it?
>
> Apart from the above, I have modified a few comments.
I'll include these with some grammar corrections in the next version.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
