Date: Mon, 20 May 2019 17:47:17 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter_e@gmx.net>,
	Michael Paquier <michael.paquier@gmail.com>,
	Fujii Masao <masao.fujii@gmail.com>,
	Fujii Masao <fujii@postgresql.org>,
	pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Make VACUUM accept 1 and 0 as a boolean value.
Message-ID: <20190521004717.qsktdsugj3shagco@alap3.anarazel.de>
References: <26460.1558366996@sss.pgh.pa.us>
 <CAHGQGwEvM9X66Ni4OKv4dOQQddw7z=cNUxnG=wEv84sqj9t_ig@mail.gmail.com>
 <20190520163652.l7ibpe52pbq6lecc@alap3.anarazel.de>
 <CAHGQGwEixkoMJy4JHUvBdM0MacR1_h=wmE43KTOTErpA2Z5P0w@mail.gmail.com>
 <20190520164908.t653bdbfjcn3d35p@alap3.anarazel.de>
 <23559.1558374415@sss.pgh.pa.us>
 <20190520180027.zyxosts5yrf2i7mh@alap3.anarazel.de>
 <24497.1558375780@sss.pgh.pa.us>
 <20190520203505.zoeitjlehqypgx26@alap3.anarazel.de>
 <21766.1558397960@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <21766.1558397960@sss.pgh.pa.us>
Precedence: bulk

Hi,

On 2019-05-20 20:19:20 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-05-20 14:09:40 -0400, Tom Lane wrote:
> >> What I'd like, for both prove and pg_regress, is to print something
> >> about failing tests and otherwise be quiet.  Simple redirection won't
> >> do that.  Plus it'd be hard to fit that in with the case where you
> >> don't want it to be quiet.
> 
> > The most annoying noise imo is the pg_upgrade test. The set -x and the
> > fact that it resets MAKEFLAGS makes that pretty annoying.
> 
> Yeah.  I just experimented with running "make -s check-world >/dev/null"
> and found that *all* of the useless chatter on stderr is the pg_upgrade
> test's fault.  With the attached quick-hack patch, a successful run is
> silent --- and if it fails, make tells you which directory it failed
> in, which is enough to go find the problem.  So that's already a huge
> usability improvement over where we are.

I think we really ought to apply something pretty much like that, at
least in regards to the set -x. I'm mildly inclined to go for the others
you have too.


> > 2) We overwrite MAKELEVEL, MAKEFLAGS, breaking jobserver etc - that
> >    shouldn't be required anymore if test.sh were to be changed to behave
> >    like dcae5faccab64
> 
> Hmm.  AFAICS, that commit removed that code without any explanation of
> why it was safe to remove it, so I'm unclear on whether it would be
> safe to do likewise in test.sh.

I think the removal in dcae5faccab64 ought to be safe, as after
dcae5faccab64 pg_regress doesn't invoke make anymore. I'm not 100% sure
we actually don't need it in test.sh anymore, even if we don't perform
the installation therein anymore.

See next:

> > 3) The fact that src/bin/pg_upgrade/Makefile invokes test.sh with
> >    MAKE=$(MAKE) triggers make, for reasons I do not yet understand, to
> >    *disable* output synchronization. Which annoys the heck out of me,
> >    because it makes parallel check output neigh unparsable.
> 
> Probably the same thing as your 2)?

It's not quite. I've not yet fully understood it yet. It's *not* tied to
MAKEFLAGS. If I understand correctly, when make sees $(MAKE)/${MAKE} in
a recipe, or the recipe starts with +, it assumes that the sub-command
is *also* make. And the reason that output synchronization doesn't work
is that make *intentionally* doesn't enable that for submakes - it only
wants it for commands run by those submakes (otherwise it could not
individually output individual targets processed in submakes).

I think the right approach might be to just *never* invoke make inside
test.sh:
1) We can fairly easily fix the make install to be unnecessary
2) I think the installcheck-parallel could be replaced by instead
   passing in the the necessary pg_regress command. That'd also have the
   benefit of not having another make invocation stat'ing a lot of files
   etc.

I'll play with these.


> The other thing I had to do below was to suppress "NOTICE: database
> "regression" does not exist, skipping".  The added createdb is a
> mighty expensive and grotty way to do that, but I didn't immediately
> see a better one.

Hm. Perhaps we ought to just have pg_regress set client_min_messages to
something less noisy when running DROP DATABASE? I don't think any
pg_regress caller benefits from having it.

Greetings,

Andres Freund