Support extended protocol and prepared statements #147

JelteF · 2024-08-22T14:53:05Z

Previously prepared statements and the extended protocol were broken. We need to support this feature because pretty much every client library uses it. The reason that prepared statements did not work is that for prepared statements to work, a Postgres plan needs to be fully copyable using Postgres functionality. We were previously storing the DuckDB plan in the Postgres plan, but since Postgres did not know how to copy the DuckDB plan it would fail.

To work around that problem we now store the full Postgres Query structure inside the DuckDB CustomScan node. Then during execution we use this Query structure to re-plan the query during execution. This works but it has the big downside that we're now planning the query twice in DuckDB, once during planning to get the the column types of the result and once during execution. For now that seems acceptable, so we can at least support prepared statements and the extended protocol. In the future we might want to do something smarter though, like serializing and deserializing the DuckDB plan that is created at planning time.

Note: This does not support all our supported types as arguments to a prepared statement yet. After merging I'll open separate issue to track adding the rest of these types.

Fixes #118

src/pgduckdb_duckdb.cpp

JelteF · 2024-08-23T16:29:28Z

src/pgduckdb_types.cpp

+	case FLOAT8OID: {
+		return duckdb::Value::DOUBLE(DatumGetFloat8(value));
+	}
+	// case duckdb::LogicalTypeId::DECIMAL: {


As you can see there's a few types that we support in other places that I didn't implement here yet. Also if you look at the added tests you'll see that there are a bunch of issues with floats and string arguments for some reason not working as expected in comparisons.

In my opinion it's important to support them and fix those errors, but I don't think it necessarily needs to be part of this PR. This PR is already a bit larger than I'd like it to be. So I'd rather merge this soonish if we're happy with the general design, and then fix the remaining issues in follow up PRs. Especially since making DuckDB a singleton is a significant change in architecture that I feel would benefit from early use by developers to find bugs.

This function is only used for binding? Maybe rename it, also we don't need to have commented out what is not supported (probably ticket should be created at this point).

I renamed the function a bit and removed the commented out code. After we merge this I'll create a ticket to track support for the other types.

src/pgduckdb_planner.cpp

Y--

Overall LGTM, thanks for picking up! Would like to hear what other thinks too cc @Tishj @mkaruza

src/pgduckdb_node.cpp

src/pgduckdb_types.cpp

mkaruza

Personally i would like to see this PR split int two parts. One is which enabled singleton duckdb object and another that deals with binding.

src/pgduckdb_duckdb.cpp

mkaruza · 2024-09-02T08:18:43Z

src/pgduckdb_types.cpp

+	case FLOAT8OID: {
+		return duckdb::Value::DOUBLE(DatumGetFloat8(value));
+	}
+	// case duckdb::LogicalTypeId::DECIMAL: {


This function is only used for binding? Maybe rename it, also we don't need to have commented out what is not supported (probably ticket should be created at this point).

@Y--

Based on work by @Y-- and PR #102 and #147. This basically extracts the work from there into a separate PR, so it can be merged before some of the open TODOs related to prepared statements/extended protocol are resolved. Co-authored-by: Y. <[email protected]>

The base branch was changed.

JelteF · 2024-09-17T11:34:58Z

test/pycheck/explain_test.py

@@ -2,7 +2,7 @@


 def test_explain(cur: Cursor):
-    cur.sql("CREATE TABLE test_table (id int primary key, name text)")
+    cur.sql("CREATE TABLE test_table (id int, name text)")


I needed to change this definition to not create an index because index scans are currently broken, see #183 for details.

JelteF force-pushed the duckdb-singleton branch 2 times, most recently from 3bdc96b to 98376a1 Compare August 22, 2024 16:29

JelteF commented Aug 22, 2024

View reviewed changes

src/pgduckdb_duckdb.cpp Outdated Show resolved Hide resolved

JelteF force-pushed the duckdb-singleton branch from 24c63d4 to 0a41720 Compare August 23, 2024 14:32

JelteF changed the title ~~Make DuckDB a connection singleton to support extended protocol~~ Make DuckDB connection a singleton to support extended protocol Aug 23, 2024

JelteF requested review from Tishj and mkaruza August 23, 2024 16:21

JelteF commented Aug 23, 2024

View reviewed changes

src/pgduckdb_planner.cpp Outdated Show resolved Hide resolved

JelteF requested a review from Y-- August 23, 2024 16:38

