Обсуждение: enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes

Hi,

Currently pg_log_backend_memory_contexts() doesn't log the memory
contexts of auxiliary processes such as bgwriter, checkpointer, wal
writer, archiver, startup process and wal receiver. It will be useful
to look at the memory contexts of these processes too, for debugging
purposes and better understanding of the memory usage pattern of these
processes. Inside the code, we could use the AuxiliaryPidGetProc() to
get the PGPROC of these processes. Note that, neither
AuxiliaryPidGetProc() nor BackendPidGetProc() can return PGPROC(as
they don't have PGPROC entries at all) entries for the syslogger,
stats collector processes.

Open points:
1) I'm not sure if it's a good idea to log postmaster memory usage
too. Thoughts?
2) Since with this change pg_log_backend_memory_contexts() will work
for auxiliary processes too, do we need to change the function name
from pg_log_backend_memory_contexts() to
pg_log_backend_memory_contexts()/pg_log_memory_contexts()/some other
name? Or is it a good idea to have a separate function for auxiliary
processes alone, pg_log_auxilliary_process_memory_contexts()?
Thoughts?

I will attach the patch, if possible with test cases, once we agree on
the above open points.

Regards,
Bharath Rupireddy.

Thanks for working on this!

On 2021-10-09 22:23, Bharath Rupireddy wrote:
> Hi,
> 
> Currently pg_log_backend_memory_contexts() doesn't log the memory
> contexts of auxiliary processes such as bgwriter, checkpointer, wal
> writer, archiver, startup process and wal receiver. It will be useful
> to look at the memory contexts of these processes too, for debugging
> purposes and better understanding of the memory usage pattern of these
> processes.

As the discussion below, we thought logging memory contexts of other 
than client backends is possible but were not sure how useful it is.
After all, we have ended up restricting the target process to client 
backends for now.

   
https://www.postgresql.org/message-id/0b0657d5febd0e46565a6bc9c62ba3f6%40oss.nttdata.com

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.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Mon, Oct 11, 2021 at 8:21 AM torikoshia <torikoshia@oss.nttdata.com> wrote:
>
> Thanks for working on this!
>
> On 2021-10-09 22:23, Bharath Rupireddy wrote:
> > Hi,
> >
> > Currently pg_log_backend_memory_contexts() doesn't log the memory
> > contexts of auxiliary processes such as bgwriter, checkpointer, wal
> > writer, archiver, startup process and wal receiver. It will be useful
> > to look at the memory contexts of these processes too, for debugging
> > purposes and better understanding of the memory usage pattern of these
> > processes.
>
> As the discussion below, we thought logging memory contexts of other
> than client backends is possible but were not sure how useful it is.
> After all, we have ended up restricting the target process to client
> backends for now.
>
>
> https://www.postgresql.org/message-id/0b0657d5febd0e46565a6bc9c62ba3f6%40oss.nttdata.com
>
> 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.

Regards,
Bharath Rupireddy.

On Mon, Oct 11, 2021 at 9:55 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Oct 11, 2021 at 8:21 AM torikoshia <torikoshia@oss.nttdata.com> wrote:
> >
> > Thanks for working on this!
> >
> > On 2021-10-09 22:23, Bharath Rupireddy wrote:
> > > Hi,
> > >
> > > Currently pg_log_backend_memory_contexts() doesn't log the memory
> > > contexts of auxiliary processes such as bgwriter, checkpointer, wal
> > > writer, archiver, startup process and wal receiver. It will be useful
> > > to look at the memory contexts of these processes too, for debugging
> > > purposes and better understanding of the memory usage pattern of these
> > > processes.
> >
> > As the discussion below, we thought logging memory contexts of other
> > than client backends is possible but were not sure how useful it is.
> > After all, we have ended up restricting the target process to client
> > backends for now.
> >
> >
> > https://www.postgresql.org/message-id/0b0657d5febd0e46565a6bc9c62ba3f6%40oss.nttdata.com
> >
> > 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/

Regards,
Bharath Rupireddy.

At Fri, 29 Oct 2021 22:25:04 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Mon, Oct 11, 2021 at 9:55 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 8:21 AM torikoshia <torikoshia@oss.nttdata.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.).


+SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('autovacuum launcher'+SELECT
pg_log_backend_memory_contexts(memcxt_get_proc_pid('logicalreplication 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.  On the other hand,
the test is missing the most significant target of the startup
process.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Nov 1, 2021 at 6:42 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Fri, 29 Oct 2021 22:25:04 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> > On Mon, Oct 11, 2021 at 9:55 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 8:21 AM torikoshia <torikoshia@oss.nttdata.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('logicalreplication 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.

On Thu, Nov 4, 2021 at 9:35 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> > 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.

I added a note there (with XXX) describing the fact that we explicitly
need to send invalid backend id to SendProcSignal.

> > +SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('autovacuum launcher'+SELECT
pg_log_backend_memory_contexts(memcxt_get_proc_pid('logicalreplication 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.

I retained the test case just for the checkpointer.

> > 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?

I've not added a TAP test case for the startup process, I see it as
unnecessary. I've tested the startup process case manually here which
just works.

PSA v2 patch and review it.

Regards,
Bharath Rupireddy.

On Fri, Nov 5, 2021 at 11:12 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> PSA v2 patch and review it.

I've modified the docs part a bit, please consider v3 for review.

Regards,
Bharath Rupireddy.

On Mon, Nov 15, 2021 at 7:47 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Nov 5, 2021 at 11:12 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > PSA v2 patch and review it.
>
> I've modified the docs part a bit, please consider v3 for review.

Thanks for the update patch, Few comments:
1) Should we change "CHECK_FOR_INTERRUPTS()" to
"CHECK_FOR_INTERRUPTS() or process specific interrupt handlers"
/*
* pg_log_backend_memory_contexts
* Signal a backend process to log its memory contexts.
*
* By default, only superusers are allowed to signal to log the memory
* contexts because allowing any users to issue this request at an unbounded
* rate would cause lots of log messages and which can lead to denial of
* service. Additional roles can be permitted with GRANT.
*
* On receipt of this signal, a backend sets the flag in the signal
* handler, which causes the next CHECK_FOR_INTERRUPTS() to log the
* memory contexts.
*/
Datum
pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)

2) Should we mention Postmaster process also along with logger and
statistics collector process
+        <glossterm linkend="glossary-backend">backend</glossterm> or the
+        <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or the
+        <glossterm linkend="glossary-auxiliary-proc">auxiliary
process</glossterm>
+        with the specified process ID. All of the
+        <glossterm linkend="glossary-auxiliary-proc">auxiliary
processes</glossterm>
+        are supported except the <glossterm
linkend="glossary-logger">logger</glossterm>
+        and the <glossterm
linkend="glossary-stats-collector">statistics collector</glossterm>
+        as they are not connected to shared memory the function can
not make requests.
+        The backtrace will be logged at <literal>LOG</literal> message level.
+        They will appear in the server log based on the log configuration set
+        (See <xref linkend="runtime-config-logging"/> for more information),
+        but will not be sent to the client regardless of

Regards,
Vignesh

On Mon, Nov 15, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Nov 15, 2021 at 7:47 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, Nov 5, 2021 at 11:12 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > PSA v2 patch and review it.
> >
> > I've modified the docs part a bit, please consider v3 for review.
>
> Thanks for the update patch, Few comments:
> 1) Should we change "CHECK_FOR_INTERRUPTS()" to
> "CHECK_FOR_INTERRUPTS() or process specific interrupt handlers"

Done.

> 2) Should we mention Postmaster process also along with logger and
> statistics collector process
> +        <glossterm linkend="glossary-backend">backend</glossterm> or the
> +        <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or the
> +        <glossterm linkend="glossary-auxiliary-proc">auxiliary
> process</glossterm>
> +        with the specified process ID. All of the
> +        <glossterm linkend="glossary-auxiliary-proc">auxiliary
> processes</glossterm>
> +        are supported except the <glossterm
> linkend="glossary-logger">logger</glossterm>
> +        and the <glossterm
> linkend="glossary-stats-collector">statistics collector</glossterm>
> +        as they are not connected to shared memory the function can
> not make requests.
> +        The backtrace will be logged at <literal>LOG</literal> message level.
> +        They will appear in the server log based on the log configuration set
> +        (See <xref linkend="runtime-config-logging"/> for more information),
> +        but will not be sent to the client regardless of

Done.

Attaching v4 patch, please review it further.

Regards,
Bharath Rupireddy.

On Mon, Nov 15, 2021 at 10:27 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Nov 15, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Nov 15, 2021 at 7:47 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Fri, Nov 5, 2021 at 11:12 AM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > PSA v2 patch and review it.
> > >
> > > I've modified the docs part a bit, please consider v3 for review.
> >
> > Thanks for the update patch, Few comments:
> > 1) Should we change "CHECK_FOR_INTERRUPTS()" to
> > "CHECK_FOR_INTERRUPTS() or process specific interrupt handlers"
>
> Done.
>
> > 2) Should we mention Postmaster process also along with logger and
> > statistics collector process
> > +        <glossterm linkend="glossary-backend">backend</glossterm> or the
> > +        <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or the
> > +        <glossterm linkend="glossary-auxiliary-proc">auxiliary
> > process</glossterm>
> > +        with the specified process ID. All of the
> > +        <glossterm linkend="glossary-auxiliary-proc">auxiliary
> > processes</glossterm>
> > +        are supported except the <glossterm
> > linkend="glossary-logger">logger</glossterm>
> > +        and the <glossterm
> > linkend="glossary-stats-collector">statistics collector</glossterm>
> > +        as they are not connected to shared memory the function can
> > not make requests.
> > +        The backtrace will be logged at <literal>LOG</literal> message level.
> > +        They will appear in the server log based on the log configuration set
> > +        (See <xref linkend="runtime-config-logging"/> for more information),
> > +        but will not be sent to the client regardless of
>
> Done.
>
> Attaching v4 patch, please review it further.

