MIME-Version: 1.0
References: 
 <OSOPR01MB1215385EAC6A76650444E40B0F54FA@OSOPR01MB12153.jpnprd01.prod.outlook.com>
 <CAEqnbaU=EgqhxEx0ig4TdY8pdt0Vn+vmCJBuoORRHOzovW9dWA@mail.gmail.com>
 <TYRPR01MB12156F431E4B83ECFF14034BDF54CA@TYRPR01MB12156.jpnprd01.prod.outlook.com>
 <CAEqnbaXb-TKUm7P-=_zrgQ=shRXkkZscPOHEL9OS6Cb2V8YT8Q@mail.gmail.com>
 <CAA4eK1JiAzaY7UYnya7uDhX-kAL1PrsOAxBtys7c35t4q4H3_A@mail.gmail.com>
 <CAEqnbaVF0yXQk=VVzr-8V23E=iUpUqyah6hxx2fy0ZVJh+CaGA@mail.gmail.com>
 <CAJpy0uAwPNG3GN5+jTajnccLe+F1mW6dZh-9112u3QQQYugA=Q@mail.gmail.com>
 <CAA4eK1JyYHPgaPTow5co8wBh8Ga+fx=vqHLhk2zHeqdOGYN4Vg@mail.gmail.com>
 <OS9PR01MB12149FECFFF25973C167F9CFAF54BA@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <CAEqnbaULSSQcir8Z8KDXduV4o4NZvJLeBMZN0VW3wh5QZjpEnw@mail.gmail.com>
 <OS9PR01MB1214926DB01BF19C09368188EF548A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS9PR01MB1214926DB01BF19C09368188EF548A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Wed, 25 Mar 2026 10:14:08 +1100
Message-ID: 
 <CAHut+PsmdpYZnXKLYJ8n07-ftaPAbvxH9xuNrvm2c6zxS09z7Q@mail.gmail.com>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Gyan Sreejith <gyan.sreejith@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	shveta malik <shveta.malik@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>,
	vignesh C <vignesh21@gmail.com>, Euler Taveira <euler@eulerto.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPsmdpYZnXKLYJ8n07-ftaPAbvxH9xuNrvm2c6zxS09z7Q%40mail.gmail.com>
Precedence: bulk

Hi. Here are some review comments for v18-0001.

======

1.
+#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server"
+#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal"

Those are not the names of logfiles; they are just the first part of
the names. Why not also put the ".log" part here, instead of burying
it in format strings scattered in the code?

SUGGESTION (e.g., more similar to pg_upgrade.h)
#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server.log"
#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal.log"

~~~

2.
+static char logdir[MAXPGPATH]; /* Directory log files are put (if specified) */

Missing words in that comment?

e.g.
Directory where log files are written ...
Directory for log files ...

It might also be useful to mention that this is more than just the
original user-specified --logdir directory; it also includes the
timestamp sub-dir.

~~~

3.
 /*
- * Report a message with a given log level
+ * Report a message with a given log level to stderr and log file
+ * (if specified).
  */
+static void
+report_createsub_log_v(enum pg_log_level level, enum pg_log_part part,
+    const char *pg_restrict fmt, va_list args)

This is a function comment, but there is no way to specify the log
file by using this function, so maybe reword that "if specified" part
for clarity.

SUGGESTION:
* Report a message with a given log level.
* Writes to stderr, and also to the log file (if pg_createsubscriber
--logdir option was specified).

~~~

4.
+ /* Build timestamp directory path */
+ len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
+
+ if (len >= MAXPGPATH)
+ report_createsub_fatal("directory path for log files, %s/%s, is too long",
+    logdir, timestamp);

IIUC, when the snprintf exceeds MAXPGPATH, then 'logdir' is going to
contain a truncated string. And, maybe that already includes a partial
truncated timestamp part:
"myreallylongname/20260"

AFAICT, the subsequent fatal message is going to report that
incorrectly when it appends the timestamp a second time, like:
"... myreallylongname/20260/20260119T204317.204 is too long"

Notice that pg_upgrade doesn't try to report the names of files that
are too long. It just says things like "directory path for new cluster
is too long".

======
Kind Regards,
Peter Smith.
Fujitsu Australia