How can I commit() this transaction?

help
1

In the below code I'm getting this error:

error[E0599]: no method named `commit` found for mutable reference `&mut sqlx::PgConnection` in the current scope
   --> src/main.rs:103:12
    |
103 |         db.commit().await?;
    |            ^^^^^^ method not found in `&mut PgConnection`

I need to commit if the db is None since I'm starting a new transaction with: &mut self.pool.begin().await?.

It's ok to use db: Option<&mut sqlx::PgConnection> as argument type or should I use something else, like Acquire trait?

Code on Rust Playground

impl PlayerService {
    pub async fn find_player_id(
        &self,
        db: Option<&mut sqlx::PgConnection>,
        id: &str,
        team_id: &str,
    ) -> Result<String, Error> {
        let db = match db {
            Some(db) => db,
            None => &mut self.pool.begin().await?,
        };

        let _ = self.team_service.find_team_id(Some(db), team_id).await?;

        let player_id = self.repo.find_player_id(Some(db), id).await?;

        db.commit().await?;

        Ok(player_id)
    }
}

I opened rust - Why do I need to import a trait to use the methods it defines for a type? - Stack Overflow too but they closed it as duplicated which is not true. And How can I `commit()` this transaction? · launchbadge/sqlx · Discussion #3405 · GitHub.

2

It's API design 101 that Transaction::commit() is by-value (and so is rollback). It would make no sense whatsoever to use the transaction after it's committed or rolled back.

1 Like
3

Thanks @paramagnetic. Can you please add a few more words because I don't understand what you mean?

4

You shouldn't take a mutable reference to the transaction.

5

Thank you again, really.

If a function can be called with a transaction or a direct connection (and so it should start a new transaction from there because it needs one) what do you suggest to use?

Pseudo code:

async fn find_player_id_and_assign_goal(
    &self,
    db: Option<&mut sqlx::PgConnection>,
    // db: Option<&mut sqlx::Transaction<'_, sqlx::Postgres>>,
    id: &str,
    team_id: &str,
) -> Result<String, Error> {
    let mut tx = None;

    let db = match db {
        Some(db) => db,
        None => &mut *tx.insert(self.pool.begin().await?),
    };

    let _ = self.team_service.find_team_id(Some(db), team_id).await?;

    let player_id = self.repo.find_player_id(Some(db), id).await?;

    self.goal_service.assign_to_player(Some(db), id).await?;

    if let Some(tx) = tx {
        tx.commit().await?;
    }

    Ok(player_id)
}

This can work, but I only have one issue:

If Some(db) => db I need to detect if we're already in a transaction. If not I should start one.

Do you think is there a better way to accomplish this?

6

I'd do this with two functions: one takes a transaction, and does the work inside the transaction. The second takes a connection, and calls the first function inside a transaction.

Note that PostgreSQL is capable of tracking savepoints within a transaction, which effectively acts as sub-transactions, and SQLx has chosen to expose this as nested transactions; it doesn't let you know if you're inside a transaction, and you can nest arbitrarily, so you can't detect a transaction by trying to start a new one and failing.

1 Like
7

I'd do this with two functions: one takes a transaction, and does the work inside the transaction. The second takes a connection, and calls the first function inside a transaction.

I thought about this but since I have tons of methods I Think this can be a nightmar for maintenance.

PostgreSQL is capable of tracking savepoints within a transaction, which effectively acts as sub-transactions, and SQLx has chosen to expose this as nested transactions

you can nest arbitrarily

So I can start a transaction even if I don't know if the PgConnection is already one, right?

8

Yes - SQLx will convert to savepoints if you're already inside a transaction, allowing you to nest SQLx transactions freely.

1 Like
9

What do you think about:

pub async fn find_player_id(
    &self,
    db: Option<&mut sqlx::PgConnection>,
    id: &str,
    team_id: &str,
) -> Result<String, Error> {
    let mut db = match db {
        Some(db) => db.begin().await?,
        None => self.pool.begin().await?,
    };

    let _ = self
        .team_service
        .find_team_id(Some(&mut db), team_id)
        .await?;

    let player_id = self.repo.find_player_id(Some(&mut db), id).await?;

    db.commit().await?;

    Ok(player_id)
}
10

I think you're making pain for yourself by allowing the caller to optionally provide a db to every call, or rely on your internal pool. Pick one:

  1. PlayerService contains a connection pool, and you get connections from the pool as needed.
  2. Callers pass in a Transaction, and you do your work in there without committing the passed-in transaction - either you open a nested transaction (which becomes a savepoint in PostgreSQL), or you do your work and leave it uncommitted.
1 Like
11

Thanks for continuing to answer me.

  1. All methods of PlayerService can be called with a transaction or with a connection to the DB (non-transaction); sometimes with one, sometimes with the other; so I can't just use the pool;

  2. as mentioned before, sometimes the callers don't pass a transaction, but it is a direct call and some methods could call in a transaction other methods even from the repository.

