Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAEqnbaULSSQcir8Z8KDXduV4o4NZvJLeBMZN0VW3wh5QZjpEnw@mail.gmail.com>
Date: Tue, 24 Mar 2026 11:02:12 +0800
Cc: =?utf-8?B?Ikt1cm9kYSwgSGF5YXRvL+m7kueUsCDpmrzkuroi?=
 <kuroda.hayato@fujitsu.com>,
 Amit Kapila <amit.kapila16@gmail.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-Transfer-Encoding: quoted-printable
Message-Id: <EB06AE63-F29D-41EB-94F1-1B7F21C44A62@gmail.com>
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>
To: Gyan Sreejith <gyan.sreejith@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/EB06AE63-F29D-41EB-94F1-1B7F21C44A62%40gmail.com>
Precedence: bulk


> On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> =
wrote:
>=20
> 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, =
timestamp);
> +
> +       if (len >=3D MAXPGPATH)
> +               pg_fatal("directory path for log files, %s/%s, is too =
long",
> +                                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 =
long. 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_fatal, you are printing (=E2=80=9C%s/%s=E2=80=9D, logdir, timestamp), =
here logdir 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 =
too long=E2=80=9D,
                      log_basedir, timestamp);
```

>=20
> > 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;
>=20
> > 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 =
pointed 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.

>=20
> >The other problem is, with internal_log_file_write, HINT, DETAIL =
prefix are no longer printed, is that intentional?
> I could add a switch-case to print it out. Is that important? What do =
you think?

I personally prefer to keep those prefixes, which helps keep the log =
messages in a consistent style.

>=20
> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>

One comment on v17.
```
+	if (internal_log_file_fp !=3D NULL)
+	{
+		if (fclose(internal_log_file_fp) !=3D 0)
+			report_createsub_fatal("could not close =
%s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME);
+		internal_log_file_fp =3D NULL;
+	}
```

As the error is about internal_log_file_fp, meaning it may no longer =
able to write to internal_log_file_fp anymore, we should avoid use =
report_createsub_fatal to log the error again. Maybe just a pg_fatal(), =
or just log to stderr.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/