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

Prepared statement support in MySQL and PostgreSQL #470

Closed
sunng87 opened this issue Nov 11, 2022 · 23 comments
Closed

Prepared statement support in MySQL and PostgreSQL #470

sunng87 opened this issue Nov 11, 2022 · 23 comments
Assignees
Labels
C-enhancement Category Enhancements
Milestone

Comments

@sunng87
Copy link
Member

sunng87 commented Nov 11, 2022

In order to support prepared statement, we need to implement a parse-bind-execute process in frontend caches parsed statement at connection-session scope.

@SSebo
Copy link
Contributor

SSebo commented Nov 16, 2022

@sunng87 Should some logic be added here? like parsing sql to see if it is PREPARED statement, then cache connection-session scope, when meet EXECUTE statement, find in cache then execute?

async fn on_query<'a>(

some references:
https://www.postgresql.org/docs/current/sql-prepare.html
https://en.wikipedia.org/wiki/Prepared_statement
https://dev.mysql.com/doc/refman/8.0/en/sql-prepared-statements.html

@MichaelScofield
Copy link
Collaborator

@SSebo I think "prepared" stmt should be handled here:

async fn on_prepare<'a>(

ane then "execute" here:

async fn on_execute<'a>(

@MichaelScofield MichaelScofield added the C-enhancement Category Enhancements label Nov 19, 2022
@xtang xtang mentioned this issue Nov 30, 2022
24 tasks
@xtang xtang added this to the Release v0.1 milestone Nov 30, 2022
@SSebo
Copy link
Contributor

SSebo commented Dec 3, 2022

@MichaelScofield This issue is dependent on #600, right?

@MichaelScofield
Copy link
Collaborator

@SSebo yes, part of it. The session crate was introduced in this PR already, I think prepared stmt can be stored in the QueryContext now.

@SSebo
Copy link
Contributor

SSebo commented Dec 3, 2022

@SSebo yes, part of it. The session crate was introduced in this PR already, I think prepared stmt can be stored in the QueryContext now.

Good job! you can assign this to me.

@SSebo
Copy link
Contributor

SSebo commented Dec 27, 2022

@MichaelScofield I found prepare statement is not work for msql-srv crate. fn on_prepare(&mut self, _query: &str, info: StatementMetaWriter<W>) -> io::Result<()> is not called back when execute PREPARE stmt1 FROM 'SELECT SQRT(POW(?,2) + POW(?,2)) AS hypotenuse';

@MichaelScofield
Copy link
Collaborator

MichaelScofield commented Dec 28, 2022

@SSebo It seems you have to use COM_STMT_PREPARE to trigger the callback of on_prepare. It is not text protocol.

@SSebo
Copy link
Contributor

SSebo commented Dec 28, 2022

@MichaelScofield Thanks a lot!
I use sqlx to successfully trigger on_prepare callback .
It looks like mysql-client will always treat it as COM_QUERY even if it is prepare statement, and mysql-server will properly handle that case as fallback.
Should that case be supported?
Is there text protocol or byte protocol things in Postgres?
pgwire has no on_prepare callback.

@MichaelScofield
Copy link
Collaborator

@SSebo prepare stmt in mysql could be executed in mysql-cli like this, and this feature request is tracked in #769 . I think in this issue, we can focus on the "COM_STMT_PREPARE" type of prepare stmt. At last, it's the most often used type of prepare stmt. (Applications use some mysql drivers tend to use binary protocol to submit prepare stmt.)

As for pg, there're "simple" and "extended" query protocols, and prepare stmt uses the latter one. pgwire has the corresponding "simple" and "extended" handler for us to implement prepare stmt.

@SSebo
Copy link
Contributor

SSebo commented Dec 29, 2022

@SSebo prepare stmt in mysql could be executed in mysql-cli like this

the prepare statement PREPARE stmt1 FROM 'SELECT SQRT(POW(?,2) + POW(?,2)) AS hypotenuse'; I used to execute is from this to mysql-cli, it can not trigger on_prepare.

