public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Anthonin Bonnefoy <[email protected]>
Cc: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Set query_id for query contained in utility statement
Date: Wed, 2 Oct 2024 13:38:43 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAO6_XqoXPyoChZmjaWbcKt0B8jAo_0hc4+jv-ej-ZT4vGz4WRg@mail.gmail.com>
References: <CAO6_XqqM6S9bQ2qd=75W+yKATwoazxSNhv5sjW06fjGAtHbTUA@mail.gmail.com>
<CAO6_XqoXFCRMviF5QOgqnmQbgbUOKdWmPo9uNob_hzP3d+-GjQ@mail.gmail.com>
<CACJufxF_XxHZ8h5RkJtsjSyNtqdxuQg04OTaoGg-RrBH8RSHRw@mail.gmail.com>
<CAO6_Xqo5J-DP4aRGUc5J6pu5sSTgjFdNHQ4sWz+NbZx5VGFPOg@mail.gmail.com>
<CACJufxGo5dMZ+=g3F_xje_C-kKe8RH_vjsTaPdZNv8P===jAvw@mail.gmail.com>
<CAO6_XqoXPyoChZmjaWbcKt0B8jAo_0hc4+jv-ej-ZT4vGz4WRg@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (7+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Set query_id for query contained in utility statement
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox