Feature Request: sqlite

[poelzi]

Setup

Versions

• Diesel_async: 0.3.1
• Database: sqlite

Problem Description

sqlite is not supported

What are you trying to accomplish?

sqlite is nice for development & testing, when a fill DB is overkill

Checklist

• I have already looked over the issue tracker for similar possible closed issues.

[weiznich]

I've converted this to an issue to a feature proposal discussion as that's not strictly a bug report at all.

That written: I would open to accept a PR for such a connection implementation if it can be shown that the implementation is similar performant than the current sync diesel SqliteConnection implementation. Otherwise I personally will likely not spend time working on this as I do not have needs for such a connection implementation at all.

As a more general note: It's usually a bad idea to use a different database system for development and testing than for the production instance. You will miss bugs and subtle issues.

[quentinmit]

Perhaps instead of implementing an async sqlite there could be a generic AsyncConnection -> Connection adapter that could be used with any existing Connection implementation, either blocking or with a separate thread ala tokio::task::spawn_blocking.

AIUI, the main challenge with "just use the sync version" is that you need to sprinkle/remove a bunch of .awaits in your code every time you want to move between sqlite and an async connection. I don't personally care about the performance aspect for unit testing/local development (and since sqlite is typically a local disk/memory database, it's much lower latency than a mysql or postgres server).

[weiznich]

Perhaps instead of implementing an async sqlite there could be a generic AsyncConnection -> Connection adapter that could be used with any existing Connection implementation, either blocking or with a separate thread ala tokio::task::spawn_blocking.

That would also be an possibility. I would happily accept a PR for this. Again as I do not need that for my own use cases do not expect any fast development from my side here.

I don't personally care about the performance aspect for unit testing/local development (and since sqlite is typically a local disk/memory database, it's much lower latency than a mysql or postgres server).

I'm not sure how often I need to repeat that: Using a different database for unit tests and local development compared to your production database is a "great" way to get a lot of strange performance bugs and other issues. It's an antipattern therefore it's at least from my point of view an advantage that switching the database is something that's not simple to do.

[andreapavoni]

I’ve seen there’s this crate which implements an async sqlite connection.

https://github.com/ryanfowler/async-sqlite

Edit: to give some context, I’m totally ok for using sqlite in production. It covers a lot of use cases where a database server might be overkill (IMHO, it’s overkill in many situations).

That said, I agree that using different database engines for testing vs production is an anti-pattern.

I don’t have enough expertise to craft a PR, but I’d be happy to contribute in any way.

[andreapavoni]

Also found this one: https://github.com/programatik29/tokio-rusqlite

[weiznich]

As written above: I personally won't spend time on that, as it will likely result in an implementation with optimistically performance. If you want to use sqlite in a production setting (which is totally fine), you are get likely a better performance by just using plain old diesel without any considerations of whether or not something is async. And no that will likely not cause issues, as for most queries you should be able to stay well below the "what is considered" as blocking limit for sqlite.

[momobel]

Hi @weiznich,

I started implementing sqlite support into diesel_async. As a first step I am trying the approach suggested by @quentinmit here #95 (comment), basically creating a SyncConnectionWrapper that adapts a Connection to an AsyncConnection.

I understand you are not willing to implement this yourself but would appreciate your input in creating a proper implementation.
My current work can be found here. Please be aware that I am a Rust user but by no mean an expert and a beginner at diesel/diesel_async.

The main blocking point is how to plug onto underlying Connection methods. The solution I can imagine would be creating a SqlQuery from the built SQL string and bind the BindCollector Buffers to it in the async task which would require tweaking the BindCollector, potentially adding a trait to expose its underlying data. Unfortunately I don't have much diesel/diesel_async expertise to find the most appropriate way according to waht you expect.

If this doesn't look like the right approach, the next option would be to implement a Sqlite specific AsyncConnection re-using as much as possible from Diesel implementation.

Could you please give me some ideas? I would really appreciate your feedback and knowledge. Thanks!

[weiznich]

