Re: Printing backtrace of postgres processes

On Thu, Jan 28, 2021 at 5:22 PM vignesh C <vignesh21@gmail.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?

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

3) How about validate_backend_pid or some better name instead of
check_valid_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")));

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")));

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?

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);
}

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")));

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

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

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

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?

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?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com