Subject: [Proposal] Adding Log File Capability to pg_createsubscriber

From: Gyan Sreejith <[email-masked]>
Date: Tue, 9 Dec 2025 17:16:40 -0500

Background:

   -

   pg_createsubscriber currently outputs all messages (internal validation
   messages, standby server start/stop logs, recovery progress output, and
   output from utilities) directly to the console. As a result, users may find
   debugging and handling errors difficult. It would be more convenient if
   messages were separated and stored in different log files. There is 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 command would be easier to use as users
will only have to read individual log files rather than parse through lots
of possibly irrelevant 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

---

From: Peter Smith <[email-masked]>
Date: Thu, 11 Dec 2025 18:29:07 +1100

On Wed, Dec 10, 2025 at 9:17 AM Gyan Sreejith <[email-masked]> wrote:
>
> Background:
>
> pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs, recovery progress output, and output from utilities) directly to the console. As a result, users may find debugging and handling errors difficult. It would be more convenient if messages were separated and stored in different log files. There is 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 command would be easier to use as users will only have to read individual log files rather than parse through lots of possibly irrelevant output messages. I have attached the patch for this change.
>

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

---

From: Gyan Sreejith <[email-masked]>
Date: Thu, 11 Dec 2025 20:33:15 -0500

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 up into multiple files depending
on what was being invoked. Do you have another opinion?

Thank you once again,
Gyan Sreejith

On Thu, Dec 11, 2025 at 2:29 AM Peter Smith <[email-masked]> wrote:

> > logdir/pg_createsubscriber_server.log which captures all logs related to
> starting and stopping the standby server.
> >
> > Overall, this proposed solution could make the pg_createsubscriber
> command output messages more organized. The command would be easier to use
> as users will only have to read individual log files rather than parse
> through lots of possibly irrelevant output messages. I have attached the
> patch for this change.
> >

---

From: Gyan Sreejith <[email-masked]>
Date: Sun, 14 Dec 2025 10:47:36 -0500

I have included the patch file after making the changes to the SGML docs.

Thanks for your help,
Gyan Sreejith

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 16 Dec 2025 07:01:45 +0000

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

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Dec 2025 10:49:28 +1100

On Fri, Dec 12, 2025 at 12:33 PM Gyan Sreejith <[email-masked]> wrote:
> 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 up into 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

---

From: vignesh C <[email-masked]>
Date: Wed, 17 Dec 2025 15:37:09 +0530

>    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

---

From: "Euler Taveira" <[email-masked]>
Date: Wed, 17 Dec 2025 22:28:54 -0300

> the data directory, eliminating the need for any additional handling

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/

---

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Dec 2025 12:19:02 +0530

On Thu, Dec 18, 2025 at 6:59 AM Euler Taveira <[email-masked]> wrote:
> 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.

---

From: Gyan Sreejith <[email-masked]>
Date: Tue, 23 Dec 2025 18:22:11 -0500

Thank you for the feedback everybody. As I read through this email chain, I
found differing opinions on how logging should be implemented. This
ambiguity leaves me unsure as to which solution(s) to pursue. As of right
now, I have attached the git-format patch like Hayato Kuroda recommended
(but it does not have any new changes). I am willing to implement whatever
solution when we reach a consensus.

Thank you for all of the help,
Gyan Sreejith

---

From: vignesh C <[email-masked]>
Date: Mon, 29 Dec 2025 16:40:46 +0530

On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <[email-masked]> wrote:
>
> Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging should be implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have attached the git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to implement whatever 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

---

From: "Euler Taveira" <[email-masked]>
Date: Thu, 05 Mar 2026 11:48:57 -0300

On Tue, Feb 24, 2026, at 8:55 PM, Gyan Sreejith wrote:
> I have made the changes you suggested and have attached the patch below. 
>

[Avoid top-posting ...]

I took another look at this patch. Comments are below:

+/*
+ * Open a new logfile with proper permissions.
+ * From src/backend/postmaster/syslogger.c
+ */
+static FILE *
+logfile_open(const char *filename, const char *mode)
+{

Don't duplicate code. If you are reusing a function, my advice is to move it to
src/common. You can always use "ifdef FRONTEND" to use the appropriate log
message (elog/ereport vs pg_error, for example).

-	 * XXX this code was extracted from BootStrapXLOG().
+	 * XXX this code was extracted from BootStrapXpg_log_info().

This is a typo.

+			case 'l':
+				{
+					char		timestamp[128];
+					struct timeval tval;
+					time_t		now;
+					struct tm	tmbuf;

I would expect to have this code in a function. That's the pattern it is using.

+					strftime(timestamp, sizeof(timestamp), "%Y-%m-%d-%H-%M-%S", localtime_r(&now, &tmbuf));
+					/* append microseconds */
+					snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp),
+							 ".%06u", (unsigned int) (tval.tv_usec));
+					log_timestamp = pg_strdup(timestamp);

