Thread: Set query_id for query contained in utility statement
Hi all, Some utility statements like Explain, CreateTableAs and DeclareCursor contain a query which will be planned and executed. During post parse, only the top utility statement is jumbled, leaving the contained query without a query_id set. Explain does the query jumble in ExplainQuery but for the contained query but CreateTableAs and DeclareCursor were left with unset query_id. This leads to extensions relying on query_id like pg_stat_statements to not be able to track those nested queries as the query_id was 0. This patch fixes this by recursively jumbling queries contained in those utility statements during post_parse, setting the query_id for those contained queries and removing the need for ExplainQuery to do it for the Explain statements. Regards, Anthonin Bonnefoy
Attachment
I've realised my initial approach was wrong, calling the post parse
for all nested queries in analyze.c prevents extension like pgss to
correctly track the query's nesting level.
I've changed the approach to replicate what's done in ExplainQuery to
both CreateTableAs and DeclareCursor: Jumble the query contained by
the utility statement and call the post parse hook before it is
planned or executed. Additionally, explain's nested query can itself
be a CreateTableAs or DeclareCursor which also needs to be jumbled.
The issue is visible when explaining a CreateTableAs or DeclareCursor
Query, the queryId is missing despite the verbose.
EXPLAIN (verbose) create table test_t as select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
Output: 1
Post patch, the query id is correctly displayed.
EXPLAIN (verbose) create table test_t as select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
Output: 1
Query Identifier: 2800308901962295548
Regards,
Anthonin Bonnefoy
Attachment
On Mon, Aug 5, 2024 at 3:19 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> I've realised my initial approach was wrong, calling the post parse
> for all nested queries in analyze.c prevents extension like pgss to
> correctly track the query's nesting level.
>
> I've changed the approach to replicate what's done in ExplainQuery to
> both CreateTableAs and DeclareCursor: Jumble the query contained by
> the utility statement and call the post parse hook before it is
> planned or executed. Additionally, explain's nested query can itself
> be a CreateTableAs or DeclareCursor which also needs to be jumbled.
> The issue is visible when explaining a CreateTableAs or DeclareCursor
> Query, the queryId is missing despite the verbose.
>
> EXPLAIN (verbose) create table test_t as select 1;
> QUERY PLAN
> ------------------------------------------
> Result (cost=0.00..0.01 rows=1 width=4)
> Output: 1
>
> Post patch, the query id is correctly displayed.
>
> EXPLAIN (verbose) create table test_t as select 1;
> QUERY PLAN
> ------------------------------------------
> Result (cost=0.00..0.01 rows=1 width=4)
> Output: 1
> Query Identifier: 2800308901962295548
>
play with pg_stat_statements. settings:
name | setting
-----------------------------------+---------
pg_stat_statements.max | 5000
pg_stat_statements.save | on
pg_stat_statements.track | all
pg_stat_statements.track_planning | on
pg_stat_statements.track_utility | on
SELECT pg_stat_statements_reset();
select 1;
select 2;
SELECT queryid, left(query, 60),plans, calls, rows FROM
pg_stat_statements ORDER BY calls DESC LIMIT 5;
returns:
queryid | left
| plans | calls | rows
----------------------+--------------------------------------------------------------+-------+-------+------
2800308901962295548 | select $1
| 2 | 2 | 2
The output is what we expect.
now after applying your patch.
SELECT pg_stat_statements_reset();
EXPLAIN (verbose) create table test_t as select 1;
EXPLAIN (verbose) create table test_t as select 2;
SELECT queryid, left(query, 60),plans, calls, rows FROM
pg_stat_statements ORDER BY calls DESC LIMIT 5;
the output is:
queryid | left
| plans | calls | rows
----------------------+--------------------------------------------------------------+-------+-------+------
2800308901962295548 | EXPLAIN (verbose) create table test_t as
select 1; | 2 | 2 | 0
2093602470903273926 | EXPLAIN (verbose) create table test_t as
select $1 | 0 | 2 | 0
-2694081619397734273 | SELECT pg_stat_statements_reset()
| 0 | 1 | 1
2643570658797872815 | SELECT queryid, left(query, $1),plans, calls,
rows FROM pg_s | 1 | 0 | 0
"EXPLAIN (verbose) create table test_t as select 1;" called twice,
is that what we expect?
should first row, the normalized query becomes
EXPLAIN (verbose) create table test_t as select $1;
?
On Mon, Aug 26, 2024 at 4:55 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
/* Evaluate parameters, if any */
if (entry->plansource->num_params)
{
- ParseState *pstate;
-
- pstate = make_parsestate(NULL);
- pstate->p_sourcetext = queryString;
you deleted the above these lines, but passed (ParseState *pstate) in
ExplainExecuteQuery
how do you make sure ExplainExecuteQuery passed (ParseState *pstate)
the p_next_resno is 1 and p_resolve_unknowns is true.
maybe we can add some Asserts like in ExplainExecuteQuery
/* Evaluate parameters, if any */
if (entry->plansource->num_params)
{
Assert(pstate->p_next_resno == 1);
Assert(pstate->p_resolve_unknowns == 1);
}
also it's ok to use passed (ParseState *pstate) for
{
estate = CreateExecutorState();
estate->es_param_list_info = params;
paramLI = EvaluateParams(pstate, entry, execstmt->params, estate);
}
?
I really don't know.
some of the change is refactoring, maybe you can put it into a separate patch.
PREPARE test_prepare_pgss1(int, int) AS select generate_series($1, $2);
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
EXECUTE test_prepare_pgss1 (1,2);
EXECUTE test_prepare_pgss1 (1,3);
SELECT toplevel, calls, query, queryid, rows FROM pg_stat_statements
ORDER BY query COLLATE "C", toplevel;
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
---the above works just fine. just for demo purpose
explain(verbose) EXECUTE test_prepare_pgss1(1, 2);
explain(verbose) EXECUTE test_prepare_pgss1(1, 3);
SELECT toplevel, calls, query, queryid, rows FROM pg_stat_statements
ORDER BY query COLLATE "C", toplevel;
toplevel | calls | query
| queryid | rows
----------+-------+------------------------------------------------------------------------+----------------------+------
f | 2 | PREPARE test_prepare_pgss1(int, int) AS select
generate_series($1, $2) | -3421048434214482065 | 0
t | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
| 3366652201587963567 | 1
t | 0 | SELECT toplevel, calls, query, queryid, rows FROM
pg_stat_statements +| -6410939316132384446 | 0
| | ORDER BY query COLLATE "C", toplevel
| |
t | 1 | explain(verbose) EXECUTE test_prepare_pgss1(1, 2)
| 7618807962395633001 | 0
t | 1 | explain(verbose) EXECUTE test_prepare_pgss1(1, 3)
| -2281958002956676857 | 0
Is it possible to normalize top level utilities explain query, make
these two have the same queryid?
explain(verbose) EXECUTE test_prepare_pgss1(1, 2);
explain(verbose) EXECUTE test_prepare_pgss1(1, 3);
I guess this is a corner case.
On Mon, Sep 30, 2024 at 5:14 PM jian he <jian.universality@gmail.com> wrote: > Is it possible to normalize top level utilities explain query, make > these two have the same queryid? > explain(verbose) EXECUTE test_prepare_pgss1(1, 2); > explain(verbose) EXECUTE test_prepare_pgss1(1, 3); > > I guess this is a corner case. This seems to be a known issue. The test_prepare_pgss1's parameters are A_Const nodes. Those nodes have a custom query jumble which doesn't record location[1] and thus can't be normalised by pgss. That could be fixed by replacing the switch on nodeTag by JUMBLE_LOCATION(location) but this will impact a lot of DDL queries and the result doesn't look great (for example, "BEGIN TRANSACTION NOT DEFERRABLE, READ ONLY, READ WRITE, DEFERRABLE" would be normalised as "BEGIN TRANSACTION $1 DEFERRABLE, $2 ONLY, $3 WRITE, $4") Looking at the commit for the A_Const's jumble, this is mentioned by Michael[2]: > (FWIW, I'd like to think that there is an argument to normalize the > A_Const nodes for a portion of the DDL queries, by ignoring their > values in the query jumbling and mark a location, which would be > really useful for some workloads, but that's a separate discussion I > am keeping for later.) I haven't found any recent discussion but this should live in a different thread as this is a separate issue. [1]: https://github.com/postgres/postgres/blob/cf4401fe6cf56811343edcad29c96086c2c66481/src/backend/nodes/queryjumblefuncs.c#L323-L355 [2]: https://www.postgresql.org/message-id/Y9+HuYslMAP6yyPb@paquier.xyz
On Fri, Oct 4, 2024 at 5:05 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> I agree that tracking 2 identical statements with different queryIds
> and nesting levels is very confusing. On the other hand, from an
> extension developer point of view (not necessarily limited to
> pg_stat_statements), I would like to have the queryId available and
> the post_parse hook called so the query can be normalised and tracked
> in a hashmap.
>
> However, the repeated statements did bug me a lot so I took a stab at
> trying to find a possible solution. I made an attempt in 0001 by
> tracking the statements' locations of explainable statements (Select,
> Insert, Update, Merge, Delete...) during parse and propagate them in
> the generated Query during transform. With the change, we now have the
> following result:
>
> SET pg_stat_statements.track = 'all';
> explain (costs off) select 1;
> select 1;
> select queryid, calls, query, toplevel from pg_stat_statements
> where query ~'select \$1';
> queryid | calls | query | toplevel
> ---------------------+-------+-------------------------------+----------
> 2800308901962295548 | 1 | select $1 | t
> 2800308901962295548 | 1 | select $1; | f
> 3797185112479763582 | 1 | explain (costs off) select $1 | t
>
> The top level and nested select statement now share both the same
> queryId and query string. The additional ';' for the nested query is
> due to not having the statement length and taking the whole
> statement.
>
about v5 0001
select_with_parens:
'(' select_no_parens ')' { $$ = $2; }
| '(' select_with_parens ')' { $$ = $2; }
;
toplevel | calls | query
----------+-------+-------------------------------------------------------
t | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
t | 0 | SELECT toplevel, calls, query FROM pg_stat_statements+
| | ORDER BY query COLLATE "C", toplevel
t | 1 | explain (select $1)
f | 1 | select $1);
query "select $1);" looks weird. not sure how to improve it,
or this should be the expected behavior?
in gram.y
| values_clause { $$ = $1; }
| TABLE relation_expr
for TABLE relation_expr
we can add `n->location = @1;`
for values_clause we can do also,
then in transformValuesClause do the same as in transformSelectStmt.
On Mon, Oct 7, 2024 at 7:39 AM Michael Paquier <michael@paquier.xyz> wrote:
> GOod point, this is confusing. The point is that having only
> stmt_location is not enough to detect where the element in the query
> you want to track is because it only points at its start location in
> the full query string. In an ideal world, what we should have is its
> start and end, pass it down to pgss_store(), and store only this
> subquery between the start and end positions in the stats entry.
> Making that right through the parser may be challenging, though.
One of the issues is that we don't track the length in the parser,
only location[1]. The only place we can have some information about
the statement length (or at least, the location of the ';') is for
multi statement query.
On Mon, Oct 7, 2024 at 6:17 PM jian he <jian.universality@gmail.com> wrote:
> turns out UPDATE/DELETE/MERGE and other utilities stmt cannot have
> arbitrary parenthesis with EXPLAIN.
Yes, it is also possible to get the length of the Select statement
within parenthesis through the parser by using the location of ')' for
the select_no_parens.
> the main gotcha is to add location information for the statement that
> is being explained.
I've found that there are other possible issues with not having the
statement length and including the opening parenthesis won't be
enough. On HEAD, we have the following:
explain(verbose) SELECT 1, 2, 3\; explain SELECT 1, 2, 3, 4;
SELECT toplevel, query FROM pg_stat_statements
ORDER BY toplevel desc, query;
toplevel | query
----------+-----------------------------------------------------------------
t | SELECT pg_stat_statements_reset() IS NOT NULL AS t
t | explain SELECT $1, $2, $3, $4
t | explain(verbose) SELECT $1, $2, $3
f | explain(verbose) SELECT $1, $2, $3; explain SELECT 1, 2, 3, 4;
f | explain(verbose) SELECT 1, 2, 3; explain SELECT $1, $2, $3, $4;
The nested statement will have the whole query string. To fix this, we
need to propagate the statement length from the RawStmt (probably
using the ParserState?) and adjust the nested query's location and
length when the statement is transformed. I'm still working on the
details and edge cases on this.
[1]: https://github.com/postgres/postgres/blob/REL_17_STABLE/src/backend/parser/gram.y#L69-L79
On Wed, Oct 9, 2024 at 4:49 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> Here is a new version of the patchset.
>
> 0001: Add tests to cover the current behaviour: Missing nested
> statements for CreateTableAs, DeclareCursor and MaterializedViews,
> nested statements reported by explain including the whole string
> (multi statement or whole utility statement). I've tried to be
> exhaustive, testing both all and top tracking, but I may have missed
> some possible corner cases.
>
> 0002: Track the location of explainable statements. We keep RawStmt's
> location and length in the ParseState and use it to compute the
> transformed statement's length, this is done to handle the
> multi-statement query issue. For SelectStmt, we also track the
> statement length when select is inside parentheses and use it when
> available.
> For with clauses, I've switched to directly getting the correct
> location from the parser with the 'if (@$ < 0) @$ = @2;' trick. Select
> is handled differently and the location is updated in
> insertSelectOptions.
> Tracking the statement length as the benefit of having consistent
> query string between the top and nested statement. A 'Select 1' should
> be reported with the same string, without the ';' in both cases.
>
hi.
Query *
transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree)
{
Query *result;
/* We're at top level, so allow SELECT INTO */
result = transformOptionalSelectInto(pstate, parseTree->stmt);
result->stmt_location = parseTree->stmt_location;
result->stmt_len = parseTree->stmt_len;
return result;
}
function call chain:
transformTopLevelStmt transformOptionalSelectInto transformStmt
transformSelectStmt
in transformSelectStmt we do
makeNode(Query), assign Query's stmt_len, stmt_location value.
if in transformSelectStmt we did it wrong, then
transformTopLevelStmt
result->stmt_location = parseTree->stmt_location;
result->stmt_len = parseTree->stmt_len;
will override values we've previously assigned at transformSelectStmt.
this feel not safe?
+qry->stmt_len = pstate->p_stmt_len - (stmt->location -
pstate->p_stmt_location);
i feel like, the comments didn't explain very well the difference between
stmt->location and pstate->p_stmt_location.
i know it's related to multi-statement, possibly through \;
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
explain (values (1));
explain ((values (1)));
explain table tenk1;
explain ((table tenk1));
SELECT toplevel, calls, query FROM pg_stat_statements ORDER BY query
COLLATE "C", toplevel;
final output:
toplevel | calls | query
----------+-------+--------------------------------------------------------------------------------------------
t | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
t | 0 | SELECT toplevel, calls, query FROM
pg_stat_statements ORDER BY query COLLATE "C", toplevel
t | 2 | explain (values ($1))
f | 2 | explain (values ($1));
f | 2 | explain table tenk1
t | 2 | explain table tenk1
I already mentioned this at the end of [1].
Can you try to also normalize these cases, since we've normalized the
nested select query in explain statement.
[1] https://www.postgresql.org/message-id/CACJufxF9hqyfmKEdpiG%3DPbrGdKVNP2BQjHFJh4q6639sV7NmvQ%40mail.gmail.com
hi.
explain(verbose) SELECT 1, 2, 3\; explain SELECT 1, 2, 3, 4;
will transformed to
explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4;
it seems to me your patch care about following position.
explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4;
^
but this patch [1] at another thread will get the top level statement
(passed the raw parse, no syntax error) beginning more effortless.
explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4;
^ ^
can you try to looking at [1]. it may help to resolve this patch problem.
[1] https://www.postgresql.org/message-id/2245576.1728418678%40sss.pgh.pa.us
On Tue, Oct 15, 2024 at 3:23 PM jian he <jian.universality@gmail.com> wrote: > explain(verbose) SELECT 1, 2, 3\; explain SELECT 1, 2, 3, 4; > will transformed to > explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4; > > it seems to me your patch care about following position. > explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4; > ^ > but this patch [1] at another thread will get the top level statement > (passed the raw parse, no syntax error) beginning more effortless. > explain(verbose) SELECT 1, 2, 3; explain SELECT 1, 2, 3, 4; > ^ ^ > > can you try to looking at [1]. it may help to resolve this patch problem. > > [1] https://www.postgresql.org/message-id/2245576.1728418678%40sss.pgh.pa.us The top level statement beginning doesn't have an impact on this patch. The patch's focus is on the nested statement and RawStmt's location and length are only used to get the nested statement length. I will need the nested statement's location and length no matter what, to handle cases like "COPY (UPDATE ...) to stdout;" and only report the statement within parentheses. The linked patch will allow a simplification: the "if (@$ < 0) @$ = @2;" I've added won't be needed anymore. But I think that's the only possible simplification? I've run the tests on top of the linked patch and there was no change in the regression output.
Hi Jian,
On Thu, Oct 17, 2024 at 5:59 AM jian he <jian.universality@gmail.com> wrote:
> in [3] I mentioned adding "ParseLoc location" to ExplainStmt, then you
> found some problems at [4] with multi statements,
> now I found a way to resolve it.
> I also add "ParseLoc location;" to typedef struct CopyStmt.
> copy (select 1) to stdout;
> I tested my change can tracking
> beginning location and length of the nested query ("select 1")
>
> I didn't touch other nested queries cases yet, so far only ExplainStmt
> and CopyStmt1
> IMHO, it's more neat than your patches.
> Can you give it a try?
I'm not sure about this approach. It moves the responsibility of
tracking the location and length from the nested statement to the top
level statement.
- The location you added in ExplainStmt and CopyStmt has a different
meaning from all others and tracks the nested location and not the
location of the statement itself. This creates some inconsistencies.
- The work will need to be done for all statements with nested
queries: Prepare, Create table as, Declare Cursor, Materialised View.
Whereas by tracking the location of PreparableStatements, there's no
need for additional logic. For example, v8 0002 fixes the existing
behaviour for Prepare statements thanks to SelectStmt's modifications.
I feel like while it looks simpler, it will need more work to handle
all cases while introducing an outlier in how locations are tracked.
On Wed, Oct 23, 2024 at 2:10 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Oct 22, 2024 at 11:34:55AM +0200, Anthonin Bonnefoy wrote:
> > On Tue, Oct 22, 2024 at 7:06 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> Something that
> >> worries me a bit is that this changes makes the code less clean, by
> >> having a SELECT-INTO specific routine called in the parse-analyze
> >> paths, while adding three individual paths in charge of setting
> >> pstate->p_stmt_len and p_stmt_location.
> >
> > I've moved pstate's p_stmt_len and p_stmt_location assignment to
> > transformTopLevelStmt (and also restored transformTopLevelStmt). This
> > will remove the multiple assignment paths.
> >
> >> + n->stmt_len = @3 - @2;
> >>
> >> This specific case deserves a comment.
> >
> > I think I went over this 3 times thinking "maybe I should add a
> > comment here". Added.
>
> Thanks. These changes look OK.
>
> > This is doable. I've moved the query's location and length assignment
> > to the end of transformStmt which will call setQueryLocationAndLength.
> > The logic of manipulating locations and lengths will only happen in a
> > single place. That creates an additional switch on the node's type as
> > a small trade off.
>
> Grouping both assignments in a single setQueryLocationAndLength() is
> less confusing.
>
> > Also, there was an unnecessary cast in analyze.c "result->utilityStmt
> > = (Node *) parseTree;" as parseTree is already a Node. I removed it in
> > 0001.
>
> Indeed. It does not matter one way or another and we have plenty of
> these in the tree.
>
> I have some more minor comments.
>
> - if (@$ < 0) /* see comments for YYLLOC_DEFAULT */
> - @$ = @2;
>
> With 14e5680eee19 now in the tree (interesting timing as this did not
> exist until yesterday), it looks like we don't need these ones
> anymore, no?
>
I commented out
> - if (@$ < 0) /* see comments for YYLLOC_DEFAULT */
> - @$ = @2;
the test suite (regess, pg_stat_statements) still passed.
so i think it's not needed.
I am not sure of the meaning of "@$", though.
I do understand the meaning of "@2" meaning.
I think the 14e5680eee19 will make sure the statement start location
is (not empty, not related to comments).
/*
* transformTopLevelStmt -
* transform a Parse tree into a Query tree.
* This function is just responsible for transferring statement location data
* from the RawStmt into the finished Query.
*/
Query *
transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree)
{
Query *result;
/* Store RawStmt's length and location in pstate */
pstate->p_stmt_len = parseTree->stmt_len;
pstate->p_stmt_location = parseTree->stmt_location;
/* We're at top level, so allow SELECT INTO */
result = transformOptionalSelectInto(pstate, parseTree->stmt);
return result;
}
do we need to change the comments?
since it's transformOptionalSelectInto inner function setQueryLocationAndLength
transfer location data to the finished query.
> While reviewing the whole, I've done some changes, mostly stylistic.
> Please see the attach about them, that I have kept outside of your
> v11-0001 for clarity. I still need to dive deeper into v11-0002 (not
> attached here), but let's take one step at a time and conclude on
> v11-0001 first..
> --
i manually checked out contrib/pg_stat_statements/expected/level_tracking.out
changes in v12-0001 it looks fine.
On 2024-Oct-24, Michael Paquier wrote: > Track more precisely query locations for nested statements I just noticed that this commit broke pgaudit pretty thoroughly. I'm not sure if this means pgaudit needs changes, or this commit needs to be reconsidered in some way; at this point I'm just raising the alarm. (FWIW there are a few other pgaudit-breaking changes in 18, but they don't seem as bad as this one, to the extent that the fixes likely belong into pgaudit.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I apologize for the confusion in my previous responses. There appears to be an error." (ChatGPT)