Re: Rewriting the test of pg_upgrade as a TAP test

On 2017-04-05 10:50:19 -0400, Stephen Frost wrote:
> Andres,
> 
> * Andres Freund (andres@anarazel.de) wrote:
> > On 2017-04-05 10:40:41 -0400, Stephen Frost wrote:
> > > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > > > Stephen Frost <sfrost@snowman.net> writes:
> > > > > I believe that what Peter was getting at is that the pg_dump TAP tests
> > > > > create a whole slew of objects in just a few seconds and are able to
> > > > > then exercise those code-paths in pg_dump, without needing to run the
> > > > > entire serial regression test run.
> > > > 
> > > > Right.  But there's a certain amount of serendipity involved in using the
> > > > core regression tests' final results.  For example, I don't know how long
> > > > it would've taken us to understand the problems around dumping and
> > > > reloading child tables with inconsistent column orders, had there not been
> > > > examples of that in the regression tests.  I worry that creating a sterile
> > > > set of objects for testing pg_dump will leave blind spots, because it will
> > > > mean that we only test cases that we explicitly created test cases for.
> > > 
> > > We don't need to only create sterile sets of objects in the pg_dump TAP
> > > tests.
> > 
> > I really, really don't understand why we're conflating making pg_upgrade
> > tests less fragile / duplicative with changing what we use to test it.
> > This seems to have the sole result that we're not going to get anywhere.
> 
> Probably because the point was brought up that the regression tests for
> pg_upgrade spend a bunch of time doing something which, ultimately,
> don't actually add any real value.  Yes, there are bits of the core
> regression tests that currently add value over what we have through
> other approaches, but that's not where the bulk of running those tests
> go.

Create a separate patch [& thread] about that, don't conflate the
topics.  I'm very much in favor of this rewrite, I'm very much not in
favor of only using some targeted testsuite.  By combining two
independent changes, you're just making it less likely that anything
happens.


> > > I don't believe we need to populate GIN indexes or vacuum them
> > > to test pg_dump/pg_upgrade either (at least, not if we're going to stick
> > > to the pg_upgrade test basically being if pg_dump returns the same
> > > results before-and-after).
> > 
> > I think we *should* have populated GIN indexes. Yes, the coverage isn't
> > perfect, but the VACUUM definitely gives a decent amount of coverage
> > whether the gin index looks halfway sane after the upgrade.
> 
> We don't look at the gin index after the upgrade in the current
> pg_upgrade testing, so I don't see why you feel it's at all valuable.

It's be trivial to add a VACUUM to the point where analyze_new_cluster
is currently run.  And I've previously run more manual tests.  Is that
perfect - no, definitely not.


Greetings,

Andres Freund