Is the problem and my proposed solution clearer now?

12

Or maybe I can use this:

pub trait Queryer<'c>: sqlx::Acquire<'c, Database = sqlx::Postgres> {}

impl<'c> Queryer<'c> for &sqlx::PgPool {}
impl<'c> Queryer<'c> for &'c mut sqlx::PgConnection {}

impl PlayerService {
    pub async fn find_player_id<'c, D: Queryer<'c>>(
        &self,
        db: D,
        id: &str,
        team_id: &str,
    ) -> Result<String, Error> {
        let mut tx = db.begin().await?;

        let _ = self.team_service.find_team_id(&mut *tx, team_id).await?;

        let player_id = self.repo.find_player_id(&mut *tx, id).await?;

        tx.commit().await?;

        Ok(player_id)
    }
}

But I still need to have an optional argument on service methods, right?

13

The problem you're describing is one that makes your solution messy; if you can't change the problem, then you're going to be in a bad position forever, because you're describing constraints that stop you doing a decent job at this layer.

Your description says that some callers absolutely know that there's a database involved, and cannot work if this layer hides the database from them. Other callers must not know there's a database involved, and cannot work if this layer requires them to know about the database. That's the underlying source of mess here, and until you fix that, you're going to have trouble doing a good job at this layer.

Option one is to say that all callers should know about the database, and you will handle callers that "must not" know about the database by giving them an easy route to handle transactions using a pool you manage. Callers that have a bare connection can get a transaction trivially by calling begin(), and can call commit() when they've done their work, and your functions never call commit() without a matching begin() (so they can do sub-transactions, but the caller is required to handle the outer transaction.

Option 2 is to hide the database from the callers completely, and have an internal pool; you get a connection from the pool, start a transaction, do your work, commit or rollback, and return the connection to the pool. The caller now can't put your work inside a larger transaction, but the caller doesn't have to care about the database.

You're trying to implement option 3 - allow for callers who know about the database, callers who treat the database as an opaque "thing", and callers who don't know about the database, all inside the individual service functions. This is always going to result in less-clear code, since you're trying to handle three very different classes of caller.

