> On Mar 22, 2026, at 07:09, Gyan Sreejith <[email protected]> wrote:
>
>
> On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <[email protected]> wrote:
>
> Based on the above information, can we consider renaming the above functions
> to report_createsub_log() and report_createsub_fatal()?
>
> Other than the above point, 0001 LGTM.
>
> I have renamed the functions.
>
> Regards,
> Gyan
> <v16-0001-pg_createsubscriber-use-own-reporting-functions.patch><v16-0002-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>
0001 looks good.
Some comments on 0002:
1
```
+ <listitem>
+ <para>
+ <literal>pg_createsubscriber_server.log</literal> which captures logs
+ related to stopping and starting the standby server,
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <literal>pg_createsubscriber_internal.log</literal> which captures
+ internal diagnostic output (validations, checks, etc.)
+ </para>
+ </listitem>
```
I think we can use tag <filename> for the two file names rather than <literal>.
2
```
+static void
+report_createsub_log_v(enum pg_log_level level, enum pg_log_part part,
+ const char *pg_restrict fmt, va_list
args)
+{
+ if (internal_log_file_fp != NULL)
+ {
+ /* Output to both stderr and the log file */
+ va_list arg_cpy;
+
+ va_copy(arg_cpy, args);
+ pg_log_generic_v(level, part, fmt, arg_cpy);
+ va_end(arg_cpy);
+
+ internal_log_file_write(level, fmt, args);
+ }
+ else
+ pg_log_generic_v(level, part, fmt, args);
+}
```
I think this function can be simplified a little bit as:
```
static void
report_createsub_log_v(enum pg_log_level level, enum pg_log_part part,
const char *pg_restrict fmt, va_list
args)
{
if (internal_log_file_fp != NULL)
{
va_list arg_cpy;
va_copy(arg_cpy, args);
internal_log_file_write(level, fmt, arg_cpy);
va_end(arg_cpy);
}
pg_log_generic_v(level, part, fmt, args);
}
```
A few lines shorter, and avoid duplicating of pg_log_generic_v.
3
```
+ if (internal_log_file_fp != NULL)
+ {
+ fclose(internal_log_file_fp);
```
We should check return value of fclose(), see
69c57466a7521ee146cfdde766713181d45a2d36.
4
```
+ /* 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.
5
```
+ if (internal_log_file_fp != NULL)
+ fclose(internal_log_file_fp);
+
return 0;
}
```
In the end of main(), we don’t need to close internal_log_file_fp again,
because that has been done in cleanup_objects_atexit().
6
```
+static void
+internal_log_file_write(enum pg_log_level level, const char *pg_restrict fmt,
+ va_list args)
+{
+ Assert(internal_log_file_fp);
+
+ /* Do nothing if log level is too low. */
+ if (level < __pg_log_level)
+ return;
+
+ vfprintf(internal_log_file_fp, _(fmt), args);
+
+ fprintf(internal_log_file_fp, "\n");
+ fflush(internal_log_file_fp);
+}
```
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.
The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no
longer printed, is that intentional?
7 I think the test script misses a test case for %m.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