Do we really need microseconds? I would say milliseconds is sufficient. I
suggest to remove the dash (-); it is using a different style from existing
code (pg_upgrade).

+					opt.log_dir = pg_strdup(optarg);
+					canonicalize_path(opt.log_dir);

You didn't have a check for long path. It is not a good idea in general. See
MAXPGPATH examples. It would be a good idea if the path are restricted to
MAXPGPATH length.

+						if (errno == ENOENT)
+						{
+							if (mkdir(opt.log_dir, S_IRWXU) == 0)
+								pg_log_info("log directory created");
+							else if (errno == EACCES)
+								pg_fatal("permission denied trying to create log directory \"%s\": %m", opt.log_dir);
+							else
+								pg_fatal("could not create log directory \"%s\": %m", opt.log_dir);
+						}
+						else if (errno == EACCES)
+							pg_fatal("permission denied trying to access directory \"%s\": %m", opt.log_dir);
+						else
+							pg_fatal("could not access directory \"%s\": %m", opt.log_dir);

The "permission denied" is redundant here because it will be in %m. Instead, I
suggest that you use

  could not create directory \"%s\": %m

The main advantage is that this sentence is already available. It avoids
translation effort.

+#undef pg_log_info
+#define pg_log_info(...) do{\
+	if (internal_log_file_fp != NULL) \
+		internal_log_file_write(__VA_ARGS__); \
+	else \
+		pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\
+} while(0)

I don't like the fact that internal_log_file_fp is not declared before this
#define.

One of the arguments to have this feature was that pg_createsubscriber mixes the
server and tool messages. Couldn't we fix it adding "marks" on the output saying
the server log messages starts here and the server log messages ends here?

---

From: Amit Kapila <[email-masked]>
Date: Fri, 6 Mar 2026 16:20:51 +0530

> the server log messages starts here and the server log messages ends here?

It will simplify the output to some extent but still it will be mixed
at different intervals in the LOG which is not good for grepping
required info. Also, I am not sure if we use such markers at other
places.

-- 
With Regards,
Amit Kapila.

---

From: Gyan Sreejith <[email-masked]>
Date: Mon, 9 Mar 2026 18:55:56 -0400

On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <[email-masked]> wrote:

Don't duplicate code. If you are reusing a function, my advice is to move
> it to

I have made all the changes except for this one, and I am deferring to Amit
Kapila regarding the marks.
Regarding duplicating code - I was trying to keep the change contained. Is
it okay to make that change in a separate check in since it would also
touch src/backend/postmaster/syslogger.c?

Thank you!
Gyan Sreejith

---

From: vignesh C <[email-masked]>
Date: Wed, 11 Mar 2026 15:34:55 +0530

On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <[email-masked]> wrote:
> I have made all the changes except for this one, and I am deferring to Amit Kapila regarding the marks.

Few comments:
1) You are not checking log level because of which the contents are
logged irrespective of the log level:
+#undef pg_log_info
+#define pg_log_info(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\
+} while(0)
+
+#undef pg_log_info_hint
+#define pg_log_info_hint(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\
+} while(0)
+
+#undef pg_log_debug
+#define pg_log_debug(...) do{\
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
+                       pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY,
__VA_ARGS__); \
+} while(0)

2) Instead of just checking if the file is created or not, let's check
for some contents from the file:
+# 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");

3) This change is not required, let's remove this:
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber');
 program_version_ok('pg_createsubscriber');
 program_options_handling_ok('pg_createsubscriber');

-my $datadir = PostgreSQL::Test::Utils::tempdir;
+my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir";

4) No need of '{' as it is a single line statement
if (opt->log_dir != NULL)
{
appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir,
log_timestamp, SERVER_LOG_FILE_NAME);
}

Regards,
Vignesh

---

From: Gyan Sreejith <[email-masked]>
Date: Sun, 15 Mar 2026 19:23:11 -0400

> +                       pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY,

The log level is passed to and checked by pg_log_generic_v() which is
called by pg_log_generic().

> for some contents from the file:
Added checks to ensure that the log files are not empty, thanks!

> +my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir";

Fixed

> log_timestamp, SERVER_LOG_FILE_NAME);
Fixed

Thank you! I have attached the changes.
Regards,
Gyan

---

From: shveta malik <[email-masked]>
Date: Tue, 17 Mar 2026 11:35:53 +0530

> Thank you! I have attached the changes.

Thank You for the patch.

1)
 I observed that when we give both --verbose and --logdir options, it
skips verbose and only dumps logs to logdir. Is this intentional?

I don't see any other postgres command with both the options provided
and thus cannot compare the behaviour. But IMO, if the user has
explicitly given both options, we should dump the log on both (stdout
and logfile). Thoughts?  (Apologies if this has already been discussed
and I missed it.)

2)
I noticed a minor difference when running the command with both
--verbose and --logdir, depending on the order in which they are
specified.

