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>
In-Reply-To: 
 <CAEqnbaU50vLy031AbvmfXJ3_qv9iS4pVaMdmsnmpF87r=EqW7Q@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Wed, 11 Mar 2026 15:34:55 +0530
Message-ID: 
 <CALDaNm1P5pZ2tTcaVRvjUAXLAyT7S18GPwp-4bc0xKXKZoXi0w@mail.gmail.com>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
To: Gyan Sreejith <gyan.sreejith@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Euler Taveira <euler@eulerto.com>,
	"kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.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/CALDaNm1P5pZ2tTcaVRvjUAXLAyT7S18GPwp-4bc0xKXKZoXi0w%40mail.gmail.com>
Precedence: bulk

On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan.sreejith@gmail.com> wrote=
:
>
> On Thu, Mar 5, 2026 at 9:49=E2=80=AFAM Euler Taveira <euler@eulerto.com> =
wrote:
>
>> Don't duplicate code. If you are reusing a function, my advice is to mov=
e it to
>> src/common. You can always use "ifdef FRONTEND" to use the appropriate l=
og
>> message (elog/ereport vs pg_error, for example).
>
>
> I have made all the changes except for this one, and I am deferring to Am=
it Kapila regarding the marks.

Few comments:
1) You are not checking log level because of which the contents are
logged irrespective of the log level:
+#undef pg_log_info
+#define pg_log_info(...) do{\
+       if (internal_log_file_fp !=3D NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\
+} while(0)
+
+#undef pg_log_info_hint
+#define pg_log_info_hint(...) do{\
+       if (internal_log_file_fp !=3D NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\
+} while(0)
+
+#undef pg_log_debug
+#define pg_log_debug(...) do{\
+       if (internal_log_file_fp !=3D NULL) \
+               internal_log_file_write(__VA_ARGS__); \
+       else \
+               if (unlikely(__pg_log_level <=3D PG_LOG_DEBUG)) \
+                       pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY,
__VA_ARGS__); \
+} while(0)

2) Instead of just checking if the file is created or not, let's check
for some contents from the file:
+# Check that the log files were created
+my @server_log_files =3D glob "$logdir/*/pg_createsubscriber_server.log";
+is( scalar(@server_log_files), 1, "
+    pg_createsubscriber_server.log file was created");
+my @internal_log_files =3D glob "$logdir/*/pg_createsubscriber_internal.lo=
g";
+is( scalar(@internal_log_files), 1, "
+    pg_createsubscriber_internal.log file was created");

3) This change is not required, let's remove this:
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber');
 program_version_ok('pg_createsubscriber');
 program_options_handling_ok('pg_createsubscriber');

-my $datadir =3D PostgreSQL::Test::Utils::tempdir;
+my $datadir =3D PostgreSQL::Test::Utils::tempdir + "/datadir";

4) No need of '{' as it is a single line statement
if (opt->log_dir !=3D NULL)
{
appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir,
log_timestamp, SERVER_LOG_FILE_NAME);
}

Regards,
Vignesh