Y-- previously approved these changes Aug 27, 2024

View reviewed changes

src/pgduckdb_node.cpp Outdated Show resolved Hide resolved

src/pgduckdb_node.cpp Outdated Show resolved Hide resolved

src/pgduckdb_types.cpp Outdated Show resolved Hide resolved

mkaruza requested changes Sep 2, 2024

View reviewed changes

JelteF force-pushed the duckdb-singleton branch 2 times, most recently from 2136ffe to 7a5087a Compare September 12, 2024 11:27

JelteF mentioned this pull request Sep 12, 2024

Make duckdb a singleton #174

Merged

JelteF force-pushed the duckdb-singleton branch from 7a5087a to 829bcfb Compare September 12, 2024 11:55

JelteF changed the base branch from main to duckdb-singleton-only September 12, 2024 11:55

Base automatically changed from duckdb-singleton-only to main September 12, 2024 13:42

JelteF force-pushed the duckdb-singleton branch from 829bcfb to bbf508e Compare September 16, 2024 13:50

JelteF commented Sep 17, 2024

View reviewed changes

JelteF mentioned this pull request Sep 17, 2024

update duckdb to v1.1.0 #182

Merged

JelteF changed the title ~~Make DuckDB connection a singleton to support extended protocol~~ Support extended protocol and prepared statements Sep 17, 2024

wuputah added this to the 0.1.0 milestone Sep 17, 2024

JelteF mentioned this pull request Sep 18, 2024

Comparisons with double precision type return wrong results #189

Closed

JelteF force-pushed the duckdb-singleton branch from 42c8794 to b50d061 Compare September 19, 2024 15:20

JelteF changed the base branch from main to str-filter-op September 19, 2024 15:55

JelteF requested a review from mkaruza September 19, 2024 15:55

JelteF force-pushed the duckdb-singleton branch from a0d3df5 to ee6865f Compare September 19, 2024 15:59

mkaruza force-pushed the str-filter-op branch from 618277c to 7303512 Compare September 20, 2024 12:59

JelteF force-pushed the duckdb-singleton branch from 28d333c to 9985939 Compare September 20, 2024 13:41

mkaruza force-pushed the str-filter-op branch from 7303512 to 689cec9 Compare September 20, 2024 14:19

Base automatically changed from str-filter-op to main September 20, 2024 14:29

Y-- and others added 13 commits September 20, 2024 16:32

Make DuckDB a singleton per connection process

ca54b67

Apply review feedback

917a74b

Don't crash on explain

7eab0fe

Test extended explain queries

e667ec7

Support prepared statements

e4d0fae

Some more prepared statement tests

97871ba

Remove leftover comment and debugging elog

feb8b19

Remove unnecessary TODO

988e2c1

Update test and add logging

c96d0af

Add tests for all types

ec5acdf

Remove commented out code and support JSON parameter

ba34b3b

Remove test that was moved to the python suite

f0a69c3

Use Datum -> Date/Timestamp conversion functions

6f028f2

JelteF force-pushed the duckdb-singleton branch from 9985939 to 6f028f2 Compare September 20, 2024 14:32

mkaruza approved these changes Sep 23, 2024

View reviewed changes

JelteF merged commit 007d882 into main Sep 23, 2024
3 checks passed

JelteF deleted the duckdb-singleton branch September 23, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support extended protocol and prepared statements #147

Support extended protocol and prepared statements #147

JelteF commented Aug 22, 2024 •

edited

Loading

JelteF Aug 23, 2024 •

edited

Loading

mkaruza Sep 2, 2024

JelteF Sep 19, 2024

Y-- left a comment

mkaruza left a comment

mkaruza Sep 2, 2024

JelteF Sep 17, 2024

Support extended protocol and prepared statements #147

Support extended protocol and prepared statements #147

Conversation

JelteF commented Aug 22, 2024 • edited Loading

JelteF Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

mkaruza Sep 2, 2024

Choose a reason for hiding this comment

JelteF Sep 19, 2024

Choose a reason for hiding this comment

Y-- left a comment

Choose a reason for hiding this comment

mkaruza left a comment

Choose a reason for hiding this comment

mkaruza Sep 2, 2024

Choose a reason for hiding this comment

JelteF Sep 17, 2024

Choose a reason for hiding this comment

JelteF commented Aug 22, 2024 •

edited

Loading

JelteF Aug 23, 2024 •

edited

Loading