public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Kapila <[email protected]>
To: Chao Li <[email protected]>
Cc: Gyan Sreejith <[email protected]>
Cc: Kuroda, Hayato/黒田 隼人 <[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 12:01:44 +0530
Message-ID: <CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
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>
<[email protected]>
On Tue, Mar 24, 2026 at 8:32 AM Chao Li <[email protected]> wrote:
>
> > 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.
>
+1 to keep HINT, DETAIL kind of prefixes. I have one additional
comment in the latest patch:
+ mode_t oumask;
+
+ oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG
| S_IRWXO)));
+ fh = 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.
--
With Regards,
Amit Kapila.
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: <CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ@mail.gmail.com>
* 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