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

On Mon, Oct 22, 2018 at 12:14 PM John Naylor <jcnaylor@gmail.com> wrote:
>
> On 10/16/18, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I think we can avoid using prevBlockNumber and try_every_block, if we
> > maintain a small cache which tells whether the particular block is
> > tried or not.  What I am envisioning is that while finding the block
> > with free space, if we came to know that the relation in question is
> > small enough that it doesn't have FSM, we can perform a local_search.
> > In local_seach, we can enquire the cache for any block that we can try
> > and if we find any block, we can try inserting in that block,
> > otherwise, we need to extend the relation.  One simple way to imagine
> > such a cache would be an array of structure and structure has blkno
> > and status fields.  After we get the usable block, we need to clear
> > the cache, if exists.
>
> Here is the design I've implemented in the attached v6. There is more
> code than v5, but there's a cleaner separation between freespace.c and
> hio.c, as you preferred.
>

This approach seems better.

> I also think it's more robust. I've expended
> some effort to avoid doing unnecessary system calls to get the number
> of blocks.
> --
>
> For the local, in-memory map, maintain a static array of status
> markers, of fixed-length HEAP_FSM_CREATION_THRESHOLD, indexed by block
> number. This is populated every time we call GetPageWithFreeSpace() on
> small tables with no FSM. The statuses are
>
> 'zero' (beyond the relation)
> 'available to try'
> 'tried already'
>

+/* Status codes for the local map. */
+#define FSM_LOCAL_ZERO 0x00 /* Beyond the end of the relation */
+#define FSM_LOCAL_AVAIL 0x01 /* Available to try */
+#define FSM_LOCAL_TRIED 0x02 /* Already tried, not enough space */

Instead of maintaining three states, can't we do with two states
(Available and Not Available), basically combine 0 and 2 in your case.
I think it will save some cycles in
fsm_local_set, where each time you need to initialize all the entries
in the map.  I think we can argue that it is not much overhead, but I
think it is better code-wise also if we can make it happen with fewer
states.

Some assorted comments:
1.
 <para>
-Each heap and index relation, except for hash indexes, has a Free Space Map
+Each heap relation, unless it is very small, and each index relation,
+except for hash indexes, has a Free Space Map
 (FSM) to keep track of available space in the relation. It's stored

It appears that line has ended abruptly.

2.
page = BufferGetPage(buffer);
+ targetBlock = BufferGetBlockNumber(buffer);

  if (!PageIsNew(page))
  elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
- BufferGetBlockNumber(buffer),
+ targetBlock,
  RelationGetRelationName(relation));

  PageInit(page, BufferGetPageSize(buffer), 0);
@@ -623,7 +641,18 @@ loop:
  * current backend to make more insertions or not, which is probably a
  * good bet most of the time.  So for now, don't add it to FSM yet.
  */
- RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
+ RelationSetTargetBlock(relation, targetBlock);

Is this related to this patch? If not, I suggest let's do it
separately if required.

3.
 static int fsm_set_and_search(Relation rel, FSMAddress addr, uint16 slot,
-    uint8 newValue, uint8 minValue);
+ uint8 newValue, uint8 minValue);

This appears to be a spurious change.

4.
@@ -378,24 +386,15 @@ RelationGetBufferForTuple(Relation relation, Size len,
  * target.
  */
  targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace);
+
+ /*
+ * In case we used an in-memory map of available blocks, reset
+ * it for next use.
+ */
+ if (targetBlock < HEAP_FSM_CREATION_THRESHOLD)
+ ClearLocalMap();

How will you clear the local map during error?  I think you need to
clear it in abort path and you can name the function as
FSMClearLocalMap or something like that.

5.
+/*#define TRACE_TARGETBLOCK */

Debugging leftover, do you want to retain this and related stuff
during the development of patch?

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