v12-0003-Address-comments-from-Hayato-Kuroda.patch

From fcd036e0098d74d661b6dbd4f98bf216d51a3565 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Wed, 18 Mar 2026 21:43:08 +0900
Subject: [PATCH v12 3/3] Address comments from Hayato Kuroda

---
 doc/src/sgml/ref/pg_createsubscriber.sgml     | 15 +--
 src/bin/pg_basebackup/pg_createsubscriber.c   | 91 ++++++++-----------
 .../t/040_pg_createsubscriber.pl              |  2 +-
 3 files changed, 46 insertions(+), 62 deletions(-)

diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml
index 2898a5ea111..ff635ba26cb 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
@@ -143,20 +143,21 @@ PostgreSQL documentation
       <para>
        Specify the name of the log directory. A new directory is created with
        this name if it does not exist. A subdirectory with a timestamp
-       indicating the time at which pg_createsubscriber was run will be created.
-       The following two log files will be created in the subdirectory with a
-       umask of 077 so that access is disallowed to other users by default.
+       indicating the time at which <application>pg_createsubscriber</application>
+       was run will be created. The following two log files will be created in
+       the subdirectory with a umask of 077 so that access is disallowed to
+       other users by default.
        <itemizedlist>
         <listitem>
          <para>
-          pg_createsubscriber_server.log which captures logs related to stopping
-          and starting the standby server,
+          <literal>pg_createsubscriber_server.log</literal> which captures logs
+          related to stopping and starting the standby server,
          </para>
         </listitem>
         <listitem>
          <para>
-          pg_createsubscriber_internal.log which captures internal diagnostic
-          output (validations, checks, etc.)
+          <literal>pg_createsubscriber_internal.log</literal> which captures
+          internal diagnostic output (validations, checks, etc.)
          </para>
         </listitem>
        </itemizedlist>
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index bd15b9c1015..5a2bc43ce63 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -189,8 +189,7 @@ static char *pg_ctl_path = NULL;
 static char *pg_resetwal_path = NULL;
 
 static FILE *internal_log_file_fp = NULL;	/* File ptr to log all messages to */
-static char *log_timestamp = NULL;	/* Timestamp to be used in all log file
-									 * names */
+static char logdir[MAXPGPATH];	/* Directory log files are put (if specified) */
 
 /* standby / subscriber data directory */
 static char *subscriber_dir = NULL;
@@ -372,7 +371,7 @@ usage(void)
 			 "                                  databases and databases that don't allow connections\n"));
 	printf(_("  -d, --database=DBNAME           database in which to create a subscription\n"));
 	printf(_("  -D, --pgdata=DATADIR            location for the subscriber data directory\n"));
-	printf(_("  -l, --logdir=LOGDIR             location for the new log directory\n"));
+	printf(_("  -l, --logdir=LOGDIR             location for the log directory\n"));
 	printf(_("  -n, --dry-run                   dry run, just show what would be done\n"));
 	printf(_("  -p, --subscriber-port=PORT      subscriber port number (default %s)\n"), DEFAULT_SUB_PORT);
 	printf(_("  -P, --publisher-server=CONNSTR  publisher connection string\n"));
@@ -846,8 +845,13 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
 		pg_createsub_log(PG_LOG_INFO, PG_LOG_PRIMARY,
 						 "running pg_resetwal on the subscriber");
 
-	if (opt->log_dir != NULL)
-		out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+	/*
+	 * Redirecting the output to the logfile if specified. Since the output
+	 * would be very short, around one line, we do not provide a separate file
+	 * for it; it's done as a part of the server log.
+	 */
+	if (opt->log_dir)
+		out_file = psprintf("%s/%s.log", logdir, SERVER_LOG_FILE_NAME);
 	else
 		out_file = DEVNULL;
 
@@ -1079,13 +1083,13 @@ static void
 internal_log_file_write(enum pg_log_level level, const char *pg_restrict fmt,
 						va_list args)
 {
-	if (level < __pg_log_level)
-		return;
+	Assert(internal_log_file_fp);
 
-	if (internal_log_file_fp == NULL)
+	/* Do nothing if log level is too low. */
+	if (level < __pg_log_level)
 		return;
 
-	vfprintf(internal_log_file_fp, fmt, args);
+	vfprintf(internal_log_file_fp, _(fmt), args);
 
 	fprintf(internal_log_file_fp, "\n");
 	fflush(internal_log_file_fp);
@@ -1122,33 +1126,12 @@ logfile_open(const char *filename, const char *mode)
 }
 
 static void
