Re: pg_upgrade test chatter - Mailing list pgsql-hackers

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