Support `insert_into_select` for Postgres table #688

YuweiXiao · 2025-03-21T14:56:08Z

Partially fixes #30

YuweiXiao · 2025-03-21T14:59:17Z

hi~ @JelteF The test cases aren't fully covered yet. would appreciate some feedback before finalizing the details. Thanks!

src/pgduckdb_planner.cpp

JelteF · 2025-03-21T15:20:09Z

src/pgduckdb_hooks.cpp

+			if (query->onConflict || query->returningList) {
+				elog(elevel, "DuckDB does not support INSERT ON CONFLICT or RETURNING on Postgres tables");
+				return false;
+			}


Why not allow these? Do they not work? I would expect that if we insert using postgres its functionality that thees things would automatically work.

To be clear, not saying that this should be implemented in the first version of this feature. But I'd at least like a code comment explaining why we do not support it.

Ah, block them since I haven't tested them yet. I've done some initial testing, and RETURNING works out of box while onConflict doesn't. Will dig deeper into the WHY and add comments to clarify.

ON CONFLICT is also working

src/pgduckdb_hooks.cpp

JelteF

CI is failing in many places, because duckdb.force_execution is true in our tests. Apparently something breaks when selecting from generate_series() in duckdb for a PG INSERT.

JelteF · 2025-03-21T15:32:01Z

My example code from #30:

create table t(a int, x text);
-- 1. easiest to make work (still not super easy)
insert into t select r['a']::int, r['x']::text from duckdb.query($$ SELECT 1 a, 'abc' x $$) r;

Fails with this error:

ERROR:  42804: table row type and query-specified row type do not match
DETAIL:  Table has type text at ordinal position 2, but query expects character varying.
LOCATION:  ExecCheckPlanOutput, nodeModifyTable.c:212

I think that would be worked around by #583, but then the opposite would hold (i.e. inserting into a varchar column woudl fail). So I think we need some logic to make sure that the types match against eachother, and maybe add some code to convert the type that we get back from DuckDB to the type that PG wants for the INSERT. I think that would also fix the generate_series() issue that all of the tests are failing on currently.

YuweiXiao · 2025-03-22T09:40:21Z

I think that would be worked around by #583, but then the opposite would hold (i.e. inserting into a varchar column woudl fail). So I think we need some logic to make sure that the types match against eachother, and maybe add some code to convert the type that we get back from DuckDB to the type that PG wants for the INSERT. I think that would also fix the generate_series() issue that all of the tests are failing on currently.

YES, the type check and conversion are substantial. I manually added an Expr to handle the coerce and it seems working so far (e.g., varchar and generate_series issue). I'll add more test cases to ensure if I haven't missed (or broken) anything, and code re-org (& cleanup) are also in progress. Thanks for the feedback, @JelteF !

YuweiXiao · 2025-03-24T12:38:34Z

The MR is ready for review~ @JelteF Please take a look when you have a chance.

Currently, the regression is failing for hugeint_numeric. The issue seems related to the following behavior, which is reproducible in the main branch. Do you have any insights on this?

select * from duckdb.query($$ SELECT a FROM (VALUES (32942348563242.111222333444555666777888::numeric(38,24)), (NULL::numeric), (123456789.000000000000000000000001::numeric(38,24))) t(a) $$);

                   a
