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

Remove r2d2_sqlite dependency #4050

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Remove r2d2_sqlite dependency #4050

merged 1 commit into from
Feb 17, 2023

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Feb 17, 2023

No description provided.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 17, 2023

The whole r2r2 connection pool is an overkill for DC usage. Not being able to reliably close the database due to a long-standing issue sfackler/r2d2#99 causes problems like #4049 where the test migrates the database before it is closed and cannot determine when it will be closed.

@link2xt link2xt force-pushed the link2xt/no-r2d2-sqlite branch 4 times, most recently from c2e75e0 to aaa5ece Compare February 17, 2023 10:57
Copy link
Member

@flub flub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to get rid of r2d2 entirely someday. We really don't get any benefit out of multiple connections and may as well have a single tokio task that handles all the Db requests communicating over some channels. But that's just dreaming of the future.


/// SQLCipher database passphrase.
/// Empty string if database is not encrypted.
passphrase: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate that we need to keep this in memory the whole time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also because of r2d2 abstraction, it expects that connections may fail because underlying network connections fail, and have to be reestablished. Which is not the case with SQLite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #4053.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 17, 2023

It'd be nice to get rid of r2d2 entirely someday. We really don't get any benefit out of multiple connections and may as well have a single tokio task that handles all the Db requests communicating over some channels. But that's just dreaming of the future.

This is what sqlx does, it creates "connection workers" which receive commands and send back results over channels. We previously dropped sqlx because it accessed the same connection from different threads and had performance problems as a result. This problem is likely solved now, but I would not try to reintroduce sqlx again for now, first want to solve r2d2 problems the simplest way, likely by putting a single connection behind RwLock and just using it.

@link2xt link2xt merged commit 870527d into master Feb 17, 2023
@link2xt link2xt deleted the link2xt/no-r2d2-sqlite branch February 17, 2023 11:30
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

Successfully merging this pull request may close these issues.

2 participants