Обсуждение: [Proposal] Adding Log File Capability to pg_createsubscriber

On Wed, Dec 10, 2025 at 9:17 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Background:
>
> pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs,
recoveryprogress output, and output from utilities) directly to the console. As a result, users may find debugging and
handlingerrors difficult. It would be more convenient if messages were separated and stored in different log files.
Thereis already a similar implementation in pg_upgrade. 
>
> Proposed Solution:
>
> Based on issues mentioned previously, I would like to propose a new argument -l <logdir> which can be specified for
pg_createsubscriber.Using it would create the following log files: 
>
> logdir/pg_createsubscriber_server.log which captures all logs related to starting and stopping the standby server.
>
> logdir/pg_createsubscriber_resetwal.log which captures the output of pg_resetwal
>
> logdir/pg_createsubscriber_internal.log which captures internal diagnostic output from pg_createsubscriber
(validations,checks, etc.) 
>
> Overall, this proposed solution could make the pg_createsubscriber command output messages more organized. The
commandwould be easier to use as users will only have to read individual log files rather than parse through lots of
possiblyirrelevant output messages. I have attached the patch for this change. 
>
> Special thanks to Vignesh C. for his offlist guidance on this project.
>
>
> Regards, Gyan Sreejith
>

Hi Gyan.

I haven't yet looked at this patch in any detail, but here are some
quick comments:

======

