On Fri, Jan 26, 2024 at 11:58:00PM +0530, Bharath Rupireddy wrote:
> You probably were looking at v21, the above change isn't there in
> versions after that. Can you please review the latest version v26
> attached here?
>
> We might want this patch extended to the newly added walsummarizer
> process which I'll do so in the next version.
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
+bool
+SendProcSignalBackendOrAuxproc(int pid, ProcSignalReason reason)
+{
Looking at 0001. This API is a thin wrapper of SendProcSignal(), that
just checks that we have actually a process before using it.
Shouldn't it be in procsignal.c.
Now looking at 0002, this new routine is used in one place. Seeing
that we have something similar in pgstatfuncs.c, wouldn't it be
better, instead of englobing SendProcSignal(), to have one routine
that's able to return a PID for a PostgreSQL process?
All the backtrace-related handling is stored in procsignal.c, could it
be cleaner to move the internals into a separate, new file, like
procbacktrace.c or something similar?
LoadBacktraceFunctions() is one more thing we need to set up in all
auxiliary processes. That's a bit sad to require that in all these
places, and we may forget to add it. Could we put more efforts in
centralizing these initializations? The auxiliary processes are one
portion of the problem, and getting stack traces for backend processes
would be useful on its own. Another suggestion that I'd propose to
simplify the patch would be to focus solely on backends for now, and
do something for auxiliary process later on. If you do that, the
strange layer with BackendOrAuxproc() is not a problem anymore, as it
would be left out for now.
+<screen>
+logging current backtrace of process with PID 3499242:
+postgres: ubuntu postgres [local] INSERT(+0x61a355)[0x5559b94de355]
+postgres: ubuntu postgres [local] INSERT(procsignal_sigusr1_handler+0x9e)[0x5559b94de4ef]
This is IMO too much details for the documentation, and could be
confusing depending on how the code evolves in the future. I'd
suggest to keep it minimal, cutting that to a few lines. I don't see
a need to mention ubuntu, either.
--
Michael
