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(at)gmail(dot)com> wrote:

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