1.
+ printf(_("  -l, --logdir=LOGDIR             location for the new log
directory\n"));

The patch is missing SGML docs updates for pg_createsubscriber new
option, and any explanation of the split of logfiles.

2.
I might be mistaken, but IIUC it seems the splitting of the logfile
only works when --logdir is specified. Is that correct?
Why should --logdir have any side-effect other than assigning the log
destination folder?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Gyan,

+1 for the idea. This point has already been discussed since the initial commit
[1], but it has left till now. I'm happy if you can take initiative.
Of course I can review your patches.

Per my understanding, pg_upgrade puts logfiles at the directory, under
"${PGDATANEW}/pg_upgrade_output.d/${timestamp}". See Note part in [2].
I feel more straightforward way is to follow that approach:

1. pg_createsubscriber creates a directory pg_createsubscriber_output.d/${timestamp}.
   ${timestamp} has the same format as ISO 8601 (%Y%m%dT%H%M%S).
2. pg_craetesubscriber saves outputs under the directory.
3. Outputs can be retained when the command failed or --retain is specified.
   Otherwise, they are removed at the end.

Are there benefits to provide -l option?

Regarding the patch format, our community prefers patches generated by
git format-patch. Can you see the blogpost [3] and try to create patches based on the command?
One benefit is we can easily do versioning.

[1]: https://www.postgresql.org/message-id/60b45b8a-3047-4a21-ba2a-ddb15daa638f%40eisentraut.org
[2]: https://www.postgresql.org/docs/devel/pgupgrade.html
[3]: https://peter.eisentraut.org/blog/2023/05/09/how-to-submit-a-patch-by-email-2023-edition

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Dec 12, 2025 at 12:33 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thanks for the feedback, Peter.
>
> I am currently working on the SGML docs update, and will promptly get back with an update.
>
> For your second point, currently, all output goes directly to the console. I thought it made more sense to break it
upinto multiple files depending on what was being invoked. Do you have another opinion? 
>

The point I was trying to make was that if you are going to have a
"--logdir" option, then IMO that option should do nothing other than
change the destination for the logs. In other words, current behaviour
is effectively --logdir=console, so --logdir=some_other_folder should
not have some side-effect causing the log to get split into multiple
files.

If you want to split logs, then I thought Kuroda-San's suggestion [1]
sounded better ---  (a) mimic pg_upgrade more closely and (b)
reconsider if -logdir is needed at all.

======
[1]
https://www.postgresql.org/message-id/OSCPR01MB14966FD0961F512B29BD46D6BF5AAA%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, 16 Dec 2025 at 12:31, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Gyan,
>
> +1 for the idea. This point has already been discussed since the initial commit
> [1], but it has left till now. I'm happy if you can take initiative.
> Of course I can review your patches.
>
> Per my understanding, pg_upgrade puts logfiles at the directory, under
> "${PGDATANEW}/pg_upgrade_output.d/${timestamp}". See Note part in [2].
> I feel more straightforward way is to follow that approach:
>
> 1. pg_createsubscriber creates a directory pg_createsubscriber_output.d/${timestamp}.
>    ${timestamp} has the same format as ISO 8601 (%Y%m%dT%H%M%S).
> 2. pg_craetesubscriber saves outputs under the directory.
> 3. Outputs can be retained when the command failed or --retain is specified.
>    Otherwise, they are removed at the end.

If I recall correctly, this was implemented that way earlier, but the
approach was abandoned around [1]. The primary reason was that when
users take a backup of the data directory, they would need to
explicitly manage the exclusion of this data, which was considered
undesirable.

> Are there benefits to provide -l option?

By providing this as an option, users can store the log files outside
the data directory, eliminating the need for any additional handling
during backups.

[1] - https://www.postgresql.org/message-id/d546c4bb-92d1-4e2d-898f-48234b12ed25%40app.fastmail.com

Regards,
Vignesh

On Wed, Dec 17, 2025, at 7:07 AM, vignesh C wrote:
>
> By providing this as an option, users can store the log files outside
> the data directory, eliminating the need for any additional handling
> during backups.
>

Do we really need an option to capture the stdout / stderr output to a file? I
doubt it. There is already various ways to capture. psql and pg_upgrade are the
only tools that have this option. And honestly, I've never seen these options
used in the field.


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/

On Thu, Dec 18, 2025 at 6:59 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Dec 17, 2025, at 7:07 AM, vignesh C wrote:
> >
> > By providing this as an option, users can store the log files outside
> > the data directory, eliminating the need for any additional handling
> > during backups.
> >
>
> Do we really need an option to capture the stdout / stderr output to a file? I
> doubt it. There is already various ways to capture. psql and pg_upgrade are the
> only tools that have this option.
>

pg_ctl also has the -l option. I think any place where long
text/errors can be outputted, a log file is preferred because one
could later parse it to know the exact details. Also, splitting the
log as proposed here or in pg_upgrade helps to navigate the LOG like
is the problem in start/stop of the server or a pub-sub setup?
Similarly the log can be splitted for pub/sub specific information.
There appears to be some useful information like:

pg_createsubscriber: warning: two_phase option will not be enabled for
replication slots
pg_createsubscriber: detail: Subscriptions will be created with the
two_phase option disabled. Prepared transactions will be replicated at
COMMIT PREPARED.
pg_createsubscriber: hint: You can use the command-line option
--enable-two-phase to enable two_phase.

I think it will be useful to LOG this separately from the main LOG [1]
(which can contain server specific info as follows) so that users can
consider running pg_createsubscriber with additional options or
changing the subscriber configuration once setup is complete.

[1]:
[startup] LOG:  database system was interrupted; last known up at
2025-12-17 14:46:07 IST
[startup] LOG:  starting backup recovery with redo LSN 0/06000028,
checkpoint LSN 0/06000080, on timeline ID 1
[startup] LOG:  entering standby mode
[startup] LOG:  redo starts at 0/06000028
[startup] LOG:  completed backup recovery with redo LSN 0/06000028 and
end LSN 0/06000120
[startup] LOG:  consistent recovery state reached at 0/06000120

--
With Regards,
Amit Kapila.

On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging
shouldbe implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have
attachedthe git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to
implementwhatever solution when we reach a consensus. 

Few comments:
1) The file permissions are 664 for pg_createsubscriber_internal.log,
pg_createsubscriber_resetwal.log but 600 for
pg_createsubscriber_server.log. The permissions should be the same for
all the files.
...
if (opt->log_dir != NULL)
out_file = psprintf("%s/pg_createsubscriber_resetwal.log", opt->log_dir);
else
out_file = DEVNULL;

cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path,
   subscriber_dir, out_file);

