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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-3
Make VACUUM accept 1 and 0 as a boolean value.

Commit 41b54ba78e allowed existing VACUUM options to take a boolean
argument. It's documented that valid boolean values that VACUUM can
accept are true, false, on, off, 1, and 0. But previously the parser
failed to accept 1 and 0 as a boolean value in VACUUM syntax because
of a lack of NumericOnly clause for vac_analyze_option_arg in gram.y.

This commit adds such NumericOnly clause so that VACUUM options
can take also 1 and 0 as a boolean value.

Discussion: https://postgr.es/m/CAHGQGwGYg82A8UCQxZe7Zn9MnyUBGdyB=1CNpKF3jBny+RbyfA@...

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/fc7c281f87467c1ff24fd72e0cc313dd6a71873f

Modified Files
--------------
src/backend/parser/gram.y | 1 +
1 file changed, 1 insertion(+)

Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-2
On Tue, May 21, 2019 at 12:43 AM Tom Lane <[hidden email]> wrote:
>
> Fujii Masao <[hidden email]> 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

update_analyze_test.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> On Tue, May 21, 2019 at 12:43 AM Tom Lane <[hidden email]> wrote:
> >
> > Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-2
On Tue, May 21, 2019 at 1:36 AM Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-05-21 01:03:06 +0900, Fujii Masao wrote:
> > On Tue, May 21, 2019 at 12:43 AM Tom Lane <[hidden email]> wrote:
> > >
> > > Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> On Tue, May 21, 2019 at 1:36 AM Andres Freund <[hidden email]> wrote:
> > On 2019-05-21 01:03:06 +0900, Fujii Masao wrote:
> > > On Tue, May 21, 2019 at 12:43 AM Tom Lane <[hidden email]> wrote:
> > > >
> > > > Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 66e69b946b..15f98e29bc 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -22,7 +22,8 @@ unset MAKELEVEL
 standard_initdb() {
  # To increase coverage of non-standard segment size and group access
  # without increasing test runtime, run these tests with a custom setting.
- "$1" -N --wal-segsize 1 -g
+ # Also, specify "-A trust" explicitly to suppress initdb's warning.
+ "$1" -N --wal-segsize 1 -g -A trust
  if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
  then
  cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
@@ -150,9 +151,6 @@ done
 EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT"
 export EXTRA_REGRESS_OPTS
 
-# enable echo so the user can see what is being executed
-set -x
-
 standard_initdb "$oldbindir"/initdb
 "$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
 
@@ -169,6 +167,9 @@ createdb "$dbname1" || createdb_status=$?
 createdb "$dbname2" || createdb_status=$?
 createdb "$dbname3" || createdb_status=$?
 
+# Create regression database explicitly to avoid noise from pg_regress
+createdb regression
+
 if "$MAKE" -C "$oldsrc" installcheck-parallel; then
  oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
 
@@ -256,10 +257,6 @@ esac
 pg_dumpall --no-sync -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
 pg_ctl -m fast stop
 
-# no need to echo commands anymore
-set +x
-echo
-
 if [ -n "$pg_dumpall2_status" ]; then
  echo "pg_dumpall of post-upgrade database cluster failed"
  exit 1
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> 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


diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index a1a3d48..57a154c 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1087,6 +1087,7 @@ psql_command(const char *database, const char *query,...)
  char query_formatted[1024];
  char query_escaped[2048];
  char psql_cmd[MAXPGPATH + 2048];
+ const char *quiet;
  va_list args;
  char   *s;
  char   *d;
@@ -1106,11 +1107,23 @@ psql_command(const char *database, const char *query,...)
  }
  *d = '\0';
 
+ /*
+ * If the query uses IF EXISTS or IF NOT EXISTS, suppress useless
+ * "skipping" notices.  We intentionally consider only the constant
+ * "query" string, not what was interpolated into it.
+ */
+ if (strstr(query, "IF EXISTS") != NULL ||
+ strstr(query, "IF NOT EXISTS") != NULL)
+ quiet = " -c \"SET client_min_messages = warning\"";
+ else
+ quiet = "";
+
  /* And now we can build and execute the shell command */
  snprintf(psql_cmd, sizeof(psql_cmd),
- "\"%s%spsql\" -X -c \"%s\" \"%s\"",
+ "\"%s%spsql\" -X%s -c \"%s\" \"%s\"",
  bindir ? bindir : "",
  bindir ? "/" : "",
+ quiet,
  query_escaped,
  database);
 
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

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

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
In reply to this post by Andres Freund
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 <[hidden email]> 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], 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] https://postgr.es/m/20190521191918.z7kwnrlj45mk2k67%40alap3.anarazel.de

v2-0001-pg_upgrade-Don-t-use-separate-installation-for-te.patch (3K) Download Attachment
v2-0002-pg_upgrade-Avoid-check-target-accidentally-breaki.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

On 2019-05-21 16:47:55 -0400, Tom Lane wrote:
> Andres Freund <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
On Tue, May 21, 2019 at 4:50 PM Andres Freund <[hidden email]> 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


12