From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Printing backtrace of postgres processes |
Date: | 2021-01-28 14:10:36 |
Message-ID: | CALj2ACXfXefmdUWiF0twoYEfgJ6Xk4saXctoYyzXtzkjizCdBw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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?
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
From | Date | Subject | |
---|---|---|---|
Next Message | 'Alvaro Herrera' | 2021-01-28 14:13:53 | Re: libpq debug log |
Previous Message | Andrew Dunstan | 2021-01-28 14:05:02 | Re: multi-install PostgresNode |