-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
r2d2 doesn't work well with diesel sqlite - register_diesel_sql_functions probably locks database #2365
Comments
Can you reproduce this behaviour without any of those
It's right that In general I advice not using sqlite in such a multithreaded context, because it's basically not designed for that use case. Use a full blown dbms like postgresql instead. |
While true for highly multithreaded environments that expect to have concurrent writers to a database with fine graded locking, this is not quite true as a generalized statement but needs to be evaluated per use case. SQLite is perfectly well designed for concurrent systems in WAL mode with lots of readers and just a few writers that can live with being gated to a single writer having append access to the WAL which includes webapps if they fit this prerequisite. |
Looking into this some more, it seems the |
I didn't mean that it is not possible to use SQLite in such an environment, only that it is in general easier to just use PostgreSQL here, as SQLite requires some configuration to make this even work and PostgreSQL just works out of the box. |
I've created a repository that can be used to reproduce this, it's creating a database, spawns 25 threads that increment one field 100 times each and then verifies the field has the expected value. The sql executed is exactly the same, but diesel returns errors for some function calls while rusqlite does not. Removing the PRAGMAs causes both tests to fail. https://github.com/kpcyrd/rust-diesel-bug-2365 Postgres is only easier for the developer, the user would have to install and configure postgres on their own which would make the setup significantly harder. |
After some more debugging, rusqlite automatically sets a busy_timeout for 5s. I've prepared a patch to do the same and it fixes the example above. Would you be interested in a patch like that? |
Thanks for providing the reproducible example. According to the sqlite documentation executing Here you are setting all your pragmas at once. The same thing is done here for the diff --git a/src/main.rs b/src/main.rs
index a376adf..10a8ea4 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -101,6 +100,7 @@ struct Diesel {}
impl Testable for Diesel {
fn run() -> Result<()> {
let db = diesel::SqliteConnection::establish(PATH)?;
+ db.execute("PRAGMA busy_timeout = 5000;")?;
db.batch_execute(SETUP)
.context("Failed to execute PRAGMAs")?;
for _ in 0..LOOPS { to make the code equivalent to the corresponding So the remaining question is: "Should we do that as part of our connection setup or not?" |
Hi, I'm not as knowledgable and this is an old issue I have stumbled upon, so I'll add just a bit. I have tried the test here and for me simply adding the pragma does not help. Furthermore, it consistently fails for both diesel and rustls for me. I am running it on Windows and am using bundled versions of the crate, but I don't think that should have an effect. What does work for me is running the setup once before the test begins. I think that the issue is that changing journal mode to WAL needs exclusive lock and it seems to ignore the busy timeout option. With just that one operation in setup both rustls and diesel inetermittenly failed. So I don't think that this can be solved without retrying. I don't know if that is something that should be handled by diesel. When I run the setup once before starting the test it acts as described, i.e. rusqlite works everytime and diesel sometimes fails to write pragmas. This is indeed fixed with the busy wait. I personally view diesel as pretty high level (but I might be wrong here) so I would expect it to set reasonable settings that would prevent errors like this if it can. Maybe 5 seconds is too much, but I think having it set a busy timeout (which could also be configurable) is not a bad thing. But I would rather have it safer by default. That said I don't understand how setting busy wait should cause data loss. I would rather expect it the other way around when writes fail and users don't check for failure. |
Can you clarify if you refer to the original posted example or to the "fixed" version with the patch from the post above applied. Because I cannot reproduce the issue with the above patch applied. If it's the second case: Please provide an reproducible example that we can use to understand your bug.
Retrying is something that should always be decided on application layer, therefore that's nothing that should be part of diesel itself in my opinion. We try to return meaningful error types there so that you can match on them and retry stuff if that's meaningful for your use case. That really depends on the use case. It might be fine for your use case, but that's not necessarily true for other use cases, where you really want to know that a query is applied fast or discard the result. If you feel that the default busy timeout should be something different I would argue that the correct place to change that would be in sqlite itself.
A to large busy timeout does not necessarily directly cause data loss, but it could block operations longer than expected, which in turn could break other things. |
Setup
I'm writing a webapp that uses actix, r2d2, diesel and sqlite. r2d2 is used to keep a generic pool of diesel handles that are then handed out to functions handling concurrent requests.
Full code can be found here: https://github.com/diesel-rs/diesel/blob/00f7de703aea02659bddad8d168b9fe6d4aee276/diesel/src/sqlite/connection/mod.rs
Versions
Feature Flags
["sqlite", "r2d2", "chrono"]
Problem Description
Starting the app already causes r2d2 to lock up (which I've worked around with retries), but write queries still fail randomly afterwards.
I'm executing some fluff to make sqlite work well with multiple threads:
This seems to lock the database somehow. Assuming the PRAGMA doesn't lock I've looked into
SqliteConnection::establish
:It seems that it's always executing
register_diesel_sql_functions
before myPRAGMA
are executed, which means if this causes a write it's going to lock the database for everybody. It seems It's executingCREATE TRIGGER
among other things.Do you have any suggestions on how to get this to work correctly?
What are you trying to accomplish?
Write a webapp with actix that uses diesel with sqlite.
Are you seeing any additional errors?
No
Steps to reproduce
My full code base is public, but the issue should be trivial to reproduce by copying the relevant r2d2+diesel code.
Checklist
closed if this is not the case)
The text was updated successfully, but these errors were encountered: