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 <[email protected]> wrote: > On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <[email protected]> > 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 >
v4-0001-Add-a-new-argument-l-logdir-to-pg_createsubscribe.patch
Description: Binary data
