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