public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Gyan Sreejith <[email protected]>
Cc: "Kuroda, Hayato/黒田 隼人" <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Tue, 24 Mar 2026 11:02:12 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEqnbaULSSQcir8Z8KDXduV4o4NZvJLeBMZN0VW3wh5QZjpEnw@mail.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>
> On Mar 24, 2026, at 09:23, Gyan Sreejith <[email protected]> wrote:
>
> On Mon, Mar 23, 2026 at 2:24 AM Chao Li <[email protected]> wrote:
> + /* Build timestamp directory path */
> + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
> +
> + if (len >= 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 “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the pg_fatal, you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too long, and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be:
```
report_createsub_fatal("directory path for log files, %s/%s, is too long”,
log_basedir, timestamp);
```
>
> > The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is:
> ```
> > errno = save_errno;
>
> > va_copy(ap2, ap);
> > required_len = 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.
>
> >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.
>
> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch>
One comment on v17.
```
+ if (internal_log_file_fp != NULL)
+ {
+ if (fclose(internal_log_file_fp) != 0)
+ report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME);
+ internal_log_file_fp = 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/
view thread (65+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox