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

On Mon, Feb 4, 2019 at 2:27 PM John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>
wrote:
>
> On Mon, Feb 4, 2019 at 8:41 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >
> > The change seems to have worked. All the buildfarm machines that were
> > showing the failure are passed now.
>
> Excellent!
>
> Now that the buildfarm is green as far as this patch goes,
>

There is still one recent failure which I don't think is related to commit
of this patch:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2019-02-04%2016%3A38%3A48

==================
pgsql.build/src/bin/pg_ctl/tmp_check/log/004_logrotate_primary.log
===================
TRAP: FailedAssertion("!(UsedShmemSegAddr != ((void *)0))", File:
"g:\prog\bf\root\head\pgsql.build\src\backend\port\win32_shmem.c", Line:
513)

I think we need to do something about this random failure, but not as part
of this thread/patch.

> I will
> touch on a few details to round out development in this area:
>
> 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.

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

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

> This might be fine,
> since it is approximate after all, but maybe a comment would be
> helpful. If this is a problem, we could tweak it to be more precise
> for tables without FSMs.
>

Sounds reasonable to me.

> Thoughts?
>
> 4. The latest patch for the pg_upgrade piece was in [2]
>

It will be good if we get this one as well. I will look into it once we
are done with the other points you have mentioned.

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