On 2021-09-03 02:09, Tom Lane wrote:
I think that these free() calls you propose to add are a complete
waste of code space. Certainly a free() right before an exit() call
is that; if anything, it's *delaying* recycling the memory space for
some useful purpose. But no part of pg_ctl runs long enough for it
to be worth worrying about small leaks.
OK, I have removed the free() before exit().
I do not find your proposed test case to be a useful expenditure
of test cycles, either. If it ever fails, we'd learn nothing,
except that that particular platform has a surprisingly small
command line length limit.
Hmm, it's a test case that fails with the current code and stops failing
with my fix, so I've put it there to show the problem. But, truly, it
does not bring much value after the fix is applied.
Attaching the new version, with the test case and free-before-exit
removed.
-- Ph.
From 4f8e39f3c5e39b5ec507ec4b07ad5d33c6610524 Mon Sep 17 00:00:00 2001
From: Phil Krylov <p...@krylov.eu>
Date: Thu, 2 Sep 2021 21:39:58 +0200
Subject: [PATCH] pg_ctl should not truncate command lines at 1024 characters
---
src/bin/pg_ctl/pg_ctl.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..e07b5f02d7 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -442,7 +442,7 @@ free_readfile(char **optlines)
static pgpid_t
start_postmaster(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
#ifndef WIN32
pgpid_t pm_pid;
@@ -487,12 +487,12 @@ start_postmaster(void)
* has the same PID as the current child process.
*/
if (log_file != NULL)
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts,
- DEVNULL, log_file);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts,
+ DEVNULL, log_file);
else
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
@@ -553,12 +553,12 @@ start_postmaster(void)
else
close(fd);
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
}
else
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
if (!CreateRestrictedProcess(cmd, &pi, false))
{
@@ -566,6 +566,7 @@ start_postmaster(void)
progname, (unsigned long) GetLastError());
exit(1);
}
+ free(cmd);
/* Don't close command process handle here; caller must do so */
postmasterProcess = pi.hProcess;
CloseHandle(pi.hThread);
@@ -828,7 +829,7 @@ find_other_exec_or_die(const char *argv0, const char *target, const char *versio
static void
do_init(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
if (exec_path == NULL)
exec_path = find_other_exec_or_die(argv0, "initdb", "initdb (PostgreSQL) " PG_VERSION "\n");
@@ -840,17 +841,18 @@ do_init(void)
post_opts = "";
if (!silent_mode)
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s",
- exec_path, pgdata_opt, post_opts);
+ cmd = psprintf("\"%s\" %s%s",
+ exec_path, pgdata_opt, post_opts);
else
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s > \"%s\"",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" %s%s > \"%s\"",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
if (system(cmd) != 0)
{
write_stderr(_("%s: database system initialization failed\n"), progname);
exit(1);
}
+ free(cmd);
}
static void
@@ -2175,7 +2177,7 @@ set_starttype(char *starttypeopt)
static void
adjust_data_dir(void)
{
- char cmd[MAXPGPATH],
+ char *cmd,
filename[MAXPGPATH],
*my_exec_path;
FILE *fd;
@@ -2207,10 +2209,10 @@ adjust_data_dir(void)
my_exec_path = pg_strdup(exec_path);
/* it's important for -C to be the first option, see main.c */
- snprintf(cmd, MAXPGPATH, "\"%s\" -C data_directory %s%s",
- my_exec_path,
- pgdata_opt ? pgdata_opt : "",
- post_opts ? post_opts : "");
+ cmd = psprintf("\"%s\" -C data_directory %s%s",
+ my_exec_path,
+ pgdata_opt ? pgdata_opt : "",
+ post_opts ? post_opts : "");
fd = popen(cmd, "r");
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
@@ -2219,6 +2221,7 @@ adjust_data_dir(void)
exit(1);
}
pclose(fd);
+ free(cmd);
free(my_exec_path);
/* strip trailing newline and carriage return */
--
2.15.2 (Apple Git-101.1)
