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

On Tue, 14 Jul 2026 at 00:39, Suraj Kharage <suraj(dot)kharage(at)enterprisedb(dot)com>
wrote:

> 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.
>
>
lgtm Suraj. I hope you can find a committer that buys you with this idea

--
Mario Gonzalez