public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Gyan Sreejith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Wed, 25 Mar 2026 10:14:08 +1100
Message-ID: <CAHut+PsmdpYZnXKLYJ8n07-ftaPAbvxH9xuNrvm2c6zxS09z7Q@mail.gmail.com> (raw)
In-Reply-To: <OS9PR01MB1214926DB01BF19C09368188EF548A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
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>
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
view thread (65+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
In-Reply-To: <CAHut+PsmdpYZnXKLYJ8n07-ftaPAbvxH9xuNrvm2c6zxS09z7Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox