Re: [Proposal] Adding Log File Capability to pg_createsubscriber

> 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 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/