Thread: Suppressing noise in successful check-world runs

Suppressing noise in successful check-world runs

From
Tom Lane
Date:
[ redirected from a thread in pgsql-committers[1] ]

As of commit eb9812f27 you can run a manual check-world with
stdout dumped to /dev/null, and get fairly clean results:

$ time make check-world -j10 >/dev/null
NOTICE:  database "regression" does not exist, skipping

real    1m43.875s
user    2m50.659s
sys     1m22.518s
$

This is a productive way to work because if you do get a failure,
make's bleating gives you enough context to see which subdirectory
to check the log files in; so you don't really need to see all the
noise that goes to stdout.  (OTOH, if you don't discard stdout,
it's a mess; if you get a failure it could easily scroll off the
screen before you ever see it, leaving you with a false impression
that the test succeeded.)

However ... there is that one NOTICE, which is annoying just because
it's the only one left.  That's coming from the pg_upgrade test's
invocation of "make installcheck" in the instance it's just built.
(Every other test lets pg_regress build its own temp instance,
and then pg_regress knows it needn't bother with "DROP DATABASE
regression".)

I experimented with the attached quick-hack patch to make pg_regress
suppress notices from its various initial DROP/CREATE IF [NOT] EXISTS
commands.  I'm not entirely convinced whether suppressing them is
a good idea though.  Perhaps some hack with effects confined to
pg_upgrade's test would be better.  I don't have a good idea what
that would look like, however.

Or we could just say this isn't annoying enough to fix.

Thoughts?

            regards, tom lane

[1] https://postgr.es/m/E1hSk9C-0002hH-Vp@gemulon.postgresql.org
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);


Re: Suppressing noise in successful check-world runs

From
Peter Geoghegan
Date:
On Wed, May 22, 2019 at 3:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I experimented with the attached quick-hack patch to make pg_regress
> suppress notices from its various initial DROP/CREATE IF [NOT] EXISTS
> commands.  I'm not entirely convinced whether suppressing them is
> a good idea though.  Perhaps some hack with effects confined to
> pg_upgrade's test would be better.  I don't have a good idea what
> that would look like, however.
>
> Or we could just say this isn't annoying enough to fix.

I think it's worth fixing.

-- 
Peter Geoghegan



Re: Suppressing noise in successful check-world runs

From
Peter Geoghegan
Date:
On Fri, May 24, 2019 at 12:31 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, May 22, 2019 at 3:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I experimented with the attached quick-hack patch to make pg_regress
> > suppress notices from its various initial DROP/CREATE IF [NOT] EXISTS
> > commands.  I'm not entirely convinced whether suppressing them is
> > a good idea though.  Perhaps some hack with effects confined to
> > pg_upgrade's test would be better.  I don't have a good idea what
> > that would look like, however.
> >
> > Or we could just say this isn't annoying enough to fix.
>
> I think it's worth fixing.

My development machine has 8 logical cores, and like you I only see
the NOTICE from pg_upgrade's tests with "-j10":

pg@bat:/code/postgresql/patch/build$ time make check-world -j10 >/dev/null
NOTICE:  database "regression" does not exist, skipping
make check-world -j10 > /dev/null  86.40s user 34.10s system 140% cpu
1:25.94 total

However, I see something else with "-j16", even after a precautionary
clean + rebuild:

pg@bat:/code/postgresql/patch/build$ time make check-world -j16 >/dev/null
NOTICE:  database "regression" does not exist, skipping
pg_regress: could not open file
"/code/postgresql/patch/build/src/test/regress/regression.diffs" for
reading: No such file or directory
make check-world -j16 > /dev/null  96.35s user 37.45s system 152% cpu
1:27.49 total

I suppose this might be because of a pg_regress/make file
"regression.diffs" race. This is also a problem for my current
workflow for running "make check-world" in parallel [1], though only
when there is definitely a regression.diffs file with actual
regressions -- there is no regression that I'm missing here, and as
far as I know this output about "regression.diffs" is just more noise.
I had intended to figure out a way of keeping "regression.diffs" with
my existing workflow, since losing the details of a test failure is a
real annoyance. Especially when there is a test that doesn't fail
reliably.

[1] https://wiki.postgresql.org/wiki/Committing_checklist#Basic_checks
-- 
Peter Geoghegan



Re: Suppressing noise in successful check-world runs

From
Tom Lane
Date:
Peter Geoghegan <pg@bowt.ie> writes:
> My development machine has 8 logical cores, and like you I only see
> the NOTICE from pg_upgrade's tests with "-j10":

> pg@bat:/code/postgresql/patch/build$ time make check-world -j10 >/dev/null
> NOTICE:  database "regression" does not exist, skipping
> make check-world -j10 > /dev/null  86.40s user 34.10s system 140% cpu
> 1:25.94 total

> However, I see something else with "-j16", even after a precautionary
> clean + rebuild:

> pg@bat:/code/postgresql/patch/build$ time make check-world -j16 >/dev/null
> NOTICE:  database "regression" does not exist, skipping
> pg_regress: could not open file
> "/code/postgresql/patch/build/src/test/regress/regression.diffs" for
> reading: No such file or directory
> make check-world -j16 > /dev/null  96.35s user 37.45s system 152% cpu
> 1:27.49 total

Yes, I see that too with sufficiently high -j.  I believe this is
what Noah was trying to fix in bd1592e85, but that patch evidently
needs a bit more work :-(

            regards, tom lane



Re: Suppressing noise in successful check-world runs

From
Peter Geoghegan
Date:
On Fri, May 24, 2019 at 4:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yes, I see that too with sufficiently high -j.  I believe this is
> what Noah was trying to fix in bd1592e85, but that patch evidently
> needs a bit more work :-(

It would be nice if this was fixed, but I don't see a problem when I
use the optimum number of jobs, so I don't consider it to be urgent.

I'm happy with the new approach, since it avoids the problem of
regression.diffs files that get deleted before I have a chance to take
a look. I should thank Noah for his work on this.

-- 
Peter Geoghegan