Re: RFC: Logging plan of the running query

On 2023-10-16 18:46, Ashutosh Bapat wrote:
> On Thu, Oct 12, 2023 at 6:51 PM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
> wrote:
>>
>> On 2023-10-11 16:22, Ashutosh Bapat wrote:
>> >
>> > Considering the similarity with auto_explain I wondered whether this
>> > function should be part of auto_explain contrib module itself? If we
>> > do that users will need to load auto_explain extension and thus
>> > install executor hooks when this function doesn't need those. So may
>> > not be such a good idea. I didn't see any discussion on this.
>>
>> I once thought about adding this to auto_explain, but I left it asis
>> for
>> below reasons:
>>
>> - One of the typical use case of pg_log_query_plan() would be
>> analyzing
>> slow query on customer environments. On such environments, We cannot
>> always control what extensions to install.
>
> The same argument applies to auto_explain functionality as well. But
> it's not part of the core.

Yeah, and when we have a situation where we want to run
pg_log_query_plan(), we can run it in any environment as long as it is
bundled with the core.
On the other hand, if it is built into auto_explain, we need to start by
installing auto_explain if we do not have auto_explain, which is often
difficult to do in production environments.

>> Of course auto_explain is a major extension and it is quite
>> possible
>> that they installed auto_explain, but but it is also possible they do
>> not.
>> - It seems a bit counter-intuitive that pg_log_query_plan() is in an
>> extension called auto_explain, since it `manually`` logs plans
>>
>
> pg_log_query_plan() may not fit auto_explain but
> pg_explain_backend_query() does. What we are logging is more than just
> plan of the query, it might expand to be closer to explain output.
> While auto in auto_explain would refer to its automatically logging
> explain outputs, it can provide an additional function which provides
> similar functionality by manually triggering it.
>
> But we can defer this to a committer, if you want.
>
> I am more interested in avoiding the duplication of code, esp. the
> first comment in my reply

If there are no objections, I will try porting it to auto_explain and
see its feasibility.

>>> There is a lot of similarity between what this feature does and what
>>> auto explain does. I see the code is also duplicated. There is some
>>> merit in avoiding this duplication
>>> 1. we will get all the features of auto_explain automatically like
>>> choosing a format (this was expressed somebody earlier in this
>>> thread), setings etc.
>>> 2. avoid bugs. E.g your code switches context after ExplainState has
>>> been allocated. These states may leak depending upon when this
>>> function gets called.
>>> 3. Building features on top as James envisions will be easier.
>
>>
>> =# select pg_log_query_plan(pid), application_name, backend_type
>> from
>> pg_stat_activity where backend_type = 'autovacuum launcher';
>> WARNING: PID 63323 is not a PostgreSQL client backend process
>> pg_log_query_plan | application_name | backend_type
>> -------------------+------------------+---------------------
>> f | | autovacuum launcher
>>
>>
>> > I am also wondering whether it's better to report the WARNING as
>> > status column in the output. E.g. instead of
>> > #select pg_log_query_plan(100);
>> > WARNING: PID 100 is not a PostgreSQL backend process
>> > pg_log_query_plan
>> > -------------------
>> > f
>> > (1 row)
>> > we output
>> > #select pg_log_query_plan(100);
>> > pg_log_query_plan | status
>> > -------------------+---------------------------------------------
>> > f | PID 100 is not a PostgreSQL backend process
>> > (1 row)
>> >
>> > That looks neater and can easily be handled by scripts, applications
>> > and such. But it will be inconsistent with other functions like
>> > pg_terminate_backend() and pg_log_backend_memory_contexts().
>>
>> It seems neater, but it might be inconvenient because we can no longer
>> use it in select list like the following query as you wrote:
>>
>> #select pg_log_query_plan(pid), application_name, backend_type from
>> pg_stat_activity where backend_type = 'autovacuum launcher';
>
> Why is that?

Sorry, I misunderstood and confirmed we can run queries like below:

```
=# CREATE OR REPLACE FUNCTION pg_log_query_plan_stab(i int)
RETURNS TABLE(

pg_log_query_plan bool,
status text

) AS $$
DECLARE

BEGIN

RETURN QUERY SELECT false::bool, 'PID 100 is not a PostgreSQL
backend process'::text; END;

$$ LANGUAGE plpgsql;

=# select pg_log_query_plan_stab(pid), application_name, backend_type
from pg_stat_activity where backend_type = 'autovacuum launcher';
pg_log_query_plan_stab | application_name |
backend_type
---------------------------------------------------+------------------+---------------------
(f,"PID 100 is not a PostgreSQL backend process") | |
autovacuum launcher
```
--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation