On Fri, Jan 11, 2019 at 3:54 AM John Naylor <john.nay...@2ndquadrant.com> wrote: > > On Wed, Jan 9, 2019 at 10:50 PM Amit Kapila <amit.kapil...@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
v13-0001-Avoid-creation-of-the-free-space-map-for-small-heap-.patch
Description: Binary data
