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

Hi Suraj,

I have a few observations regarding the latest v4 patch:

1. Assigning two different subfields or elements of the same column in a
single row is rejected,
even though the equivalent column-list INSERT syntax accepts it:

postgres=# create type comp_t as (x int, y int);
CREATE TYPE
postgres=# create table t2 (id int primary key, c comp_t);
CREATE TABLE
postgres=# insert into t2 (id, c.x, c.y) values (1, 5, 6);
INSERT 0 1
postgres=# insert into t2 set id=2, c.x=7, c.y=8;
ERROR: column "c" specified more than once
LINE 1: insert into t2 set id=2, c.x=7, c.y=8;
^

The same failure occurs with an array column, without requiring a custom
type:

postgres=# create table t3 (id int primary key, arr int[]);
CREATE TABLE
postgres=# insert into t3 (id, arr[1], arr[2]) values (1, 10, 20);
INSERT 0 1
postgres=# insert into t3 set id=2, arr[1]=30, arr[2]=40;
ERROR: column "arr" specified more than once
LINE 1: insert into t3 set id=2, arr[1]=30, arr[2]=40;
^

2. There is a silent misassignment across rows in multi-row SET syntax:

postgres=# create table t7 (id int primary key, arr int[]);
CREATE TABLE
postgres=# insert into t7 set (id=1, arr[1]=111), (id=2, arr[2]=222);
INSERT 0 2
postgres=# select * from t7;
id | arr
----+-------
1 | {111}
2 | {222}
(2 rows)

Although row 2 explicitly specifies arr[2]=222, the code only tracks
columns by name and
not by the specific element or field targeted. It retains the tracking from
row 1 ("arr → index [1]")
and applies it to subsequent rows. As a result, the value for row 2
silently lands in arr[1] instead
of arr[2], leaving arr[2] as NULL without throwing an error or warning.

This differs from the standard VALUES limitation (e.g., INSERT INTO t7 (id,
arr[1]) VALUES (1,111),(2,222)),
where applying arr[1] to both rows is expected because it is defined once
in the shared header.
In this multi-row SET case, the explicit per-row target is ignored and
silently corrupted rather than being rejected as unsupported.

Regards,
Vaibhav

On Tue, Aug 25, 2026 at 8:40 PM Mario González <gonzalemario(at)gmail(dot)com>
wrote:

> 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
>