Re: Set query_id for query contained in utility statement - Mailing list pgsql-hackers

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.