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

High Level Prepare - Execute #227

Open
ice-ares opened this issue Nov 15, 2022 · 17 comments
Open

High Level Prepare - Execute #227

ice-ares opened this issue Nov 15, 2022 · 17 comments
Labels
1sp documentation Improvements or additions to documentation teamE

Comments

@ice-ares
Copy link

Can we have a wrapper over .Prepare() and .Execute(), I.E. called .PrepareExecute() that first checks a cache for statementIDs based on the query, and if not runs Prepare, otherwise just call Execute with the new args.

  • some management for that cache, ofc. I.E. When it expires
@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Nov 15, 2022

It is better to implement such specific logic (especially cache expiration) on the client side rather than on the library side. Are there any restrictions for this now?

@ice-ares
Copy link
Author

I think the library has a better value to have some batteries included (or default implementations), because developers, will eventually implement this, but why should they?

Isn't it better to have at least a standard/normalised implementation of this very very common use case?

Another point: Prepare() is useless from a business stand point. It's just a technical feature that brings 0 value for the business, the end goal is to run an SQL query as performantly as possible.

Forcing developers to write their own version of a very common use case either makes them for the repo and spend time understanding how & where to change stuff, or they drop using Tarantool al together.

As for the cache expiration, a basic sync.Map with no time expiration (just override) is more than enough for 99% of the use cases. Applications don't have more than thousands of queries.

@R-omk
Copy link

R-omk commented Nov 15, 2022

The current implementation of prepared queries looks weird in principle.
It is quite different from how it works in the standard sql library.
However, I believe that there should be no cache (I mean there should be no eviction or expiration logic) . Any prepared statement must exist until it is explicitly closed.

Here's how it's done in golang https://go.dev/doc/database/prepared-statements

@ice-ares
Copy link
Author

You need cache, otherwise you have 2RTTs for each query. Also the Golang stdlib caches the prepared statement and returns a wrapper for u to use.

@R-omk
Copy link

R-omk commented Nov 15, 2022

@ice-ares

Also the Golang stdlib caches the prepared statement

For example, there is no such thing in the mysql driver. An object is created that stores the ID of the open prepared query. It exists until the client itself closes it. There is no automatic expiration.

https://github.com/go-sql-driver/mysql/blob/0c62bb2791485d4260371bcc6017321de93b2430/connection.go#L533
https://github.com/go-sql-driver/mysql/blob/0c62bb2791485d4260371bcc6017321de93b2430/statement.go#L19
https://github.com/go-sql-driver/mysql/blob/0c62bb2791485d4260371bcc6017321de93b2430/packets.go#L838

@ice-ares
Copy link
Author

sure, but that's wasteful, very wasteful. Using a prepared statement once, or a few times is wasting RTTs and computing power on the database server.

@R-omk
Copy link

R-omk commented Nov 15, 2022

There is no double RTT. One time to create statement (https://github.com/go-sql-driver/mysql/blob/0c62bb2791485d4260371bcc6017321de93b2430/connection.go#L170) and one for EACH execution of statement (https://github.com/go-sql-driver/mysql/blob/0c62bb2791485d4260371bcc6017321de93b2430/statement.go#L58)

..Or maybe I don't understand what you're talking about.

@ice-ares
Copy link
Author

Any normal application has around 1-1000 sql queries, which means 1-1000 calls to prepare statement for the entire runtime, every X time (until the db engine expires the prepared statement).

The approach from database/sql requires a new prepare statement for every business call ( IE. http endpoint execution, background process execution, etc).

Thats wasteful.

Because instead of having 1-1000 calls to prepare, for the whole application, you then have 1-1000 for every flow/ user session, etc.

Its wasteful, cuz a prepared statement is something static, the same for all users, only its parameters differ.

@R-omk
Copy link

R-omk commented Nov 15, 2022

I don't see any contradiction. You can create the necessary statements once for the entire runtime.

@ice-ares
Copy link
Author

Yea, ofc. I was argueing the API. The go stdlib forces me to cache it. This lib should not do that, it should atleast provide a default and basic implementation which covers 90% of the usecases out there.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Nov 15, 2022

Thats wasteful. Because instead of having 1-1000 calls to prepare, for the whole application, you then have 1-1000 for every flow/ user session, etc.

Unfortunately, we can not share a prepared statement between connections due to a Tarantool implementation:

https://www.tarantool.io/en/doc/latest/reference/reference_lua/box_sql/prepare/

The prepared statement cache (which is also called the prepared statement holder) is «shared», that is, there is one cache for all sessions. However, session X cannot execute a statement prepared by session Y. 

@ice-ares
Copy link
Author

Ofc, and we should not, each connection should have its own sync.Map

@ice-ares
Copy link
Author

1-1000 prepare statement calls, per connection

@ice-ares
Copy link
Author

@oleg-jukovec @DifferentialOrange would you guys be OK with accepting a contribution(a PR) on this ?

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Nov 28, 2022

We would not like to include it in the public api of the library because it can be implemented on the client side. But we consider it a good idea to give an example of the implementation.

@ice-ares
Copy link
Author

What about if impl is in the connection pool package? Since prepared statement cache is per connection, we could add caching in the connection wrappers there

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Nov 29, 2022

What about if impl is in the connection pool package?

Thank you. It sounds like a good idea because it can be too tricky to implement on a client side. But let's look at the proposed public API before implementation.

@oleg-jukovec oleg-jukovec added teamE 1sp documentation Improvements or additions to documentation labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1sp documentation Improvements or additions to documentation teamE
Projects
None yet
Development

No branches or pull requests

5 participants