feat: support prepare pushdown #140

kysshsy · 2024-09-28T15:11:23Z

Ticket(s) Closed

Closes PREPARE statement doesn't work #51

What

Why

How

Pushdown PREPARE, EXECUTE and DEALLOCATE statement to duckdb directly.

Tests

kysshsy · 2024-10-02T17:01:56Z

Note: There is a issue that PostgreSQL will replan the query when certain catalog changes occur, such as changes to the search path or when a table is deleted. In contrast, DuckDB does not replan when the search path is changed.
If there are two foreign tables in different schemas and the prepared statements do not specify the schemas, it may lead to ambiguity or errors when referencing the tables.

create foreign table public.t1(xxx);
create foreign table s1.t1(xxx);

prepare q1 as select * from t1; 
execute q1; (query table public.t1);

set search_path to 's1';

execute q1; (query table public.t1 in duckdb, but it should be s1.t1 in Postgres);

philippemnoel · 2024-10-02T18:29:39Z

Note: There is a issue that PostgreSQL will replan the query when certain catalog changes occur, such as changes to the search path or when a table is deleted. In contrast, DuckDB does not replan when the search path is changed. If there are two foreign tables in different schemas and the prepared statements do not specify the schemas, it may lead to ambiguity or errors when referencing the tables.
create foreign table public.t1(xxx);
create foreign table s1.t1(xxx);

prepare q1 as select * from t1; 
execute q1; (query table public.t1);

set search_path to 's1';

execute q1; (query table public.t1 in duckdb, but it should be s1.t1 in Postgres);

Can we force DuckDB to replan via some API call? We want to imitate the Postgres behaviour as much as possible.

kysshsy · 2024-10-03T15:19:01Z

Note: There is a issue that PostgreSQL will replan the query when certain catalog changes occur, such as changes to the search path or when a table is deleted. In contrast, DuckDB does not replan when the search path is changed. If there are two foreign tables in different schemas and the prepared statements do not specify the schemas, it may lead to ambiguity or errors when referencing the tables.
create foreign table public.t1(xxx);
create foreign table s1.t1(xxx);

prepare q1 as select * from t1; 
execute q1; (query table public.t1);

set search_path to 's1';

execute q1; (query table public.t1 in duckdb, but it should be s1.t1 in Postgres);
Can we force DuckDB to replan via some API call? We want to imitate the Postgres behaviour as much as possible.

Yes. We could execute prepare in Duckdb again. It will override the plan. I will fix it tomorrow.

philippemnoel · 2024-10-03T18:12:54Z

Note: There is a issue that PostgreSQL will replan the query when certain catalog changes occur, such as changes to the search path or when a table is deleted. In contrast, DuckDB does not replan when the search path is changed. If there are two foreign tables in different schemas and the prepared statements do not specify the schemas, it may lead to ambiguity or errors when referencing the tables.
create foreign table public.t1(xxx);
create foreign table s1.t1(xxx);

prepare q1 as select * from t1; 
execute q1; (query table public.t1);

set search_path to 's1';

execute q1; (query table public.t1 in duckdb, but it should be s1.t1 in Postgres);
Can we force DuckDB to replan via some API call? We want to imitate the Postgres behaviour as much as possible.
Yes. We could execute prepare in Duckdb again. It will override the plan. I will fix it tomorrow.

Sweet. That sounds wonderful. We want to be as "Postgres idiomatic as possible", so whatever the user experience Postgres users expect is is what we should try to replicate.

kysshsy · 2024-10-04T16:21:29Z

Note: There is a issue that PostgreSQL will replan the query when certain catalog changes occur, such as changes to the search path or when a table is deleted. In contrast, DuckDB does not replan when the search path is changed. If there are two foreign tables in different schemas and the prepared statements do not specify the schemas, it may lead to ambiguity or errors when referencing the tables.
create foreign table public.t1(xxx);
create foreign table s1.t1(xxx);

prepare q1 as select * from t1; 
execute q1; (query table public.t1);

set search_path to 's1';

execute q1; (query table public.t1 in duckdb, but it should be s1.t1 in Postgres);
Can we force DuckDB to replan via some API call? We want to imitate the Postgres behaviour as much as possible.
Yes. We could execute prepare in Duckdb again. It will override the plan. I will fix it tomorrow.
Sweet. That sounds wonderful. We want to be as "Postgres idiomatic as possible", so whatever the user experience Postgres users expect is is what we should try to replicate.

