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

Review database pool abstraction #487

Open
aumetra opened this issue Feb 10, 2024 · 2 comments
Open

Review database pool abstraction #487

aumetra opened this issue Feb 10, 2024 · 2 comments

Comments

@aumetra
Copy link
Member

aumetra commented Feb 10, 2024

Our abstration right now adds an additional layer of indentation in most cases which isn't really needed. The only instance we would need these closures is if you want to pipeline your queries (since you need them to share the same connection to actually pipeline).

For most other cases it would actually be more ergonomic to allow the pool to be used just like a connection. That way we only borrow a connection as long as we need to and avoid the level of indentation.

@aumetra
Copy link
Member Author

aumetra commented Feb 10, 2024

New API idea:

  1. Rename .with_connection to .pipeline to change the semantics by naming
  2. Implement the required diesel-async traits for the pool which internally just borrow a connection and execute it on the connection (traits being AsyncConnection, SimpleAsyncConnection, and UpdateAndFetchResults)

This would allow pipelining the same way as before:

pool.pipeline(|conn| async move {
       [query]
   }
   .scoped()
).await?;

and would simplify single queries that can't be pipelined:

users::table.find(user_id).load(&mut pool).await?

@aumetra
Copy link
Member Author

aumetra commented Feb 10, 2024

One of the tradeoffs here is one more layer of boxing futures. Since diesel-async uses async-trait for various traits, we would have to box again meaning we poll a boxed future inside a boxed future, missing some inline opportunities.

Won't break the world and we will still be faster by multiple magnitudes than other implementations, just something to keep in mind for now.

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

No branches or pull requests

1 participant