One small comment:
1) There should be a space in between "<literal>LOG</literal>message level"
+        it can) for memory contexts. These memory contexts will be logged at
+        <literal>LOG</literal>message level. They will appear in the server log
+        based on the log configuration set (See <xref
linkend="runtime-config-logging"/>
+        for more information), but will not be sent to the client regardless of

The rest of the patch looks good to me.

Regards,
Vignesh

On Sun, Nov 28, 2021 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > Attaching v4 patch, please review it further.
>
> One small comment:
> 1) There should be a space in between "<literal>LOG</literal>message level"
> +        it can) for memory contexts. These memory contexts will be logged at
> +        <literal>LOG</literal>message level. They will appear in the server log
> +        based on the log configuration set (See <xref
> linkend="runtime-config-logging"/>
> +        for more information), but will not be sent to the client regardless of

Done.

> The rest of the patch looks good to me.

Thanks for the review. Here's the v5 patch.

Regards,
Bharath Rupireddy.

On Sun, Nov 28, 2021 at 12:25 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sun, Nov 28, 2021 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > Attaching v4 patch, please review it further.
> >
> > One small comment:
> > 1) There should be a space in between "<literal>LOG</literal>message level"
> > +        it can) for memory contexts. These memory contexts will be logged at
> > +        <literal>LOG</literal>message level. They will appear in the server log
> > +        based on the log configuration set (See <xref
> > linkend="runtime-config-logging"/>
> > +        for more information), but will not be sent to the client regardless of
>
> Done.
>
> > The rest of the patch looks good to me.
>
> Thanks for the review. Here's the v5 patch.

Thanks for the updated patch, one comment:
1)  The function can be indented similar to other functions in the same file:
+CREATE FUNCTION memcxt_get_proc_pid(text)
+RETURNS int
+LANGUAGE SQL
+AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';

Something like:
+CREATE FUNCTION memcxt_get_proc_pid(text)
+  RETURNS int
+  LANGUAGE SQL
+  AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';

Regards,
Vignesh

On Sun, Nov 28, 2021 at 5:21 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the updated patch, one comment:
> 1)  The function can be indented similar to other functions in the same file:
> +CREATE FUNCTION memcxt_get_proc_pid(text)
> +RETURNS int
> +LANGUAGE SQL
> +AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';
>
> Something like:
> +CREATE FUNCTION memcxt_get_proc_pid(text)
> +  RETURNS int
> +  LANGUAGE SQL
> +  AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';

Done. PSA v6 patch.

Regards,
Bharath Rupireddy.

On Sun, Nov 28, 2021 at 7:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sun, Nov 28, 2021 at 5:21 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the updated patch, one comment:
> > 1)  The function can be indented similar to other functions in the same file:
> > +CREATE FUNCTION memcxt_get_proc_pid(text)
> > +RETURNS int
> > +LANGUAGE SQL
> > +AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';
> >
> > Something like:
> > +CREATE FUNCTION memcxt_get_proc_pid(text)
> > +  RETURNS int
> > +  LANGUAGE SQL
> > +  AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';
>
> Done. PSA v6 patch.

Thanks for the updated patch. The patch applies neatly, make
check-world passes and the documentation looks good. I did not find
any issues with the v6 patch, I'm marking the patch as  Ready for
Committer.

Regards,
Vignesh

On 2021/11/29 11:44, vignesh C wrote:
> Thanks for the updated patch. The patch applies neatly, make
> check-world passes and the documentation looks good. I did not find
> any issues with the v6 patch, I'm marking the patch as  Ready for
> Committer.

I started reading the patch.

