On 2021-11-13 22:29, Bharath Rupireddy wrote:
Thanks for your review!
> On Wed, Oct 13, 2021 at 7:58 PM Ekaterina Sokolova
> <e.sokolova@postgrespro.ru> wrote:
>> Thank you for working on this issue. I would be glad to continue to
>> follow the development of this issue.
>
> Thanks for the patch. I'm not sure if v11 is the latest patch, if yes,
> I have the following comments:
>
> 1) Firstly, v11 patch isn't getting applied on the master -
> http://cfbot.cputube.org/patch_35_3142.log.
Updated the patch.
> 2) I think we are moving away from if (!superuser()) checks, see the
> commit [1]. The goal is to let the GRANT-REVOKE system deal with who
> is supposed to run these system functions. Since
> pg_log_current_query_plan also writes the info to server logs, I think
> it should do the same thing as commit [1] did for
> pg_log_backend_memory_contexts.
>
> With v11, you are re-introducing the superuser() check in the
> pg_log_backend_memory_contexts which is wrong.
Yeah, I removed superuser() check and make it possible to be executed by
non-superusers when users are granted to do so.
>
> 3) I think SendProcSignalForLogInfo can be more generic, meaning, it
> can also send signal to auxiliary processes if asked to do this will
> simplify the things for pg_log_backend_memory_contexts and other
> patches like pg_print_backtrace. I would imagine it to be "bool
> SendProcSignalForLogInfo(pid_t pid, ProcSignalReason reason, bool
> signal_aux_proc);".
I agree with your idea.
Since sending signals to auxiliary processes to dump memory contexts and
pg_print_backtrace is still under discussion, IMHO it would be better to
refactor SendProcSignalForLogInfo after these patches are commited.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION