On Mon, Feb 4, 2019 at 2:27 PM John Naylor <john.nay...@2ndquadrant.com> wrote: > > On Mon, Feb 4, 2019 at 8:41 AM Amit Kapila <amit.kapil...@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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
