pgsql: Make VACUUM accept 1 and 0 as a boolean value.

Fujii Masao <fujii@postgresql.org> writes:

Make VACUUM accept 1 and 0 as a boolean value.

This was not a particularly great thing to be pushing on a release
wrap day, and pushing it without having even run the core regression
tests was downright negligent. Please fix or revert, NOW.

regards, tom lane

On Tue, May 21, 2019 at 12:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <fujii@postgresql.org> writes:

Make VACUUM accept 1 and 0 as a boolean value.

This was not a particularly great thing to be pushing on a release
wrap day, and pushing it without having even run the core regression
tests was downright negligent. Please fix or revert, NOW.

Sorry, this is my shameful fault.. Attached is the patch for fix.
I will commit this after confirming that regression test reports
no error.

Regards,

--
Fujii Masao

Hi,

On 2019-05-21 01:03:06 +0900, Fujii Masao wrote:

On Tue, May 21, 2019 at 12:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <fujii@postgresql.org> writes:

Make VACUUM accept 1 and 0 as a boolean value.

This was not a particularly great thing to be pushing on a release
wrap day, and pushing it without having even run the core regression
tests was downright negligent. Please fix or revert, NOW.

Sorry, this is my shameful fault.. Attached is the patch for fix.
I will commit this after confirming that regression test reports
no error.

This was 30min ago. I've now pushed test adjustment, so the buildfarm
becomes green again. Still should either add some tests for the change,
or be revert.

Greetings,

Andres Freund

On Tue, May 21, 2019 at 1:36 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2019-05-21 01:03:06 +0900, Fujii Masao wrote:

On Tue, May 21, 2019 at 12:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <fujii@postgresql.org> writes:

Make VACUUM accept 1 and 0 as a boolean value.

This was not a particularly great thing to be pushing on a release
wrap day, and pushing it without having even run the core regression
tests was downright negligent. Please fix or revert, NOW.

Sorry, this is my shameful fault.. Attached is the patch for fix.
I will commit this after confirming that regression test reports
no error.

This was 30min ago. I've now pushed test adjustment, so the buildfarm
becomes green again. Still should either add some tests for the change,
or be revert.

Thanks for the commit! And sorry, make check-world took very long time
and has just finished right now in my env... I'd like to confirm that
make check-world reports no error just in case.

Regards,

--
Fujii Masao

Hi,

On 2019-05-21 01:43:35 +0900, Fujii Masao wrote:

On Tue, May 21, 2019 at 1:36 AM Andres Freund <andres@anarazel.de> wrote:

On 2019-05-21 01:03:06 +0900, Fujii Masao wrote:

On Tue, May 21, 2019 at 12:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <fujii@postgresql.org> writes:

Make VACUUM accept 1 and 0 as a boolean value.

This was not a particularly great thing to be pushing on a release
wrap day, and pushing it without having even run the core regression
tests was downright negligent. Please fix or revert, NOW.

Sorry, this is my shameful fault.. Attached is the patch for fix.
I will commit this after confirming that regression test reports
no error.

This was 30min ago. I've now pushed test adjustment, so the buildfarm
becomes green again. Still should either add some tests for the change,
or be revert.

Thanks for the commit! And sorry, make check-world took very long time
and has just finished right now in my env... I'd like to confirm that
make check-world reports no error just in case.

make -j16 -s && make -Otarget -j10 -s check-world && echo success || echo fail

makes that much more bearable, by running the slow TAP tests in
parallel. Doesn't work < 10 though.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-05-21 01:43:35 +0900, Fujii Masao wrote:

Thanks for the commit! And sorry, make check-world took very long time
and has just finished right now in my env... I'd like to confirm that
make check-world reports no error just in case.

make -j16 -s && make -Otarget -j10 -s check-world && echo success || echo fail

makes that much more bearable, by running the slow TAP tests in
parallel. Doesn't work < 10 though.

Yeah, parallelization makes all the difference in the world, assuming
you have a multi-core machine. You can parallelize within TAP suites
too. I use

make -s check-world -j4 PROVE_FLAGS='-j4 --quiet --nocolor --nocount'

on an 8-core machine, and get typical runtimes under 2m30s.