+CREATE FUNCTION memcxt_get_proc_pid(text)
+  RETURNS int
+  LANGUAGE SQL
+  AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';
+
+SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('checkpointer'));
+
+DROP FUNCTION memcxt_get_proc_pid(text);

Why is memcxt_get_proc_pid() still necessary? ISTM that we can just replace the above with the following query,
instead.

     SELECT pg_log_backend_memory_contexts(pid) FROM pg_stat_activity WHERE backend_type = 'checkpointer'

-        Requests to log the memory contexts of the backend with the
-        specified process ID.  These memory contexts will be logged at
-        <literal>LOG</literal> message level. They will appear in
-        the server log based on the log configuration set
-        (See <xref linkend="runtime-config-logging"/> for more information),
-        but will not be sent to the client regardless of
+        Requests to log memory contexts of the <glossterm linkend="glossary-backend">backend</glossterm>
+        or the <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or
+        the <glossterm linkend="glossary-auxiliary-proc">auxiliary process</glossterm>
+        with the specified process ID. This function cannot request
+        <glossterm linkend="glossary-postmaster">postmaster process</glossterm> or
+        <glossterm linkend="glossary-logger">logger</glossterm> or
+        <glossterm linkend="glossary-stats-collector">statistics collector</glossterm>
+        (all other <glossterm linkend="glossary-auxiliary-proc">auxiliary processes</glossterm>

ISTM that you're trying to list all possible processes that pg_log_backend_memory_contexts() can handle. But why didn't
youlist autovacuum worker (while other special backend, WAL sender, is picked up) and background worker like logical
replicationlauncher? Because the term "backend" implicitly includes those processes? If so, why did you pick up WAL
senderseparately?
 

I'm tempted to replace these descriptions as follows. Because the following looks simpler and easier to read and
understand,to me.
 

----------------------
Requests to log the memory contexts of the process with the specified process ID.  Possible processes that this
functioncan send the request to are: backend, WAL sender, autovacuum worker, auxiliary processes except logger and
statscollector, and background workers.
 
----------------------

or

----------------------
Requests to log the memory contexts of the backend with the specified process ID.  This function can send the request
toalso auxiliary processes except logger and stats collector.
 
----------------------

+    /* See if the process with given pid is an auxiliary process. */
+    if (proc == NULL)
+    {
+        proc = AuxiliaryPidGetProc(pid);
+        is_aux_proc = true;
+    }

As Horiguchi-san told upthread, IMO it's simpler not to use is_aux_proc flag. For example, you can replace this code
with

------------------------
proc = BackendPidGetProc(pid);

if (proc != NULL)
   backendId = proc->backendId;
else
   proc = AuxiliaryPidGetProc(pid);
------------------------


Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, Jan 7, 2022 at 9:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2021/11/29 11:44, vignesh C wrote:
> > Thanks for the updated patch. The patch applies neatly, make
> > check-world passes and the documentation looks good. I did not find
> > any issues with the v6 patch, I'm marking the patch as  Ready for
> > Committer.
>
> I started reading the patch.

Thanks.

> +CREATE FUNCTION memcxt_get_proc_pid(text)
> +  RETURNS int
> +  LANGUAGE SQL
> +  AS 'SELECT pid FROM pg_stat_activity WHERE backend_type = $1';
> +
> +SELECT pg_log_backend_memory_contexts(memcxt_get_proc_pid('checkpointer'));
> +
> +DROP FUNCTION memcxt_get_proc_pid(text);
>
> Why is memcxt_get_proc_pid() still necessary? ISTM that we can just replace the above with the following query,
instead.
>
>      SELECT pg_log_backend_memory_contexts(pid) FROM pg_stat_activity WHERE backend_type = 'checkpointer'

Changed.

> I'm tempted to replace these descriptions as follows. Because the following looks simpler and easier to read and
understand,to me.
 
> ----------------------
> Requests to log the memory contexts of the backend with the specified process ID.  This function can send the request
toalso auxiliary processes except logger and stats collector.
 
> ----------------------

Changed.

> As Horiguchi-san told upthread, IMO it's simpler not to use is_aux_proc flag. For example, you can replace this code
with
>
> ------------------------
> proc = BackendPidGetProc(pid);
>
> if (proc != NULL)
>    backendId = proc->backendId;
> else
>    proc = AuxiliaryPidGetProc(pid);
> ------------------------

Changed.

PSA v7 patch.

Regards,
Bharath Rupireddy.

On 2022/01/08 1:50, Bharath Rupireddy wrote:
> PSA v7 patch.

Thanks for updating the patch!
I applied some cosmetic changes and pushed the patch. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION