Re: Printing backtrace of postgres processes

Thanks Bharath for your review comments. Please find my comments inline below.

On Thu, Jan 28, 2021 at 7:40 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Jan 28, 2021 at 5:22 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > Thanks for the comments, I have fixed and attached an updated patch
> > with the fixes for the same.
> > Thoughts?
>
> Thanks for the patch. Here are few comments:
>
> 1) I think it's return SIGNAL_BACKEND_SUCCESS; instead of return 0; in
> check_valid_pid?
>

I did not want to use SIGNAL_BACKEND_SUCCESS as we have not yet
signalled the backend process at this time. I have added
BACKEND_VALIDATION_SUCCESS macro and used it here for better
readability.

> 2) How about following in pg_signal_backend for more readability
> + if (ret != SIGNAL_BACKEND_SUCCESS)
> + return ret;
> instead of
> + if (ret)
> + return ret;
>

Modified it to ret != BACKEND_VALIDATION_SUCCESS

> 3) How about validate_backend_pid or some better name instead of
> check_valid_pid?
>

Modified it to validate_backend_pid

> 4) How about following
> + errmsg("must be a superuser to print backtrace
> of backend process")));
> instead of
> + errmsg("must be a superuser to print backtrace
> of superuser query process")));
>

Here the message should include superuser, we cannot remove it. Non
super user can log non super user provided if user has permissions for
it.

> 5) How about following
> errmsg("must be a member of the role whose backed
> process's backtrace is being printed or member of
> pg_signal_backend")));
> instead of
> + errmsg("must be a member of the role whose
> backtrace is being logged or member of pg_signal_backend")));
>

Modified it.

> 6) I'm not sure whether "backtrace" or "call stack" is a generic term
> from the user/developer perspective. In the patch, the function name
> and documentation says callstack(I think it is "call stack" actually),
> but the error/warning messages says backtrace. IMHO, having
> "backtrace" everywhere in the patch, even the function name changed to
> pg_print_backtrace, looks better and consistent. Thoughts?
>

Modified it to pg_print_backtrace.

> 7) How about following in pg_print_callstack?
> {
> int bt_pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
> bool result = false;
>
> if (r == SIGNAL_BACKEND_SUCCESS)
> {
> if (EmitProcSignalPrintCallStack(bt_pid))
> result = true;
> else
> ereport(WARNING,
> (errmsg("failed to send signal to postmaster: %m")));
> }
>
> PG_RETURN_BOOL(result);
> }
>

Modified similarly with slight change.

> 8) How about following
> + (errmsg("backtrace generation is not supported by
> this PostgresSQL installation")));
> instead of
> + (errmsg("backtrace generation is not supported by
> this installation")));
>

I used the existing message to maintain consistency with
set_backtrace. I feel we can keep it the same.

> 9) Typo - it's "example" +2) Using "addr2line -e postgres address", For exmple:
>

Modified it.

> 10) How about
> + * Handle print backtrace signal
> instead of
> + * Handle receipt of an print backtrace.
>

I used the existing message to maintain consistency similar to
HandleProcSignalBarrierInterrupt. I feel we can keep it the same.

> 11) Isn't below in documentation specific to Linux platform. What
> happens if GDB is not there on the platform?
> +<programlisting>
> +1) "info line *address" from gdb on postgres executable. For example:
> +gdb ./postgres
> +GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-115.el7
>

I have made changes "You can get the file name and line number by
using gdb/addr2line in linux platforms, as a prerequisite users must
ensure gdb/addr2line is already installed".

User will get an error like this in windows:
select pg_print_backtrace(pg_backend_pid());
WARNING: backtrace generation is not supported by this installation
pg_print_callstack
--------------------
f
(1 row)

The backtrace will not be logged in case of windows, it will throw a
warning "backtrace generation is not supported by this installation"
Thoughts?

> 12) +The callstack will be logged in the log file. What happens if the
> server is started without a log file , ./pg_ctl -D data start? Where
> will the backtrace go?
>

Updated to: The backtrace will be logged to the log file if logging is
enabled, if logging is disabled backtrace will be logged to the
console where the postmaster was started.

> 13) Not sure, if it's an overkill, but how about pg_print_callstack
> returning a warning/notice along with true, which just says, "See
> <<<full log file name along with log directory>>>". Thoughts?

As you rightly pointed out it will be an overkill, I feel the existing
is easily understandable.

Attached v5 patch has the fixes for the same.
Thoughts?

Regards,
Vignesh