Make functions work with both connection and pool

[dullbananas]

This gets rid of repetitive calls to get_conn in db_schema::impls.

More importantly, it allows the functions to be used in transactions because the actions in a transaction can't use different connections.

#1161 (comment)

[Nutomic]

The code is not very nice with &mut everywhere. Maybe you could add a method context.conn() which can replace context.pool() directly?

[dullbananas]

context.pool() is now replaced with context.conn().await?.

The functions now take impl DbConn, which makes it possible to use &mut *conn instead. This would be needed for transactions.

[Nutomic]

You need to run ./scripts/fix-clippy.sh

[phiresky]

I would caution against this change because it makes the pool less effective. You're allocating a postgresql connection (which is a fairly expensive whole process) when you don't actually need it, making other concurrent queries wait if the pool is empty.

The default pool size is only 5 (which should be increased), but even with larger sizes you can easily run out if you keep holding database connections while doing unrelated stuff (e.g. HTTP calls).

If there's no reason for two queries to need the same Connection, then you shouldn't force them to use the same connection.

Of course, if there has to be a transaction somewhere they will need to use the same connection, but most code, especially read-only queries, shouldn't care about it.

[dullbananas]

You're allocating a postgresql connection (which is a fairly expensive whole process) when you don't actually need it, making other concurrent queries wait if the pool is empty.

I will fix this by reverting the commit "Get rid of repetitive &mut *context.conn().await?"

[dullbananas]

The CI steps are stuck on "This step hasn't started yet."

[dessalines]

This def does make the DB calls more low-level and flexible. This was a ton of work, thanks for doing this.

@phiresky does this look okay now?

[dullbananas]

The functions now work with both connections and pools. I had a long fight with the borrow checker. I am very excited to finally make it work.

I added this to lemmy_db_schema::utils:

 pub enum DbPool<'a> {
   Pool(&'a ActualDbPool),
   Conn(&'a mut AsyncPgConnection),
 }

 pub enum DbConn<'a> {
   Pool(PooledConnection<AsyncPgConnection>),
   Conn(&'a mut AsyncPgConnection),
 }

 pub async fn get_conn<'a, 'b: 'a>(pool: &'a mut DbPool<'b>) -> Result<DbConn<'a>, DieselError> {
   Ok(match pool {
     DbPool::Pool(pool) => DbConn::Pool(pool.get().await.map_err(|e| QueryBuilderError(e.into()))?),
     DbPool::Conn(conn) => DbConn::Conn(conn),
   })
 }

Functions need to take &mut DbPool<'_> instead of DbPool<'_> because implicit reborrowing only works when the parameter type is &mut. I tried it without the &mut, but that would require creating a DbPool method that does the reborrow, which would need to be called a lot.

rust-lang/rfcs#1403

Also, this change now causes code to fail borrow checking if a DbPool is passed to another function before an existing associated DbConn is dropped. I had to fix most of these borrow check errors by moving a get_conn call to a different line to get rid of unnecessary connection overlap, so this is a useful effect for most code. There was also some code that failed borrow checking because of using something like futures::try_join to concurrently run multiple async functions that take the same pool. I fixed this by creating a try_join_with_pool macro that runs the functions concurrently for DbPool::Pool just like before, and sequentially for DbPool::Conn.

[dullbananas]

CI ran out of disk space

[Nutomic]

This will lead to misleading code, where it appears that queries will run in parallel, but actually they dont. So I would prefer to remove this case and put unimplemented!().

[dullbananas]

It would be more misleading for a function to take DbPool but not work with the Conn variant

[Nutomic]

The code looks good, only thing I dont like is how it requires &mut and lifetimes all over the code. I wonder if that could be avoided by wrapping the pool into a Cell or another data type. But this can be done later.

Anyway I confirmed that tests are all passing, so CI is fine. We should merge this asap in order to avoid conflicts.

[dessalines]

Agree, this is next in line for merge.

[phiresky]

Is it still possible after this PR to get an actually owned DbPool that doesn't borrow something? I'm trying to fix #3605 after merging this but can't figure out how to pass DbPools to tokio::spawned tasks.

[dullbananas]

Is it still possible after this PR to get an actually owned DbPool that doesn't borrow something? I'm trying to fix #3605 after merging this but can't figure out how to pass DbPools to tokio::spawned tasks.

Use context.inner_pool().clone() to get an ActualDbPool, which a task should be able to receive. In the task, convert it using &mut DbPool::Pool(&pool).

https://github.com/LemmyNet/lemmy/blob/main/crates/db_schema/src/utils.rs

[dullbananas]

You can also convert it using &mut (&pool).into()

[SorteKanin]

Hey, sorry for necroing this old PR.

I'm still new to the code base and I'm very confused about this apporach of an enum over pool vs connection. Why not make the functions generic over the requisite traits? Wouldn't that work just as well without having to use this (in my opinion, quite strange) enum?