Re: Printing backtrace of postgres processes

On Fri, Apr 15, 2022 at 11:49 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Thu, 14 Apr 2022 10:33:50 +0530, vignesh C <vignesh21(at)gmail(dot)com> wrote in
> > On Wed, Apr 6, 2022 at 12:29 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Apr 5, 2022 at 9:18 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > > This looks like a grotty hack.
> > >
> > > I have changed it so that the backtrace is set and returned to the
> > > caller. The caller will take care of logging or setting it to the
> > > error data context. Thoughts?
>
> The point here I think is that the StringInfo is useless here in the
> first place. Addition to that, it is outside the use of StringInfo
> hammering in a string pointer into it.

Modified

> By the way, as Andres suggested very early in this thread, backrace()
> can be called from signal handlers with some preparation. So we can
> run backtrace() in HandleLogBacktraceInterrupt() and backtrace_symbols
> in ProceeLogBacktraceInterrupt(). This make this a bit complex, but
> the outcome should be more informative.

Using that approach we could only send the trace to stderr, when we
are trying to log it to the logs it might result in deadlock as the
signal handler can be called in between malloc lock. We dropped that
approach to avoid issues pointed in [1].

> > > > Incidentally changing the behavior of pg_signal_backend() doesn't seem
> > > > like a great idea. We can do that as a separate commit, after
> > > > considering whether documentation changes are needed. But it's not
> > > > something that should get folded into a commit on another topic.
> > >
> > > Agreed. I have kept the logic of pg_signal_backend as it is.
> > > pg_log_backtrace and pg_log_backend_memory_contexts use the same
> > > functionality to check and send signal. I have added a new function
> > > "CheckProcSendDebugSignal" which has the common code implementation
> > > and will be called by both pg_log_backtrace and
> > > pg_log_backend_memory_contexts.
>
> The name looks like too specific than what it actually does, and
> rather doesn't manifest what it does.
> SendProcSignalBackendOrAuxproc() might be more eescriptive.
>
> Or we can provide BackendOrAuxiliaryPidGetProc(int pid, BackendId &backendid).

Modified it to SendProcSignalBackendOrAuxproc

> > > > + /*
> > > > + * BackendPidGetProc() and AuxiliaryPidGetProc() return NULL if the pid
> > > > + * isn't valid; but by the time we reach kill(), a process for which we
> > > > + * get a valid proc here might have terminated on its own. There's no way
> > > > + * to acquire a lock on an arbitrary process to prevent that. But since
> > > > + * this mechanism is usually used to debug a backend or an auxiliary
> > > > + * process running and consuming lots of memory, that it might end on its
> > > > + * own first without logging the requested info is not a problem.
> > > > + */
> > > >
> > > > This comment made a lot more sense where it used to be than it does
> > > > where it is now. I think more work is required here than just cutting
> > > > and pasting.
> > >
> > > This function was called from pg_signal_backend earlier, this logic is
> > > now moved to CheckProcSendDebugSignal which will be called only by
> > > pg_log_backtrace and pg_log_backend_memory_contexts. This looks
> > > appropriate in CheckProcSendDebugSignal with slight change to specify
> > > it can consume lots of memory or a long running process.
> > > Thoughts?
>
> For example. do you see the following part correct as well for
> pg_log_backtrace()?
">
> > + * this mechanism is usually used to debug a backend or an auxiliary
> > + * process running and consuming lots of memory, that it might end on its

I felt it was ok since I mentioned it as "consuming lots of memory or
a long running process", let me know if you want to change it to
something else, I will change it.

[1] - https://www.postgresql.org/message-id/1361750.1606795285%40sss.pgh.pa.us

The attached v21 patch has the changes for the same.

Regards,
Vignesh