Yes, I completely agree. I detect changes in the search path and determine whether to replan in DuckDB. It's ready for review. @rebasedming

comment: add test comment

philippemnoel · 2024-10-09T20:32:20Z

src/hooks/utility/explain.rs

+    }
+
+    if unsafe { !(*stmt).options.is_null() } {
+        error!("the EXPLAIN options provided are not supported for DuckDB pushdown queries.");


Am I correct in understanding that this is fixed in your other PR? Or are there other options that won't be supported?

Yes, I've just moved the code. I will implement options support in another PR.

philippemnoel · 2024-10-09T20:50:07Z

src/hooks/utility/prepare.rs

+            return Ok(true);
+        }
+
+        // we need to make Duckdb replan the `PREPARE` statement when search path changed.


to match Postgres' default behaviour

philippemnoel · 2024-10-09T20:50:56Z

src/hooks/utility/prepare.rs

+        #[cfg(feature = "pg13")]
+        let cached_plan = pg_sys::GetCachedPlan(plan_source, null_mut(), false, null_mut());
+
+        #[cfg(not(feature = "pg13"))]
+        let cached_plan = pg_sys::GetCachedPlan(plan_source, null_mut(), null_mut(), null_mut());


Why do you need to call GetCachedPlan twice here? Could you please add a comment to explain?

philippemnoel

this looks good to me! @Weijun-H could you please review this? If this works with you, I'm happy to get it in

Weijun-H · 2024-10-11T02:33:55Z

I will carefully review this PR today or tomorrow.

kysshsy requested review from rebasedming, neilyio and philippemnoel as code owners September 28, 2024 15:11

kysshsy marked this pull request as draft September 29, 2024 01:04

kysshsy force-pushed the utility_prepare branch from 0030c41 to 9cc49c9 Compare October 2, 2024 15:11

kysshsy marked this pull request as ready for review October 2, 2024 15:11

kysshsy force-pushed the utility_prepare branch 2 times, most recently from f08ae75 to df5d685 Compare October 4, 2024 15:58

philippemnoel mentioned this pull request Oct 8, 2024

Enhance explain #146

Draft

kysshsy added 13 commits October 9, 2024 21:16

feat: support prepare statement

14a452b

test: add prepare test

365b16e

feat: add Query plan check

ce973a6

fix: run fmt

074ea2c

feat: change the way to get cached plan

2d17a6c

fix: run fmt

10421f2

fix: pg13 compile

1d0133c

fix: fix compile

a5ad5db

fix: replan in Duckdb when search_path changed

44cd8c3

fix: pg17 function name change

43909d1

test: add search path test

e055dfa

comment: add test comment

fix: change warning message url

63cf73f

fix: fix pg13 compile

6b96f8c

kysshsy force-pushed the utility_prepare branch from c6c5b68 to 6b96f8c Compare October 9, 2024 13:17

philippemnoel reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support prepare pushdown #140

feat: support prepare pushdown #140

kysshsy commented Sep 28, 2024 •

edited

Loading

kysshsy commented Oct 2, 2024

philippemnoel commented Oct 2, 2024

kysshsy commented Oct 3, 2024

philippemnoel commented Oct 3, 2024

kysshsy commented Oct 4, 2024

philippemnoel Oct 9, 2024

kysshsy Oct 11, 2024 •

edited

Loading

philippemnoel Oct 9, 2024

philippemnoel Oct 9, 2024

philippemnoel left a comment

Weijun-H commented Oct 11, 2024

feat: support prepare pushdown #140

Are you sure you want to change the base?

feat: support prepare pushdown #140

Conversation

kysshsy commented Sep 28, 2024 • edited Loading

Ticket(s) Closed

What

Why

How

Tests

kysshsy commented Oct 2, 2024

philippemnoel commented Oct 2, 2024

kysshsy commented Oct 3, 2024

philippemnoel commented Oct 3, 2024

kysshsy commented Oct 4, 2024

philippemnoel Oct 9, 2024

Choose a reason for hiding this comment

kysshsy Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

philippemnoel Oct 9, 2024

Choose a reason for hiding this comment

philippemnoel Oct 9, 2024

Choose a reason for hiding this comment

philippemnoel left a comment

Choose a reason for hiding this comment

Weijun-H commented Oct 11, 2024

kysshsy commented Sep 28, 2024 •

edited

Loading

kysshsy Oct 11, 2024 •

edited

Loading