On Mon, Mar 23, 2026 at 2:24 AM Chao Li <[email protected]> wrote: + /* Build timestamp directory path */ + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); + + if (len >= MAXPGPATH) + pg_fatal("directory path for log files, %s/%s, is too long", + logdir, timestamp); ``` > In the pg_fatal call, I believe logdir should be log_basedir. We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir.
> The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: ``` > errno = save_errno; > va_copy(ap2, ap); > required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; > va_end(ap2); ``` > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you! >The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? I could add a switch-case to print it out. Is that important? What do you think? I have fixed the rest of your suggestions. Thank you, Chao Li! On Mon, Mar 23, 2026 at 5:55 AM shveta malik <[email protected]> wrote: > We can get rid of below alignment related changes in unrelated test parts. They are added when I run pgperltidy. Anyone else trying to change the file after this would see the same thing if we don't change it. Should I move it into another patch? I have fixed the rest of it. Thank you, Shveta Malik! On Mon, Mar 23, 2026 at 5:55 AM Kuroda, Hayato/黒田 隼人 < [email protected]> wrote: > 01. > Found that pg_log_generic_v() has some prefix but > internal_log_file_write() does not. > It means output strings are not the same. For example, on terminal: > > ``` > pg_createsubscriber: error: standby server is running > pg_createsubscriber: hint: Stop the standby server and try again. > ``` > > But on log file: > ``` > standby server is running > Stop the standby server and try again. > ``` > > It's because pg_log_generic_v() has the format like below. I.e., the > program name > is printed at the begining, and some prefix also exists in some cases. > > ${program name}: {error: |warning: |detail: |hint: } content > > I cannot find such a difference on pg_upgrade: no prefix exists in any > cases. > So, what should be here? My preference is to basically follow > pg_log_generic_v() > But remove the program name. How about others? > I haven't changed the output of pg_log_generic_v() yet. Shall I add the prefix to the output? I have done the rest of your suggestions. Thank you! Regards, Gyan Sreejith
From b83f96eb26f26162e06542c4f05226cca6b5613e Mon Sep 17 00:00:00 2001 From: Gyan Sreejith <[email protected]> Date: Mon, 23 Mar 2026 21:21:56 -0400 Subject: [PATCH v17] Add a new argument -l <logdir> to pg_createsubscriber. Enabling the option to write messages to log files in the specified directory. A new directory is created if required. A subdirectory is created with timestamp as its name, and it will contain two new logfiles: 1. pg_createsubscriber_server.log - captures messages related to starting and stopping the standby server. 2. pg_createsubscriber_internal.log - captures internal diagnostic output from pg_createsubscriber. For example, if we specify -l abc as an argument, and if the timestamp on running it is 20260119T204317.204, a directory abc is created if it doesn't exist already, with 20260119T204317.204 as its subdirectory and it will contain the two log files pg_createsubscriber_server.log and pg_createsubscriber_internal.log --- doc/src/sgml/ref/pg_createsubscriber.sgml | 29 +++ src/bin/pg_basebackup/pg_createsubscriber.c | 174 +++++++++++++++++- .../t/040_pg_createsubscriber.pl | 48 ++++- 3 files changed, 239 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml index 6e17cee18eb..5ac61c7b558 100644 --- a/doc/src/sgml/ref/pg_createsubscriber.sgml +++ b/doc/src/sgml/ref/pg_createsubscriber.sgml @@ -136,6 +136,35 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>-l <replaceable class="parameter">directory</replaceable></option></term> + <term><option>--logdir=<replaceable class="parameter">directory</replaceable></option></term> + <listitem> + <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 <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> + <filename>pg_createsubscriber_server.log</filename> which captures logs + related to stopping and starting the standby server, + </para> + </listitem> + <listitem> + <para> + <filename>pg_createsubscriber_internal.log</filename> which captures + internal diagnostic output (validations, checks, etc.) + </para> + </listitem> + </itemizedlist> + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>-n</option></term> <term><option>--dry-run</option></term> diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index b2bc9dae0b8..63ffd337613 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -20,6 +20,7 @@ #include "common/connect.h" #include "common/controldata_utils.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "common/logging.h" #include "common/pg_prng.h" @@ -49,10 +50,14 @@ #define INCLUDED_CONF_FILE "pg_createsubscriber.conf" #define INCLUDED_CONF_FILE_DISABLED INCLUDED_CONF_FILE ".disabled" +#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server" +#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal" + /* Command-line options */ struct CreateSubscriberOptions { char *config_file; /* configuration file */ + char *log_dir; /* log directory name */ char *pub_conninfo_str; /* publisher connection string */ char *socket_dir; /* directory for Unix-domain socket, if any */ char *sub_port; /* subscriber port number */ @@ -149,8 +154,14 @@ static void get_publisher_databases(struct CreateSubscriberOptions *opt, static void report_createsub_log(enum pg_log_level, enum pg_log_part, const char *pg_restrict fmt,...) pg_attribute_printf(3, 4); +static void report_createsub_log_v(enum pg_log_level level, enum pg_log_part part, + const char *pg_restrict fmt, va_list args) + pg_attribute_printf(3, 0); pg_noreturn static void report_createsub_fatal(const char *pg_restrict fmt,...) pg_attribute_printf(1, 2); +static void internal_log_file_write(enum pg_log_level level, + const char *pg_restrict fmt, va_list args) + pg_attribute_printf(2, 0); #define WAIT_INTERVAL 1 /* 1 second */ @@ -172,6 +183,9 @@ static pg_prng_state prng_state; 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 logdir[MAXPGPATH]; /* Directory log files are put (if specified) */ + /* standby / subscriber data directory */ static char *subscriber_dir = NULL; @@ -180,8 +194,29 @@ static bool standby_running = false; static bool recovery_params_set = false; /* - * Report a message with a given log level + * Report a message with a given log level to stderr and log file + * (if specified). */ +static void +report_createsub_log_v(enum pg_log_level level, enum pg_log_part part, + const char *pg_restrict fmt, va_list args) +{ + int save_errno = errno; + + if (internal_log_file_fp != NULL) + { + /* Output to both stderr and the log file */ + va_list arg_cpy; + + va_copy(arg_cpy, args); + internal_log_file_write(level, fmt, arg_cpy); + va_end(arg_cpy); + /* Restore errno in case internal_log_file_write changed it */ + errno = save_errno; + } + pg_log_generic_v(level, part, fmt, args); +} + static void report_createsub_log(enum pg_log_level level, enum pg_log_part part, const char *pg_restrict fmt,...) @@ -190,7 +225,7 @@ report_createsub_log(enum pg_log_level level, enum pg_log_part part, va_start(args, fmt); - pg_log_generic_v(level, part, fmt, args); + report_createsub_log_v(level, part, fmt, args); va_end(args); } @@ -205,7 +240,7 @@ report_createsub_fatal(const char *pg_restrict fmt,...) va_start(args, fmt); - pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args); + report_createsub_log_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args); va_end(args); @@ -313,6 +348,13 @@ cleanup_objects_atexit(void) if (standby_running) stop_standby_server(subscriber_dir); + + if (internal_log_file_fp != NULL) + { + if (fclose(internal_log_file_fp) != 0) + report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); + internal_log_file_fp = NULL; + } } static void @@ -327,6 +369,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 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")); @@ -761,6 +804,7 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt) bool crc_ok; struct timeval tv; + char *out_file; char *cmd_str; report_createsub_log(PG_LOG_INFO, PG_LOG_PRIMARY, @@ -799,8 +843,20 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt) report_createsub_log(PG_LOG_INFO, PG_LOG_PRIMARY, "running pg_resetwal on the subscriber"); - cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path, - subscriber_dir, DEVNULL); + /* + * 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; + + cmd_str = psprintf("\"%s\" -D \"%s\" >> \"%s\"", pg_resetwal_path, + subscriber_dir, out_file); + if (opt->log_dir) + pg_free(out_file); report_createsub_log(PG_LOG_DEBUG, PG_LOG_PRIMARY, "pg_resetwal command is: %s", cmd_str); @@ -817,6 +873,7 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt) } pg_free(cf); + pg_free(cmd_str); } /* @@ -1023,6 +1080,89 @@ server_is_in_recovery(PGconn *conn) return ret == 0; } +static void +internal_log_file_write(enum pg_log_level level, const char *pg_restrict fmt, + va_list args) +{ + Assert(internal_log_file_fp); + + /* Do nothing if log level is too low. */ + if (level < __pg_log_level) + return; + + vfprintf(internal_log_file_fp, _(fmt), args); + + fprintf(internal_log_file_fp, "\n"); + fflush(internal_log_file_fp); +} + +/* + * Open a new logfile with proper permissions. + * From src/backend/postmaster/syslogger.c + */ +static FILE * +logfile_open(const char *filename, const char *mode) +{ + FILE *fh; + mode_t oumask; + + oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG | S_IRWXO))); + fh = fopen(filename, mode); + umask(oumask); + + if (fh) + { + setvbuf(fh, NULL, PG_IOLBF, 0); + +#ifdef WIN32 + /* use CRLF line endings on Windows */ + _setmode(_fileno(fh), _O_TEXT); +#endif + } + else + report_createsub_fatal("could not open log file \"%s\": %m", + filename); + + return fh; +} + +static void +make_output_dirs(const char *log_basedir) +{ + char timestamp[128]; + struct timeval tval; + time_t now; + struct tm tmbuf; + int len; + + /* Generate timestamp */ + gettimeofday(&tval, NULL); + now = tval.tv_sec; + + strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%S", + localtime_r(&now, &tmbuf)); + + /* Append milliseconds */ + snprintf(timestamp + strlen(timestamp), + sizeof(timestamp) - strlen(timestamp), ".%03u", + (unsigned int) (tval.tv_usec / 1000)); + + /* Build timestamp directory path */ + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); + + if (len >= MAXPGPATH) + report_createsub_fatal("directory path for log files, %s/%s, is too long", + logdir, timestamp); + + /* Create base directory (ignore if exists) */ + if (mkdir(log_basedir, pg_dir_create_mode) < 0 && errno != EEXIST) + report_createsub_fatal("could not create directory \"%s\": %m", log_basedir); + + /* Create a timestamp-named subdirectory under the base directory */ + if (mkdir(logdir, pg_dir_create_mode) < 0) + report_createsub_fatal("could not create directory \"%s\": %m", logdir); +} + /* * Is the primary server ready for logical replication? * @@ -1781,6 +1921,9 @@ 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) + appendPQExpBuffer(pg_ctl_cmd, " -l \"%s/%s.log\"", logdir, SERVER_LOG_FILE_NAME); + report_createsub_log(PG_LOG_DEBUG, PG_LOG_PRIMARY, "pg_ctl command is: %s", pg_ctl_cmd->data); rc = system(pg_ctl_cmd->data); @@ -2351,6 +2494,7 @@ main(int argc, char **argv) {"all", no_argument, NULL, 'a'}, {"database", required_argument, NULL, 'd'}, {"pgdata", required_argument, NULL, 'D'}, + {"logdir", required_argument, NULL, 'l'}, {"dry-run", no_argument, NULL, 'n'}, {"subscriber-port", required_argument, NULL, 'p'}, {"publisher-server", required_argument, NULL, 'P'}, @@ -2409,6 +2553,7 @@ main(int argc, char **argv) /* Default settings */ subscriber_dir = NULL; opt.config_file = NULL; + opt.log_dir = NULL; opt.pub_conninfo_str = NULL; opt.socket_dir = NULL; opt.sub_port = DEFAULT_SUB_PORT; @@ -2439,7 +2584,7 @@ main(int argc, char **argv) get_restricted_token(); - while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:TU:v", + while ((c = getopt_long(argc, argv, "ad:D:l:np:P:s:t:TU:v", long_options, &option_index)) != -1) { switch (c) @@ -2460,6 +2605,10 @@ main(int argc, char **argv) subscriber_dir = pg_strdup(optarg); canonicalize_path(subscriber_dir); break; + case 'l': + opt.log_dir = pg_strdup(optarg); + canonicalize_path(opt.log_dir); + break; case 'n': dry_run = true; break; @@ -2607,6 +2756,19 @@ 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); + + /* logfile_open() will exit if there is an error */ + internal_log_file_fp = logfile_open(internal_log_file, "a"); + pg_free(internal_log_file); + } + if (dry_run) report_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 0c27fca7bb7..239ea58d9a0 100644 --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl @@ -5,6 +5,8 @@ use strict; use warnings FATAL => 'all'; +use File::Basename; +use File::stat; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; @@ -14,6 +16,7 @@ program_version_ok('pg_createsubscriber'); program_options_handling_ok('pg_createsubscriber'); my $datadir = PostgreSQL::Test::Utils::tempdir; +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. @@ -362,9 +365,40 @@ command_ok( '--subscription' => 'sub2', '--database' => $db1, '--database' => $db2, + '--logdir' => $logdir, ], 'run pg_createsubscriber --dry-run on node S'); +# Check that the log files were created +my @server_log_files = glob "$logdir/*/pg_createsubscriber_server.log"; +is(scalar(@server_log_files), + 1, "pg_createsubscriber_server.log file was created"); +my $server_log_file_size = -s $server_log_files[0]; +isnt($server_log_file_size, 0, + "pg_createsubscriber_server.log file not empty"); +my $server_log = slurp_file($server_log_files[0]); +like( + $server_log, + qr/consistent recovery state reached/, + "server reached consistent recovery state"); + +my @internal_log_files = glob "$logdir/*/pg_createsubscriber_internal.log"; +is(scalar(@internal_log_files), + 1, "pg_createsubscriber_internal.log file was created"); +my $internal_log_file_size = -s $internal_log_files[0]; +isnt($internal_log_file_size, 0, + "pg_createsubscriber_internal.log file not empty"); +my $internal_log = slurp_file($internal_log_files[0]); +like( + $internal_log, + qr/target server reached the consistent state/, + "log shows consistent state reached"); +my $timestamp_dir = dirname($internal_log_files[0]); +my $timestamp_dir_stat = stat($timestamp_dir); +my $timestamp_dir_mode = $timestamp_dir_stat->mode & 07777; +is($timestamp_dir_mode, 0700, + "Directory with .log files has permissions S_IRWXU"); + # Check if node S is still a standby $node_s->start; is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'), @@ -444,7 +478,8 @@ is(scalar(() = $stderr =~ /would create subscription/g), # Create a user-defined publication, and a table that is not a member of that # publication. -$node_p->safe_psql($db1, qq( +$node_p->safe_psql( + $db1, qq( CREATE PUBLICATION test_pub3 FOR TABLE tbl1; CREATE TABLE not_replicated (a int); )); @@ -540,8 +575,7 @@ second row third row), "logical replication works in database $db1"); $result = $node_s->safe_psql($db1, 'SELECT * FROM not_replicated'); -is($result, qq(), - "table is not replicated in database $db1"); +is($result, qq(), "table is not replicated in database $db1"); # Check result in database $db2 $result = $node_s->safe_psql($db2, 'SELECT * FROM tbl2'); @@ -555,8 +589,10 @@ my $sysid_s = $node_s->safe_psql('postgres', isnt($sysid_p, $sysid_s, 'system identifier was changed'); # Verify that pub2 was created in $db2 -is($node_p->safe_psql($db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'pub2'"), - '1', "publication pub2 was created in $db2"); +is( $node_p->safe_psql( + $db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'pub2'"), + '1', + "publication pub2 was created in $db2"); # Get subscription and publication names $result = $node_s->safe_psql( @@ -581,7 +617,7 @@ $result = $node_s->safe_psql( ) ); -is($result, qq($db1|{test_pub3} +is( $result, qq($db1|{test_pub3} $db2|{pub2}), "subscriptions use the correct publications"); -- 2.43.0
