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
signature.asc
Description: PGP signature