More bugs when binding insert with unaligned source schema

[y-wei]

Describe the bug

We may need to better deal with unaligned source and insert schema.

To Reproduce

dev=> create table tn (v1 int, v2 int);
CREATE_TABLE
dev=> create table ipt (v1 int);
CREATE_TABLE
dev=> insert into tn select * from ipt;
ERROR:  QueryError: Bind error: INSERT has more target columns than expressions
dev=> insert into tn (v1) select * from ipt;
ERROR:  QueryError: Bind error: INSERT has more target columns than expressions

Expected behavior

The two insert should be executed per Postgres.

Additional context

No response

[y-wei]

I don't see a solution under current design and maybe some discussion is needed 🥲, together with #9012

[BugenZhao]

Good point. Actually, I think it's okay that we only support a subset of the DML syntax from Postgres. For example, not supporting only providing the values for a prefix of columns. However, it makes no sense that the second statement cannot be executed. 😄

Would you mind issuing a fix for this?

[y-wei]

Yes I'd love to, yet I haven't figured out a way.😭 Can we pad manual Exprimpls (in this case, nulls) to BoundQuery? If so, how?

[y-wei]

I think this is a side effect of #8770. This PR actually forced the input schema to be aligned with the table schema (by align I mean the field at corresponding index can be casted), while it shouldn't, if the input is a query rather than values. Above is a failed case while I find one more interesting one:

dev=> create table t2 (v1 int, v2 int);
CREATE_TABLE
dev=> insert into t2 values (1, 2);
INSERT 0 1
dev=> insert into t2 (v1) select * from t2;
INSERT 0 1
dev=> select * from t2;
 v1 | v2 
----+----
  1 |  2
  1 |  2
(2 rows)

[y-wei]

My idea is that

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

Lines 315 to 326 in 5a61944

	// columns that are in the target table but not in the provided target columns
	if col_indices_to_insert.len() != cols_to_insert_in_table.len() {
	for col in cols_to_insert_in_table {
	if let Some(col_to_insert_idx) = col_name_to_idx.get(col.name()) {
	if *col_to_insert_idx != usize::MAX {
	col_indices_to_insert.push(*col_to_insert_idx);
	}
	} else {
	unreachable!();
	}
	}
	}

shall be removed and (parts of) the "padding nulls" logic in values shall be recovered. Do you have any concerns or points to add? @xxchan @broccoliSpicy

[y-wei]

Actually I'd prefer postpone the logic of padding nulls to the executor and the binder just gives a list of column indices that need to filled with null (or maybe other non-null default value as I'm working on😇). Otherwise I don't see a way to solve the insert from query case. cc. @BugenZhao

[broccoliSpicy]

I think this is a side effect of #8770. This PR actually forced the input schema to be aligned with the table schema (by align I mean the field at corresponding index can be casted), while it shouldn't, if the input is a query rather than values. Above is a failed case while I find one more interesting one:

dev=> create table t2 (v1 int, v2 int);
CREATE_TABLE
dev=> insert into t2 values (1, 2);
INSERT 0 1
dev=> insert into t2 (v1) select * from t2;
INSERT 0 1
dev=> select * from t2;
 v1 | v2 
----+----
  1 |  2
  1 |  2
(2 rows)

I don't see how schema alignment gets involved here...
but our current behavior for this query here is semantically wrong, I suppose?
it might be helpful if we have a comprehensive picture of postgres's insert semantic and a conclusion of to what degree risingwave conforms

[y-wei]

#9208