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
