Re: Printing backtrace of postgres processes

On Fri, Jan 29, 2021 at 7:10 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> 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?

1.
+ elog(LOG_SERVER_ONLY, "current backtrace:%s", errtrace.data);
+
+ errno = save_errno;

Can you add some comments that why we have chosen LOG_SERVER_ONLY?

2.
+pg_print_backtrace(PG_FUNCTION_ARGS)
+{
+ int bt_pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
+

The variable name bt_pid is a bit odd, can we just use pid?

3.
+Datum
+pg_print_backtrace(PG_FUNCTION_ARGS)
+{
+ int bt_pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
+
+#ifdef HAVE_BACKTRACE_SYMBOLS
+ {
+ bool result = false;
...
+
+ PG_RETURN_BOOL(result);
+ }
+#else
+ {
+ ereport(WARNING,
+ (errmsg("backtrace generation is not supported by this installation")));
+ PG_RETURN_BOOL(false);
+ }
+#endif

The result is just initialized to false and it is never updated?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com