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>
In-Reply-To: 
 <CAEqnbaVF0yXQk=VVzr-8V23E=iUpUqyah6hxx2fy0ZVJh+CaGA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Mon, 23 Mar 2026 15:25:04 +0530
Message-ID: 
 <CAJpy0uCrGenynPf_0fdhh+M_wACc3ktzD+GhjZ4fYk5k1VUeKg@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>,
	=?UTF-8?B?S3Vyb2RhLCBIYXlhdG8v6buS55SwIOmavOS6ug==?=
 <kuroda.hayato@fujitsu.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>,
	shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uCrGenynPf_0fdhh%2BM_wACc3ktzD%2BGhjZ4fYk5k1VUeKg%40mail.gmail.com>
Precedence: bulk

On Sun, Mar 22, 2026 at 4:39=E2=80=AFAM Gyan Sreejith <gyan.sreejith@gmail.=
com> wrote:
>
>
> On Sat, Mar 21, 2026 at 5:57=E2=80=AFAM Amit Kapila <amit.kapila16@gmail.=
com> wrote:
>>
>>
>> Based on the above information, can we consider renaming the above
>>
>> functions to report_createsub_log() and report_createsub_fatal()?
>>
>> Other than the above point, 0001 LGTM.
>
>
> I have renamed the functions.
>

Thanks. Few comments on 002:

1)

We can get rid of below alignment related changes in unrelated test parts.

----
-$node_p->safe_psql($db1, qq(
+$node_p->safe_psql(
+ $db1, qq(

-is($result, qq(),
- "table is not replicated in database $db1");
+is($result, qq(), "table is not replicated in database $db1");

-is($node_p->safe_psql($db2, "SELECT COUNT(*) FROM pg_publication
WHERE pubname =3D 'pub2'"),
- '1', "publication pub2 was created in $db2");
+is( $node_p->safe_psql(
+ $db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname =3D 'pub2'"),
+ '1',
+ "publication pub2 was created in $db2");

-is($result, qq($db1|{test_pub3}
+is( $result, qq($db1|{test_pub3}
 ----


2)
Can we simplify the logic of report_createsub_log_v to:
---
        if (internal_log_file_fp !=3D NULL)
        {
                va_list         arg_cpy;

                va_copy(arg_cpy, args);
                internal_log_file_write(level, fmt, arg_cpy);
                va_end(arg_cpy);
        }
        pg_log_generic_v(level, part, fmt, args);
---

We need not to invoke pg_log_generic_v in both if and else.


3)
+ /* Create base directory (ignore if exists) */
+ if (mkdir(log_basedir, S_IRWXU) < 0 && errno !=3D EEXIST)
+ pg_fatal("could not create directory \"%s\": %m", log_basedir);
+
+ /* Create BASE_DIR/$timestamp */
+ if (mkdir(logdir, S_IRWXU) < 0)
+ pg_fatal("could not create directory \"%s\": %m", logdir);
--

Instead of S_IRWXU directly, shall we use pg_dir_create_mode (which
means S_IRWXU) similar to other modules (pg_upgrade, initdb, pg_dump
etc)

See pg_upgrade:
        if (mkdir(log_opts.logdir, pg_dir_create_mode) < 0)
                pg_fatal("could not create directory \"%s\": %m",
log_opts.logdir);

4)
+ /* Create BASE_DIR/$timestamp */

Above comment refers to BASE_DIR which looks like some variable, but
it is not. Can we please change this comment to:
/* Create a timestamp-named subdirectory under the base directory */

5)
+ /* append milliseconds */
append -->Append

thanks
Shveta