pg_log_debug("pg_resetwal command is: %s", cmd_str);
...

...
if (opt->log_dir != NULL)
{
appendPQExpBuffer(pg_ctl_cmd, " -l %s/pg_createsubscriber_server.log",
opt->log_dir);
}

pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
rc = system(pg_ctl_cmd->data);
...
2)  Can you gracefully handle the case where permissions are not
enough in the directory and throw proper error:
if (stat(opt.log_dir, &statbuf) != 0)
{
if (errno == ENOENT)
{
mkdir(opt.log_dir, S_IRWXU);
pg_log_info("log directory created");
}
else
pg_fatal("could not access directory \"%s\": %m", opt.log_dir);
}

3) Currently there is no timestamp included for
pg_createsubscriber_internal and pg_createsubscriber_resetwal log file
contents. Without that it is difficult to tell when the operations
were done. It will be good to include them.

4) The patch does not apply on the head, kindly rebase on top of head.

5) Do you need to open and close the log file each time?
...
if (internal_log_file != NULL)
{
if ((fp = fopen(internal_log_file, "a")) == NULL)
pg_fatal("could not write to log file \"%s\": %m", internal_log_file);

fprintf(fp, "checking settings on subscriber\n");
fclose(fp);
}
else
pg_log_info("checking settings on subscriber");
...

...
if (internal_log_file != NULL)
{
if ((fp = fopen(internal_log_file, "a")) == NULL)
pg_fatal("could not write to log file \"%s\": %m", internal_log_file);

fprintf(fp, "checking settings on publisher\n");
fclose(fp);
}
else
pg_log_info("checking settings on publisher");
...

Regards,
Vignesh

On Wed, Dec 10, 2025 at 3:47 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs,
recoveryprogress output, and output from utilities) directly to the console. As a result, users may find debugging and
handlingerrors difficult. It would be more convenient if messages were separated and stored in different log files.
Thereis already a similar implementation in pg_upgrade. 
>
> Proposed Solution:
>
> Based on issues mentioned previously, I would like to propose a new argument -l <logdir> which can be specified for
pg_createsubscriber.Using it would create the following log files: 
>
> logdir/pg_createsubscriber_server.log which captures all logs related to starting and stopping the standby server.
>
> logdir/pg_createsubscriber_resetwal.log which captures the output of pg_resetwal
>
> logdir/pg_createsubscriber_internal.log which captures internal diagnostic output from pg_createsubscriber
(validations,checks, etc.) 
>

I think we don't need pg_createsubscriber_resetwal.log separately as
it just prints "Write-ahead log reset", it could as well be part of
pg_createsubscriber_server.log. Also, I noticed that even when -l
option is used, the additional LOG in verbose mode still goes to
console which is difficult to parse. I fell that should also be part
of .log files created by this patch.

Additionally, I think it is better to use a folder with time to store
as LOG files as used in pg_upgrade. This helps to distinguish log
files for different runs (say in case of failures).

I also thought to further split pg_createsubscriber_internal.log into
publisher and subscriber specific LOGs but not sure if that is worth,
so we can leave that for now.

--
With Regards,
Amit Kapila.

On Mon, 29 Dec 2025 at 16:40, vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
> >
> > Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging
shouldbe implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have
attachedthe git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to
implementwhatever solution when we reach a consensus. 
>
> Few comments:
> 1) The file permissions are 664 for pg_createsubscriber_internal.log,
> pg_createsubscriber_resetwal.log but 600 for
> pg_createsubscriber_server.log. The permissions should be the same for
> all the files.
> ...
> if (opt->log_dir != NULL)
> out_file = psprintf("%s/pg_createsubscriber_resetwal.log", opt->log_dir);
> else
> out_file = DEVNULL;
>
> cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path,
>    subscriber_dir, out_file);
>
> pg_log_debug("pg_resetwal command is: %s", cmd_str);
> ...
>
> ...
> if (opt->log_dir != NULL)
> {
> appendPQExpBuffer(pg_ctl_cmd, " -l %s/pg_createsubscriber_server.log",
> opt->log_dir);
> }
>
> pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
> rc = system(pg_ctl_cmd->data);
> ...

