Re: enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes

On Mon, Nov 1, 2021 at 6:42 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Fri, 29 Oct 2021 22:25:04 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> > On Mon, Oct 11, 2021 at 9:55 AM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 8:21 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> wrote:
> > > > If we can use debuggers, it's possible to know the memory contexts e.g.
> > > > using MemoryContextStats().
> > > > So IMHO if it's necessary to know memory contexts without attaching gdb
> > > > for other than client backends(probably this means using under
> > > > production environment), this enhancement would be pay.
> > >
> > > Thanks for providing your thoughts. Knowing memory usage of auxiliary
> > > processes is as important as backends (user session processes) without
> > > attaching debugger in production environments.
> > >
> > > There are some open points as mentioned in my first mail in this
> > > thread, I will start working on this patch once we agree on them.
> >
> > I'm attaching the v1 patch that enables
> > pg_log_backend_memory_contexts() to log memory contexts of auxiliary
> > processes. Please review it.
> >
> > Here's the CF entry - https://commitfest.postgresql.org/35/3385/
>
> After the patch applied the function looks like this
>
> proc = BackendPidGetProc(pid);
> if (proc == NULL)
> <try aux processes>
> <set is_aux_proc>
> if (proc == NULL)
> <error>
> if (!is_aux_proc)
> <set local backend id>
> SendProcSignal(.., the backend id);
>
> is_aux_proc lookslike making the code complex. I think we can remove
> it.
>
>
> + /* Only regular backends will have valid backend id, auxiliary processes don't. */
> + if (!is_aux_proc)
> + backendId = proc->backendId;
>
> I think the reason we need to do this is not that aux processes have
> the invalid backend id (=InvalidBackendId) but that "some" auxiliary
> processes may have a broken proc->backendId in regard to
> SendProcSignal (we know that's the startup for now.).

I wanted to not have any problems signalling the startup process with
the current code. Yes, the startup process is the only auxiliary
process that has a valid backind id and we have other threads fixing
it. Let's keep the way it is in the v1 patch. Based on whichever patch
gets in we can modify the code.

> +SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('autovacuum launcher'+SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('logical replication launcher'));
> ...
>
> Maybe we can reduce (a quite bit of) run time of the test by
> loopingover the processes but since the test only checks if the
> function doesn't fail to send a signal, I'm not sure we need to
> perform the test for all of the processes here.

Okay, let me choose the checkpointer for this test, I will remove other tests.

> On the other hand,
> the test is missing the most significant target of the startup
> process.

If we were to have tests for the startup process, then it needs to be
in TAP tests as we have to start a hot standby where the startup
process will be in continuous mode. Is there any other way that we can
add the test case in a .sql file? Do we need to get into this much
complexity for the test case?

Regards,
Bharath Rupireddy.