On Mon, Feb 4, 2019 at 2:27 PM John Naylor <
john.naylor@2ndquadrant.com> wrote:
>
> On Mon, Feb 4, 2019 at 8:41 AM Amit Kapila <
amit.kapila16@gmail.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.