Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

On 2023-10-16 18:46, Ashutosh Bapat wrote:
> On Thu, Oct 12, 2023 at 6:51 PM torikoshia <torikoshia@oss.nttdata.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