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

> On Mar 22, 2026, at 07:09, Gyan Sreejith <gyan.sreejith@gmail.com> wrote:
>
>
> On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> 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
theimplementation 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/