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>
 <CAJpy0uAwPNG3GN5+jTajnccLe+F1mW6dZh-9112u3QQQYugA=Q@mail.gmail.com>
 <CAA4eK1JyYHPgaPTow5co8wBh8Ga+fx=vqHLhk2zHeqdOGYN4Vg@mail.gmail.com>
 <OS9PR01MB12149FECFFF25973C167F9CFAF54BA@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <CAEqnbaULSSQcir8Z8KDXduV4o4NZvJLeBMZN0VW3wh5QZjpEnw@mail.gmail.com>
 <EB06AE63-F29D-41EB-94F1-1B7F21C44A62@gmail.com>
In-Reply-To: <EB06AE63-F29D-41EB-94F1-1B7F21C44A62@gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 24 Mar 2026 12:01:44 +0530
Message-ID: 
 <CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ@mail.gmail.com>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
To: Chao Li <li.evan.chao@gmail.com>
Cc: Gyan Sreejith <gyan.sreejith@gmail.com>,
	=?UTF-8?B?S3Vyb2RhLCBIYXlhdG8v6buS55SwIOmavOS6ug==?=
 <kuroda.hayato@fujitsu.com>,
	shveta malik <shveta.malik@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/CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 24, 2026 at 8:32=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
> > On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> wrot=
e:
> >
> > On Mon, Mar 23, 2026 at 2:24=E2=80=AFAM Chao Li <li.evan.chao@gmail.com=
> wrote:
> > +       /* Build timestamp directory path */
> > +       len =3D snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, times=
tamp);
> > +
> > +       if (len >=3D MAXPGPATH)
> > +               pg_fatal("directory path for log files, %s/%s, is too l=
ong",
> > +                                logdir, timestamp);
> > ```
> > > In the pg_fatal call, I believe logdir should be log_basedir.
> > We are writing into logdir, and the if will be true only if it is too l=
ong. Hence we should be checking logdir.
>
> Not sure if I stated my comment clearly. The =E2=80=9Clogdir=E2=80=9D is =
build from (=E2=80=9C%s/%s, log_basedir, timestamp), however, in the pg_fat=
al, you are printing (=E2=80=9C%s/%s=E2=80=9D, logdir, timestamp), here log=
dir has included a truncated timestamp as it is too long, and so the fatal =
message will be =E2=80=9Clog_basedir/truncated-timestamp/timestamp=E2=80=9D=
. So the pg_fatal should be:
> ```
>         report_createsub_fatal("directory path for log files, %s/%s, is t=
oo long=E2=80=9D,
>                       log_basedir, timestamp);
> ```
>
> >
> > > The biggest problem I see with this patch is here. internal_log_file_=
write doesn=E2=80=99t handle =E2=80=9C%m=E2=80=9D. I think we can check the=
 implementation of pg_log_generic_v how %m is handled. The key code snippet=
 is:
> > ```
> > > errno =3D save_errno;
> >
> > > va_copy(ap2, ap);
> > > required_len =3D vsnprintf(NULL, 0, fmt, ap2) + 1;
> > > va_end(ap2);
> > ```
> > > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr =
to handle %m.
> > I have saved and restored errno similar to that. The code snippet you p=
ointed out is, as far as I understand, where they are calculating how much =
space to allocate (including the \0 at the end). I think it will be handled=
 automatically as long as errno is not overwritten - which it will now be. =
Thank you!
>
> I verified with v17, %m works now.
>
> >
> > >The other problem is, with internal_log_file_write, HINT, DETAIL prefi=
x are no longer printed, is that intentional?
> > I could add a switch-case to print it out. Is that important? What do y=
ou think?
>
> I personally prefer to keep those prefixes, which helps keep the log mess=
ages in a consistent style.
>

+1 to keep HINT, DETAIL kind of prefixes. I have one additional
comment in the latest patch:

+ mode_t oumask;
+
+ oumask =3D umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG
| S_IRWXO)));
+ fh =3D fopen(filename, mode);
+ umask(oumask);

I think we should follow the permissions model for this file same as
what pg_ctl does for log_file given with -l option. Basically, start
with with 077 permission and then switch to data_directory
permissions. This is because to start server we anyway use pg_ctl with
-l option which will use a specific way to assign permissions to
server log file, so we should use same for internal file, otherwise,
we will end up with different file permissions for internal and server
logfile.

--=20
With Regards,
Amit Kapila.