If --verbose is specified first, I see the following message on the console:
pg_createsubscriber: directory ../../pg_createsub/20260317T112204.848 created

But if --logdir is specified first, this message does not appear on
the console. Is this behavior intentional?

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Tue, 17 Mar 2026 16:10:53 +0530

On Tue, Mar 17, 2026 at 11:36 AM shveta malik <[email-masked]> wrote:
> and logfile).

The entire log should be dumped in the specified log_file. I think
this is similar to one setting server's log_level to debug1 and then
start server with -l option in which case the entire log is dumped to
server log file.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 17 Mar 2026 12:18:41 +0000

Dear Gayn,

Thanks for updating the patch. I resumed reviewing.

01.
```
+static void
+                       internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2)));
```

pg_attribute_printf seems to be used in postgres.

02.
```
+static void
+make_dir(char *dir)
```

Since there are only two callers, I think no need to introduce the funciton.
E.g, make_outpudirs in pg_upgrade does mkdir() four times.

03.
```
#undef pg_log_debug
#define pg_log_debug(...) do{\
              if (internal_log_file_fp != NULL) \
                            internal_log_file_write(__VA_ARGS__); \
              else \
                            if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
                                          pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
} while(0)
```

The patch ignores setting of log level if -l is specified. By default only
warnings/errors/fatals should be output, but even debug messages are output with
-l option. Checking logic is in pg_log_generic()->pg_log_generic_v() but we
cannot reach if internal_log_file_fp is set.

Also, are there any examples to undefine these macros? It's bit surprising for
me; I prefer some inline functions instead.

04.
```
+                       case 'l':
+                               opt.log_dir = pg_strdup(optarg);
+                               canonicalize_path(opt.log_dir);
+                               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);
```

I feel creating the log file is too early, otherwise the issue Shveta raised the
at #2 could happen [1].
I feel it's OK putting after the if (opt.pub_conninfo_str == NULL). Because
there is the first place where call pg_log_info, after doing very simple validation.

05.
Let me confirm one point. IIUC, freopen() can be used to replace the stderr to
the file, and this may able to remove all #undef macros. The downside of this
approach is that even ERROR/FATAL messages would be on the file and nothing
appears on the terminal. See DebugFileOpen() the example.

Do you think that we may not use freopen() as-is? Do others have variants?

06.
I added this thread and the cfbot cannot accept your patch [2]. Please fix.