If there were a way to get prove to be absolutely quiet (its --quiet is
a joke unfortunately), I'd lobby for a switch to make pg_regress quiet
too. The amount of useless noise this recipe generates is annoying,
and tends to make it hard to find where the actual error is when
there is one.

regards, tom lane

Hi,

On 2019-05-20 13:46:55 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2019-05-21 01:43:35 +0900, Fujii Masao wrote:

Thanks for the commit! And sorry, make check-world took very long time
and has just finished right now in my env... I'd like to confirm that
make check-world reports no error just in case.

make -j16 -s && make -Otarget -j10 -s check-world && echo success || echo fail

makes that much more bearable, by running the slow TAP tests in
parallel. Doesn't work < 10 though.

Kinda wonder whether we should backpatch the necessary changes to make
-j .. check work back to 9.4.

Yeah, parallelization makes all the difference in the world, assuming
you have a multi-core machine. You can parallelize within TAP suites
too. I use

make -s check-world -j4 PROVE_FLAGS='-j4 --quiet --nocolor --nocount'

on an 8-core machine, and get typical runtimes under 2m30s.

FWIW, I found that parallelizing on the top-level to get better
performance than within prove. The system load is more predicatable.

If there were a way to get prove to be absolutely quiet (its --quiet is
a joke unfortunately), I'd lobby for a switch to make pg_regress quiet
too. The amount of useless noise this recipe generates is annoying,
and tends to make it hard to find where the actual error is when
there is one.

Indeed. Couldn't we just invent that on our end? I.e. in our prove
invocation, just redirect the output?

The most annoying noise imo is the pg_upgrade test. The set -x and the
fact that it resets MAKEFLAGS makes that pretty annoying.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-05-20 13:46:55 -0400, Tom Lane wrote:

If there were a way to get prove to be absolutely quiet (its --quiet is
a joke unfortunately), I'd lobby for a switch to make pg_regress quiet
too. The amount of useless noise this recipe generates is annoying,
and tends to make it hard to find where the actual error is when
there is one.

Indeed. Couldn't we just invent that on our end? I.e. in our prove
invocation, just redirect the output?

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.

Agreed, that could be improved independently of other things.

regards, tom lane

Hi,

On 2019-05-20 14:09:40 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2019-05-20 13:46:55 -0400, Tom Lane wrote:

If there were a way to get prove to be absolutely quiet (its --quiet is
a joke unfortunately), I'd lobby for a switch to make pg_regress quiet
too. The amount of useless noise this recipe generates is annoying,
and tends to make it hard to find where the actual error is when
there is one.

Indeed. Couldn't we just invent that on our end? I.e. in our prove
invocation, just redirect the output?

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.

Ah, I thought of just doing something roughly (in real syntax etc) like

if QUIET_TEST:
$(PROVE) $(PROVE_FLAGS) ... > $(CURDIR)/prove.log 2>&1 && echo prove in $CURDIR succeeded || echo prove in $CURDIR failed
else
$(PROVE) $(PROVE_FLAGS) ...

maybe additionally outputting the path to the logfile in the failure
case. While far from being as good as what you're desiring, it still
seems like it could be a huge improvement over the status quo
nevertheless?

The most annoying noise imo is the pg_upgrade test. The set -x and the
fact that it resets MAKEFLAGS makes that pretty annoying.

Agreed, that could be improved independently of other things.

Hm, there's a lot of oddities around the pg_upgrade test:

1) we still do an "open coded" install, regardless of dcae5faccab64 And
that's *in addition* to to the install from dcae5faccab64. That seems
pretty unnecessary.

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

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.

4) set -x spews a lot of things

Greetings,

Andres Freund

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.

1) we still do an "open coded" install, regardless of dcae5faccab64 And
that's *in addition* to to the install from dcae5faccab64. That seems
pretty unnecessary.

Agreed, that could be worth improving.

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.

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

4) set -x spews a lot of things

Yeah. I'm definitely in favor of losing the set -x. 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.

regards, tom lane

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

Andres Freund <andres@anarazel.de> writes:

On 2019-05-20 20:19:20 -0400, Tom Lane wrote:

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.

