Re: [PATCH] Add support for INSERT ... SET syntax - Mailing list pgsql-hackers

From Vaibhav Dalvi
Subject Re: [PATCH] Add support for INSERT ... SET syntax
Date
Msg-id CA+vB=AEbDGcAiJ54iu0arRje3Wnm++HUi4EmDQhXsgA6u-gaxQ@mail.gmail.com
Whole thread
In response to [PATCH] Add support for INSERT ... SET syntax  (Suraj Kharage <suraj.kharage@enterprisedb.com>)
List pgsql-hackers
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?

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.kharage@enterprisedb.com> wrote:

On Fri, Aug 28, 2026 at 11:39 AM Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.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@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@2495950=#INSERT INTO ta VALUES (70,array[1]),(71,array[2]),(72, default);
INSERT 0 3
postgres@2495950=#select * from ta;
 id | arr
----+-----
 70 | {1}
 71 | {2}
 72 | {1}
(3 rows)


postgres@2495950=#insert into ta set (id=170, arr=array[1]), (id=171, arr=array[2]), (id=172, arr=default);
INSERT 0 3
postgres@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.kharage@enterprisedb.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, 



On Wed, Aug 26, 2026 at 6:32 PM Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.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@gmail.com> wrote:
On Tue, 14 Jul 2026 at 00:39, Suraj Kharage <suraj.kharage@enterprisedb.com> wrote:
Thanks Mario for the review.

On Mon, Jul 13, 2026 at 12:18 AM Mario González Troncoso <gonzalemario@gmail.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

pgsql-hackers by date:

Previous
From: Rafia Sabih
Date:
Subject: Re: Bypassing cursors in postgres_fdw to enable parallel plans
Next
From: Floris Van Nee
Date:
Subject: Re: OOM kill during planning - pg_terminate_backend is ignored