MIME-Version: 1.0
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>
 <fa3bce4d-23d0-4d62-a7e0-1d4070cab693@app.fastmail.com>
 <CAA4eK1+jd6zH-LL8FOrw01chrNetRoZGKXSqV7GNo_--UXYc_w@mail.gmail.com>
 <CAEqnbaU50vLy031AbvmfXJ3_qv9iS4pVaMdmsnmpF87r=EqW7Q@mail.gmail.com>
 <CALDaNm1P5pZ2tTcaVRvjUAXLAyT7S18GPwp-4bc0xKXKZoXi0w@mail.gmail.com>
 <CAEqnbaWxYp7TbnhKfkjW8c=jRPARsQ1on86bt_pcqvK9vCtLhQ@mail.gmail.com>
 <CANhcyEWaCV7B54oOhuE_BhVCF887h23jKmkHDDWvJRKZ=pBbBQ@mail.gmail.com>
 <CAEqnbaVRQwPNYqXVJu3YmPiS0d-kG00H81fEBR8ZULP0GJrp7Q@mail.gmail.com>
 <CAA4eK1K1GLKiuFmcpcKJzhS-o2o2-aKEuaGWmHz-MfJhttaKFg@mail.gmail.com>
 <OS9PR01MB1214902CC518A8102333FA88DF54EA@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <OSOPR01MB1215385EAC6A76650444E40B0F54FA@OSOPR01MB12153.jpnprd01.prod.outlook.com>
 <CAEqnbaU=EgqhxEx0ig4TdY8pdt0Vn+vmCJBuoORRHOzovW9dWA@mail.gmail.com>
In-Reply-To: 
 <CAEqnbaU=EgqhxEx0ig4TdY8pdt0Vn+vmCJBuoORRHOzovW9dWA@mail.gmail.com>
From: Nisha Moond <nisha.moond412@gmail.com>
Date: Fri, 20 Mar 2026 16:00:03 +0530
Message-ID: 
 <CABdArM54_cVUMfE2DGSnJfDBZO1hmaVb82Z4b+1t0fp+xqXaMQ@mail.gmail.com>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
To: Gyan Sreejith <gyan.sreejith@gmail.com>
Cc: =?UTF-8?B?S3Vyb2RhLCBIYXlhdG8v6buS55SwIOmavOS6ug==?=
 <kuroda.hayato@fujitsu.com>,
	Amit Kapila <amit.kapila16@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>,
 Peter Smith <smithpb2250@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABdArM54_cVUMfE2DGSnJfDBZO1hmaVb82Z4b%2B1t0fp%2BxqXaMQ%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 19, 2026 at 11:59=E2=80=AFPM Gyan Sreejith <gyan.sreejith@gmail=
.com> wrote:
>
> Thank you, Kuroda-san, for all the work.
> I have made a small change - I added a pg_free() to free internal_log_fil=
e.
>>
>>

Hi Gyan,
I reviewed/tested the patches, please find below comments for v13-002 patch=
 -

File: pg_createsubscriber.c
1)
+
+   if ((internal_log_file_fp =3D logfile_open(internal_log_file, "a")) =3D=
=3D NULL)
+     pg_fatal("could not open log file \"%s\": %m", internal_log_file);
+

IIUC, the pg_fatal() call here seems unreachable as the function
logfile_open() itself calls pg_fatal() and exits if it fails to open
the file.
I think it should just be -
        internal_log_file_fp =3D logfile_open(internal_log_file, "a");

Please correct me if I'm missing something.
~~~

2)
+ if (opt->log_dir !=3D NULL)
+   out_file =3D psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp,
SERVER_LOG_FILE_NAME);
+ else
+   out_file =3D DEVNULL;
+
+ cmd_str =3D psprintf("\"%s\" -D \"%s\" >> \"%s\"", pg_resetwal_path,
+            subscriber_dir, out_file);

Similar to internal_log_file, why are 'out_file' and 'cmd_str' above not fr=
eed?
~~~

3) Typo - extra blank line after va_end(args);
@@ -205,10 +243,11 @@ pg_fatal(const char *pg_restrict fmt,...)
  va_start(args, fmt);

- pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args);
+ pg_createsub_log_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args);

  va_end(args);

+
  exit(1);
~~~

4) File: t/040_pg_createsubscriber.pl
+is( scalar(@server_log_files), 1, "
+    pg_createsubscriber_server.log file was created");
...
...
+is( scalar(@internal_log_files), 1, "
+    pg_createsubscriber_internal.log file was created");

Above introduces newlines in result log, test 29 and 32 looks like -

[14:46:59.071](0.639s) ok 28 - run pg_createsubscriber --dry-run on node S
[14:46:59.071](0.000s) ok 29 -
[14:46:59.071](0.000s) #     pg_createsubscriber_server.log file was create=
d
[14:46:59.071](0.000s) ok 30 - pg_createsubscriber_server.log file not empt=
y
[14:46:59.071](0.000s) ok 31 - server reached consistent recovery state
[14:46:59.072](0.000s) ok 32 -
[14:46:59.072](0.000s) #     pg_createsubscriber_internal.log file was crea=
ted
[14:46:59.072](0.000s) ok 33 - pg_createsubscriber_internal.log file not em=
pty

These should be single-line results similar to others.

--
Thanks,
Nisha