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
