Thread: pg_upgrade test chatter

pg_upgrade test chatter

From
"Bossart, Nathan"
Date:
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


Attachment

Re: pg_upgrade test chatter

From
Tom Lane
Date:
"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



Re: pg_upgrade test chatter

From
Tom Lane
Date:
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

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 05296f7ee1..cfadc0cd70 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -122,7 +122,9 @@ static void make_directory(const char *dir);

 static void header(const char *fmt,...) pg_attribute_printf(1, 2);
 static void status(const char *fmt,...) pg_attribute_printf(1, 2);
-static void psql_command(const char *database, const char *query,...) pg_attribute_printf(2, 3);
+static StringInfo psql_start_command(void);
+static void psql_add_command(StringInfo buf, const char *query,...) pg_attribute_printf(2, 3);
+static void psql_end_command(StringInfo buf, const char *database);

 /*
  * allow core files if possible.
@@ -1136,51 +1138,94 @@ config_sspi_auth(const char *pgdata, const char *superuser_name)
 #endif                            /* ENABLE_SSPI */

 /*
- * Issue a command via psql, connecting to the specified database
+ * psql_start_command, psql_add_command, psql_end_command
+ *
+ * Issue one or more commands within one psql call.
+ * Set up with psql_start_command, then add commands one at a time
+ * with psql_add_command, and finally execute with psql_end_command.
  *
  * Since we use system(), this doesn't return until the operation finishes
  */
+static StringInfo
+psql_start_command(void)
+{
+    StringInfo    buf = makeStringInfo();
+
+    appendStringInfo(buf,
+                     "\"%s%spsql\" -X",
+                     bindir ? bindir : "",
+                     bindir ? "/" : "");
+    return buf;
+}
+
 static void
-psql_command(const char *database, const char *query,...)
+psql_add_command(StringInfo buf, const char *query,...)
 {
-    char        query_formatted[1024];
-    char        query_escaped[2048];
-    char        psql_cmd[MAXPGPATH + 2048];
-    va_list        args;
-    char       *s;
-    char       *d;
+    StringInfoData cmdbuf;
+    const char *cmdptr;
+
+    /* Add each command as a -c argument in the psql call */
+    appendStringInfoString(buf, " -c \"");

     /* Generate the query with insertion of sprintf arguments */
-    va_start(args, query);
-    vsnprintf(query_formatted, sizeof(query_formatted), query, args);
-    va_end(args);
+    initStringInfo(&cmdbuf);
+    for (;;)
+    {
+        va_list        args;
+        int            needed;
+
+        va_start(args, query);
+        needed = appendStringInfoVA(&cmdbuf, query, args);
+        va_end(args);
+        if (needed == 0)
+            break;                /* success */
+        enlargeStringInfo(&cmdbuf, needed);
+    }

     /* Now escape any shell double-quote metacharacters */
-    d = query_escaped;
-    for (s = query_formatted; *s; s++)
+    for (cmdptr = cmdbuf.data; *cmdptr; cmdptr++)
     {
-        if (strchr("\\\"$`", *s))
-            *d++ = '\\';
-        *d++ = *s;
+        if (strchr("\\\"$`", *cmdptr))
+            appendStringInfoChar(buf, '\\');
+        appendStringInfoChar(buf, *cmdptr);
     }
-    *d = '\0';

-    /* And now we can build and execute the shell command */
-    snprintf(psql_cmd, sizeof(psql_cmd),
-             "\"%s%spsql\" -X -c \"%s\" \"%s\"",
-             bindir ? bindir : "",
-             bindir ? "/" : "",
-             query_escaped,
-             database);
+    appendStringInfoChar(buf, '"');
+
+    pfree(cmdbuf.data);
+}

-    if (system(psql_cmd) != 0)
+static void
+psql_end_command(StringInfo buf, const char *database)
+{
+    /* Add the database name --- assume it needs no extra escaping */
+    appendStringInfo(buf,
+                     " \"%s\"",
+                     database);
+
+    /* And now we can execute the shell command */
+    if (system(buf->data) != 0)
     {
         /* psql probably already reported the error */
-        fprintf(stderr, _("command failed: %s\n"), psql_cmd);
+        fprintf(stderr, _("command failed: %s\n"), buf->data);
         exit(2);
     }
+
+    /* Clean up */
+    pfree(buf->data);
+    pfree(buf);
 }

+/*
+ * Shorthand macro for the common case of a single command
+ */
+#define psql_command(database, ...) \
+    do { \
+        StringInfo buf = psql_start_command(); \
+        psql_add_command(buf, __VA_ARGS__); \
+        psql_end_command(buf, database); \
+    } while (0)
+
 /*
  * Spawn a process to execute the given shell command; don't wait for it
  *
@@ -2012,8 +2057,13 @@ open_result_files(void)
 static void
 drop_database_if_exists(const char *dbname)
 {
+    StringInfo    buf = psql_start_command();
+
     header(_("dropping database \"%s\""), dbname);
-    psql_command("postgres", "DROP DATABASE IF EXISTS \"%s\"", dbname);
+    /* Set warning level so we don't see chatter about nonexistent DB */
+    psql_add_command(buf, "SET client_min_messages = warning");
+    psql_add_command(buf, "DROP DATABASE IF EXISTS \"%s\"", dbname);
+    psql_end_command(buf, "postgres");
 }

 static void
@@ -2055,8 +2105,13 @@ create_database(const char *dbname)
 static void
 drop_role_if_exists(const char *rolename)
 {
+    StringInfo    buf = psql_start_command();
+
     header(_("dropping role \"%s\""), rolename);
-    psql_command("postgres", "DROP ROLE IF EXISTS \"%s\"", rolename);
+    /* Set warning level so we don't see chatter about nonexistent role */
+    psql_add_command(buf, "SET client_min_messages = warning");
+    psql_add_command(buf, "DROP ROLE IF EXISTS \"%s\"", rolename);
+    psql_end_command(buf, "postgres");
 }

 static void

Re: pg_upgrade test chatter

From
"Bossart, Nathan"
Date:
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


Re: pg_upgrade test chatter

From
Alvaro Herrera
Date:
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/



Re: pg_upgrade test chatter

From
Tom Lane
Date:
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



Re: pg_upgrade test chatter

From
Alvaro Herrera
Date:
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)