[1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.gmail.com
[2]: https://commitfest.postgresql.org/patch/6592/

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 17 Mar 2026 18:18:27 +0530

Hi Gyan,

Thanks for working on the patch.

On Mon, 16 Mar 2026 at 04:53, Gyan Sreejith <[email-masked]> wrote:
> The log level is passed to and checked by pg_log_generic_v() which is called by pg_log_generic().

But we are not checking the log level when we are writing the logs in
the logfiles.
Due to this, extra logs can appear.

> Added checks to ensure that the log files are not empty, thanks!
I think along with it we should also check the actual contents of each file.

> Thank you! I have attached the changes.

I noticed that we do not define pg_log_warning, pg_log_warning_detail
and pg_log_warning_hint, due to this the contents when option
'--logdir' is specified and when it is not defined can differ. Should
we define these as well?

Also  internal_log_file_write is declared as:
+static void
+           internal_log_file_write(const char *format,...)
__attribute__((format(printf, 1, 2)));
Should we use 'pg_attribute_printf' instead of
__attribute__((format(printf, 1, 2)))?

I have added the changes for above in the topup patch. I also did some
cosmetic changes.
I have also addressed the comment (2) by Shveta in [1].

If you agree with these changes, please feel free to include them in the patch.
[1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.gmail.com

Thanks,
Shlok Kyal

---

From: Gyan Sreejith <[email-masked]>
Date: Tue, 17 Mar 2026 19:24:53 -0400

Thanks a lot, Shlok, for all the changes. I have included the changes from
topup.patch to my changes.

I believe it handles all the review comments. Please take a look.

Regards,
Gyan

---

From: Kuroda, Hayato/黒田 隼人<[email-masked]>
Date: Wed, 18 Mar 2026 06:12:41 +0000

Dear Gyan,

Thanks for updating. Not sure you have addressed my comments as well,
but I reviewed again.

01.
```
+#undef pg_log_warning
+#define pg_log_warning(...) \
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
+       pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__)
```

IIUC it's more common that to have do {...} while if the marco function has
several lines.

02.
```
+#undef pg_log_info
+#define pg_log_info(...) do { \
+       if (internal_log_file_fp != NULL) \
+               internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
+       else \
+               pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
+} while(0)
```

Missing update?

03.

I think pg_log_error faimily should be also output on the file, but it misses.

04.
```
+static void
+make_dir(const char *dir)
```

I think this is not needed, we can follow the approach done on the pg_upgrade.
The message "directory %s created" may be removed, but I think it's ok.

05.
```
+       /* Build timestamp directory path */
+       len = snprintf(timestamp_dir, MAXPGPATH, "%s/%s", log_dir, timestamp);
+
+       if (len >= MAXPGPATH)
+               pg_fatal("directory path for log files, %s/%s, is too long",
+                                log_dir, timestamp);
```

These checks should be done before creating the base directory.

06.
```
+static char *log_timestamp = NULL;     /* Timestamp to be used in all log file
+                                                                        * names */
```

I feel it's more efficient to directly have the directory where log exists.

07.
```
+       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);
+       }
```

I still think it can be put bit later; after validating simple messages.
How about others?

08.
```
+static void
+internal_log_file_write(enum pg_log_level level, const char *format,...)
+{
+       va_list         args;
+
+       if (level < __pg_log_level)
+               return;
+
+       if (internal_log_file_fp == NULL)
+               return;
+
+       va_start(args, format);
+       vfprintf(internal_log_file_fp, format, args);
+       va_end(args);
+
+       fprintf(internal_log_file_fp, "\n");
+       fflush(internal_log_file_fp);
+}
```

internal_log_file_fp has already been checked before calling, so it can be Assert().
Also, the translated string should be passed to vfprintf().
My small tests shown that changing from "format" to "_(format)" is enough, but not sure
other platforms. Some tests are needed.

09.
```
+       if (opt->log_dir != NULL)
+               out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+       else
+               out_file = DEVNULL;
```

I still think comments should be atop here.

Attached patch includes above changes.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Mar 2026 15:33:06 +0530

> I still think it can be put bit later; after validating simple messages.

+1, otherwise it will simply create a directory and error out after
that if the command is wrong. We can avoid creating a directory.

Patch has a compilation issue, logfile_open() needs a second argument in main().
If we fix that, it gives a segmentation fault in make_output_dirs() as
logdir is not allocated, earlier it was an array, now a NULL pointer.
Kuroda-san is going to post a new patch soon.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Mar 2026 18:45:17 +0530

On Wed, Mar 18, 2026 at 4:55 AM Gyan Sreejith <[email-masked]> wrote:
>
> Thanks a lot, Shlok, for all the changes. I have included the changes from topup.patch to my changes.
>

+#undef pg_log_warning
+#define pg_log_warning(...) \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
+ pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__)
+
+#undef pg_log_warning_detail
+#define pg_log_warning_detail(...) \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
+ pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__)
+
+#undef pg_log_warning_hint
+#define pg_log_warning_hint(...) \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
+ pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__)
+
+#undef pg_log_info
+#define pg_log_info(...) do { \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
+ else \
+ pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
+} while(0)
+
+#undef pg_log_info_hint
+#define pg_log_info_hint(...) do { \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
+ else \
+ pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__); \
+} while(0)
+
+#undef pg_log_debug
+#define pg_log_debug(...) do { \
+ if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) { \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_DEBUG, __VA_ARGS__); \
+ else \
+ pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
+ } \
+} while(0)
+
+#undef pg_fatal
+#define pg_fatal(...) do { \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__); \
+ pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+ exit(1); \
+} while(0)

This looks odd to me. I think it would be better to encapsulate this
in one function (something like we have in pg_log_v) and then based on
log level, do required handling.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 18 Mar 2026 13:44:37 +0000

Dear Amit,

> log level, do required handling.

Based on the suggestion, I updated patches. Please see attached series.

v11-0001 replaces existing pg_log_xxx families to new reporting function
pg_createsub_log(). Now only pg_fatal is overwritten, which is same as pg_uprade.

v11-0002 is the rebased version of v10. Nothing new, but codes are adjusted based
on 0001.

v11-0003 contains changes from me [1]. 0002 and 0003 can be combined if it's OK.

[1]: https://www.postgresql.org/message-id/OS9PR01MB12149C7DE09F13C3BCBD9357DF54EA%40OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Kuroda, Hayato/黒田 隼人<[email-masked]>
Date: Thu, 19 Mar 2026 11:55:00 +0000

Hi,

While checking again, I found an issue in the new logging function - same va_list
is used twice, but it was the undefined behavior. va_copy() is used to fix the issue.

Also, below points were found and fixed in 0003. Thanks Peter Smith to work on.

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

But there is a possibility that existing directory is specified, right?

```
+my $logdir = PostgreSQL::Test::Utils::tempdir + "/logdir";
```

Isn't it enough to just use tempdir here? At least, I think using "+" is not
appropriate, "." is used to connect strings.

```
+       if (opt->log_dir != NULL)
+               appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
```

I do not have 100% confident, but do we have to quote the logfile here?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Gyan Sreejith <[email-masked]>
Date: Thu, 19 Mar 2026 14:29:38 -0400

Thank you, Kuroda-san, for all the work.
I have made a small change - I added a pg_free() to free internal_log_file.

Regards,
Gyan

On Thu, Mar 19, 2026 at 7:55 AM Kuroda, Hayato/黒田 隼人 <
[email-masked]> wrote:

