Re: Set query_id for query contained in utility statement

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com>
Cc: jian he <jian(dot)universality(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Set query_id for query contained in utility statement
Date: 2024-10-02 04:38:43
Message-ID: ZvzOU9BbvPCpzgec@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Aug 30, 2024 at 09:37:03AM +0200, Anthonin Bonnefoy wrote:
> Thanks for the review. I think the parser state is mostly used for the
> error callbacks and parser_errposition but I'm not 100% sure. Either
> way, you're right and it probably shouldn't be in the patch. I've
> modified the patch to restrict the changes to only add the necessary
> query jumble and post parse hook calls.

So this adds four calls post_parse_analyze_hook, leading to more data
added to pgss for non-toplevel queries: one in createas.c for the CTAS
internal query, one in portalcmds.c for the inner query of DECLARE,
and two for utilities in EXPLAIN.

This is a rather old problem, trying to bring more consistency across
the board, and it comes down to this bit with EXPLAIN, that can be
seen on HEAD:
SET pg_stat_statements.track = 'all';
explain (costs off) select 1;

=# select calls, query, toplevel from pg_stat_statements
where query ~'explain';
calls | query | toplevel
-------+--------------------------------+----------
1 | explain (costs off) select $1; | f
1 | explain (costs off) select $1 | t
(2 rows)

FWIW, I've always found this case with EXPLAIN with two entries
confusing, so what's the advantage in trying to apply this rule for
the rest? We know that EXPLAIN, DECLARE and CTAS run a query attached
to their DDL, hence isn't it sufficient to register a single entry for
the top-level query, then nothing for the internal one. The
documentation tells about inner queries when pg_stat_statements.track
= all, like the ones in PL functions, DO blocks, because we have a
clear view of the query string, creating a unique one-one mapping
between a query string and its ID. This patch maps the same query
string to more than one query ID, spreading that.

So it seems that there are arguments for not doing what this patch
proposes, but also make sure that EXPLAIN logs a single entry, not
two currently when using pg_stat_statements.track = all.

Side note. It looks like the patch is forgetting about CREATE VIEW
and CREATE MATERIALIZED VIEW, creating only a top-level entry when
running these utilities.
--
Michael

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hunaid Sohail 2024-10-02 04:48:08 Re: Psql meta-command conninfo+
Previous Message Tristan Partin 2024-10-02 04:17:15 Re: Converting README documentation to Markdown