Re: [Proposal] Adding Log File Capability to pg_createsubscriber - Mailing list pgsql-hackers

> On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
> On Mon, Mar 23, 2026 at 2:24 AM Chao Li <li.evan.chao@gmail.com> wrote:
> +       /* Build timestamp directory path */
> +       len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
> +
> +       if (len >= MAXPGPATH)
> +               pg_fatal("directory path for log files, %s/%s, is too long",
> +                                logdir, timestamp);
> ```
> > In the pg_fatal call, I believe logdir should be log_basedir.
> We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir.

Not sure if I stated my comment clearly. The “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the
pg_fatal,you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too
long,and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be: 
```
        report_createsub_fatal("directory path for log files, %s/%s, is too long”,
                      log_basedir, timestamp);
```

>
> > The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can
checkthe implementation of pg_log_generic_v how %m is handled. The key code snippet is: 
> ```
> > errno = save_errno;
>
> > va_copy(ap2, ap);
> > required_len = vsnprintf(NULL, 0, fmt, ap2) + 1;
> > va_end(ap2);
> ```
> > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m.
> I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where
theyare calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically
aslong as errno is not overwritten - which it will now be. Thank you! 

I verified with v17, %m works now.

>
> >The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional?
> I could add a switch-case to print it out. Is that important? What do you think?

I personally prefer to keep those prefixes, which helps keep the log messages in a consistent style.

>
> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>

One comment on v17.
```
+    if (internal_log_file_fp != NULL)
+    {
+        if (fclose(internal_log_file_fp) != 0)
+            report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME);
+        internal_log_file_fp = NULL;
+    }
```

As the error is about internal_log_file_fp, meaning it may no longer able to write to internal_log_file_fp anymore, we
shouldavoid use report_createsub_fatal to log the error again. Maybe just a pg_fatal(), or just log to stderr. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/