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

Thank you Vaibhav for the review.

I have fixed these issues in the attached v4 patch. Please have a look.
--

Thanks & Regards,
Suraj kharage,

enterprisedb.com <https://www.enterprisedb.com/>

On Wed, Aug 26, 2026 at 6:32 PM Vaibhav Dalvi <
vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:

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