On Tue, 14 Jul 2026 at 00:39, Suraj Kharage <[email protected]> wrote:
> 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. > > lgtm Suraj. I hope you can find a committer that buys you with this idea -- Mario Gonzalez