For this, you can align the file handling and umask behavior with the
logic used in syslogger.c (logfile_open). Doing so will ensure that
the resulting log files are created with consistent permissions across
all cases. Additionally you can include "The umask is set to 077, so
access to the log file is disallowed to other users by default" in the
documentation of the pg_createsubscriber log option similar to how it
is mentioned in [1].
[1] - https://www.postgresql.org/docs/current/app-pg-ctl.html

Regards,
Vignesh

On Tue, 20 Jan 2026 at 06:28, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you for all your input. I have attached the latest version of the patch that includes the changes proposed by
Vigneshand Amit. Please let me know if you have any questions or suggestions.
 

1)  Currently you are creating directories like
specifiedlogdir_timestamp for each run, so it generates log
directories like:
logdir_2026-01-20-18-15-55.267510
logdir_2026-01-20-18-16-49.468882

Instead can you change it to specifiedlogdir/exec_timestamp1.
specifiedlogdir/exec_timestamp2, etc

+                               populate_timestamp(timestamp,
sizeof(timestamp));
+                               log_dir = psprintf("%s_%s", optarg, timestamp);
+                               opt.log_dir = pg_strdup(log_dir);
+                               canonicalize_path(opt.log_dir);
+
+                               if (stat(opt.log_dir, &statbuf) != 0)
+                               {
+                                       if (errno == ENOENT)
+                                       {
+                                               mkdir(opt.log_dir, S_IRWXU);


2) Your patch is based on a slightly older code, it does not apply on
HEAD, Kindly rebase your patch on top of HEAD,

Regards,
Vignesh

On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you, I have made the changes and attached the patch.

Currently the files are created like this with the v3 patch:
If -l logdir is specified:
logdir
├── pg_createsubscriber_internal_2026-01-30-10-00-58.300264.log
├── pg_createsubscriber_server_2026-01-30-10-00-58.300264.log
├── pg_createsubscriber_internal_2026-01-30-10-08-54.270230.log
└── pg_createsubscriber_server_2026-01-30-10-08-54.270230.log

You might have slightly misunderstood my previous comment, but the
expected is like:
logdir
├── 20260130T100618.912
│   ├── pg_createsubscriber_internal.log
│   └── pg_createsubscriber_server.log
└── 20260130T101320.952
    ├── pg_createsubscriber_internal.log
    └── pg_createsubscriber_server.log

Let's keep the similar structure as created by pg_upgrade. You can
execute pg_upgrade once see how it is created. You can refer to
make_outputdirs for the same.

Regards,
Vignesh

On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you, I have made the changes and attached the patch.

Few comments:
1) Adding \n at the end will assert as pg_log_generic_v has the
following Assert:
Assert(fmt[strlen(fmt) - 1] != '\n');

@@ -1106,7 +1220,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo)
        int                     max_reporigins;
        int                     max_wprocs;

-       pg_log_info("checking settings on subscriber");
+       INFO("checking settings on subscriber\n");

You can run in verbose mode without -l to see this issue

2) There is a chance that directory creation can fail, it should be
checked and error should be thrown:
+                               if (stat(opt.log_dir, &statbuf) != 0)
+                               {
+                                       if (errno == ENOENT)
+                                       {
+                                               mkdir(opt.log_dir, S_IRWXU);
+                                               INFO("log directory created");

3) Can you include an fflush after the fprintf so that there is no log
content lost in case of abrupt failure:
+               va_start(args, format);
+               vfprintf(internal_log_file_fp, format, args);
+               fprintf(internal_log_file_fp, "\n");
+               va_end(args);

4) Since you are closing the log file early, the logs after this point
like the drop publication/drop replication slot in error flow will be
lost. They will neither appear in the console nor in the log file:
  * Clean up objects created by pg_createsubscriber.
