Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Thu, 05 Mar 2026 11:48:57 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Gyan Sreejith" <gyan.sreejith@gmail.com>,
 "vignesh C" <vignesh21@gmail.com>
Cc: "Amit Kapila" <amit.kapila16@gmail.com>,
 "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>,
 "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 "Peter Smith" <smithpb2250@gmail.com>
Message-Id: <fa3bce4d-23d0-4d62-a7e0-1d4070cab693@app.fastmail.com>
In-Reply-To: 
 <CAEqnbaWjoSby+_FQOKqTiDwf9AsWVjcGzRn-BQtdivC8xn0ADw@mail.gmail.com>
References: 
 <CAEqnbaUthOQARV1dscGvB_EsqC-YfxiM6rWkVDHc+G+f4oSUHw@mail.gmail.com>
 <CAHut+PvizcpeHA1Twf_hwe=wANQ1LV5zY6_q+39gTFJc7+bCKg@mail.gmail.com>
 <CAEqnbaVNZYbB_YufchM49d=XC1ZGrVV8ikCPmGotWoCZASY3Uw@mail.gmail.com>
 <CAEqnbaV8QMbXZtt25QtGUPAaQZnD-B0HniFSroapMh+QTZgKsQ@mail.gmail.com>
 <OSCPR01MB14966FD0961F512B29BD46D6BF5AAA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CALDaNm0gX2D6fD5yur-R5gagA7+AfLmLZU5Z8+tgjo61b-Y01w@mail.gmail.com>
 <48261e54-c4f1-4bde-a4a9-4f3698a6b380@app.fastmail.com>
 <CAA4eK1+2PYU-y2weY_zFeysP8Ux45iBNdaYMZuJtMTqXwCxawQ@mail.gmail.com>
 <CAEqnbaVVp_g1m1nhOBZjtiAz5W-xwPQCmWH4hjYYehA+ktYg9Q@mail.gmail.com>
 <CALDaNm2T__Uha1fn274bP3jKDSwm37ewWYvA+vOo75FTT2t3SA@mail.gmail.com>
 <CALDaNm1fF6yBa2PCv9gO_MVcjKLQFgi9eAL=g5cU=xtbOr=qZw@mail.gmail.com>
 <CAEqnbaUa27R9OcVDHkNuwsZ1uCpy8sTXpH78N1ug0kj60mnchw@mail.gmail.com>
 <CALDaNm1Mj+eNVETmYAYd4ojrbTbQU5iCX20Fso24r2VuXF69AQ@mail.gmail.com>
 <CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>
 <CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com>
 <CAEqnbaVgFU2Pr=xhhDmA=sK7XPBDBxECovqbSht91ZbHmnteUg@mail.gmail.com>
 <CALDaNm17ujrJ2xHyz6r81WAiFUs38RcT8D5ebdxssASpGko0HA@mail.gmail.com>
 <CAEqnbaUbVgGMXBNFbk=sqFJT=_TkAUYGYsFHJKFw5K492M_B=A@mail.gmail.com>
 <CALDaNm2SYq_dO7qXHn+BnxmxvmBupH6tofUnx4DKC09h5xfAyQ@mail.gmail.com>
 <CAEqnbaWjoSby+_FQOKqTiDwf9AsWVjcGzRn-BQtdivC8xn0ADw@mail.gmail.com>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/fa3bce4d-23d0-4d62-a7e0-1d4070cab693%40app.fastmail.com>
Precedence: bulk

On Tue, Feb 24, 2026, at 8:55 PM, Gyan Sreejith wrote:
> I have made the changes you suggested and have attached the patch below. 
>

[Avoid top-posting ...]

I took another look at this patch. Comments are below:

+/*
+ * Open a new logfile with proper permissions.
+ * From src/backend/postmaster/syslogger.c
+ */
+static FILE *
+logfile_open(const char *filename, const char *mode)
+{

Don't duplicate code. If you are reusing a function, my advice is to move it to
src/common. You can always use "ifdef FRONTEND" to use the appropriate log
message (elog/ereport vs pg_error, for example).

-	 * XXX this code was extracted from BootStrapXLOG().
+	 * XXX this code was extracted from BootStrapXpg_log_info().

This is a typo.

+			case 'l':
+				{
+					char		timestamp[128];
+					struct timeval tval;
+					time_t		now;
+					struct tm	tmbuf;

I would expect to have this code in a function. That's the pattern it is using.

+					strftime(timestamp, sizeof(timestamp), "%Y-%m-%d-%H-%M-%S", localtime_r(&now, &tmbuf));
+					/* append microseconds */
+					snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp),
+							 ".%06u", (unsigned int) (tval.tv_usec));
+					log_timestamp = pg_strdup(timestamp);

Do we really need microseconds? I would say milliseconds is sufficient. I
suggest to remove the dash (-); it is using a different style from existing
code (pg_upgrade).

+					opt.log_dir = pg_strdup(optarg);
+					canonicalize_path(opt.log_dir);

You didn't have a check for long path. It is not a good idea in general. See
MAXPGPATH examples. It would be a good idea if the path are restricted to
MAXPGPATH length.

+						if (errno == ENOENT)
+						{
+							if (mkdir(opt.log_dir, S_IRWXU) == 0)
+								pg_log_info("log directory created");
+							else if (errno == EACCES)
+								pg_fatal("permission denied trying to create log directory \"%s\": %m", opt.log_dir);
+							else
+								pg_fatal("could not create log directory \"%s\": %m", opt.log_dir);
+						}
+						else if (errno == EACCES)
+							pg_fatal("permission denied trying to access directory \"%s\": %m", opt.log_dir);
+						else
+							pg_fatal("could not access directory \"%s\": %m", opt.log_dir);

The "permission denied" is redundant here because it will be in %m. Instead, I
suggest that you use

  could not create directory \"%s\": %m

The main advantage is that this sentence is already available. It avoids
translation effort.

+#undef pg_log_info
+#define pg_log_info(...) do{\
+	if (internal_log_file_fp != NULL) \
+		internal_log_file_write(__VA_ARGS__); \
+	else \
+		pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\
+} while(0)

I don't like the fact that internal_log_file_fp is not declared before this
#define.

One of the arguments to have this feature was that pg_createsubscriber mixes the
server and tool messages. Couldn't we fix it adding "marks" on the output saying
the server log messages starts here and the server log messages ends here?


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/