Re: [Proposal] Adding Log File Capability to pg_createsubscriber

On Thu, Mar 19, 2026 at 11:59 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> Thank you, Kuroda-san, for all the work.
> 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
+is( scalar(@server_log_files), 1, "
+    pg_createsubscriber_server.log file was created");
...
...
+is( scalar(@internal_log_files), 1, "
+    pg_createsubscriber_internal.log file was created");

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