[Phil Krylov <phil@krylov.eu>]

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.

Attachments:

0001-pg_ctl-should-not-truncate-command-lines-at-1024-cha.patch (text/x-diff, 7.0K) download
From 6f2e70025208fa4d0034ddbe5254e2cb9759dd24 Mon Sep 17 00:00:00 2001
From: Phil Krylov <phil@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)