Re: Set query_id for query contained in utility statement

On Wed, Oct 9, 2024 at 4:49 PM Anthonin Bonnefoy
<anthonin(dot)bonnefoy(at)datadoghq(dot)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