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: <EB06AE63-F29D-41EB-94F1-1B7F21C44A62@gmail.com>
Date: Tue, 24 Mar 2026 11:31:02 +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: <F9553B89-A242-44E7-984C-710F4F3E91E9@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>
 <EB06AE63-F29D-41EB-94F1-1B7F21C44A62@gmail.com>
To: Gyan Sreejith <gyan.sreejith@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/F9553B89-A242-44E7-984C-710F4F3E91E9%40gmail.com>
Precedence: bulk


> On Mar 24, 2026, at 11:02, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
>=20
>> 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.
>=20
> 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
>>=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!
>=20
> I verified with v17, %m works now.
>=20
>>=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?
>=20
> I personally prefer to keep those prefixes, which helps keep the log =
messages in a consistent style.
>=20
>>=20
>> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>
>=20
> 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;
> + }
> ```
>=20
> 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.
>=20

I forgot to mention one small suggestion.

Since the internal log file path includes a timestamped directory, it is =
a bit inconvenient to figure out the full path of the file after the =
command finishes. I think it would be helpful to print the full path of =
the internal log file at the end, something like:
```
	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;
		printf("check the internal log file at \"%s/%s.log\"", =
logdir, INTERNAL_LOG_FILE_NAME);
	}
```

Note that I used printf() here, because based on my test, pg_log_info() =
does not work in this place.

The other thing I noticed while trying the above code is that =
cleanup_objects_atexit() has this:
```
    if (success)
        return;
```

That means on the success path, internal_log_file_fp will not be closed. =
So this part would need some adjustment as well. Maybe one way is to =
jump to the log-file cleanup part before returning, for example with a =
goto, so that internal_log_file_fp is always closed.

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