> On Mar 24, 2026, at 11:02, Chao Li <[email protected]> wrote:
>
>
>
>> On Mar 24, 2026, at 09:23, Gyan Sreejith <[email protected]> wrote:
>>
>> On Mon, Mar 23, 2026 at 2:24 AM Chao Li <[email protected]> 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 check the 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 they are calculating how much
>> space to allocate (including the \0 at the end). I think it will be handled
>> automatically as long 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 should avoid use
> report_createsub_fatal to log the error again. Maybe just a pg_fatal(), or
> just log to stderr.
>
I forgot to mention one small suggestion.
Since the internal log file path includes a timestamped directory, it is a bit
inconvenient to figure out the full path of the file after the command
finishes. I think it would be helpful to print the full path of the internal
log file at the end, something like:
```
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;
printf("check the internal log file at \"%s/%s.log\"", logdir,
INTERNAL_LOG_FILE_NAME);
}
```
Note that I used printf() here, because based on my test, pg_log_info() does
not work in this place.
The other thing I noticed while trying the above code is that
cleanup_objects_atexit() has this:
```
if (success)
return;
```
That means on the success path, internal_log_file_fp will not be closed. So
this part would need some adjustment as well. Maybe one way is to jump to the
log-file cleanup part before returning, for example with a goto, so that
internal_log_file_fp is always closed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
