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