On Tue, Feb 6, 2024 at 4:21 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> --- 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?
I liked the idea of going ahead with logging backtraces for only
backends for now, so a separate wrapper like this isn't needed.
> 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?
+1. Moved all the code to a new file.
> 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?
If we were to do it for only backends (including bg workers)
InitProcess() is the better place. If we were to do it for both
backends and auxiliary processes, BaseInit() is best.
> 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.
+1 to keep it simple for now; that is, log backtraces of only backends
leaving auxiliary processes aside.
> +<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.
Well, that 'ubuntu' is the default username there, I've changed it now
and kept the output short.
I've simplified the tests, now we don't need two separate output files
for tests. Please see the attached v27 patch.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