@@ -212,6 +315,9 @@ cleanup_objects_atexit(void)
        if (success)
                return;

+       if (internal_log_file_fp != NULL)
+               fclose(internal_log_file_fp);
+

5) Since there is only one caller for this function and not needed by
anyone else, this code can be moved to the caller:
+static void
+populate_timestamp(char *timestr, size_t ts_len)
+{
+       struct timeval tval;
+       time_t          now;
+       struct tm       tmbuf;
+
+       gettimeofday(&tval, NULL);
+
+       /*
+        * MSVC's implementation of timeval uses a long for tv_sec, however,
+        * localtime() expects a time_t pointer.  Here we'll assign tv_sec to a
+        * local time_t variable so that we pass localtime() the correct pointer
+        * type.
+        */
+       now = tval.tv_sec;
+       strftime(timestr, ts_len,
+                        "%Y-%m-%d-%H-%M-%S",
+                        localtime_r(&now, &tmbuf));
+       /* append microseconds */
+       snprintf(timestr + strlen(timestr), ts_len - strlen(timestr),
+                        ".%06u", (unsigned int) (tval.tv_usec));
+}

Regards,
Vignesh

On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you!
>
> I have made the suggested changes.

You have missed to address the comments from [1]. Please change it as suggested.

[1] - https://www.postgresql.org/message-id/CALDaNm3Kx%3Dqv2XOf9x-bsOQx8R2aRBuJKxeTU0%2BSKR70G9WjZA%40mail.gmail.com

Regards,
Vignesh

On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you!
>
> I have made the suggested changes. In addition, I added a wrapper for pg_fatal to write to the file and then do
everythingthat pg_fatal would do.
 
> I have attached the patch.

Few comments:
1) Can we change the macro names:
INFO -> pg_log_info
INFO_HINT -> pg_log_info_hint
DEBUG -> pg_log_debug
FATAL -> pg_fatal

if required do an #undef and call pg_log_generic in the else similar
to how it is done in pg_backup_utils.h

+#define INFO(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_info(__VA_ARGS__);\
+} while(0)
+
+#define INFO_HINT(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_info_hint(__VA_ARGS__);\
+} while(0)
+
+#define DEBUG(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_debug(__VA_ARGS__);\
+} while(0)
+
+#define FATAL(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       pg_fatal(__VA_ARGS__); /* call pg_fatal after writing to logs */ \
+} while(0)

2) You can keep the variabled in case 'l' handling to keep the scope
accordingly:
+               char            timestamp[128];
+               struct timeval tval;
+               time_t          now;

3) Instead of creating a new standby and running with -l option, can
you run it with -l for one of the existing tests:
+$node_p->backup('backup_3');
+
+# Set up node R as a logical replica node
+my $node_r = PostgreSQL::Test::Cluster->new('node_r');
+$node_r->init_from_backup($node_p, 'backup_3', has_streaming => 1);
+$node_r->append_conf(
+       'postgresql.conf', qq[
+primary_conninfo = '$pconnstr dbname=postgres'
+hot_standby_feedback = on
+]);
+$node_r->set_standby_mode();
+
+# Test that --logdir works for pg_createsubscriber
+command_ok(
+       [
+               'pg_createsubscriber',
+               '--verbose',
+               '--pgdata' => $node_r->data_dir,
+               '--publisher-server' => $pconnstr,
+               '--database' => 'postgres',
+               '--logdir' => $logdir,
+       ],
+       'check for log file creation for pg_createSubscriber');
+
+# 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 @internal_log_files = glob "$logdir/pg_createsubscriber_internal_*.log";
+is( scalar(@internal_log_files), 1, "
+    pg_createsubscriber_internal.log file was created");

Regards,
Vignesh