Re: Set query_id for query contained in utility statement

On Tue, Oct 22, 2024 at 7:06 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> -Query *
> -transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree)
>
> What's the advantage of removing this routine? Is that because you're
> pushing the initialization of stmt_location and stmt_len into
> transformOptionalSelectInto(), making it mostly moot?

Yeah, the removal of the stmt_location and stmt_len initialization
left the function with only one thing, the call to
transformOptionalSelectInto.

> 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.

> I don't have the best
> understanding of this area yet (need more analysis), but With the
> existing updateRawStmtEnd() and RawStmt also tracking this
> information, I am wondering if we could be smarter with less paths
> manipulating the start locations and lengths. And your patch adds a
> new setQueryStmtLen() that does the same kind of job. Hmm.

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.

> FWIW, I don't feel strongly about 0004 that tries to make the REFRESH
> handling smarter. I am not sure that it even makes sense as-is by
> hacking into a wrapper of pg_get_viewdef_worker() to get the query
> string, leading it to not be normalized. This has also a small
> footprint in 0003. I think that the bits in ExecRefreshMatView()
> should be discarded from 0003, as a result.

Good point on the query not being normalised. I'm fine with dropping
the materialised view part.

Also, there was an unnecessary cast in analyze.c "result->utilityStmt
= (Node *) parseTree;" as parseTree is already a Node. I removed it in
0001.