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

On Fri, Nov 23, 2018 at 11:56 AM John Naylor <jcnaylor@gmail.com> wrote:
>
> On 11/16/18, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> >
> > 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.
>
> 2 and 3 are separated into 0001.
>

Is the point 3 change related to pgindent?  I think even if you want
these, then don't prepare other patches on top of this, keep it
entirely separate.

>
> Also, we don't quite have a consensus on the threshold value, but I
> have set it to 4 pages for v8. If this is still considered too
> expensive (and basic tests show it shouldn't be), I suspect it'd be
> better to interleave the available block numbers as described a couple
> days ago than lower the threshold further.
>

Can you please repeat the copy test you have done above with
fillfactor as 20 and 30?

> I have looked at zhio.c, and it seems trivial to adapt zheap to this patchset.
>

Cool, I also think so.

Few more comments:
-------------------------------
1. I think we can add some test(s) to test the new functionality, may
be something on the lines of what Robert has originally provided as an
example of this behavior [1].

2.
@@ -2554,6 +2555,12 @@ AbortTransaction(void)
  s->parallelModeLevel = 0;
  }

+ /*
+ * In case we aborted during RelationGetBufferForTuple(),
+ * clear the local map of heap pages.
+ */
+ FSMClearLocalMap();
+

The similar call is required in AbortSubTransaction function as well.
I suggest to add it after pgstat_progress_end_command in both
functions.

3.
GetPageWithFreeSpace(Relation rel, Size spaceNeeded)
{
..
+ if (target_block == InvalidBlockNumber &&
+ rel->rd_rel->relkind == RELKIND_RELATION)
+ {
+ nblocks = RelationGetNumberOfBlocks(rel);
+
+ if (nblocks > HEAP_FSM_CREATION_THRESHOLD)
+ {
+ /*
+ * If the FSM knows nothing of the rel, try the last page before
+ * we give up and extend.  This avoids one-tuple-per-page syndrome
+ * during bootstrapping or in a recently-started system.
+ */
+ target_block = nblocks - 1;
+ }
..
}

Moving this check inside GetPageWithFreeSpace has one disadvantage, we
will always consider last block which can have some inadvertent
effects.  Consider when this function gets called from
RelationGetBufferForTuple just before extension, it can consider to
check for the last block even though that is already being done in the
begining when GetPageWithFreeSpace was called.  I am not completely
sure how much this is a case to worry because  it will help to check
last block when the same is concurrently added and FSM is not updated
for same.  I am slightly worried because the unpatched code doesn't
care for such case and we have no intention to change this behaviour.
What do you think?

4. You have mentioned above that "system catalogs created during
bootstrap still have a FSM if they have any data." and I can also see
this behavior, have you investigated this point further?

5. Your logic to update FSM on standby seems okay, but can you show
some tests which proves its sanity?

[1] - https://www.postgresql.org/message-id/CA%2BTgmoac%2B6qTNp2U%2BwedY8-PU6kK_b6hbdhR5xYGBG3GtdFcww%40mail.gmail.com

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