pg_upgrade test chatter

"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

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

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

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/

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

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)