Hello,
Lacking a tool to edit postgresql.conf programmatically, people resort
to passing cluster options on the command line. While passing all
non-default options in this way may sound like an abuse of the feature,
IMHO pg_ctl should not blindly truncate generated command lines at
MAXPGPATH (1024 characters) and then run that, resulting in:
/bin/sh: Syntax error: end of file unexpected (expecting word)
pg_ctl: could not start server
Examine the log output.
The attached patch tries to fix it in the least intrusive way.
While we're at it, is it supposed that pg_ctl is a very short-lived
process and is therefore allowed to leak memory? I've noticed some
places where I would like to add a free() call.
-- Ph.
From 6f2e70025208fa4d0034ddbe5254e2cb9759dd24 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 | 47 ++++++++++++++++++++++----------------
src/bin/pg_ctl/t/001_start_stop.pl | 41 ++++++++++++++++++++++++++++++++-
2 files changed, 67 insertions(+), 21 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..199d13c1fb 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,14 +487,15 @@ 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);
+ free(cmd);
/* exec failed */
write_stderr(_("%s: could not start server: %s\n"),
@@ -553,19 +554,21 @@ 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))
{
write_stderr(_("%s: could not start server: error code %lu\n"),
progname, (unsigned long) GetLastError());
+ free(cmd);
exit(1);
}
+ free(cmd);
/* Don't close command process handle here; caller must do so */
postmasterProcess = pi.hProcess;
CloseHandle(pi.hThread);
@@ -828,7 +831,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 +843,19 @@ 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);
+ free(cmd);
exit(1);
}
+ free(cmd);
}
static void
@@ -2175,7 +2180,7 @@ set_starttype(char *starttypeopt)
static void
adjust_data_dir(void)
{
- char cmd[MAXPGPATH],
+ char *cmd,
filename[MAXPGPATH],
*my_exec_path;
FILE *fd;
@@ -2207,18 +2212,20 @@ 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)
{
write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd);
+ free(cmd);
exit(1);
}
pclose(fd);
+ free(cmd);
free(my_exec_path);
/* strip trailing newline and carriage return */
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index 1d8d6bbb70..da05cc8f8b 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -9,7 +9,7 @@ use Fcntl ':mode';
use File::stat qw{lstat};
use PostgresNode;
use TestLib;
-use Test::More tests => 24;
+use Test::More tests => 26;
my $tempdir = TestLib::tempdir;
my $tempdir_short = TestLib::tempdir_short;
@@ -68,6 +68,45 @@ command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop');
command_fails([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ],
'second pg_ctl stop fails');
+command_ok([ 'pg_ctl', 'start', '-D', "$tempdir/data",
+ '-w', '-s', '-m', 'fast',
+ '-o', '-c', '-o', 'update_process_title=on',
+ '-o', '-c', '-o', 'effective_io_concurrency=0',
+ '-o', '-c', '-o', 'synchronous_commit=off',
+ '-o', '-c', '-o', 'full_page_writes=off',
+ '-o', '-c', '-o', 'wal_init_zero=off',
+ '-o', '-c', '-o', 'wal_recycle=off',
+ '-o', '-c', '-o', 'effective_cache_size=3200MB',
+ '-o', '-c', '-o', 'checkpoint_completion_target=0.9',
+ '-o', '-c', '-o', 'random_page_cost=1.1',
+ '-o', '-c', '-o', 'parallel_setup_cost=200.0',
+ '-o', '-c', '-o', 'log_min_duration_statement=100',
+ '-o', '-c', '-o', 'log_checkpoints=on',
+ '-o', '-c', '-o', 'log_temp_files=0',
+ '-o', '-c', '-o', 'temp_buffers=32MB',
+ '-o', '-c', '-o', 'work_mem=50MB',
+ '-o', '-c', '-o', 'maintenance_work_mem=50MB',
+ '-o', '-c', '-o', 'max_wal_size=500MB',
+ '-o', '-c', '-o', 'default_statistics_target=10000',
+ '-o', '-c', '-o', 'autovacuum_max_workers=8',
+ '-o', '-c', '-o', 'autovacuum_naptime=15min',
+ '-o', '-c', '-o', 'autovacuum_vacuum_threshold=1000',
+ '-o', '-c', '-o', 'autovacuum_analyze_threshold=1000',
+ '-o', '-c', '-o', 'autovacuum_freeze_max_age=1000000000',
+ '-o', '-c', '-o', 'vacuum_freeze_min_age=10000000',
+ '-o', '-c', '-o', 'vacuum_freeze_table_age=800000000',
+ '-o', '-c', '-o', 'jit_above_cost=-1',
+ '-o', '-c', '-o', 'jit_optimize_above_cost=10000000',
+ '-o', '-c', '-o', 'max_locks_per_transaction=128',
+ '-o', '-c', '-o', 'default_text_search_config=pg_catalog.english',
+ '-o', '-c', '-o', 'timezone=Europe/London',
+ '-o', '-c', '-o', 'shared_buffers=400MB',
+ '-o', '-c', '-o', 'port=5433',
+ '-o', '-c', '-o', 'cluster_name=13/mytestcluster',
+ '-o', '-c', '-o', 'max_connections=200' ],
+ 'pg_ctl start with lots of parameters');
+command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop');
+
# Log file for default permission test. The permissions won't be checked on
# Windows but we still want to do the restart test.
my $logFileName = "$tempdir/data/perm-test-600.log";
--
2.15.2 (Apple Git-101.1)
