Horton

Thread

Commits

  1. Improve pg_regress.c's infrastructure for issuing psql commands.

  1. pg_upgrade test chatter

    Nathan Bossart <bossartn@amazon.com> — 2021-10-19T17:41:29Z

    Hi hackers,

I run 'make check-world' a lot, and I typically use parallelism and
redirect stdout to /dev/null as suggested in the docs [0].  This seems
to eliminate all of the test chatter except for this one message:

        NOTICE:  database "regression" does not exist, skipping

This is emitted by the installcheck-parallel run in the pg_upgrade
test.  Sending stderr to stdout clears it up, but presumably we don't
want to miss other errors, too.  We could also just create the
database it is trying to drop to silence the NOTICE.  This is what the
attached patch does.

This is admittedly just a pet peeve, but maybe it is bothering others,
too.

Nathan

[0] https://www.postgresql.org/docs/devel/regress-run.html

  2. Re: pg_upgrade test chatter

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-19T18:00:40Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> I run 'make check-world' a lot, and I typically use parallelism and
> redirect stdout to /dev/null as suggested in the docs [0].  This seems
> to eliminate all of the test chatter except for this one message:

>         NOTICE:  database "regression" does not exist, skipping

Yeah, that's bugged me too ever since we got to the point where that
was the only output ...

> We could also just create the
> database it is trying to drop to silence the NOTICE.

... but that seems like a mighty expensive way to fix it.
createdb is pretty slow on older/slower buildfarm animals.

Maybe we could run the stderr output through "grep -v", or the like?

			regards, tom lane

  3. Re: pg_upgrade test chatter

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-19T19:36:14Z

    I wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> I run 'make check-world' a lot, and I typically use parallelism and
>> redirect stdout to /dev/null as suggested in the docs [0].  This seems
>> to eliminate all of the test chatter except for this one message:
>> NOTICE:  database "regression" does not exist, skipping

> Yeah, that's bugged me too ever since we got to the point where that
> was the only output ...

Actually ... why shouldn't we suppress that by running the command
with client_min_messages = warning?  This would have to be a change
to pg_regress, but I'm having a hard time thinking of cases where
quieting that message would be a problem.

I tried doing this as a one-liner change in pg_regress's
drop_database_if_exists(), but the idea fell over pretty
quickly, because what underlies that is a "psql -c" call:

$ psql -c 'set client_min_messages = warning; drop database if exists foo'
ERROR:  DROP DATABASE cannot run inside a transaction block

We could dodge that, with modern versions of psql, by issuing
two -c switches.  So after a bit of hacking I have the attached
POC patch.  It's incomplete because now that we have this
infrastructure we should change other parts of pg_regress
to not launch psql N times where one would do.  But it's enough
to get through check-world without any chatter.

Any objections to polishing this up and pushing it?

			regards, tom lane

  4. Re: pg_upgrade test chatter

    Nathan Bossart <bossartn@amazon.com> — 2021-10-19T20:20:44Z

    On 10/19/21, 12:37 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Actually ... why shouldn't we suppress that by running the command
> with client_min_messages = warning?  This would have to be a change
> to pg_regress, but I'm having a hard time thinking of cases where
> quieting that message would be a problem.

I was just looking into something like this.

> We could dodge that, with modern versions of psql, by issuing
> two -c switches.  So after a bit of hacking I have the attached
> POC patch.  It's incomplete because now that we have this
> infrastructure we should change other parts of pg_regress
> to not launch psql N times where one would do.  But it's enough
> to get through check-world without any chatter.
> 
> Any objections to polishing this up and pushing it?

No objections here.  This seems like an overall improvement, and I
confirmed that it clears up the NOTICE from the pg_upgrade test.

Nathan

  5. Re: pg_upgrade test chatter

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-20T01:55:31Z

    On 2021-Oct-19, Tom Lane wrote:

> I tried doing this as a one-liner change in pg_regress's
> drop_database_if_exists(), but the idea fell over pretty
> quickly, because what underlies that is a "psql -c" call:
> 
> $ psql -c 'set client_min_messages = warning; drop database if exists foo'
> ERROR:  DROP DATABASE cannot run inside a transaction block
> 
> We could dodge that, with modern versions of psql, by issuing
> two -c switches.

Isn't it easier to pass client_min_messages via PGOPTIONS?

PGOPTIONS="-c client_min_messages=warning" psql -c "drop database if exists foo"


-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

  6. Re: pg_upgrade test chatter

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-20T02:08:52Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Oct-19, Tom Lane wrote:
>> We could dodge that, with modern versions of psql, by issuing
>> two -c switches.

> Isn't it easier to pass client_min_messages via PGOPTIONS?

> PGOPTIONS="-c client_min_messages=warning" psql -c "drop database if exists foo"

Yeah, my original thought had been to hack this at the test level.
However, I felt like it'd be worth adding this code because we could
apply it elsewhere in pg_regress.c to save several psql sessions
(and hence backend starts) per regression DB creation.  That's not a
huge win, but it'd add up.

			regards, tom lane

  7. Re: pg_upgrade test chatter

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-20T02:37:28Z

    On 2021-Oct-19, Tom Lane wrote:

> Yeah, my original thought had been to hack this at the test level.
> However, I felt like it'd be worth adding this code because we could
> apply it elsewhere in pg_regress.c to save several psql sessions
> (and hence backend starts) per regression DB creation.  That's not a
> huge win, but it'd add up.

Ah, yeah, that sounds like it can be significant under valgrind and
such, so +1.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)