On Wed, Feb 7, 2024 at 9:00 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Feb 7, 2024 at 2:57 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Feb 07, 2024 at 02:04:39PM +0530, Bharath Rupireddy wrote:
> > > Well, that 'ubuntu' is the default username there, I've changed it now
> > > and kept the output short.
> >
> > I would keep it just at two or three lines, with a "For example, with
> > lines like":
>
> Done.
>
> > > I've simplified the tests, now we don't need two separate output files
> > > for tests. Please see the attached v27 patch.
> >
> > + proname => 'pg_log_backtrace', provolatile => 'v', prorettype => 'bool',
> >
> > Hmm. Would it be better to be in line with memory contexts logging
> > and use pg_log_backend_backtrace()?
>
> +1.
>
> > One thing I was wondering is that
> > there may be a good point in splitting the backtrace support into two
> > functions (backends and auxiliary processes) that could be split with
> > two execution ACLs across different roles.
>
> -1 for that unless we have any requests. I mean, backtrace is a common
> thing for all postgres processes, why different controls are needed?
> I'd go with what pg_log_backend_memory_contexts does - it supports
> both backends and auxiliary processes.
>
> > + PROCSIG_LOG_BACKTRACE, /* ask backend to log the current backtrace */
> >
> > Incorrect order.
>
> PROCSIG_XXX aren't alphabetically ordered, no?
>
> > +-- Backtrace is not logged for auxiliary processes
> >
> > Not sure there's a point in keeping that in the tests for now.
> >
> > + * XXX: It might be worth implementing it for auxiliary processes.
> >
> > Same, I would remove that.
>
> Done.
>
> > +static volatile sig_atomic_t backtrace_functions_loaded = false;
> >
> > Hmm, so you need that because of the fact that it would be called in a
> > signal as backtrace(3) says:
> > "If you need certain calls to these two functions to not allocate
> > memory (in signal handlers, for example), you need to make sure libgcc
> > is loaded beforehand".
> >
> > True that it is not interesting to only log something when having a
> > CFI, this needs to be dynamic to get a precise state of things.
>
> Right.
>
> I've also fixed some test failures. Please see the attached v28 patch
> set. 0002 extends pg_log_backend_backtrace to auxiliary processes,
> just like pg_log_backend_memory_contexts (not focused on PID
> de-duplication code yet).
I've missed adding LoadBacktraceFunctions() in InitAuxiliaryProcess
for 0002 patch. Please find the attached v29 patch set. Sorry for the
noise.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