@MichaelScofield
Copy link
Collaborator

@SSebo Yes, it's not COM_STMT_PREPARE. It's not using binary protocol, in the mysql command cli. It's the other form of prepare stmt.

@MichaelScofield
Copy link
Collaborator

ping @SSebo , any updates?

@SSebo
Copy link
Contributor

SSebo commented Feb 15, 2023

ping @SSebo , any updates?

@MichaelScofield Sorry for delay, I'll pick it up this week.

@MichaelScofield MichaelScofield self-assigned this Feb 15, 2023
@sunng87
Copy link
Member Author

sunng87 commented Feb 20, 2023

hi @SSebo , at least in modern postgresql clients, prepare keyword is no longer used. I have implemented prepared statement for postgresql in #925. Now it should work for most clients except rust-postgres, which uses parameter type inference that requires an upgrade of datafusion to support.

I suggest you to check mysql clients if prepare keyword are actually used. Mysql has a binary protocol for prepared statement: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_command_phase_ps.html

@SSebo
Copy link
Contributor

SSebo commented Feb 20, 2023

@sunng87 Got it. I'll do the MySQL binary protocol prepared statements

@SSebo
Copy link
Contributor

SSebo commented Feb 26, 2023

It takes me lots of times to figure out how to infer the placeholder types in prepared sql which used in ' COM_STMT_PREPARE_OK'.

Infer prepared statement parameter types supported by datafusion by this PR apache/datafusion#4701

I patched the latest datafusion prepared statement logic to greptime used revision apache/datafusion@main...SSebo:arrow-datafusion:prepare.

when I debug the code I checked some mysql clients. I checked the rust, golang, and c mysql connector source code, I found the type information just ignored.

Only the number of params is used. so in my PR, just generate some dummy column in prepare phase.

@sunng87
Copy link
Member Author

sunng87 commented Feb 27, 2023

The situation is quite similar in postgresql, pg has parameter type inference as well, but some clients like jdbc are not using this feature at all. However, other clients like rust-postgres fully rely on this and has strict check for returned types.

@SSebo SSebo mentioned this issue Mar 13, 2023
2 tasks
@SSebo
Copy link
Contributor

SSebo commented Mar 16, 2023

@sunng87 @MichaelScofield I'm working on the type infer version of prepare. Should statement be saved or plain query string? Since do_statement_query is removed. there is no way to execute statement.
Could you give me some advice?

@MichaelScofield
Copy link
Collaborator

@SSebo do_statement_query is removed because we are planning to not support executing SQLs directly in datanode instance, all protocol related tasks are handled in frontend. As tracked in #1010. That said, in the mean time, there's a StatementHandler implemented for datanode instance that can be used to execute stmt.

@sunng87
Copy link
Member Author

sunng87 commented Mar 16, 2023

I would suggest to cache LogicalPlan as prepared statement eventually because it's the most structured form of a query. Datafusion's LogicalPlan is capable with parameter type inference and replacement.

At the moment we only use LogicalPlan for selects but @MichaelScofield is going to change this in #1010.

I think after the refactoring, protocol servers(mysql/pg) are responsible for 1. parsing sql query string to statement in protocol server's dialect; 2. transforming statement into neutral LogicalPlan. SqlHandlers will execute LogicalPlans directly.

If we have a plan for this, I think @SSebo can hold on the work on prepared statement and join the refactoring above. @MichaelScofield WDTY

@MichaelScofield
Copy link
Collaborator

@SSebo you are welcome to join #1010 , I think we can start from implementing LogicalPlan::Prepare in query engine.

@SSebo
Copy link
Contributor

SSebo commented Mar 16, 2023

@sunng87 https://docs.pingcap.com/tidb/v5.4/sql-prepared-plan-cache this feature is very similar to your idea.

@killme2008
Copy link
Contributor

The final PR was merged #1813

We can close this issue right now, thanks to all of you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category Enhancements
Projects
Status: Done
Status: Done
Development

No branches or pull requests

6 participants