Diesel integration #658
Diesel integration #658
Conversation
|
@weiznich I would greatly appreciate if you could let me know if you think this is a good approach 🙏 |
|
@tyt2y3 @billy1624 This is ready for a first review, we still have to wait for the next diesel release before we can merge though |
| DB: Backend, | ||
| { | ||
| fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> { | ||
| out.unsafe_to_cache_prepared(); |
There was a problem hiding this comment.
It might be useful to only call this method depending on whether sea-query believes that a prepared statement is meaningful to cache or not. It will significantly improve performance for queries that are executed more than once.
Diesel generally tries to cache all queries that are somewhat static, which means all queries that do not contain a user input dependent component (beside specific bind parameters).
There was a problem hiding this comment.
Yeah that's a good point, I think it will require a hint from sea-query side to implement that but we could offer an API that the user can specify (build_diesel_cached or something). I am unsure how the caching exactly works when using a pool though? Usually the prepared statement is per connection, does diesel try to re-use the same connection? @tyt2y3 @billy1624 if you have any experience with that from the sqlx side that we could translate to diesel that would be super!
There was a problem hiding this comment.
It might be already worth to have a simple heuristics there: There are no bound paramaters -> cache the query, as it does not change for different calls.
For more targeted caching you would need to access the query ast somehow to check if there is for example no IN expression with a variable number of binds in there or no INSERT statement with a variable number of rows.
There was a problem hiding this comment.
Also there is quite a lot of detailed documentation of diesels query caching here: https://docs.diesel.rs/2.1.x/diesel/connection/statement_cache/index.html#a-primer-on-prepared-statement-caching-in-diesel
There was a problem hiding this comment.
Thanks for the info. May be later we can have an API that walks the AST and return whether:
- any custom expression used
- any
inandnot_inclause - insert many
Sure, I am okay with it |
|
@tyt2y3 Ready for review |
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| - run: cargo update --manifest-path sea-query-diesel/Cargo.toml --workspace -p bigdecimal:0.4.1 --precise 0.3.1 |
There was a problem hiding this comment.
This shouldn't be required anymore as diesel 2.1.1 added support for bigdecimal 0.4: https://crates.io/crates/diesel/2.1.1/dependencies
There was a problem hiding this comment.
Ha the issue is on sea-query side, they only support 0.3 and the resolver takes 0.4 for diesel so it causes a build failure because the types are incompatible. Its a PITA that we still don't have a way to pin a sub dependency without a lockfile.
|
Released in https://crates.io/crates/sea-query-diesel/0.1.0 |
|
Thanks much appreciated! |
PR Info
See discussion: #168
BLOCKED BY diesel-rs/diesel#3691 and diesel-rs/diesel#3694
New Features