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

On Tue, Feb 5, 2019 at 3:25 PM John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> wrote:
>
> On Tue, Feb 5, 2019 at 4:04 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Feb 4, 2019 at 2:27 PM John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> wrote:
> > >
> > > 1. Earlier, I had a test to ensure that free space towards the front
> > > of the relation was visible with no FSM. In [1], I rewrote it without
> > > using vacuum, so we can consider adding it back now if desired. I can
> > > prepare a patch for this.
> > >
> >
> > Yes, this is required. It is generally a good practise to add test (unless it takes a lot of time) which covers new code/functionality.
> >
> > > 2. As a follow-on, since we don't rely on vacuum to remove dead rows,
> > > we could try putting the fsm.sql test in some existing group in the
> > > parallel schedule, rather than its own group is it is now.
> > >
> >
> > +1.
>
> This is done in 0001.
>

This is certainly a good test w.r.t code coverage of new code, but I
have few comments:
1. The size of records in test still depends on alignment (MAXALIGN).
Though it doesn't seem to be a problematic case, I still suggest we
can avoid using records whose size depends on alignment. If you
change the schema as CREATE TABLE fsm_check_size (num1 int, num2 int,
str text);, then you can avoid alignment related issues for the
records being used in test.
2.
+-- Fill most of the last block
..
+-- Make sure records can go into any block but the last one
..
+-- Insert large record and make sure it does not cause the relation to extend

The comments in some part of the test seems too focussed towards the
algorithm used for in-memory map. I think we can keep these if we
want, but it is required to write a more generic comment stating what
is the actual motive of additional tests (basically we are testing the
functionality of in-memory map (LSM) for the heap, so we should write
about it.).

> > > 3. While looking at 0d1fe9f74e, it occurred to me that I ignored this
> > > patch's effects on GetRecordedFreeSpace(), which will return zero for
> > > tables with no FSM.
> > >
> >
> > Right, but what exactly we want to do for it? Do you want to add a comment atop of this function?
>
> Hmm, the comment already says "according to the FSM", so maybe it's
> already obvious. I was thinking more about maybe commenting the
> callsite where it's helpful, as in 0002.
>
> > > The other callers are in:
> > > contrib/pg_freespacemap/pg_freespacemap.c
> > > contrib/pgstattuple/pgstatapprox.c
> > >
> > > For pg_freespacemap, this doesn't matter, since it's just reporting
> > > the facts. For pgstattuple_approx(), it might under-estimate the free
> > > space and over-estimate the number of live tuples.
> > >
> >
> > Sure, but without patch also, it can do so, if the vacuum hasn't updated freespace map.
>
> Okay, then maybe we don't need to do anything else here.
>

Shall we add a note to the docs of pg_freespacemap and
pgstattuple_approx indicating that for small relations, FSM won't be
created, so these functions won't give appropriate value? Or other
possibility could be that we return an error if the block number is
less than the threshold value, but not sure if that is a good
alternative as that can happen today also if the vacuum hasn't run on
the table.

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