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 < [email protected]> 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 <[email protected]> > wrote: > >> On Tue, 14 Jul 2026 at 00:39, Suraj Kharage < >> [email protected]> wrote: >> >>> Thanks Mario for the review. >>> >>> On Mon, Jul 13, 2026 at 12:18 AM Mario González Troncoso < >>> [email protected]> 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 >> >
v5-0001-Add-support-for-INSERT-.-SET-syntax.patch
Description: Binary data
