Re: [PATCH] Add support for INSERT ... SET syntax

Thanks Mario for the review.

On Mon, Jul 13, 2026 at 12:18 AM Mario González Troncoso <
gonzalemario(at)gmail(dot)com> 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/>