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