Re: Set query_id for query contained in utility statement

On Wed, Oct 23, 2024 at 8:10 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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?

Yes, 14e5680eee19 makes the if(@$<0) unnecessaries. I saw this
yesterday and planned to remove them but you beat me to it.

> + ParseLoc p_stmt_location; /* start location, or -1 if unknown */
> + ParseLoc p_stmt_len; /* length in bytes; 0 means "rest of string" */
>
> So, the reason why these two fields are added to the ParseState is
> that the lower layers in charge of the query transforms don't have to
> RawStmt so as the location and lengths can be adjusted when queries
> are between parenthesis. I was first wondering if we should push
> RawStmt deeper into the argument stack, but based on the stmt_location
> assignments for the DMLs and WITH, storing that in the ParseState
> looks neater. The patch is lacking a description of these two fields
> at the top of the ParseState structure in parse_node.h. This stuff
> needs to be explained, aka we need them to be able to adjust the
> locations and lengths depending on inner clauses of the queries we are
> dealing with, or something like that.

True, added comments for both fields.

On Wed, Oct 23, 2024 at 10:36 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> /*
> * 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.

Yes, transformTopLevelStmt's comment was outdated. I've updated it.

An issue I've realised with calling setQueryLocationAndLength in
transformStmt: it was possible for pstate's location and length to be
0, leading to a Query with negative size. This wouldn't be visible in
tests since the only time Query's locations are used (AFAIK) is during
post_parse_hook which always have pstate's location information.
However, this is definitely something to avoid. I've added an
additional Assert(qry->stmt_len >= 0); to catch that. The fix is to
not do anything when pstate doesn't have location information.

This also answers another issue I was wondering about. Should the
child's parsestate inherit the location information when
make_parsestate is called? That would be incorrect since this is used
for sub-statement, pstate should reflect the size of the whole
sub-statement. However, since this is unused, it is fine to leave the
child parser with unset location data, which would in turn leave the
statement's location unset in setQueryLocationAndLength.

Patch includes Micheal changes. I've left out 0002 for now to focus on 0001.