Thanks for starting to work on this. I know that this is not trivial to implement due to various shortcomings in how certain async specific requirements can be expressed at language level.

The main blocking point is how to plug onto underlying Connection methods. The solution I can imagine would be creating a SqlQuery from the built SQL string and bind the BindCollector Buffers to it in the async task which would require tweaking the BindCollector, potentially adding a trait to expose its underlying data. Unfortunately I don't have much diesel/diesel_async expertise to find the most appropriate way according to waht you expect.

I agree that this is likely the optimal solution to have such a general wrapper. That written I don't think that splitting up the query source before putting it into a background thread will help here. The underlying problem is that the diesel sqlite connection essentially borrows bind data, which means that even if you have the BindCollector it will continue to contain these potential non-Send values as there is just no serialized version of the binds. That's mostly for performance reasons as it is just not meaningful to copy strings if you could just borrow into the buffer. This points also to another currently hidden issue: spawn_blocking requires that anything that goes in there lives for a 'static lifetime. That is not the case for borrowed binds.

I think both issues (!Send and !'static) can be solved by essentially the same solution. The split up you've already started doing there is likely the right way to go in combination with some custom SqlQuery variant (for caching reasons). I think for the two issues mentioned earlier we essentially need to have some sort of method on the BindCollector trait/type that allows to convert it into a Send/'static variant of itself. For Sqlite that should be trivially possible as we cant just "clone" the bind values:
https://github.com/diesel-rs/diesel/blob/ce16e91bea0bd85342ff1cf1d683a8b8cb1a6ffc/diesel/src/sqlite/connection/bind_collector.rs#L98 (all the variants are already send). For Mysql and Postgres this should be even simpler as the bind values essentially contains a bunch of bytes. Likely that needs to be method in a new "private" extension trait in diesel itself.

[momobel]

Thanks for the pointers, I appreciate it a lot!

I took some time to understand and see how that would fit within diesel and diesel_async before diving into implementation. This is to ensure 2 things, that I understand what you suggest and that the approach fits your vision.
Please find in this gist pseudocode of changes I understood that could fit.

I will adjust the approach or not depending on your feedback.
Thanks again!

[weiznich]

Thanks for writing this summary. That looks roughly like what I imagined, although I would expect that there will be problems that I missed.

[momobel]

The execute implementation is now more aligned to what you imagined and working fine with modifications on diesel and diesel_async.

I started the load implementation, which will likely be more difficult to get right due to the difficulty to pass back the results. This is due to the concrete Cursor/Row for all 3 connections (postgres, sqlite, mysql) not being Send (because of Rc or borrowed values) and because the borrowing is trickier for the QueryResult<Cursor> because it gets borrows from the query and the connection within the mutex.

I will try to find a proper way to workaround those (I think I will add a method on SyncConnectionWrapper to somehow attach QueryResult lifetime to it instead of the underlying connection but this one looks tricky. An alternative could be implementing a new LoadConnection mode for owned data that could be implemented by some connections.

Meanwhile, do you think there would be a better way to unlock these new discovered points?

[weiznich]

Thanks for the update.

I started the load implementation, which will likely be more difficult to get right due to the difficulty to pass back the results. This is due to the concrete Cursor/Row for all 3 connections (postgres, sqlite, mysql) not being Send (because of Rc or borrowed values) and because the borrowing is trickier for the QueryResult because it gets borrows from the query and the connection within the mutex.

As you implement a new connection type you have control over the Cursor/Row types. They don't need to match those from diesel, we can use more fitting definitions here. As a starting solution I would likely just collect everything internally into a Vec<Something> and build a cursor on top of that. This cursor then would own the data.

[RingOfStorms]

I will put in my two cents and hopefully give @momobel some inspiration that I am looking for sqlite support as well.

I'd like to write a service that supports both postgres and sqlite and would very much prefer to not have to copy paste every single query just without an .await on it. Performance is less of a concern, it is more about wanting a common interface across the codebase for writing queries.

[momobel]

Thanks to @weiznich help, this is now possible with the SyncConnectionWrapper starting from diesel_async 0.5.0.