I therefore think you are better off having the service functions handle one class of caller (I'd suggest the one that has a transaction for you to do the work in), and providing utility functions for running a given call inside a transaction (like sqlx::Connection::transaction provides for bare connections), and for getting a connection from a pool, getting a transaction from that pool, running the service function, then committing the transaction and returning the connection.

That way, your service functions have one job - do my thing inside an outer transaction. Your callers get to decide, via the utility functions, whether they ask the service function to work inside an existing outer transaction, inside an existing connection, or inside a fresh connection obtained for this one call.

3 Likes
14

Thank you again for answering me.

I think we are talking about the same ideas, maybe.

Given:

pub struct TeamRepository {}

pub struct PlayerRepository {}

pub struct TeamService {
    pool: sqlx::PgPool,
    repo: Arc<TeamRepository>,
}

pub struct PlayerService {
    pool: sqlx::PgPool,
    repo: Arc<PlayerRepository>,
    team_service: TeamService,
}

I have:

  1. PlayerService methods like:

    impl PlayerService {
    pub async fn player_create(
        &self,
        // no DB connection here
        input: PlayerInput,
    ) -> Result<Player> {
        let db_conn = &mut self.pool.acquire().await?;

        /* ... */

        let txn = db_conn.begin().await?;

        // use txn here

        txn.commit().await?;
    }
}

  2. PlayerService methods like:

    impl PlayerService {
    pub async fn coach_create(
        &self,
        // it CAN get a DB connection here (a transaction already maybe)
        db_conn: Option<&mut sqlx::PgConnection>,
        input: CoachInput,
    ) -> Result<Coach> {
        let db_conn = match db_conn {
            Some(db_conn) => db_conn,
            None => &mut self.pool.acquire().await?,
        };

        // can begin and commit or rollback a transaction here
    }
}

  3. PlayerRepository methods like:

    impl PlayerRepository {
    pub async fn player_insert(
        &self,
        // it NEEDS a DB connection here, transaction or not
        db_conn: &mut sqlx::PgConnection,
        input: PlayerInput,
    ) -> Result<Player> {
        // use db_conn freely here and can
        // begin and commit or rollback a transaction too
    }
}

Why do you think this is bad? Can you write a small example?

Maybe you think it's wrong to have an argument for the db connection/transaction? And how do I can use "self.db_conn" instead? Your option n° 2?

15

Why? That's exactly what looks like the problem. If you are inside a transaction, you should not be accessing the DB by circumventing the transaction – that defeats the whole point.

16

Of the three you've given, option 1 is fine - hide the database completely from the caller, and they don't have to care.

Option 2 is problematic; why should I give you a db_conn other than None? What does it mean if I give you the "wrong" db_conn?

And both options 2 and 3 make it non-obvious how I get you to do your work inside an existing transaction; it happens, thanks to SQLx's API, that you can open a nested transaction, but that's not clear from the API.

Assuming that you don't want to completely hide the database from the user, I would prefer:

pub struct TeamRepository {}

pub struct PlayerRepository {}

pub struct TeamService {
    repo: Arc<TeamRepository>,
}

pub struct PlayerService {
    repo: Arc<PlayerRepository>,
    team_service: TeamService,
}

showing that nothing has the ability to get DB connections itself, then all 3 methods to look like:

impl PlayerService {
    pub async fn player_create(
        &self,
        txn: &mut sqlx::Transaction,
        input: PlayerInput,
    ) -> Result<Player> {
        // use txn here
    }
}
impl PlayerService {
    pub async fn coach_create(
        &self,
        txn: &mut sqlx::Transaction,
        input: CoachInput,
    ) -> Result<Coach> {
        // use txn here
    }
}
impl PlayerRepository {
    pub async fn player_insert(
        &self,
        txn: &mut sqlx::Transaction,
        input: PlayerInput,
    ) -> Result<Player> {
        // use txn here
    }
}

This then forces the user to make the database-relevant choices themselves, and pushes the "failure" cases into the user's code - for example, it stops the user assuming (falsely) that your internal database pool "tracks" the last connection the user gave you, since you don't have an internal database pool.

It also makes the transaction control the user's problem, so they get to choose whether (e.g.) you can do a player_create followed by a player_insert as a single atomic transaction, or as two transactions where the player_create can succeed without the player_insert succeeding. This avoids the problem of a user with a bad mental model of the system assuming that a player_create will always be paired with the following player_insert from a transaction point of view.

1 Like
17

There are a couple of ways I might design this, depending on the answer to this question: in the design you have in mind, is PlayerService responsible for transaction demarcation, or not?

If it is, then I would implement find_player_id as something akin to

    pub async fn find_player_id(
        &self,
        db: &sqlx::PgConnection,
        id: &str,
        team_id: &str,
    ) -> Result<String, Error> {
        let mut tx = db.begin().await?;

        let _ = self.team_service.find_team_id(&mut tx, team_id).await?;
        let player_id = self.repo.find_player_id(&mut tx, id).await?;

        tx.commit().await?;

        Ok(player_id)
    }

making suitable changes to find_team_id and find_player_id in the process.

If PlayerService is not responsible for transaction demarcation, then I would instead implement it as something like

    pub async fn find_player_id<'c>(
        &self,
        tx: &sqlx::Transaction<'c, sqlx::Postgres>,
        id: &str,
        team_id: &str,
    ) -> Result<String, Error> {
        let _ = self.team_service.find_team_id(&mut *tx, team_id).await?;
        let player_id = self.repo.find_player_id(&mut *tx, id).await?;

        Ok(player_id)
    }

Depending on the definition of PlayerService there are some variations on this theme, such as storing the transaction as a field of self, as well.

Trying to design an interface where methods optionally demarcate transactions if and only if they do not receive a transaction as an argument is going to be very complicated to develop and very complicated to use, and I would strongly encourage you to pick one or the other and use it consistently.

3 Likes
18

Why? That's exactly what looks like the problem. If you are inside a transaction, you should not be accessing the DB by circumventing the transaction – that defeats the whole point.

@paramagnetic, why not? slqx is OK with it: creates SAVEPOINT automatically so I can open a transaction inside another transaction. Is this what do you mean?

And for the "why": I need to call that method from other methods that creates a direct connection or a transaction. I don't want to duplicate the same method signature for having one with db argument and one without. Why is this that bad?

19

@farnz

Option 2 is problematic; why should I give you a db_conn other than None? What does it mean if I give you the "wrong" db_conn?

Because the fn coach_create can be called from methods that already have a DB transaction ongoing. So this transaction is passed as argument to it. I don't see any other way to do this.

I don't know how to avoid that argument for this use case. Can you suggest one?

What do you mean by ""wrong" db_conn"? A db connection instead of a transaction? Maybe should I change it to txn: Option<&mut sqlx::Transaction>?

it stops the user assuming (falsely) that your internal database pool "tracks" the last connection the user gave you, since you don't have an internal database pool.

I don't understand what you mean by this. The pool is simply the pool created by sqlx::PgPool::connect().

Maybe this is the information missing from the beginning: all the Service methods are getting called by the HTTP API handlers. I don't want to leak there too my DB specifics (sqlx or something else).

So in that case I call methods with db_conn: Option<&mut sqlx::PgConnection> passing None to them and they know how to create a new connection with self.pool.acquire().

One way I can handle this maybe is to create for each method an fn handle_coach_create() which creates the DB transaction and passes it to fn coach_create but this means duplication of each method and more code to maintain.

20

@derspiny

Thank you for answering me.

The thing is that find_player_id (but I have tons of them) can be called with or without the transaction. And I don't want to repeat each methods both with and without it.