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 <pe...@eisentraut.org>
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_DECL char cmd[MAXPGPATH]; FILE *cmdfd
+#define PG_CMD_DECL FILE *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;
char headerline[MAXPGPATH];
@@ -1530,16 +1533,17 @@ bootstrap_template1(void)
/* Also ensure backend isn't confused by this environment var: */
unsetenv("PGCLIENTENCODING");
- snprintf(cmd, sizeof(cmd),
- "\"%s\" --boot -X %d %s %s %s %s",
- backend_exec,
- wal_segment_size_mb * (1024 * 1024),
- data_checksums ? "-k" : "",
- boot_options, extra_options,
- debug ? "-d 5" : "");
+ initPQExpBuffer(&cmd);
+
+ printfPQExpBuffer(&cmd, "\"%s\" --boot %s %s", backend_exec,
boot_options, extra_options);
+ appendPQExpBuffer(&cmd, " -X %d", wal_segment_size_mb * (1024 * 1024));
+ if (data_checksums)
+ appendPQExpBuffer(&cmd, " -k");
+ if (debug)
+ appendPQExpBuffer(&cmd, " -d 5");
- PG_CMD_OPEN;
+ PG_CMD_OPEN(cmd.data);
for (line = bki_lines; *line != NULL; line++)
{
@@ -1547,8 +1551,9 @@ bootstrap_template1(void)
free(*line);
}
- PG_CMD_CLOSE;
+ PG_CMD_CLOSE();
+ termPQExpBuffer(&cmd);
free(bki_lines);
check_ok();
@@ -2951,6 +2956,7 @@ void
initialize_data_directory(void)
{
PG_CMD_DECL;
+ PQExpBufferData cmd;
int i;
setup_signals();
@@ -3014,12 +3020,11 @@ initialize_data_directory(void)
fputs(_("performing post-bootstrap initialization ... "), stdout);
fflush(stdout);
- snprintf(cmd, sizeof(cmd),
- "\"%s\" %s %s template1 >%s",
- backend_exec, backend_options, extra_options,
- DEVNULL);
+ initPQExpBuffer(&cmd);
+ printfPQExpBuffer(&cmd, "\"%s\" %s %s template1 >%s",
+ backend_exec, backend_options,
extra_options, DEVNULL);
- PG_CMD_OPEN;
+ PG_CMD_OPEN(cmd.data);
setup_auth(cmdfd);
@@ -3054,7 +3059,8 @@ initialize_data_directory(void)
make_postgres(cmdfd);
- PG_CMD_CLOSE;
+ PG_CMD_CLOSE();
+ termPQExpBuffer(&cmd);
check_ok();
}
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3627b69e2a..36f134137f 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1564,11 +1564,14 @@ dumpDatabases(PGconn *conn)
static int
runPgDump(const char *dbname, const char *create_opts)
{
- PQExpBuffer connstrbuf = createPQExpBuffer();
- PQExpBuffer cmd = createPQExpBuffer();
+ PQExpBufferData connstrbuf;
+ PQExpBufferData cmd;
int ret;
- appendPQExpBuffer(cmd, "\"%s\" %s %s", pg_dump_bin,
+ initPQExpBuffer(&connstrbuf);
+ initPQExpBuffer(&cmd);
+
+ printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
pgdumpopts->data, create_opts);
/*
@@ -1576,27 +1579,27 @@ runPgDump(const char *dbname, const char *create_opts)
* format.
*/
if (filename)
- appendPQExpBufferStr(cmd, " -Fa ");
+ appendPQExpBufferStr(&cmd, " -Fa ");
else
- appendPQExpBufferStr(cmd, " -Fp ");
+ appendPQExpBufferStr(&cmd, " -Fp ");
/*
* Append the database name to the already-constructed stem of
connection
* string.
*/
- appendPQExpBuffer(connstrbuf, "%s dbname=", connstr);
- appendConnStrVal(connstrbuf, dbname);
+ appendPQExpBuffer(&connstrbuf, "%s dbname=", connstr);
+ appendConnStrVal(&connstrbuf, dbname);
- appendShellString(cmd, connstrbuf->data);
+ appendShellString(&cmd, connstrbuf.data);
- pg_log_info("running \"%s\"", cmd->data);
+ pg_log_info("running \"%s\"", cmd.data);
fflush(NULL);
- ret = system(cmd->data);
+ ret = system(cmd.data);
- destroyPQExpBuffer(cmd);
- destroyPQExpBuffer(connstrbuf);
+ termPQExpBuffer(&cmd);
+ termPQExpBuffer(&connstrbuf);
return ret;
}
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 60d34a40b2..3e4fd918dd 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2293,6 +2293,7 @@ regression_main(int argc, char *argv[],
if (temp_instance)
{
+ StringInfoData cmd;
FILE *pg_conf;
const char *env_wait;
int wait_seconds;
@@ -2318,23 +2319,28 @@ regression_main(int argc, char *argv[],
make_directory(buf);
/* initdb */
- snprintf(buf, sizeof(buf),
- "\"%s%sinitdb\" -D \"%s/data\" --no-clean
--no-sync%s%s > \"%s/log/initdb.log\" 2>&1",
- bindir ? bindir : "",
- bindir ? "/" : "",
- temp_instance,
- debug ? " --debug" : "",
- nolocale ? " --no-locale" : "",
- outputdir);
+ initStringInfo(&cmd);
+ appendStringInfo(&cmd,
+ "\"%s%sinitdb\" -D \"%s/data\"
--no-clean --no-sync",
+ bindir ? bindir : "",
+ bindir ? "/" : "",
+ temp_instance);
+ if (debug)
+ appendStringInfo(&cmd, " --debug");
+ if (nolocale)
+ appendStringInfo(&cmd, " --no-locale");
+ appendStringInfo(&cmd, " > \"%s/log/initdb.log\" 2>&1",
outputdir);
fflush(NULL);
- if (system(buf))
+ if (system(cmd.data))
{
bail("initdb failed\n"
"# Examine \"%s/log/initdb.log\" for the
reason.\n"
"# Command was: %s",
- outputdir, buf);
+ outputdir, cmd.data);
}
+ pfree(cmd.data);
+
/*
* Adjust the default postgresql.conf for regression testing.
The user
* can specify a file to be appended; in any case we expand
logging
--
2.41.0
