On Tue, Oct 22, 2024 at 7:06 AM Michael Paquier <mich...@paquier.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.
v11-0001-Track-location-to-extract-relevant-part-in-neste.patch
Description: Binary data
v11-0002-Set-query_id-for-queries-contained-in-utility-st.patch
Description: Binary data
