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 <pe...@eisentraut.org>
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,
char infile[MAXPGPATH];
char outfile[MAXPGPATH];
char expectfile[MAXPGPATH];
- char psql_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(const char *testname,
char infile[MAXPGPATH];
char outfile[MAXPGPATH];
char expectfile[MAXPGPATH];
- char psql_cmd[MAXPGPATH * 3];
- size_t offset = 0;
+ StringInfoData psql_cmd;
char *appnameenv;
/*
@@ -62,40 +62,29 @@ psql_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);
/*
* Use HIDE_TABLEAM to hide different AMs to allow to use regression
tests
* against different AMs without unnecessary differences.
*/
- offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
- "\"%s%spsql\" -X -a -q -d \"%s\" %s
< \"%s\" > \"%s\" 2>&1",
- bindir ? bindir : "",
- bindir ? "/" : "",
- dblist->str,
- "-v HIDE_TABLEAM=on -v
HIDE_TOAST_COMPRESSION=on",
- infile,
- outfile);
- if (offset >= sizeof(psql_cmd))
- {
- fprintf(stderr, _("command too long\n"));
- exit(2);
- }
+ appendStringInfo(&psql_cmd,
+ "\"%s%spsql\" -X -a -q -d \"%s\" %s <
\"%s\" > \"%s\" 2>&1",
+ bindir ? bindir : "",
+ bindir ? "/" : "",
+ dblist->str,
+ "-v HIDE_TABLEAM=on -v
HIDE_TOAST_COMPRESSION=on",
+ infile,
+ outfile);
appnameenv = psprintf("pg_regress/%s", testname);
setenv("PGAPPNAME", appnameenv, 1);
free(appnameenv);
- pid = spawn_process(psql_cmd);
+ pid = spawn_process(psql_cmd.data);
if (pid == INVALID_PID)
{
@@ -106,6 +95,8 @@ psql_start_test(const char *testname,
unsetenv("PGAPPNAME");
+ pfree(psql_cmd.data);
+
return pid;
}
base-commit: 774bcffe4a9853a24e61d758637c0aad2871f1fb
--
2.43.0
From 5c47c810568020830f9a44fa701d261bb91d1c3b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sun, 4 Feb 2024 18:32:16 +0100
Subject: [PATCH 2/2] WIP: pg_upgrade: No more "command too long"
Use extensive buffers to assemble command lines instead of fixed size.
Similar to the changes done for pg_rewind in a33e17f210.
XXX requires exporting appendPQExpBufferVA() from libpq
XXX should pg_upgrade be using StringInfo instead?
---
src/bin/pg_upgrade/exec.c | 37 ++++++++++++++++----------------
src/interfaces/libpq/exports.txt | 1 +
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index fec8dc4c2f7..2ea231d6f62 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -13,6 +13,7 @@
#include "common/string.h"
#include "pg_upgrade.h"
+#include "pqexpbuffer.h"
static void check_data_dir(ClusterInfo *cluster);
static void check_bin_dir(ClusterInfo *cluster, bool check_versions);
@@ -87,13 +88,11 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
bool report_error, bool exit_on_error, const char *fmt,...)
{
int result = 0;
- int written;
char log_file[MAXPGPATH];
-
-#define MAXCMDLEN (2 * MAXPGPATH)
- char cmd[MAXCMDLEN];
+ PQExpBufferData cmd;
FILE *log;
va_list ap;
+ bool done;
#ifdef WIN32
static DWORD mainThreadId = 0;
@@ -105,18 +104,16 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
snprintf(log_file, MAXPGPATH, "%s/%s", log_opts.logdir, log_filename);
- written = 0;
- va_start(ap, fmt);
- written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap);
- va_end(ap);
- if (written >= MAXCMDLEN)
- pg_fatal("command too long");
- written += snprintf(cmd + written, MAXCMDLEN - written,
- " >> \"%s\" 2>&1", log_file);
- if (written >= MAXCMDLEN)
- pg_fatal("command too long");
+ initPQExpBuffer(&cmd);
+ do
+ {
+ va_start(ap, fmt);
+ done = appendPQExpBufferVA(&cmd, fmt, ap);
+ va_end(ap);
+ } while (!done);
+ appendPQExpBuffer(&cmd, " >> \"%s\" 2>&1", log_file);
- pg_log(PG_VERBOSE, "%s", cmd);
+ pg_log(PG_VERBOSE, "%s", cmd.data);
#ifdef WIN32
@@ -132,7 +129,7 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
if (mainThreadId != GetCurrentThreadId())
{
fflush(NULL);
- result = system(cmd);
+ result = system(cmd.data);
}
#endif
@@ -165,7 +162,7 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
if (mainThreadId == GetCurrentThreadId())
fprintf(log, "\n\n");
#endif
- fprintf(log, "command: %s\n", cmd);
+ fprintf(log, "command: %s\n", cmd.data);
#ifdef WIN32
/* Are we printing "command:" after its output? */
if (mainThreadId != GetCurrentThreadId())
@@ -184,7 +181,7 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
#endif
{
fflush(NULL);
- result = system(cmd);
+ result = system(cmd.data);
}
if (result != 0 && report_error)
@@ -193,7 +190,7 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
report_status(PG_REPORT, "\n*failure*");
fflush(stdout);
- pg_log(PG_VERBOSE, "There were problems executing \"%s\"", cmd);
+ pg_log(PG_VERBOSE, "There were problems executing \"%s\"",
cmd.data);
if (opt_log_file)
pg_log(exit_on_error ? PG_FATAL : PG_REPORT,
"Consult the last few lines of \"%s\" or
\"%s\" for\n"
@@ -221,6 +218,8 @@ exec_prog(const char *log_filename, const char
*opt_log_file,
fclose(log);
#endif
+ termPQExpBuffer(&cmd);
+
return result == 0;
}
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 088592deb16..2fc5130e7bf 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,4 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQchangePassword 192
PQsendPipelineSync 193
+appendPQExpBufferVA 194
--
2.43.0
