Re: RFC: Logging plan of the running query

[torikoshia]

On 2023-09-20 14:39, Lepikhov Andrei wrote:
Thanks for your reply.
On Tue, Sep 19, 2023, at 8:39 PM, torikoshia wrote:
On 2023-09-15 15:21, Lepikhov Andrei wrote:
On Thu, Sep 7, 2023, at 1:09 PM, torikoshia wrote:
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

No, I think around the hook, which would allow us to rewrite the
pg_query_state extension without additional patches by using the
functionality provided by your patch. I mean, an extension could
provide console UI, not only server logging. And obtain from target
backend so much information in the explain as the instrumentation
level of the current query can give.
It may make pg_query_state shorter and more stable.

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

I can't track the logic path  of the decision provided at this
conference. But my anxiety related to specific place, where
ActiveQueryDesc points top-level query and interruption comes during
DDL, wrapped up in stored procedure. For example:
CREATE TABLE test();
CREATE OR REPLACE FUNCTION ddl() RETURNS void AS $$
BEGIN
  EXECUTE format('ALTER TABLE test ADD COLUMN x integer;');
  ...
END; $$ LANGUAGE plpgsql VOLATILE;
SELECT ddl(), ... FROM ...;

Hmm, as a test, I made sure to call ProcessLogQueryPlanInterrupt() on 
all CFI using 
v28-0002-Testing-attempt-logging-plan-on-ever-CFI-call.patch, and then 
ran the following query but did not cause any problems.

```
=# CREATE TABLE test();
=# CREATE OR REPLACE FUNCTION ddl() RETURNS void AS $$
BEGIN
  EXECUTE format('ALTER TABLE test ADD COLUMN x integer;');
  PERFORM pg_sleep(5);
END; $$ LANGUAGE plpgsql VOLATILE;
=# SELECT ddl();
```

Is this the case you're worrying about?


--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation