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

From: Suraj Kharage <suraj(dot)kharage(at)enterprisedb(dot)com>
To: Vaibhav Dalvi <vaibhav(dot)dalvi(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: [PATCH] Add support for INSERT ... SET syntax
Date: 2026-08-31 04:49:27
Message-ID: CAF1DzPXSQvwnzNXbvXNOm_jsFPRQhv9y-QAem2dEbzkjPaSKZg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Aug 28, 2026 at 11:39 AM Vaibhav Dalvi <
vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:

> create table ta(id int primary key, arr int[] default array[1]);
>
> -- classic VALUES: row 3 gets the table's default {1}
> insert into ta values (70, array[1]), (71, array[2]), (72, default);
> -- id | arr
> -- ----+-----
> -- 70 | {1}
> -- 71 | {2}
> -- 72 | {1}
>
> -- new SET syntax: same intent, row 3 just omits arr
> insert into ta set (id=170, arr[1]=1), (id=171, arr[1]=2), (id=172);
> -- ERROR: cannot set an array element to DEFAULT
>

Thanks for reporting this. I have looked into this and feel that this is an
existing behavior for INSERT command.
transformAssignedExpr() unconditionally rejects DEFAULT for any indirection
target (array element or subfield).

For e.g. for plain insert as well it will fail -
postgres(at)2495950=#INSERT INTO ta(id, arr[1]) VALUES (70,1),(71,2),(72,
default);
ERROR: cannot set an array element to DEFAULT
LINE 1: INSERT INTO ta(id, arr[1]) VALUES (70,1),(71,2),(72, default...

It will work in both cases, if we try to insert in arr column.
postgres(at)2495950=#INSERT INTO ta VALUES (70,array[1]),(71,array[2]),(72,
default);
INSERT 0 3
postgres(at)2495950=#select * from ta;
id | arr
----+-----
70 | {1}
71 | {2}
72 | {1}
(3 rows)

postgres(at)2495950=#insert into ta set (id=170, arr=array[1]), (id=171,
arr=array[2]), (id=172, arr=default);
INSERT 0 3
postgres(at)2495950=#select * from ta;
id | arr
-----+-----
170 | {1}
171 | {2}
172 | {1}
(3 rows)

I hope this helps.

Regards,
Suraj

>
> *Classic VALUES: *if a row skips a column (or writes DEFAULT), Postgres
> asks, "What's the default for the whole column arr?" —
> table says {1}, done.
>
> *New INSERT ... SET:* if a row skips arr, but other rows in the same
> statement use arr[1]=..., Postgres instead asks "what's
> the default for just slot 1 of arr?" — and that question has always been
> illegal in Postgres (you can't default one array
> slot). So it errors, even though the row never asked for a slot at all —
> it just wanted the whole column left alone.
>
> *Why is this a bug?: *row 172 never used array-indirection syntax at all.
> it should get the plain, legal,
> whole-column default ({1}), exactly like classic VALUES gives it. Instead,
> the code wrongly forces it through the "set array
> slot 1 to default" path just because other rows in the same statement
> happened to use arr[1]. That path is always illegal
> in Postgres, so a perfectly valid statement fails with an error the user's
> SQL never requested.
>
> Regards,
> Vaibhav
>
> On Thu, Aug 27, 2026 at 4:25 PM Suraj Kharage <
> suraj(dot)kharage(at)enterprisedb(dot)com> wrote:
>
>> 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
>>>>
>>>

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Xuneng Zhou 2026-08-31 04:52:22 Re: Should the WAIT FOR command tag be "WAIT" or "WAIT FOR"?
Previous Message Amit Kapila 2026-08-31 04:39:50 Re: [PATCH] Release replication slot on error in SQL-callable slot functions