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

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 you list autovacuum worker (while other special backend, WAL sender, is picked up) and background worker like logical replication launcher? Because the term "backend" implicitly includes those processes? If so, why did you pick up WAL sender separately?

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 function can send the request to are: backend, WAL sender, autovacuum worker, auxiliary processes except logger and stats collector, and background workers.
----------------------

or

----------------------
Requests to log the memory contexts of the backend with the specified process ID. This function can send the request to also 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