-make_dir(const char *dir)
-{
-	struct stat statbuf;
-
-	if (stat(dir, &statbuf) == 0)
-		return;
-
-	if (errno != ENOENT)
-		pg_fatal("could not stat directory \"%s\": %m", dir);
-
-	if (mkdir(dir, S_IRWXU) == 0)
-	{
-		pg_log_info("directory %s created", dir);
-		return;
-	}
-
-	pg_fatal("could not create log directory \"%s\": %m", dir);
-}
-
-static void
-make_output_dirs(const char *log_dir)
+make_output_dirs(const char *log_basedir)
 {
 	char		timestamp[128];
 	struct timeval tval;
 	time_t		now;
 	struct tm	tmbuf;
-	char		timestamp_dir[MAXPGPATH];
 	int			len;
 
 	/* Generate timestamp */
@@ -1163,20 +1146,20 @@ make_output_dirs(const char *log_dir)
 			 sizeof(timestamp) - strlen(timestamp), ".%03u",
 			 (unsigned int) (tval.tv_usec / 1000));
 
-	log_timestamp = pg_strdup(timestamp);
-
-	/* Create base directory (ignore if exists) */
-	make_dir(log_dir);
-
 	/* Build timestamp directory path */
-	len = snprintf(timestamp_dir, MAXPGPATH, "%s/%s", log_dir, timestamp);
+	len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
 
 	if (len >= MAXPGPATH)
 		pg_fatal("directory path for log files, %s/%s, is too long",
-				 log_dir, timestamp);
+				 logdir, timestamp);
+
+	/* Create base directory (ignore if exists) */
+	if (mkdir(log_basedir, S_IRWXU) < 0 && errno != EEXIST)
+		pg_fatal("could not create directory \"%s\": %m", log_basedir);
 
-	/* Create timestamp directory */
-	make_dir(timestamp_dir);
+	/* Create BASE_DIR/$timestamp */
+	if (mkdir(logdir, S_IRWXU) < 0)
+		pg_fatal("could not create directory \"%s\": %m", logdir);
 }
 
 /*
@@ -1937,8 +1920,8 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
 	if (restrict_logical_worker)
 		appendPQExpBufferStr(pg_ctl_cmd, " -o \"-c max_logical_replication_workers=0\"");
 
-	if (opt->log_dir != NULL)
-		appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+	if (opt->log_dir)
+		appendPQExpBuffer(pg_ctl_cmd, " -l \"%s/%s.log\"", logdir, SERVER_LOG_FILE_NAME);
 
 	pg_createsub_log(PG_LOG_DEBUG, PG_LOG_PRIMARY,
 					 "pg_ctl command is: %s", pg_ctl_cmd->data);
@@ -2695,18 +2678,6 @@ main(int argc, char **argv)
 		}
 	}
 
-	if (opt.log_dir != NULL)
-	{
-		char	   *internal_log_file;
-
-		make_output_dirs(opt.log_dir);
-		internal_log_file = psprintf("%s/%s/%s.log", opt.log_dir, log_timestamp,
-									 INTERNAL_LOG_FILE_NAME);
-
-		if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL)
-			pg_fatal("could not open log file \"%s\": %m", internal_log_file);
-	}
-
 	/* Validate that --all is not used with incompatible options */
 	if (opt.all_dbs)
 	{
@@ -2784,6 +2755,18 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (opt.log_dir != NULL)
+	{
+		char	   *internal_log_file;
+
+		make_output_dirs(opt.log_dir);
+		internal_log_file = psprintf("%s/%s.log", logdir,
+									 INTERNAL_LOG_FILE_NAME);
+
+		if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL)
+			pg_fatal("could not open log file \"%s\": %m", internal_log_file);
+	}
+
 	if (dry_run)
 		pg_createsub_log(PG_LOG_INFO, PG_LOG_PRIMARY,
 						 "Executing in dry-run mode.\n"
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 4ddfb621a5d..d3af1561551 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -14,7 +14,7 @@ program_version_ok('pg_createsubscriber');
 program_options_handling_ok('pg_createsubscriber');
 
 my $datadir = PostgreSQL::Test::Utils::tempdir;
-my $logdir = PostgreSQL::Test::Utils::tempdir + "/logdir";
+my $logdir = PostgreSQL::Test::Utils::tempdir;
 
 # Generate a database with a name made of a range of ASCII characters.
 # Extracted from 002_pg_upgrade.pl.
-- 
2.47.3