On Tue, Mar 24, 2026 at 8:32 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > 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.
>
+1 to keep HINT, DETAIL kind of prefixes. I have one additional
comment in the latest patch:
+ mode_t oumask;
+
+ oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG
| S_IRWXO)));
+ fh = fopen(filename, mode);
+ umask(oumask);
I think we should follow the permissions model for this file same as
what pg_ctl does for log_file given with -l option. Basically, start
with with 077 permission and then switch to data_directory
permissions. This is because to start server we anyway use pg_ctl with
-l option which will use a specific way to assign permissions to
server log file, so we should use same for internal file, otherwise,
we will end up with different file permissions for internal and server
logfile.
--
With Regards,
Amit Kapila.
