On Mon, Jan 24, 2022 at 1:05 PM torikoshia <torikoshia@oss.nttdata.com> wrote:
>
> On 2022-01-14 19:48, Bharath Rupireddy wrote:
> > On Sat, Nov 20, 2021 at 11:50 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> On Fri, Nov 19, 2021 at 4:07 PM vignesh C <vignesh21@gmail.com> wrote:
> >> > The Attached v15 patch has the fixes for the same.
> >>
> >> Thanks. The v15 patch LGTM and the cf bot is happy hence marking it as
> >> RfC.
> >
> > The patch was not applying because of the recent commit [1]. I took
> > this opportunity and tried a bunch of things without modifying the
> > core logic of the pg_log_backtrace feature that Vignesh has worked on.
> >
> > I did following - moved the duplicate code to a new function
> > CheckPostgresProcessId which can be used by pg_log_backtrace,
> > pg_log_memory_contexts, pg_signal_backend and pg_log_query_plan ([2]),
>
> Thanks for refactoring!
> I'm going to use it for pg_log_query_plan after this patch is merged.
>
> > modified the code comments, docs and tests to be more in sync with the
> > commit [1], moved two of ProcessLogBacktraceInterrupt calls (archiver
> > and wal writer) to their respective interrupt handlers. Here's the v16
> > version that I've come up with.
>
> I have some minor comments.
>
> > +</screen>
> > + You can get the file name and line number from the logged details
> > by using
> > + gdb/addr2line in linux platforms (users must ensure gdb/addr2line
> > is
> > + already installed).
> > +<programlisting>
> > +1) "info line *address" from gdb on postgres executable. For example:
> > +gdb ./postgres
> > +(gdb) info line *0x71c25d
> > +Line 378 of "execMain.c" starts at address 0x71c25d
> > <literal><</literal>standard_ExecutorRun+470<literal>></literal>
> > and ends at 0x71c263
> > <literal><</literal>standard_ExecutorRun+476<literal>></literal>.
> > +OR
> > +2) Using "addr2line -e postgres address", For example:
> > +addr2line -e ./postgres 0x71c25d
> > +/home/postgresdba/src/backend/executor/execMain.c:378
> > +</programlisting>
> > + </para>
> > +
>
> Isn't it better to remove line 1) and 2) from <programlisting>?
> I just glanced at the existing sgml, but <programlisting> seems to
> contain only codes.
Modified
> > + * CheckPostgresProcessId -- check if the process with given pid is a
> > backend
> > + * or an auxiliary process.
> > + *
> > +
> > + */
>
> Isn't the 4th line needless?
Modified
> BTW, when I saw the name of this function, I thought it just checks if
> the specified pid is PostgreSQL process or not.
> Since it returns the pointer to the PGPROC or BackendId of the PID, it
> might be kind to write comments about it.
Modified
Thanks for the comments, attached v17 patch has the fix for the same.
Regards,
Vignesh