Re: [Proposal] Adding Log File Capability to pg_createsubscriber

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 everything that pg_fatal would do.
I have attached the patch.

Regards,
Gyan

On Fri, Jan 30, 2026 at 4:35 AM vignesh C <vignesh21@gmail.com> wrote:

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