| From: | Vaibhav Dalvi <vaibhav(dot)dalvi(at)enterprisedb(dot)com> |
|---|---|
| To: | Suraj Kharage <suraj(dot)kharage(at)enterprisedb(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Add support for INSERT ... SET syntax |
| Date: | 2026-10-12 12:01:39 |
| Message-ID: | CA+vB=AHZqOPKWt2wZ171SHNW9XvLjisxSU8_21TU6AV-CCx=Qw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Suraj,
This makes sense to me.
Regards,
Vaibhav
On Thu, Sep 10, 2026 at 2:58 PM Suraj Kharage <
suraj(dot)kharage(at)enterprisedb(dot)com> wrote:
>
> On Wed, Sep 2, 2026 at 6:10 PM Vaibhav Dalvi <
> vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Thanks Suraj for the clarification. I see two issues here, both stemming
>> from the same root cause:
>>
>> *Issue #1: A row that skips an array column entirely gets wrongly
>> rejected*
>> Setup:
>> create table ta(id int primary key, arr int[] default array[1]);
>>
>> Command:
>> insert into ta set (id=70, arr[1]=1), (id=71, arr[1]=2), (id=72);
>>
>> Actual result:
>> ERROR: cannot set an array element to DEFAULT
>>
>> What's wrong: row id=72 doesn't mention arr in any way, no slot, no whole
>> column, nothing.
>> In plain English, it's saying "I don't care about arr, just use whatever
>> it normally is."
>> That should give it the column's default ({1} here, since one was
>> declared, or NULL if not)
>> exactly like skipping any other ordinary column works fine. Instead,
>> because other rows in the
>> same statement touch arr[1], the code silently treats row 72's silence as
>> if it had explicitly written
>> arr[1] = DEFAULT, and setting a single array slot to DEFAULT is always
>> that asked for nothing
>> gets blamed for a request it never made, and the whole statement fails.
>>
>> Expected result:
>> id | arr
>> ----+-----
>> 70 | {1}
>> 71 | {2}
>> 72 | {1} -- the column's default, same as ny other statement
>>
>> *Issue #2: Mixing "whole column" and "one slot" across different rows
>> gets wrongly rejected*
>> Setup:
>> create table t1(id int primary key, arr int[] default array[1]);
>>
>> Command:
>> insert into t1 set (id=1, arr[1]=1), (id=2, arr=default);
>>
>> Actual result:
>> ERROR: column "arr" specified more than once
>>
>> What's wrong: Row 1 wants to set slot 1 of its own array. Row 2 wants its
>> own array to just be the default.
>> These are two completely separate rows with two completely separate array
>> values, nothing about them
>> actually conflicts. However, the code doesn't check rows independently;
>> it first lumps every row's arr-related
>> target into one shared bucket, and only then checks for conflicts. In
>> that shared bucket, "the whole column"
>> and "just slot 1" look like a contradiction, even though they aren't,
>> since they belong to two different rows, not
>> one row saying two contradictory things about the same value.
>>
>> Expected result:
>> id | arr
>> ----+-----
>> 1 | {1}
>> 2 | {1} -- the column's default
>>
>> Another thought here is: should we restrict array/field indirection
>> targets instead like below?
>>
> Thanks for looking into this.
>
> Both issues share the same root cause: multi-row SET merges all rows into
> one shared column list,
> so once a column uses arr[1]-style indirection, every row must supply that
> exact same target.
> I think if we make this work, then need bypassing core Postgres
> INSERT/rewrite internals logic and it becomes complex.
> Postgres builds one assignment expression per column and reuses it for
> every row (via Vars into the VALUES RTE),
> and the rewriter refuses to merge a whole-column value with an
> element/field assignment on the same column
> ("multiple assignments to same column" in process_matched_tle()).
>
> So I went with your suggested restriction, but narrower: if all rows set
> the same array/field target (e.g. all rows set arr[1]),
> it keeps working as before. But if rows disagree — one row skips it, or
> sets the whole column, or targets a different element/field —
> we now raise one clear error.
> Updated docs and tests accordingly.
>
> Please let me know your thoughts on the same.
>
> Regards,
> Suraj
>
>
>
>> insert into t1 set (id=1, arr[1]=1), (id=2, arr=default);
>> ERROR: array/field indirection targets are not supported in multi-row
>> INSERT ... SET
>>
>> Per my knowledge, MySQL and MariaDB support this INSERT ... SET syntax:
>> - Standard MySQL does not support multi-row INSERT ... SET directly
>> using SET ... SET.
>> In MySQL, INSERT ... SET is strictly single-row:
>>
>> INSERT INTO employees SET id = 1, name = 'Alice', salary = 70000;
>>
>> - MariaDB extends the standard INSERT ... SET syntax to support multiple
>> rows by repeating the SET clause for each row:
>>
>> INSERT INTO employees
>> SET id = 1, name = 'Alice', salary = 70000,
>> SET id = 2, name = 'Bob', salary = 80000,
>> SET id = 3, name = 'Charlie', salary = 75000;
>>
>> The point is to check if we can think in that direction?
>>
>> Regards,
>> Vaibhav
>>
>> On Mon, Aug 31, 2026 at 10:20 AM Suraj Kharage <
>> suraj(dot)kharage(at)enterprisedb(dot)com> wrote:
>>
>>>
>>> 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
>>>>>>>
>>>>>>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nazir Bilal Yavuz | 2026-10-12 12:04:04 | Re: PANIC serves too many masters |
| Previous Message | David Rowley | 2026-10-12 11:35:31 | Re: Reducing relcache memory usage: deduping index shapes |