----------------------------------------
 32942348563242.11122233344455566677789  (correct one should be `32942348563242.111222333444555666777888`

      123456789.00000000000000000000000 (correct one should be 123456789.000000000000000000000001)
(3 rows)

update: DuckDB cli has the exact same output.

include/pgduckdb/pgduckdb_planner.hpp

src/pgduckdb_planner.cpp

JelteF · 2025-04-16T19:49:57Z

Currently, the regression is failing for hugeint_numeric. The issue seems related to the following behavior, which is reproducible in the main branch. Do you have any insights on this?

I think you can probably fix the test by changing the plain NULL to NULL::NUMERIC(38, 24). It seems at least that that solves the issue when running your reproducing example in the duckdb CLI.

JelteF · 2025-04-16T19:53:40Z

src/pgduckdb_planner.cpp

+			continue;
+		}
+
+		if (attr->atttypid == BYTEAOID || attr->atttypid == XMLOID || pgduckdb::pg::IsDomainType(attr->atttypid) ||


Instead of hardcoding some unsupported types, I think we should instead determine the allowed types. Also, why are these types unsupported? Because we do support BYTEA as a type for regular duckdb execution, so that seems surprising that it doesn't work here.

Changed the behavior to whitelist. As long as the Postgres type can be converted to a DuckDB type using ConvertPostgresToDuckColumnType, it will be allowed.

test/regression/expected/postgres_insert_select.out

JelteF · 2025-04-16T20:13:08Z

src/pgduckdb_planner.cpp

+	}
+
+	if (!select_rte) {
+		elog(elevel, "DuckDB does not support INSERT without a subquery");


There are no tests for this error case afaict

src/pgduckdb_planner.cpp

JelteF · 2025-04-16T20:14:43Z

test/regression/expected/postgres_insert_select.out

+
+DROP TABLE tbl;
+-- case: ON CONFLICT DO NOTHING
+CREATE TABLE tbl (a int PRIMARY KEY, b text);


What happens with a SERIAL column or a column with GENERATED ALWAYS AS IDENTITY.

I added a regression for them, and it works perfectly.

JelteF · 2025-04-17T08:28:24Z

To set expectations: I think this is a very cool feature and your implementation looks like the right direction. But it is relatively complex and impactful, so I would like to give it an appropriate round of testing and "baking" (aka testing by merging it and having it on the main branch for a while). Since we want to release 1.0 somewhat soon, I don't think it's realistic to still do that before the 1.0 release. So I added this to the new 1.1 milestone instead. I also won't spend time reviewing this PR in depth until then.

YuweiXiao · 2025-04-17T13:03:46Z

To set expectations: I think this is a very cool feature and your implementation looks like the right direction. But it is relatively complex and impactful, so I would like to give it an appropriate round of testing and "baking" (aka testing by merging it and having it on the main branch for a while). Since we want to release 1.0 somewhat soon, I don't think it's realistic to still do that before the 1.0 release. So I added this to the new 1.1 milestone instead. I also won't spend time reviewing this PR in depth until then.

Totally agree, 1.1 makes sense. btw, any target date for 1.0 release?

JelteF · 2025-04-17T13:09:29Z

any target date for 1.0 release?

The current plan is to do it quickly after the DuckDB 1.3.0 release, so early May. But dates may obviously slip.

handle type conversion handle column mapping & cleanup fix schedule code cleanup & add restriction revert uncessary changes

JelteF reviewed Mar 21, 2025

View reviewed changes

src/pgduckdb_planner.cpp Outdated Show resolved Hide resolved

JelteF reviewed Mar 21, 2025

View reviewed changes

src/pgduckdb_hooks.cpp Outdated Show resolved Hide resolved

JelteF requested changes Mar 21, 2025

View reviewed changes

YuweiXiao marked this pull request as draft March 22, 2025 09:29

YuweiXiao changed the title ~~[DRAFT] Support insert_into_select for Postgres(heap) table~~ [DRAFT] Support insert_into_select for Postgres table Mar 22, 2025

YuweiXiao force-pushed the heap_insert_select branch from dabffc2 to e7f7aaf Compare March 24, 2025 09:19

YuweiXiao marked this pull request as ready for review March 24, 2025 12:38

YuweiXiao changed the title ~~[DRAFT] Support insert_into_select for Postgres table~~ Support insert_into_select for Postgres table Mar 24, 2025

YuweiXiao requested a review from JelteF April 2, 2025 09:56

JelteF reviewed Apr 15, 2025

View reviewed changes

include/pgduckdb/pgduckdb_planner.hpp Outdated Show resolved Hide resolved

JelteF reviewed Apr 15, 2025

View reviewed changes

src/pgduckdb_planner.cpp Outdated Show resolved Hide resolved

JelteF reviewed Apr 16, 2025

View reviewed changes

test/regression/expected/postgres_insert_select.out Outdated Show resolved Hide resolved

JelteF reviewed Apr 16, 2025

View reviewed changes

JelteF added this to the 1.1.0 milestone Apr 17, 2025

This was referenced May 5, 2025

Writing into Postgres tables #30

Open

Support parallel DuckDB threads for Postgres table scan #762

Merged

Yuwei Xiao added 2 commits May 8, 2025 06:11

initial implement for heap table insert_select

0be768f

handle type conversion handle column mapping & cleanup fix schedule code cleanup & add restriction revert uncessary changes

more regress

1b84967

YuweiXiao force-pushed the heap_insert_select branch from 3be8e43 to 1b84967 Compare May 8, 2025 06:12

more regress

93e8b8a

YuweiXiao force-pushed the heap_insert_select branch from 749203f to 93e8b8a Compare May 8, 2025 08:05

Yuwei Xiao added 2 commits May 8, 2025 08:19

format

351da7a

fix regress

d54ca6b

Support insert_into_select for Postgres table #688

Are you sure you want to change the base?

Support insert_into_select for Postgres table #688

Uh oh!

Conversation

YuweiXiao commented Mar 21, 2025 • edited by JelteF Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YuweiXiao commented Mar 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JelteF left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF commented Mar 21, 2025

Uh oh!

YuweiXiao commented Mar 22, 2025

Uh oh!

YuweiXiao commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelteF commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF commented Apr 17, 2025

Uh oh!

YuweiXiao commented Apr 17, 2025

Uh oh!

JelteF commented Apr 17, 2025

Uh oh!

Uh oh!

Support `insert_into_select` for Postgres table #688

Support `insert_into_select` for Postgres table #688

YuweiXiao commented Mar 21, 2025 •

edited by JelteF

Loading

JelteF left a comment •

edited

Loading

YuweiXiao commented Mar 24, 2025 •

edited

Loading