Re: [Proposal] Adding Log File Capability to pg_createsubscriber

On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan(dot)sreejith(at)gmail(dot)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