Clean up command argument assembly
This is a small code cleanup patch.
Several commands internally assemble command lines to call other
commands. This includes initdb, pg_dumpall, and pg_regress. (Also
pg_ctl, but that is different enough that I didn't consider it here.)
This has all evolved a bit organically, with fixed-size buffers, and
various optional command-line arguments being injected with
confusing-looking code, and the spacing between options handled in
inconsistent ways. This patch cleans all this up a bit to look clearer
and be more easily extensible with new arguments and options. We start
each command with printfPQExpBuffer(), and then append arguments as
necessary with appendPQExpBuffer(). Also standardize on using
initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress
uses StringInfo instead of PQExpBuffer, but many of the same ideas apply.From dbd9d7b7ff2c069a131c9efa8bff2597c0d9e1c8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 26 Jun 2023 11:30:24 +0200
Subject: [PATCH] Clean up command argument assembly
Several commands internally assemble command lines to call other
commands. This includes initdb, pg_dumpall, and pg_regress. (Also
pg_ctl, but that is different enough that I didn't consider it here.)
This has all evolved a bit organically, with fixed-size buffers, and
various optional command-line arguments being injected with
confusing-looking code, and the spacing between options handled in
inconsistent ways. Clean all this up a bit to look clearer and be
more easily extensible with new arguments and options. We start each
command with printfPQExpBuffer(), and then append arguments as
necessary with appendPQExpBuffer(). Also standardize on using
initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress
uses StringInfo instead of PQExpBuffer, but many of the same ideas
apply.
---
src/bin/initdb/initdb.c | 56 +++
src/bin/pg_dump/pg_dumpall.c | 27 +
src/test/regress/pg_regress.c | 26 +---
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index fc1fb363e7..3f4167682a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -309,16 +309,16 @@ void initialize_data_directory(void);
/*
* macros for running pipes to postgres
*/
-#define PG_CMD_DECLchar cmd[MAXPGPATH]; FILE *cmdfd
+#define PG_CMD_DECLFILE *cmdfd
-#define PG_CMD_OPEN \
+#define PG_CMD_OPEN(cmd) \
do { \
cmdfd = popen_check(cmd, "w"); \
if (cmdfd == NULL) \
exit(1); /* message already printed by popen_check */ \
} while (0)
-#define PG_CMD_CLOSE \
+#define PG_CMD_CLOSE() \
do { \
if (pclose_check(cmdfd)) \
exit(1); /* message already printed by pclose_check */ \
@@ -1138,13 +1138,15 @@ test_config_settings(void)
static bool
test_specific_config_settings(int test_conns, int test_buffs)
{
- PQExpBuffer cmd = createPQExpBuffer();
+ PQExpBufferData cmd;
_stringlist *gnames,
*gvalues;
int status;
+ initPQExpBuffer(&cmd);
+
/* Set up the test postmaster invocation */
- printfPQExpBuffer(cmd,
+ printfPQExpBuffer(&cmd,
"\"%s\" --check %s %s "
"-c max_connections=%d "
"-c shared_buffers=%d "
@@ -1158,18 +1160,18 @@ test_specific_config_settings(int test_conns, int
test_buffs)
gnames != NULL;/* assume lists have the same
length */
gnames = gnames->next, gvalues = gvalues->next)
{
- appendPQExpBuffer(cmd, " -c %s=", gnames->str);
- appendShellString(cmd, gvalues->str);
+ appendPQExpBuffer(&cmd, " -c %s=", gnames->str);
+ appendShellString(&cmd, gvalues->str);
}
- appendPQExpBuffer(cmd,
+ appendPQExpBuffer(&cmd,
" < \"%s\" > \"%s\" 2>&1",
DEVNULL, DEVNULL);
fflush(NULL);
- status = system(cmd->data);
+ status = system(cmd.data);
- destroyPQExpBuffer(cmd);
+ termPQExpBuffer(&cmd);
return (status == 0);
}
@@ -1469,6 +1471,7 @@ static void
bootstrap_template1(void)
{
PG_CMD_DECL;
+ PQExpBufferData cmd;
char **line;
char **bki_lines;
charheaderline[MAXPGPATH];
@@ -1530,16 +1533,17 @@ bootstrap_template1(void)
/* Also ensure backend isn't confused by this environment var: */
unsetenv("PGCLIENTENCODING");
- sn
Re: Clean up command argument assembly
On 05.07.23 07:22, Peter Eisentraut wrote:
It's a bit bogus to use PQExpBuffer for these. If you run out of
memory, you silently get an empty string instead. StringInfo, which
exits the process on OOM, would be more appropriate. We have tons of
such inappropriate uses of PQExpBuffer in all our client programs,
though, so I don't insist on fixing this particular case right now.
Interesting point. But as you say better dealt with as a separate problem.
I was inspired by a33e17f210 (for pg_rewind) to clean up some more
fixed-buffer command assembly and replace it with extensible buffers and
some more elegant code. And then I remembered this thread, and it's
really a continuation of this.
The first patch deals with pg_regress and pg_isolation_regress. It is
pretty straightforward.
The second patch deals with pg_upgrade. It would require exporting
appendPQExpBufferVA() from libpq, which might be overkill. But this
gets to your point earlier: Should pg_upgrade rather be using
StringInfo instead of PQExpBuffer? (Then we'd use appendStringInfoVA(),
which already exists, but even if not we wouldn't need to change libpq
to get it.) Should anything outside of libpq be using PQExpBuffer?
From dc3da0dce85a69314fff0a03998fd89b4ca19d8c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 4 Feb 2024 18:32:16 +0100
Subject: [PATCH 1/2] Use extensible buffers to assemble command lines
This makes use of StringInfo to assemble command lines, instead of
using fixed-size buffers and the (remote) possibility of "command too
long" errors. Also makes the code a bit simpler.
This covers the test driver programs pg_regress and
pg_isolation_regress.
Similar to the changes done for pg_rewind in a33e17f210.
---
src/test/isolation/isolation_main.c | 37 ++
src/test/regress/pg_regress_main.c | 41 +++--
2 files changed, 30 insertions(+), 48 deletions(-)
diff --git a/src/test/isolation/isolation_main.c
b/src/test/isolation/isolation_main.c
index 05e81035c1f..2a3e41d2101 100644
--- a/src/test/isolation/isolation_main.c
+++ b/src/test/isolation/isolation_main.c
@@ -12,6 +12,7 @@
#include "postgres_fe.h"
+#include "lib/stringinfo.h"
#include "pg_regress.h"
char saved_argv0[MAXPGPATH];
@@ -34,8 +35,7 @@ isolation_start_test(const char *testname,
charinfile[MAXPGPATH];
charoutfile[MAXPGPATH];
charexpectfile[MAXPGPATH];
- charpsql_cmd[MAXPGPATH * 3];
- size_t offset = 0;
+ StringInfoData psql_cmd;
char *appnameenv;
/* need to do the path lookup here, check isolation_init() for details
*/
@@ -75,34 +75,23 @@ isolation_start_test(const char *testname,
add_stringlist_item(resultfiles, outfile);
add_stringlist_item(expectfiles, expectfile);
+ initStringInfo(&psql_cmd);
+
if (launcher)
- {
- offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
- "%s ", launcher);
- if (offset >= sizeof(psql_cmd))
- {
- fprintf(stderr, _("command too long\n"));
- exit(2);
- }
- }
+ appendStringInfo(&psql_cmd, "%s ", launcher);
- offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
- "\"%s\" \"dbname=%s\" < \"%s\" >
\"%s\" 2>&1",
- isolation_exec,
- dblist->str,
- infile,
- outfile);
- if (offset >= sizeof(psql_cmd))
- {
- fprintf(stderr, _("command too long\n"));
- exit(2);
- }
+ appendStringInfo(&psql_cmd,
+"\"%s\" \"dbname=%s\" < \"%s\" >
\"%s\" 2>&1",
+isolation_exec,
+dblist->str,
+infile,
+outfile);
appnameenv = psprintf("isolation/%s", testname);
setenv("PGAPPNAME", appnameenv, 1);
free(appnameenv);
- pid = spawn_process(psql_cmd);
+ pid = spawn_process(psql_cmd.data);
if (pid == INVALID_PID)
{
@@ -113,6 +102,8 @@ isolation_start_test(const char *testname,
unsetenv("PGAPPNAME");
+ pfree(psql_cmd.data);
+
return pid;
}
diff --git a/src/test/regress/pg_regress_main.c
b/src/test/regress/pg_regress_main.c
index 18155ef97e2..d607a3023b2 100644
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -18,6 +18,7 @@
#include "postgres_fe.h"
+#include "lib/stringinfo.h"
#include "pg_regress.h"
/*
@@ -34,8 +35,7 @@ psql_start_test(
Re: Clean up command argument assembly
Peter Eisentraut writes: > Should anything outside of libpq be using PQExpBuffer? Perhaps not. PQExpBuffer's behavior for OOM cases is designed specifically for libpq, where exit-on-OOM is not okay and we can hope to include failure checks wherever needed. For most of our application code, we'd much rather just exit-on-OOM and not have to think about failure checks at the call sites. Having said that, converting stuff like pg_dump would be quite awful in terms of code churn and creating a back-patching nightmare. Would it make any sense to think about having two sets of routines with identical call APIs, but different failure behavior, so that we don't have to touch the callers? regards, tom lane
Re: Clean up command argument assembly
On 26/06/2023 12:33, Peter Eisentraut wrote: This is a small code cleanup patch. Several commands internally assemble command lines to call other commands. This includes initdb, pg_dumpall, and pg_regress. (Also pg_ctl, but that is different enough that I didn't consider it here.) This has all evolved a bit organically, with fixed-size buffers, and various optional command-line arguments being injected with confusing-looking code, and the spacing between options handled in inconsistent ways. This patch cleans all this up a bit to look clearer and be more easily extensible with new arguments and options. +1 We start each command with printfPQExpBuffer(), and then append arguments as necessary with appendPQExpBuffer(). Also standardize on using initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress uses StringInfo instead of PQExpBuffer, but many of the same ideas apply. It's a bit bogus to use PQExpBuffer for these. If you run out of memory, you silently get an empty string instead. StringInfo, which exits the process on OOM, would be more appropriate. We have tons of such inappropriate uses of PQExpBuffer in all our client programs, though, so I don't insist on fixing this particular case right now. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Clean up command argument assembly
On 04.07.23 14:14, Heikki Linnakangas wrote: On 26/06/2023 12:33, Peter Eisentraut wrote: This is a small code cleanup patch. Several commands internally assemble command lines to call other commands. This includes initdb, pg_dumpall, and pg_regress. (Also pg_ctl, but that is different enough that I didn't consider it here.) This has all evolved a bit organically, with fixed-size buffers, and various optional command-line arguments being injected with confusing-looking code, and the spacing between options handled in inconsistent ways. This patch cleans all this up a bit to look clearer and be more easily extensible with new arguments and options. +1 committed We start each command with printfPQExpBuffer(), and then append arguments as necessary with appendPQExpBuffer(). Also standardize on using initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress uses StringInfo instead of PQExpBuffer, but many of the same ideas apply. It's a bit bogus to use PQExpBuffer for these. If you run out of memory, you silently get an empty string instead. StringInfo, which exits the process on OOM, would be more appropriate. We have tons of such inappropriate uses of PQExpBuffer in all our client programs, though, so I don't insist on fixing this particular case right now. Interesting point. But as you say better dealt with as a separate problem.
