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.
> + * CheckPostgresProcessId -- check if the process with given pid is a
> backend
> + * or an auxiliary process.
> + *
> +
> + */
Isn't the 4th line needless?
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.
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
