Re: WIP: Avoid creation of the free space map for small tables

On Fri, Jan 11, 2019 at 3:54 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Wed, Jan 9, 2019 at 10:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Thanks, Mithun for performance testing, it really helps us to choose
> > the right strategy here.  Once John provides next version, it would be
> > good to see the results of regular pgbench (read-write) runs (say at
> > 50 and 300 scale factor) and the results of large copy.  I don't think
> > there will be any problem, but we should just double check that.
>
> Attached is v12 using the alternating-page strategy. I've updated the
> comments and README as needed. In addition, I've
>

Few comments:
---------------------------
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?

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.


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.

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.
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.

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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com