Thanks Mario for the review. On Mon, Jul 13, 2026 at 12:18 AM Mario González Troncoso < [email protected]> wrote:
> diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c > index 2a2e00b372e..11cb4fcd2da 100644 > --- a/src/backend/nodes/nodeFuncs.c > +++ b/src/backend/nodes/nodeFuncs.c > @@ -4370,6 +4370,8 @@ raw_expression_tree_walker_impl(Node *node, > return true; > if (WALK(stmt->selectStmt)) > return true; > + if (WALK(stmt->setClauseList)) > + return true; > if (WALK(stmt->onConflictClause)) > > you used `stmt->setClauseList` however, I read the entire > `raw_expression_tree_walker_impl` > function and it seems we don't mix "clause" with "List" in the variable > names. Reading the whole file, I just found "targetList" and "valuesList". > > If you get my point, maybe you could use "setClause" only? I know that > sounds like something that exists in setter/getters stuff. Like we're > setting a clause up but would it be worth looking for a new variable name? > I personally think so. Actually, after reading > `src/include/nodes/parsenodes.h`, > I think we should go for a change. > Renamed setClauseList as per your suggestion. > ---- > Also, in src/backend/parser/analyze.c we can change a lot of those foreach > by foreach_node, however, I need to ask, did you have a reason to not use > foreach_node() when you first wrote the code? Maybe I'm missing something. > Because this patch is on a commitfest already, I didn't want to send a > patch we might need to squash if I'm right afterwards. That's why I'd like > to show you what I did: > https://github.com/postgres/postgres/commit/7be0538f2a5d916f2fb4a39764985b358cf6d379 > If you like I could send a v4- with the squashed version. > > diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c > index 70c75d0bb20..d2f5b0edcc8 100644 > --- a/src/backend/parser/analyze.c > +++ b/src/backend/parser/analyze.c > @@ -679,31 +679,23 @@ transformInsertSetClause(ParseState *pstate, List > *setClauseList, > { > List *all_cols = NIL; /* List of all unique > column names */ > List *valuesLists = NIL; > - ListCell *outer_lc; > - ListCell *lc; > > /* > * First pass: collect all unique column names from all rows. > * We need to scan all rows first to determine the complete set of > columns. > * Also check for duplicate columns within each row. > */ > - foreach(outer_lc, setClauseList) > + foreach_node(List, set_clause, setClauseList) > { > - List *set_clause = (List *) lfirst(outer_lc); > List *row_cols = NIL; /* Columns seen in > this row */ > - ListCell *set_lc; > [...] > Used foreach_node as per your suggestion. I have addressed your review comments in the attached v4 patch. Thanks & Regards, Suraj kharage, enterprisedb.com <https://www.enterprisedb.com/>
v4-0001-Add-support-for-INSERT-.-SET-syntax.patch
Description: Binary data
