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

fix: hide sqlx_postgres::any #3254

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Conversation

Zarathustra2
Copy link
Contributor

Currently you can call begin() on a postgres transaction without causing an error.

This causes errors such as:

    let url = "postgres://dario@localhost/sqlx".parse().unwrap();
    let opts: PgConnectOptions = ConnectOptions::from_url(&url).unwrap();

    let pool = PgPoolOptions::new()
        .max_connections(10)
        .connect_with(opts)
        .await
        .expect("PgPool");

    sqlx::query("drop table if exists sqlx_issue").execute(&pool).await.unwrap();
    sqlx::query("create table sqlx_issue (name varchar(255))").execute(&pool).await.unwrap();
    sqlx::query("insert into sqlx_issue (name) values ('issue')").execute(&pool).await.unwrap();

    {
        let mut trans: Transaction<'static, Postgres> = pool.begin().await.unwrap();
        // Call begin again()
        trans.begin().await.unwrap();

        sqlx::query("delete from sqlx_issue").execute(&mut *trans).await.unwrap();
        trans.commit().await.unwrap();
    }
        

    let row = sqlx::query("select name from sqlx_issue").fetch_optional(&pool).await.unwrap();
    // This will fail because we started the transaction twice. 
    // Calling commit does not actually commit the transaction because we rewrapped the inner transaction.
    // As a result the delete query had never been committed. 
    assert!(row.is_none());

@abonander
Copy link
Collaborator

Nesting of transactions is intentional. Once inside a transaction, begin() creates a savepoint and dropping the transaction (which happens immediately) queues a ROLLBACK TO SAVEPOINT statement to be executed the next time the connection is used.

Something else is going on here.

@Zarathustra2
Copy link
Contributor Author

Zarathustra2 commented Jun 2, 2024

@abonander I see. I can take another look and try to debug it + fix it.

I assume the nested transactions does not get committed but rollbacked. Let me take a look again

@Zarathustra2
Copy link
Contributor Author

Okay, I found the issue:

use sqlx::{postgres::{PgConnectOptions, PgPoolOptions}, ConnectOptions, PgPool, Postgres, Row, Transaction};

// Comment out `use sqlx::Acquire` and comment in `sqlx::postgres::any::AnyConnectionBackend`
// and the transaction will not be committed.
//
// use sqlx::postgres::any::AnyConnectionBackend;
use sqlx::Acquire;

#[tokio::main]
async fn main() {

    let url = "postgres://dario@localhost/sqlx".parse().unwrap();
    let opts: PgConnectOptions = ConnectOptions::from_url(&url).unwrap();

    let pool = PgPoolOptions::new()
        .max_connections(10)
        .connect_with(opts)
        .await
        .expect("PgPool");


    sqlx::query("drop table if exists sqlx_issue").execute(&pool).await.unwrap();
    sqlx::query("create table sqlx_issue (name varchar(255))").execute(&pool).await.unwrap();
    sqlx::query("insert into sqlx_issue (name) values ('issue')").execute(&pool).await.unwrap();

    let row = sqlx::query("select name from sqlx_issue").fetch_optional(&pool).await.unwrap();
    println!("{:?}", row.unwrap().get::<String, _>(0));

    {
        let mut trans = pool.begin().await.unwrap();
        // Call begin again()
        trans.begin().await.unwrap();
        trans.begin().await.unwrap();
        trans.begin().await.unwrap();

        sqlx::query("delete from sqlx_issue").execute(&mut *trans).await.unwrap();
        trans.commit().await.unwrap();
    }
        

    let row = sqlx::query("select name from sqlx_issue").fetch_optional(&pool).await.unwrap();
    // Succeeds with `use sqlx::Acquire`. Fails with `use sqlx::postgres::any::AnyConnectionBackend`
    assert!(row.is_none());
}
[dependencies]
tokio = { version = "1.34.0", features = ["full"] }
sqlx = { version = "0.7.4", features = [ "runtime-tokio", "tls-native-tls", "postgres", "rust_decimal", "chrono", "uuid", "migrate", "macros" ] }

A wrong use can break the nested transactions it seems.

@abonander
Copy link
Collaborator

That's why. AnyConnectionBackend is not an end-user trait, it doesn't use the Transaction RAII guard. Your IDE probably suggested it as an import.

The any module of sqlx-postgres is public but it should be #[doc(hidden)]: https://github.com/launchbadge/sqlx/blob/main/sqlx-postgres/src/lib.rs#L29

It needs to be accessible from the sqlx facade crate which is why it can't just be private. Still, marking it hidden should mean an IDE won't recommend importing anything from it.

@Zarathustra2
Copy link
Contributor Author

Ah that makes sense. Thanks for the explanation. I have added that as well as a comment.

Thanks for the help!

@abonander abonander changed the title FIX: Do not allow nested postgres transaction fix: hide sqlx_postgres::any Jun 6, 2024
@abonander abonander merged commit c57bcb9 into launchbadge:main Jun 6, 2024
4 checks passed
jayy-lmao pushed a commit to jayy-lmao/sqlx that referenced this pull request Jun 6, 2024
jrasanen pushed a commit to jrasanen/sqlx that referenced this pull request Oct 14, 2024
jrasanen pushed a commit to jrasanen/sqlx that referenced this pull request Oct 14, 2024
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