Yeah, the first thing I tried was

-	psql_command("postgres", "DROP DATABASE IF EXISTS \"%s\"", dbname);
+	psql_command("postgres", "SET client_min_messages = warning; DROP DATABASE IF EXISTS \"%s\"", dbname);

in pg_regress.c, but that fails with "ERROR: DROP DATABASE cannot run
inside a transaction block". It's probably possible to improve this
but it'll take a bit of surgery.

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

On 2019-05-20 20:19:20 -0400, Tom Lane wrote:

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.

The least invasive way to do that seems to be as attached, building a
little knowledge into pg_regress's psql_command() function. Alternatively
we could add a "bool quiet" parameter to that function so that callers
had to say what to do, but I'm not sure that's an improvement.

regards, tom lane

Hi,

On 2019-05-21 10:20:57 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2019-05-20 20:19:20 -0400, Tom Lane wrote:

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.

The least invasive way to do that seems to be as attached, building a
little knowledge into pg_regress's psql_command() function. Alternatively
we could add a "bool quiet" parameter to that function so that callers
had to say what to do, but I'm not sure that's an improvement.

It's not overly pretty, but also not that bad. I was wondering whether
we could make psql_command accept multiple statements (annoying due to
argument passing - although I guess we could switch to using numeric
references to arguments), use putenv to put client_min_messages into
PGOPTIONS (to finnicky to remove again), have psql_command pass the
command to psql via stdin (more code needed for pipe setup). So I think
we can just go with what you're proposing. Although I also could just go
with always supressing notices in psql_command().

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-05-21 10:20:57 -0400, Tom Lane wrote:

The least invasive way to do that seems to be as attached, building a
little knowledge into pg_regress's psql_command() function. Alternatively
we could add a "bool quiet" parameter to that function so that callers
had to say what to do, but I'm not sure that's an improvement.

It's not overly pretty, but also not that bad. I was wondering whether
we could make psql_command accept multiple statements (annoying due to
argument passing - although I guess we could switch to using numeric
references to arguments), use putenv to put client_min_messages into
PGOPTIONS (to finnicky to remove again), have psql_command pass the
command to psql via stdin (more code needed for pipe setup). So I think
we can just go with what you're proposing. Although I also could just go
with always supressing notices in psql_command().

I think it's better to only suppress where we have a reason to expect
a notice; unexpected notices are the kind of thing you want a test
to find.

Another angle on this is that multiple -c switches only work back to
psql 9.6. Before that it only honors the last -c switch, so that this
change has no effect. I'm only proposing that we apply this change
in HEAD, so it would only matter for cross-version-upgrade tests,
and I think that "no effect" should be acceptable in those.

Alternatively we could go with the stdin approach, but as you say,
that seems like it takes more code than this is worth.

regards, tom lane

Hi,

On 2019-05-20 17:47:17 -0700, Andres Freund wrote:

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

1) was easy enough (posted at [1]/messages/by-id/20190521191918.z7kwnrlj45mk2k67@alap3.anarazel.de, and also attached here). I think 2)
is a bit harder, because according to src/bin/pg_upgrade/TESTING we
currently support invoking test.sh from commandline.

Therefore I think we ought to just work around that make heuristic, by
referring to $(MAKE) via an indirection. Like in the attached. After
that I don't get any interspersed output anymore if I run with -Otarget.

Greetings,

Andres Freund

[1]: /messages/by-id/20190521191918.z7kwnrlj45mk2k67@alap3.anarazel.de

Andres Freund <andres@anarazel.de> writes:

-check: test.sh all
-	MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
+check: test.sh all temp-install
+	MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)

The $(DOINST) bit should go away no?

regards, tom lane

Hi,

On 2019-05-21 16:47:55 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

-check: test.sh all
-	MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
+check: test.sh all temp-install
+	MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)

The $(DOINST) bit should go away no?

Indeed.

On Tue, May 21, 2019 at 4:50 PM Andres Freund <andres@anarazel.de> wrote:

Indeed.

At the risk of being labeled a stickler for the details, this thread
is both on the wrong mailing list and discussing a topic quite
different from what the subject line would suggest.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company