> +               appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log",
> opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
> ```
>

---

From: Nisha Moond <[email-masked]>
Date: Fri, 20 Mar 2026 16:00:03 +0530

> I have made a small change - I added a pg_free() to free internal_log_file.

Hi Gyan,
I reviewed/tested the patches, please find below comments for v13-002 patch -

File: pg_createsubscriber.c
1)
+
+   if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL)
+     pg_fatal("could not open log file \"%s\": %m", internal_log_file);
+

IIUC, the pg_fatal() call here seems unreachable as the function
logfile_open() itself calls pg_fatal() and exits if it fails to open
the file.
I think it should just be -
        internal_log_file_fp = logfile_open(internal_log_file, "a");

Please correct me if I'm missing something.
~~~

2)
+ if (opt->log_dir != NULL)
+   out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp,
SERVER_LOG_FILE_NAME);
+ else
+   out_file = DEVNULL;
+
+ cmd_str = psprintf("\"%s\" -D \"%s\" >> \"%s\"", pg_resetwal_path,
+            subscriber_dir, out_file);

Similar to internal_log_file, why are 'out_file' and 'cmd_str' above not freed?
~~~

3) Typo - extra blank line after va_end(args);
@@ -205,10 +243,11 @@ pg_fatal(const char *pg_restrict fmt,...)
  va_start(args, fmt);

- pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args);
+ pg_createsub_log_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args);

  va_end(args);

+
  exit(1);
~~~

4) File: t/040_pg_createsubscriber.pl

Above introduces newlines in result log, test 29 and 32 looks like -

[14:46:59.071](0.639s) ok 28 - run pg_createsubscriber --dry-run on node S
[14:46:59.071](0.000s) ok 29 -
[14:46:59.071](0.000s) #     pg_createsubscriber_server.log file was created
[14:46:59.071](0.000s) ok 30 - pg_createsubscriber_server.log file not empty
[14:46:59.071](0.000s) ok 31 - server reached consistent recovery state
[14:46:59.072](0.000s) ok 32 -
[14:46:59.072](0.000s) #     pg_createsubscriber_internal.log file was created
[14:46:59.072](0.000s) ok 33 - pg_createsubscriber_internal.log file not empty

These should be single-line results similar to others.

--
Thanks,
Nisha

---

From: Kuroda, Hayato/黒田 隼人<[email-masked]>
Date: Fri, 20 Mar 2026 11:37:55 +0000

Dear Gyan,

Thanks for updating the patch!

While considering it further, I started to think the combination with -l and -v
is problematic.

For now, if -l and -v are set, info/debug-level messages are written only to the
file, whereas warning/error messages are written to both.
In the case of pg_upgrade, however, all additional messages are printed to both
the terminal and the file. They are similar programs, so we should follow the
spec. Also, there is another advantage that users can check the progress of the
command from INFO/DEBUG messages.

Since all comments were OK for you, I merged 0003 into 0002, and now all
messages would be printed on both stderr/log file. How do you think?

Can you also check other comments like posted on [1] and revise if needed?

[1]: https://www.postgresql.org/message-id/[email-masked]

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Gyan Sreejith <[email-masked]>
Date: Fri, 20 Mar 2026 13:00:14 -0400

Thank you, Kuroda-san and Nisha!
I have fixed everything that Nisha suggested. And I agree that all messages
should be written to both the log file and the terminal.

Thank you,
Gyan

---

From: Amit Kapila <[email-masked]>
Date: Sat, 21 Mar 2026 15:27:40 +0530

On Fri, Mar 20, 2026 at 10:30 PM Gyan Sreejith <[email-masked]> wrote:

0001:
=====
*
+static void pg_createsub_log(enum pg_log_level, enum pg_log_part,
+ const char *pg_restrict fmt,...)
+ pg_attribute_printf(3, 4);
+pg_noreturn static void pg_fatal(const char *pg_restrict fmt,...)
+ pg_attribute_printf(1, 2);

I see similar functions in other modules are named a bit differently.
For example, see report_manifest_error(), report_backup_error(),
report_fatal_error(). Then I see many other error reporting functions
named similarly in code, some examples are: report_invalid_record(),
report_invalid_page(), report_namespace_conflict(), and
report_recovery_conflict().

Based on the above information, can we consider renaming the above
functions to report_createsub_log() and report_createsub_fatal()?

Other than the above point, 0001 LGTM.

-- 
With Regards,
Amit Kapila.

---

From: Gyan Sreejith <[email-masked]>
Date: Sat, 21 Mar 2026 19:09:05 -0400

> Other than the above point, 0001 LGTM.

I have renamed the functions.

Regards,
Gyan

---

From: shveta malik <[email-masked]>
Date: Mon, 23 Mar 2026 08:45:51 +0530

> I have renamed the functions.

001 looks good. I like the new function's name.
Commit message still refers old function name, it can be corrected
during commit.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Mon, 23 Mar 2026 12:46:36 +0530

On Mon, Mar 23, 2026 at 8:46 AM shveta malik <[email-masked]> wrote:
> 001 looks good.

Pushed 0001.

-- 
With Regards,
Amit Kapila.

---

From: Kuroda, Hayato/黒田 隼人<[email-masked]>
Date: Mon, 23 Mar 2026 09:54:59 +0000

Dear Gyan,

Can you rebase the patch because 0001 is now pushed?
Also, below contains my minor comments.

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?

02.
Can we check the permission of generated directory in the test?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Mon, 23 Mar 2026 15:25:04 +0530

> I have renamed the functions.

Thanks. Few comments on 002:

1)

We can get rid of below alignment related changes in unrelated test parts.

----
-$node_p->safe_psql($db1, qq(
+$node_p->safe_psql(
+ $db1, qq(

-is($result, qq(),
- "table is not replicated in database $db1");
+is($result, qq(), "table is not replicated in database $db1");

-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");

-is($result, qq($db1|{test_pub3}
+is( $result, qq($db1|{test_pub3}
 ----

2)
Can we simplify the logic of report_createsub_log_v to:
---
        if (internal_log_file_fp != NULL)
        {
                va_list         arg_cpy;

                va_copy(arg_cpy, args);
                internal_log_file_write(level, fmt, arg_cpy);
                va_end(arg_cpy);
        }
        pg_log_generic_v(level, part, fmt, args);
---

We need not to invoke pg_log_generic_v in both if and else.

3)
+ /* 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 BASE_DIR/$timestamp */
+ if (mkdir(logdir, S_IRWXU) < 0)
+ pg_fatal("could not create directory \"%s\": %m", logdir);
--

Instead of S_IRWXU directly, shall we use pg_dir_create_mode (which
means S_IRWXU) similar to other modules (pg_upgrade, initdb, pg_dump
etc)

See pg_upgrade:
        if (mkdir(log_opts.logdir, pg_dir_create_mode) < 0)
                pg_fatal("could not create directory \"%s\": %m",
log_opts.logdir);

4)
+ /* Create BASE_DIR/$timestamp */

Above comment refers to BASE_DIR which looks like some variable, but
it is not. Can we please change this comment to:
/* Create a timestamp-named subdirectory under the base directory */

5)
+ /* append milliseconds */
append -->Append

thanks
Shveta

---

From: Gyan Sreejith <[email-masked]>
Date: Mon, 23 Mar 2026 21:23:29 -0400

On Mon, Mar 23, 2026 at 2:24 AM Chao Li <[email-masked]> 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!

> 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!

> 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: Chao Li <[email-masked]>
Date: Tue, 24 Mar 2026 11:02:12 +0800

> On Mar 24, 2026, at 09:23, Gyan Sreejith <[email-masked]> wrote:
> 
> We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir.

Not sure if I stated my comment clearly. The “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the pg_fatal, you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too long, and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be:
```
        report_createsub_fatal("directory path for log files, %s/%s, is too long”,
                      log_basedir, timestamp);
```

> 
> > 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);
> > 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!

I verified with v17, %m works now.

> 
> >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 personally prefer to keep those prefixes, which helps keep the log messages in a consistent style.

> 
> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>

One comment on v17.
```
+	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;
+	}
```

As the error is about internal_log_file_fp, meaning it may no longer able to write to internal_log_file_fp anymore, we should avoid use report_createsub_fatal to log the error again. Maybe just a pg_fatal(), or just log to stderr.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Chao Li <[email-masked]>
Date: Tue, 24 Mar 2026 11:31:02 +0800

> On Mar 24, 2026, at 11:02, Chao Li <[email-masked]> wrote:
> 
> 
> 

I forgot to mention one small suggestion.

Since the internal log file path includes a timestamped directory, it is a bit inconvenient to figure out the full path of the file after the command finishes. I think it would be helpful to print the full path of the internal log file at the end, something like:
```
	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;
		printf("check the internal log file at \"%s/%s.log\"", logdir, INTERNAL_LOG_FILE_NAME);
	}
```

Note that I used printf() here, because based on my test, pg_log_info() does not work in this place.

The other thing I noticed while trying the above code is that cleanup_objects_atexit() has this:
```
    if (success)
        return;
```

That means on the success path, internal_log_file_fp will not be closed. So this part would need some adjustment as well. Maybe one way is to jump to the log-file cleanup part before returning, for example with a goto, so that internal_log_file_fp is always closed.

Best regards,

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 Mar 2026 12:01:44 +0530

> I personally prefer to keep those prefixes, which helps keep the log messages in a consistent style.

+1 to keep HINT, DETAIL kind of prefixes. I have one additional
comment in the latest patch:

+ mode_t oumask;
+
+ oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG
| S_IRWXO)));
+ fh = fopen(filename, mode);
+ umask(oumask);

I think we should follow the permissions model for this file same as
what pg_ctl does for log_file given with -l option. Basically, start
with with 077 permission and then switch to data_directory
permissions. This is because to start server we anyway use pg_ctl with
-l option which will use a specific way to assign permissions to
server log file, so we should use same for internal file, otherwise,
we will end up with different file permissions for internal and server
logfile.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 24 Mar 2026 09:47:07 +0000

Dear Chao,

> +			report_createsub_fatal("could not
> close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME);
> As the error is about internal_log_file_fp, meaning it may no longer able to write to
> internal_log_file_fp anymore, we should avoid use report_createsub_fatal to log
> the error again. Maybe just a pg_fatal(), or just log to stderr.

I checked and found [1] in the exit() specification. It means all file descriptors
opened by the process would be closed automatically when the process exits.
So no need to try fclose() and report the failure here. Similarly, pg_upgrade
does not have fclose(log_opts.internal) in case of failures.

[1]: 
First, all functions registered by the atexit function are called, in the reverse
order of their registration, except that a function is called after any previously
registered functions that had already been called at the time it was registered.
If, during the call to any such function, a call to the longjmp function is made
that would terminate the call to the registered function, the behavior is
undefined.
Next, all open streams with unwritten buffered data are flushed, all open streams
are closed, and all files created by the tmpfile function are removed.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 24 Mar 2026 09:50:39 +0000

Dear Chao,

> Since the internal log file path includes a timestamped directory, it is a bit
> inconvenient to figure out the full path of the file after the command finishes. I
> think it would be helpful to print the full path of the internal log file at the end,
> something like:
> ```
> 			report_createsub_fatal("could not

I don't think it's needed, because pg_upgrade does not print such points.

> Note that I used printf() here, because based on my test, pg_log_info() does not

It might be becasue you used pg_log_info(). pg_createsubscriber outputs only
warning or error level messages, info-level cannot be printed.

> That means on the success path, internal_log_file_fp will not be closed. So this
> part would need some adjustment as well. Maybe one way is to jump to the
> log-file cleanup part before returning, for example with a goto, so that
> internal_log_file_fp is always closed.

As the same reason I posted, I think it's not needed.

[1]: https://www.postgresql.org/message-id/OS9PR01MB12149F1AD9C79A2644753A18AF548A%40OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 24 Mar 2026 10:06:08 +0000

Dear Gyan,

> I haven't changed the output of pg_log_generic_v() yet. Shall I add the prefix to the output?

Since pg_log_generic_v() is the common function used even by others, we should
not fix it. Let's fix internal_log_file_write() instead.

Further comments:

01.
```
+               setvbuf(fh, NULL, PG_IOLBF, 0);
```

I think setvbuf is not needed.

IIUC, setvbuf() controls when messages like printf() are flushed from to the
stdout/stderr/files and so on. In logfile_open(), we are setting that output
messages should be flushed once per new lines. Syslogger process does not call
fflush() frequently thus it seems to be controlled by the library call.
However, pg_createsubscriber always does the fflush() every time, no need to
specify when the buffered strings are flushed on the file.

Also note that pg_upgrade does not call the library function.

02.
```
+               /* use CRLF line endings on Windows */
+               _setmode(_fileno(fh), _O_TEXT);
```

_setmode() is called in win32 system. According to [2] and code comment, it's
used to open the file as TEXT mode for unifying the new-line character to CR-LF.

However, postgres has already been done in fopen(). The function is overwritten
to pgwin32_fopen()->pgwin32_open(), and there we already specify to open with
the _O_TEXT mode. I think no need to do in the pg_createsubscriber.c.

Also note that pg_upgrade does not call the library function.

03.
If we fix above and comments from Amit [1], logfile_open() is not completely
different function from the syslogger.c. We can remove the comment atop the
function.

04.
As I told in [2], no need to do fclose() the file in the cleanup function.

I created a top-up patch set which addressed all comments from me and others.
See attached.

[1]: https://www.postgresql.org/message-id/CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/OS9PR01MB12149F1AD9C79A2644753A18AF548A%40OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 Mar 2026 17:43:43 +0530

> I created a top-up patch set which addressed all comments from me and others.

+ /* Set mask based on the PGDATA permissions */
+ if (!GetDataDirectoryCreatePerm(subscriber_dir))
+ report_createsub_fatal("could not read permissions of directory \"%s\": %m",
+    subscriber_dir);

We can add a bit more detailed comments as to why we use
data_directory permissions.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Wed, 25 Mar 2026 10:14:08 +1100

Hi. Here are some review comments for v18-0001.

======

1.
+#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server"
+#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal"

Those are not the names of logfiles; they are just the first part of
the names. Why not also put the ".log" part here, instead of burying
it in format strings scattered in the code?

SUGGESTION (e.g., more similar to pg_upgrade.h)
#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server.log"
#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal.log"

~~~

2.
+static char logdir[MAXPGPATH]; /* Directory log files are put (if specified) */

Missing words in that comment?

e.g.
Directory where log files are written ...
Directory for log files ...

It might also be useful to mention that this is more than just the
original user-specified --logdir directory; it also includes the
timestamp sub-dir.

~~~

3.
 /*
- * 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)

This is a function comment, but there is no way to specify the log
file by using this function, so maybe reword that "if specified" part
for clarity.

SUGGESTION:
* Report a message with a given log level.
* Writes to stderr, and also to the log file (if pg_createsubscriber
--logdir option was specified).

~~~

4.
+ /* 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);

IIUC, when the snprintf exceeds MAXPGPATH, then 'logdir' is going to
contain a truncated string. And, maybe that already includes a partial
truncated timestamp part:
"myreallylongname/20260"

AFAICT, the subsequent fatal message is going to report that
incorrectly when it appends the timestamp a second time, like:
"... myreallylongname/20260/20260119T204317.204 is too long"

Notice that pg_upgrade doesn't try to report the names of files that
are too long. It just says things like "directory path for new cluster
is too long".

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

---

From: Gyan Sreejith <[email-masked]>
Date: Tue, 24 Mar 2026 20:12:59 -0400

Thank you, Kuroda-san and Chao for the suggestions and the patch. I have
merged it with v18-0001, and I have fixed everything that Peter suggested.

Regards,
Gyan

---

From: Amit Kapila <[email-masked]>
Date: Wed, 25 Mar 2026 13:55:05 +0530

On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <[email-masked]> wrote:
>
> Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested.
>

Pushed after minor changes.

-- 
With Regards,
Amit Kapila.

---

From: Gyan Sreejith <[email-masked]>
Date: Sat, 28 Mar 2026 12:47:19 -0400

Thank you everybody for the help.

Regards,
Gyan

---

From: Amit Kapila <[email-masked]>
Date: Wed, 1 Apr 2026 10:26:53 +0530

On Tue, Mar 31, 2026 at 5:36 PM Peter Eisentraut <[email-masked]> wrote:
>
> On 25.03.26 09:25, Amit Kapila wrote:
> Instead of creating a new logging API in pg_createsubscriber locally,
> why not extend logging.c to support a log file.
>

Few things we may want to consider if we want to use extended
logging.c: (a) what if we want information to be written in multiple
files (say pub/sub specific info into separate files), something like
what pg_upgrade does, (b) what if we want to write some info to only
one of terminal or log_file, we can probably extend generic API for
this.

>  This is much simpler,
> doesn't require caller changes, and could be reused elsewhere.  See
> attached patches.  (The 0003 and 0004 are just some additional code
> simplifications.)
>

In 0004,
-static char logdir[MAXPGPATH]; /* Subdirectory of the user specified logdir
+static char *logdir = NULL; /* Subdirectory of the user specified logdir

At other places like pg_upgrade, we use MAXPGPATH for a similar case
probably to have standard length on all platforms, see
make_outputdirs() in pg_upgrade.c.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 1 Apr 2026 05:05:10 +0000

Dear Peter E.,

> why not extend logging.c to support a log file.

I had a concern to extend the common module. Do you have possible idea to use
pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough
for others.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 7 Apr 2026 15:36:05 +0200

> one of terminal or log_file, we can probably extend generic API for

In the proposed API, you can switch the log file at any time.  And there 
could be a separate log level for file output versus terminal output.

> make_outputdirs() in pg_upgrade.c.

I think using MAXPGPATH is kind of deprecated and useless, certainly in 
new code.  But apart from that, note that in pg_createsubscriber.c 
itself, subscriber_dir is dynamically allocated.

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 7 Apr 2026 15:37:06 +0200

> pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough

pg_upgrade was mentioned.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 8 Apr 2026 11:00:58 +0530

> pg_upgrade was mentioned.

I see that pg_upgrade has some specific requirements related to
verbose/status/report kind of messages which doesn't seem to be
directly handled by the generic API  pg_log_generic(). For example,
apart from other things, we need to unset/reset log_file when we want
to send messages only to the terminal and quite some specific handling
for status or other kinds of messages. If you think pg_log_generic
could be extended to handle various upgrade cases then I think it
would be a good use case for it and we should go-ahead with what you
are proposing. OTOH, maybe we can give it a separate try and use the
resulting API for both pg_createsubscriber and pg_upgrade. I am fine
either way you would like to go-ahead with this.

-- 
With Regards,
Amit Kapila.

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 13 Apr 2026 11:24:14 +0200

> either way you would like to go-ahead with this.

I have committed these patches.

I would like to apply the frontend logging API to pg_upgrade as well, 
but that's a bigger project for not right now.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 16 Apr 2026 00:20:15 +0000

Dear Peter,

> I have committed these patches.

Thanks!

> but that's a bigger project for not right now.

Right, it may need huge changes. Not sure it can be done for PG19 because
we have already been in feature freeze.

Best regards,
Hayato Kuroda
FUJITSU LIMITED