[WIP] using db-pool library to create a pool of databases #5846
Conversation
crates/db_schema/src/utils.rs
Outdated
| .await; | ||
|
|
||
| // TODO make compatible with ActualDbPool | ||
| db_pool.pull_immutable().await |
There was a problem hiding this comment.
I created this WIP PR to share the progress and the issue I'm stuck with currently. The crate I use operates with its own structure wrapping connection pools: code
And we have our own ActualDbPool. They are kinda same, but it's not obvious for me how to correctly convert one to another.
I had an idea to make ActualDbPool a enum with two possible values: RegularPool and ReusablePool, but stuck on trying to adapt stuff like LemmyContext, which also requires pool struct to be clone-able (and ReusablePool is not). And it seems a lot of changes to the main codebase for purely test changes.
Do you folks have any ideas how to manage that? Or should I stick to the initial plan without using this library?
There was a problem hiding this comment.
Our ActualDbPool is just a type alias for deadpool Pool<AsyncPgConnection>.
Their crate should be able to work with deadpool pools, but I'm not familiar with how to plug that into their crate... you'll have to ask them.
There was a problem hiding this comment.
Yeah, I see. I returned to this issue today after a week of a break. I'm in contact with the db-pool author and they're helping to understand a lot of moments and really willing to collaborate, so i think we'll make this work.
I'd like to clarify one moment: do we want build_db_pool_for_tests to return still ActualDbPool? db-pool has its own wrapper ReusableConnectionPool which works like a deadpool Pool, but a bit different and needs adaptation. And it might be easier to adapt tests for working with ReusableConnectionPool than converting ReusableConnectionPool to ActualDbPool
There was a problem hiding this comment.
The return type of build_db_pool_for_tests may be changed. Also, a DbPool variant may be added if needed.
crates/api/api_utils/src/context.rs
Outdated
| #[derive(Clone)] | ||
| pub struct LemmyContext { | ||
| pool: ActualDbPool, | ||
| pool: ContextPool, |
There was a problem hiding this comment.
This is the moment which currently blocks me, and i think it's better to consult with you again. LemmyContext structure must be cloneable, therefore all the fields, therefore the pool. But unfortunately, reusable pool from db-pool crate is not, and i don't have access to its fields to implement the trait here.
But before asking db-pool developer, i'd like to be sure we really need this pool cloning stuff, especially for the tests. Cloning the pool seems a bit strange to me, but i may miss something. I'm looking at the code now, but maybe you folks already have some insights on this
There was a problem hiding this comment.
Wrap it in Arc for now.
|
Update: I'm working on the topic; cannot devote much time for it, but it slowly going forward, and i keep the code in the branch up-to-date. I connected |
|
Hey! I've a question about the replaceable schema — found out it's the reason tests are failing in the branch: the tests are being run in restricted privilege mode, and have no access to the |
|
Yes I believe the main reason is so that they're easier to clean up.
Everything in the
Is this a limitation of the db-pool library? Because regular cargo tests can already run the |
Yep, it's designed the way you have privileged connection pool to prepare dbs and do migrations, and restricted connection pool to run tests. And the latest can only access |
|
Maybe its just a matter of running https://stackoverflow.com/questions/17338621/what-does-grant-usage-on-schema-do-exactly cc @dullbananas |
|
that's an option i'm thinking about. but unfortunately it has certain caveats.
however, i can change it in a fork, so it will use the same role for all the restricted connections (e.g. |
Maybe. The problem is almost certainly caused by something being different between the "public" schema and the "r" schema. Otherwise migrations would have failed before replaceable_schema runs. |
|
There's logic in the db-pool library which grants permission to restricted user fro public schema assuming it's the only one tests will deal with. |
|
|
||
| pg_ctl stop --silent | ||
| rm -rf $PGDATA | ||
| docker-compose -f docker/docker-compose-test.yml down |
There was a problem hiding this comment.
This should remain unchanged so that it uses Postgres from a local folder instead of starting a Docker container because starting a Docker container requires root. It is possible to allow docker commands without root, but then every application can execute docker commands and easily get root permissions.
This will require changes to db_pool::PrivilegedPostgresConfig. Honestly that file is unnecessary because it takes the parts from a db url and then converts it back to a db url. So its best to remove PrivilegedPostgresConfig entirely, and change the first param of DieselAsyncPostgresBackend::new from privileged_config: PrivilegedPostgresConfig to db_url: Url. The logic in privileged_database_connection_url() and restricted_database_connection_url() can be handled by Url.set_password() etc.
There was a problem hiding this comment.
Yep, I agree. I just skipped this issue in order to focus on the main part for now
| OnceCell::const_new(); | ||
| let db_pool = POOL | ||
| .get_or_init(|| async { | ||
| let conn_string = SETTINGS.get_database_url(); |
There was a problem hiding this comment.
You need to call SETTINGS.get_database_url_with_options()?; here and then pass that directly into DieselAsyncPostgresBackend::new() (as explained in my other comment). Because the db url includes the option lemmy.protocol_and_hostname. At the moment triggers are failing with unrecognized configuration parameter "lemmy.protocol_and_hostname" because that option is missing.
There was a problem hiding this comment.
Thanks much, was about to ask about that. I really don't want to give up on that feature. Didn't have time/mental capacity to work on it consistently, but now it's better. Recently made a workaround for db-pool permissions, which seems to work, so there's a slow progress. And trying to keep it up with regular lemmy updates
| db-pool = { git = "https://github.com/momentary-lapse/db-pool.git", branch = "edition2021-test-superuser", features = [ | ||
| "diesel-async-postgres", | ||
| "diesel-async-deadpool", | ||
| ] } |
There was a problem hiding this comment.
This should be a dev dependency. But dont worry about it for now, what matters is to get the parallel tests working.
Addresses: #4979