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

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.