returning fails matching specified target columns

[y-wei]

Describe the bug

No response

To Reproduce

dev=> create table tn (v1 int, v2 int);
CREATE_TABLE
dev=> insert into tn (v2) values (1) returning *;
 v1 | v2 
----+----
  1 |   
(1 row)

INSERT 0 1

Expected behavior

dev=> create table tn (v1 int, v2 int);
CREATE_TABLE
dev=> insert into tn (v2) values (1) returning *;
 v1 | v2 
----+----
   | 1  
(1 row)

INSERT 0 1

Additional context

No response

[broccoliSpicy]

when binding returing_list: the InputRef's index is derived from the original table's column layout

risingwave/src/frontend/src/binder/insert.rs

Line 133 in e294250

let (returning_list, fields) = self.bind_returning_list(returning_items)?;

but the inserted values may have a different column layout.
we can rearrange the inserted values to align with the original table's column layout, but this kind of memory shuffle may drag performance.
I suppose we can address this issue by rewire the returning InputRef's index and then disable column pruning for insertion -> projection(or disable all optimization for insertion->projection?)

if we decided to disable column pruning for insertion -> projection, the LogicalProject's logic can be entangled, can we just add a new type of PlanNode called InsertionReturningProject?

[y-wei]

Excellent analysis👏 That said I find introducing a new PlanNode maybe somewhat overkill?
I'm wondering if add one more LogicalProject::with_mapping here above L43 is sufficient?

risingwave/src/frontend/src/planner/insert.rs

Lines 42 to 44 in 5a61944

	if returning {
	plan = LogicalProject::create(plan, insert.returning_list);
	}

[broccoliSpicy]

Excellent analysisclap That said I find introducing a new PlanNode maybe somewhat overkill? I'm wondering if add one more LogicalProject::with_mapping here above L43 is sufficient?

risingwave/src/frontend/src/planner/insert.rs

Lines 42 to 44 in 5a61944

	if returning {
	plan = LogicalProject::create(plan, insert.returning_list);
	}

I will happily try your approach later
but what's frightening me is column pruning, even if we had the correct InputRef.index,
for example,

create table t (v1 int, v2 real);
insert into t (v2) values(1.5) returning v1;

in this query, since we don't need the v2 values in projection, the LogicalProject's optimizer may mess up our insert values, perhaps even lose it, but the real insertion execution haven't begin yet

[y-wei]

@broccoliSpicy After reflection I realize that your idea looks better👍. Would you like to fix it?

[broccoliSpicy]

sure, gladly, but we better finalize our approach first. would you mind some comments @st1page

[broccoliSpicy]

I implemented InputRef index rewiring and found some tricky bug.
this approach won't work.

 dev=> create table tn (v1 int, v2 int);                                     
CREATE_TABLE
dev=> insert into tn (v2) values (1) returning v2 + 1;
 ?column? 
----------
        2
(1 row)

INSERT 0 1
dev=> insert into tn (v2) values (1) returning v2+1. v1;
 v1 
----
  2
(1 row)

INSERT 0 1
dev=> insert into tn (v2) values (1) returning v2+1, v1;
 ?column? | v1 
----------+----
        2 |   
(1 row)

INSERT 0 1
dev=> insert into tn (v2) values (1,5) returning v2+1, v1;                  
ERROR:  QueryError: Bind error: INSERT has more expressions than target columns
dev=> insert into tn (v2) values (1.5) returning v2+1, v1;                  
 ?column? | v1 
----------+----
        3 |   
(1 row)

INSERT 0 1
dev=> insert into tn (v2) values (1.5) returning v2+1, v1;
 ?column? | v1 
----------+----
        3 |   
(1 row)

INSERT 0 1
dev=> insert into tn (v2) values (1.5) returning v2 + 1.1;           │ 0:psql │                                                                     │
│  ?column?                                                            └────────┘                                                                     │
│ ----------                                                                                                                                          │
│       3.1                                                                                                                                           │
│ (1 row)   

I didn't check it through but I guess during function binding, we infer the function's input and return type based on the target table's schema, then do some type coercion
I am afraid more and more subtle bug will emerge with this approach...
index rewiring was supposed to be a hack but I guess it requires a lot of delicate patches.

I am thinking about mock a binding context before we bind returning list now

[broccoliSpicy]

ha, my bad, the above behavior is actually in conform with postgres.
I guess both index rewiring and mock binding context would work.
still implemented mock binding context though

[st1page]

I used to be unfamiliar with this grammar and I find it is more powerful than I thought 🥵

dev=# with cte as (insert into t values (1, 20, 400, 5000), (2,60,600,6000) returning x,z) select sum(x) from cte;
 sum 
-----
  80
(1 row)

dev=# explain with cte as (insert into t values (1, 20, 400, 5000), (2,60,600,6000) returning x,z) select sum(x) from cte;
                                 QUERY PLAN                                 
----------------------------------------------------------------------------
 Aggregate  (cost=0.07..0.08 rows=1 width=8)
   CTE cte
     ->  Insert on t  (cost=0.00..0.03 rows=2 width=16)
           ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=16)
   ->  CTE Scan on cte  (cost=0.00..0.04 rows=2 width=4)
(5 rows)

[fuyufjh]

ha, my bad, the above behavior is actually in conform with postgres.
I guess both index rewiring and mock binding context would work.
still implemented mock binding context though

Hey, may I ask any new updates?

[broccoliSpicy]

this issued is fixed by #11080.