Re: RFC: Logging plan of the running query

On 2023-09-15 15:21, Lepikhov Andrei wrote:
> On Thu, Sep 7, 2023, at 1:09 PM, torikoshia wrote:
>> On 2023-09-06 11:17, James Coleman wrote:
>> It seems that we can know what queries were running from the stack
>> traces you shared.
>> As described above, I suspect a lock which was acquired prior to
>> ProcessLogQueryPlanInterrupt() caused the issue.
>> I will try a little more to see if I can devise a way to create the 
>> same
>> situation.
> Hi,
> I have explored this patch and, by and large, agree with the code. But
> some questions I want to discuss:
> 1. Maybe add a hook to give a chance for extensions, like
> pg_query_state, to do their job?

Do you imagine adding a hook which enables adding custom interrupt codes 
like below?

https://github.com/postgrespro/pg_query_state/blob/master/patches/custom_signals_15.0.patch

If so, that would be possible, but this patch doesn't require the 
functionality and I feel it'd be better doing in independent patch.


> 2. In this implementation, ProcessInterrupts does a lot of work during
> the explain creation: memory allocations, pass through the plan,
> systables locks, syscache access, etc. I guess it can change the
> semantic meaning of 'safe place' where CHECK_FOR_INTERRUPTS can be
> called - I can imagine a SELECT query, which would call a stored
> procedure, which executes some DDL and acquires row exclusive lock at
> the time of interruption. So, in my mind, it is too unpredictable to
> make the explain in the place of interruption processing. Maybe it is
> better to think about some hook at the end of ExecProcNode, where a
> pending explain could be created?

Yeah, I withdrew this patch once for that reason, but we are resuming 
development in response to the results of a discussion by James and 
others at this year's pgcon about the safety of this process in CFI:

https://www.postgresql.org/message-id/CAAaqYe9euUZD8bkjXTVcD9e4n5c7kzHzcvuCJXt-xds9X4c7Fw%40mail.gmail.com

BTW it seems pg_query_state also enables users to get running query 
plans using CFI.
Does pg_query_state do something for this safety concern?

> Also, I suggest minor code change with the diff in attachment.

Thanks!

This might be biased opinion and objections are welcomed, but I feel the 
original patch is easier to read since PG_RETURN_BOOL(true/false) is 
located in near place to each cases.
Also the existing function pg_log_backend_memory_contexts(), which does 
the same thing, has the same form as the original patch.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation