Robert Haas <robertmh...@gmail.com> writes:
> On Thu, Aug 25, 2022 at 10:48 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>> If we were executing a program that the user needs to have some control
>> over, sure, but what we have here is an implementation detail that I
>> doubt anyone cares about. The fact that we're using a shell at all is
>> only because nobody has cared to manually implement I/O redirection logic
>> in these places; otherwise we'd be execl()'ing the server or psql directly.
>> Maybe the best answer would be to do that, and get out of the business
>> of knowing where the shell is?
> Well that also would not be crazy.
I experimented with this, and it seems like it might not be as awful as
we've always assumed it would be. Attached is an incomplete POC that
converts pg_regress proper to doing things this way. (isolationtester
and pg_regress_ecpg are outright broken by this patch, because they rely
on pg_regress' spawn_process and I didn't fix them yet. But you can run
the core regression tests to see it works.)
The Windows side of this is completely untested and may be broken; also,
perhaps Windows has something more nearly equivalent to execvp() that we
could use instead of reconstructing a command line? It's annoying that
the patch removes all shell-quoting hazards on the Unix side but they
are still there on the Windows side.
regards, tom lane
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index a803355f8e..e1e0d5f7a2 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -19,6 +19,7 @@
#include "postgres_fe.h"
#include <ctype.h>
+#include <fcntl.h>
#include <sys/resource.h>
#include <sys/stat.h>
#include <sys/time.h>
@@ -51,10 +52,6 @@ typedef struct _resultmap
*/
char *host_platform = HOST_TUPLE;
-#ifndef WIN32 /* not used in WIN32 case */
-static char *shellprog = SHELLPROG;
-#endif
-
/*
* On Windows we use -w in diff switches to avoid problems with inconsistent
* newline representation. The actual result files will generally have
@@ -73,7 +70,7 @@ _stringlist *dblist = NULL;
bool debug = false;
char *inputdir = ".";
char *outputdir = ".";
-char *expecteddir = ".";
+char *expecteddir = ".";
char *bindir = PGBINDIR;
char *launcher = NULL;
static _stringlist *loadextension = NULL;
@@ -1052,12 +1049,71 @@ psql_end_command(StringInfo buf, const char *database)
} while (0)
/*
- * Spawn a process to execute the given shell command; don't wait for it
+ * Redirect f to the file specified by fpath, opened with given flags and mode
+ * Does not return upon failure
+ */
+static void
+redirect_to(FILE *f, const char *fpath, int flags, int mode)
+{
+ int fh;
+
+ /* Let's just be sure the FILE is idle */
+ fflush(f);
+
+ fh = open(fpath, flags, mode);
+ if (fh < 0)
+ {
+ fprintf(stderr, "could not open file \"%s\": %m\n", fpath);
+ _exit(1);
+ }
+ if (dup2(fh, fileno(f)) < 0)
+ {
+ fprintf(stderr, "could not duplicate descriptor for file \"%s\": %m\n",
+ fpath);
+ _exit(1);
+ }
+ (void) close(fh);
+}
+
+/*
+ * Redirect f to the same file used by fref
+ * Does not return upon failure
+ */
+static void
+redirect_link(FILE *f, FILE *fref)
+{
+ /* Let's just be sure the FILE is idle */
+ fflush(f);
+
+ if (dup2(fileno(fref), fileno(f)) < 0)
+ {
+ fprintf(stderr, "could not duplicate file descriptor: %m\n");
+ _exit(1);
+ }
+}
+
+/*
+ * Spawn a process to execute a sub-command; don't wait for it
*
* Returns the process ID (or HANDLE) so we can wait for it later
+ * Does not return upon failure
+ *
+ * Arguments:
+ * file: program to be executed (may be a full path, or just program name)
+ * argv: NULL-terminated array of argument strings, as for execvp(2);
+ * argv[0] should normally be the same as file
+ * proc_stdin: filename to make child's stdin read from, or NULL
+ * for no redirection
+ * proc_stdout: filename to make child's stdout write to, or NULL
+ * for no redirection
+ * proc_stderr: filename to make child's stderr write to, or NULL
+ * for no redirection, or "&1" to link it to child's stdout
*/
PID_TYPE
-spawn_process(const char *cmdline)
+spawn_process(const char *file, char *argv[],
+ const char *proc_stdin,
+ const char *proc_stdout,
+ const char *proc_stderr)
{
#ifndef WIN32
pid_t pid;
@@ -1085,35 +1141,65 @@ spawn_process(const char *cmdline)
if (pid == 0)
{
/*
- * In child
- *
- * Instead of using system(), exec the shell directly, and tell it to
- * "exec" the command too. This saves two useless processes per
- * parallel test case.
+ * In child: redirect stdio as requested, then execvp()
*/
- char *cmdline2;
-
- cmdline2 = psprintf("exec %s", cmdline);
- execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
+ if (proc_stdin)
+ redirect_to(stdin, proc_stdin, O_RDONLY, 0);
+ if (proc_stdout)
+ redirect_to(stdout, proc_stdout, O_WRONLY | O_CREAT | O_TRUNC,
+ S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
+ if (proc_stderr)
+ {
+ if (strcmp(proc_stderr, "&1") == 0)
+ redirect_link(stderr, stdout);
+ else
+ redirect_to(stderr, proc_stderr, O_WRONLY | O_CREAT | O_TRUNC,
+ S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
+ }
+ execvp(file, argv);
+ /* must have failed */
fprintf(stderr, _("%s: could not exec \"%s\": %s\n"),
- progname, shellprog, strerror(errno));
+ progname, file, strerror(errno));
_exit(1); /* not exit() here... */
}
/* in parent */
return pid;
#else
PROCESS_INFORMATION pi;
+ StringInfoData cmdline;
char *cmdline2;
HANDLE restrictedToken;
const char *comspec;
+ /*
+ * Construct the equivalent command string. We assume simplistic double
+ * quoting of the arguments is sufficient.
+ */
+ initStringInfo(&cmdline);
+ appendStringInfo(&cmdline, "\"%s\"", path);
+ for (int i = 1; argv[i] != NULL; i++)
+ appendStringInfo(&cmdline, " \"%s\"", argv[i]);
+ if (proc_stdin)
+ appendStringInfo(&cmdline, " <\"%s\"", proc_stdin);
+ if (proc_stdout)
+ appendStringInfo(&cmdline, " >\"%s\"", proc_stdout);
+ if (proc_stderr)
+ {
+ if (strcmp(proc_stderr, "&1") == 0)
+ appendStringInfo(&cmdline, " 2>&1");
+ else
+ appendStringInfo(&cmdline, " 2>\"%s\"", proc_stderr);
+ }
+
/* Find CMD.EXE location using COMSPEC, if it's set */
comspec = getenv("COMSPEC");
if (comspec == NULL)
comspec = "CMD";
+ /* Prepare command for CMD.EXE */
+ cmdline2 = psprintf("\"%s\" /c \"%s\"", comspec, cmdline.data);
+
memset(&pi, 0, sizeof(pi));
- cmdline2 = psprintf("\"%s\" /c \"%s\"", comspec, cmdline);
if ((restrictedToken =
CreateRestrictedProcess(cmdline2, &pi)) == 0)
@@ -2228,6 +2314,8 @@ regression_main(int argc, char *argv[],
FILE *pg_conf;
const char *env_wait;
int wait_seconds;
+ char *pmargv[20];
+ char *outputname;
/*
* Prepare the temp instance
@@ -2361,16 +2449,30 @@ regression_main(int argc, char *argv[],
* Start the temp postmaster
*/
header(_("starting postmaster"));
- snprintf(buf, sizeof(buf),
- "\"%s%spostgres\" -D \"%s/data\" -F%s "
- "-c \"listen_addresses=%s\" -k \"%s\" "
- "> \"%s/log/postmaster.log\" 2>&1",
- bindir ? bindir : "",
- bindir ? "/" : "",
- temp_instance, debug ? " -d 5" : "",
- hostname ? hostname : "", sockdir ? sockdir : "",
- outputdir);
- postmaster_pid = spawn_process(buf);
+ i = 0;
+ pmargv[i++] = psprintf("%s%spostgres",
+ bindir ? bindir : "",
+ bindir ? "/" : "");
+ pmargv[i++] = "-D";
+ pmargv[i++] = psprintf("%s/data", temp_instance);
+ pmargv[i++] = "-F";
+ if (debug)
+ {
+ pmargv[i++] = "-d";
+ pmargv[i++] = "5";
+ }
+ pmargv[i++] = "-c";
+ pmargv[i++] = psprintf("listen_addresses=%s",
+ hostname ? hostname : "");
+ pmargv[i++] = "-k";
+ pmargv[i++] = psprintf("%s", sockdir ? sockdir : "");
+ pmargv[i++] = NULL;
+ Assert(i <= lengthof(pmargv));
+ outputname = psprintf("%s/log/postmaster.log", outputdir);
+ postmaster_pid = spawn_process(pmargv[0], pmargv,
+ NULL,
+ outputname,
+ "&1");
if (postmaster_pid == INVALID_PID)
{
fprintf(stderr, _("\n%s: could not spawn postmaster: %s\n"),
diff --git a/src/test/regress/pg_regress.h b/src/test/regress/pg_regress.h
index d8772fec8e..a9eb1a3ba1 100644
--- a/src/test/regress/pg_regress.h
+++ b/src/test/regress/pg_regress.h
@@ -65,5 +65,8 @@ int regression_main(int argc, char *argv[],
postprocess_result_function postfunc);
void add_stringlist_item(_stringlist **listhead, const char *str);
-PID_TYPE spawn_process(const char *cmdline);
+PID_TYPE spawn_process(const char *file, char *argv[],
+ const char *proc_stdin,
+ const char *proc_stdout,
+ const char *proc_stderr);
bool file_exists(const char *file);
diff --git a/src/test/regress/pg_regress_main.c b/src/test/regress/pg_regress_main.c
index a4b354c9e6..0b5ac124e9 100644
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -34,8 +34,8 @@ psql_start_test(const char *testname,
char infile[MAXPGPATH];
char outfile[MAXPGPATH];
char expectfile[MAXPGPATH];
- char psql_cmd[MAXPGPATH * 3];
- size_t offset = 0;
+ char *psqlargv[20];
+ int i = 0;
char *appnameenv;
/*
@@ -63,40 +63,30 @@ psql_start_test(const char *testname,
add_stringlist_item(expectfiles, expectfile);
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);
- }
- }
-
- /*
- * 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);
- }
+ psqlargv[i++] = launcher;
+
+ psqlargv[i++] = psprintf("%s%spsql",
+ bindir ? bindir : "",
+ bindir ? "/" : "");
+ psqlargv[i++] = "-Xaq";
+ psqlargv[i++] = "-d";
+ psqlargv[i++] = dblist->str;
+ psqlargv[i++] = "-v";
+ /* Hide TABLEAM and compression to allow tests against different AMs */
+ psqlargv[i++] = "HIDE_TABLEAM=on";
+ psqlargv[i++] = "-v";
+ psqlargv[i++] = "HIDE_TOAST_COMPRESSION=on";
+ psqlargv[i++] = NULL;
+ Assert(i <= lengthof(psqlargv));
appnameenv = psprintf("pg_regress/%s", testname);
setenv("PGAPPNAME", appnameenv, 1);
free(appnameenv);
- pid = spawn_process(psql_cmd);
-
+ pid = spawn_process(psqlargv[0], psqlargv,
+ infile,
+ outfile,
+ "&1");
if (pid == INVALID_PID)
{
fprintf(stderr, _("could not start process for test %s\n"),
