From 48ef73fcc4b3e32486e6df21a2038f7edf2e44a7 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda Date: Wed, 18 Mar 2026 21:43:08 +0900 Subject: [PATCH v13 3/3] Address comments from Hayato Kuroda --- doc/src/sgml/ref/pg_createsubscriber.sgml | 15 +-- src/bin/pg_basebackup/pg_createsubscriber.c | 95 ++++++++----------- .../t/040_pg_createsubscriber.pl | 2 +- 3 files changed, 48 insertions(+), 64 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 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 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. - pg_createsubscriber_server.log which captures logs related to stopping - and starting the standby server, + pg_createsubscriber_server.log which captures logs + related to stopping and starting the standby server, - pg_createsubscriber_internal.log which captures internal diagnostic - output (validations, checks, etc.) + pg_createsubscriber_internal.log which captures + internal diagnostic output (validations, checks, etc.) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index bb537938a7e..690623201f4 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,20 +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); - - pg_free(internal_log_file); - } - /* Validate that --all is not used with incompatible options */ if (opt.all_dbs) { @@ -2786,6 +2755,20 @@ 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); + + pg_free(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.43.0