Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support prepare pushdown #140

Merged
merged 14 commits into from
Oct 13, 2024
Merged

Conversation

kysshsy
Copy link
Contributor

@kysshsy kysshsy commented Sep 28, 2024

Ticket(s) Closed

What

Why

How

Pushdown PREPARE, EXECUTE and DEALLOCATE statement to duckdb directly.

Tests

@kysshsy kysshsy marked this pull request as draft September 29, 2024 01:04
@kysshsy kysshsy marked this pull request as ready for review October 2, 2024 15:11
@kysshsy
Copy link
Contributor Author

kysshsy commented Oct 2, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

kysshsy commented Oct 3, 2024

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
Copy link
Collaborator

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 kysshsy force-pushed the utility_prepare branch 2 times, most recently from f08ae75 to df5d685 Compare October 4, 2024 15:58
@kysshsy
Copy link
Contributor Author

kysshsy commented Oct 4, 2024

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

Copy link
Collaborator

@philippemnoel philippemnoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

I will carefully review this PR today or tomorrow.

Copy link
Contributor

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @kysshsy

src/hooks/utility.rs Show resolved Hide resolved
tests/scan.rs Show resolved Hide resolved
@philippemnoel philippemnoel merged commit 9a218e5 into paradedb:dev Oct 13, 2024
12 checks passed
@philippemnoel
Copy link
Collaborator

Thank you <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PREPARE statement doesn't work
3 participants