Connection state gets corrupted when an SQL error occurred in nested transactions.

[KeenS]

Setup

Versions

• Rust: rustc 1.36.0 (a53f9df32 2019-07-03)
• Diesel: 1.4.2
• Database: PostrgreSQL, psql (11.4 (Ubuntu 11.4-0ubuntu0.19.04.1), server 9.6.13)
• Operating System Ubuntu 19.04

Feature Flags

• diesel: "postgres"

Problem Description

When an SQL fails in nested transactions, the connection gets corrupted and all the other transaction running as SERIALIZABLE fails in AlreadyInTransaction.

What are you trying to accomplish?

Just writing an application. Transaction is nested in an accident

What is the expected output?

Ok(1)

What is the actual output?

Err(AlreadyInTransaction)

Are you seeing any additional errors?

postgresql output:

postgresql_1     | ERROR:  column "foo" does not exist at character 8
postgresql_1     | STATEMENT:  SELECT foo
postgresql_1     | ERROR:  current transaction is aborted, commands ignored until end of transaction block
postgresql_1     | STATEMENT:  RELEASE SAVEPOINT diesel_savepoint_1

Steps to reproduce

run code below

Cargo.toml

[package]
name = "diesel-bug-report"
version = "0.1.0"
edition = "2018"

[dependencies]
diesel = {version = "1.4.0", features = ["postgres"]}

src/main.rs

use diesel::pg::PgConnection;
use diesel::prelude::*;
use diesel::result::Error;

fn main() {
    let conn = PgConnection::establish("postgres://user:password@localhost/database").unwrap();
    // fail once
    let ret = conn.transaction(|| {
        conn.transaction(|| {
            // handling error
            match conn.execute("SELECT foo") {
                // do nothing
                Ok(_) => (),
                // ignore the error
                Err(e) => eprintln!("error occurred: {}", e),
            };
            Ok::<_, Error>(())
        })
    });
    println!("{:?}", ret);
    // other transaction
    let ret = conn
        .build_transaction()
        .serializable()
        .run(|| conn.execute("SELECT 1"));
    // must be Ok(1), but get Err(AlreadyInTransaction)
    println!("{:?}", ret);
}

Then, gets output

$ cargo run
error occurred: column "foo" does not exist
Err(DatabaseError(__Unknown, "current transaction is aborted, commands ignored until end of transaction block"))
Err(AlreadyInTransaction)

The error occurs with only when you use .build_transacion().
However, internally, the connection is unsound: TransactionManager's depth is 1 which should be 0 at that point.

Checklist

• I have already looked over the issue tracker for similar issues.
• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)

[KeenS]

FYI, here are logs, one is diesel's, which I got by modifying diesel, and the other is PostgreSQL, which shows how nested transactions behave.

[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:99] transaction_manager = AnsiTransactionManager {
    transaction_depth: Cell {
        value: 0,
    },
}
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:99] dbg!(transaction_manager).begin_transaction(self) = Ok(
    (),
)
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:99] transaction_manager = AnsiTransactionManager {
    transaction_depth: Cell {
        value: 1,
    },
}
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:99] dbg!(transaction_manager).begin_transaction(self) = Ok(
    (),
)
error occurred: column "foo" does not exist
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:102] transaction_manager = AnsiTransactionManager {
    transaction_depth: Cell {
        value: 2,
    },
}
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:102] dbg!(transaction_manager).commit_transaction(self) = Err(
    DatabaseError(
        __Unknown,
        "current transaction is aborted, commands ignored until end of transaction block",
    ),
)
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:106] transaction_manager = AnsiTransactionManager {
    transaction_depth: Cell {
        value: 2,
    },
}
[/home/shun/Rust/diesel/diesel/src/connection/mod.rs:106] dbg!(transaction_manager).rollback_transaction(self) = Ok(
    (),
)
Err(DatabaseError(__Unknown, "current transaction is aborted, commands ignored until end of transaction block"))
[/home/shun/Rust/diesel/diesel/src/pg/transaction.rs:282] transaction_manager = AnsiTransactionManager {
    transaction_depth: Cell {
        value: 1,
    },
}
[/home/shun/Rust/diesel/diesel/src/pg/transaction.rs:282] dbg!(transaction_manager).begin_transaction_sql(self.connection, &sql) = Err(
    AlreadyInTransaction,
)
Err(AlreadyInTransaction)

psql (11.4 (Ubuntu 11.4-0ubuntu0.19.04.1), server 9.6.13)
Type "help" for help.

database=# BEGIN;
BEGIN
database=# SAVEPOINT hoge;
SAVEPOINT
database=# SELECT foo;
ERROR:  column "foo" does not exist
LINE 1: SELECT foo;
               ^
database=# RELEASE SAVEPOINT hoge;
ERROR:  current transaction is aborted, commands ignored until end of transaction block
database=# ROLLBACK;
ROLLBACK

I guess the correct way to address this issue is when you get an aborted error, keep it and decrement the depth.

[sgrif]

It looks like the issue here stems from the fact that we are only changing the transaction depth if the query succeeded. I don't think this is in any way limited to nested transactions, I believe you will get similar behavior by returning Ok to transaction when a database error occurs, even for the outer transaction

There are a few options here:

• Always perform the depth change, even if an error occurs
• Hold onto what the depth was before opening the transaction, and set to that instead of decrementing by 1
• Attempt to rollback if committing fails
• Do nothing. An error was explicitly ignored, the transaction API was lied to, and "we don't know if this connection is safe to use or not, it's in a poisoned state" is reasonable.

If we do change how we set the depth, we need to make sure that nothing happens if opening the transaction fails.

Always performing the depth change is somewhat of a lie, since if RELEASE SAVEPOINT fails, we're still at the same depth as before, we just can't run any queries except ROLLBACK (either the whole transaction or to a savepoint). This is probably fine since the only time we'd be using it is if you try to open another nested transaction, which would fail regardless of if we use a name that already exists or not.

Precomputing the depth change is the most "honest" solution I think, but it has the main problem of only working if the nested transaction fails. (e.g. if COMMIT fails, we'll still have a depth of 1). This is fine in the "normal" case, since if COMMIT fails, it should mean the connection itself died, so the connection is unusable anyway. However, a serialization error should result in COMMIT failing, but us being at a depth of 0 (you don't need to run ROLLBACK manually in that case). There's also the ignoring an error case above, in which case I don't know whether we need to rollback manually or not.

I'm not sure if there are any consequences to attempting to rollback if committing fails.

I'd love to see any PR tackling this (or comments that more strongly demonstrate that we should/shouldn't fix this) address the questions/concerns above, why it's the best option, and what the behavior is in all of the various cases

[KeenS]

I believe you will get similar behavior by returning Ok to transaction when a database error occurs, even for the outer transaction

Logically yes, but in most cases, COMMIT/ROLLBACK likely succeeds because it terminates failed transactions.

database=# BEGIN;
BEGIN
database=# select foo;
ERROR:  column "foo" does not exist
LINE 1: select foo;
               ^
database=# COMMIT;
ROLLBACK

One observation here is you can recover error by rolling back savepoint.

database=# BEGIN;
BEGIN
database=# SAVEPOINT hoge;
SAVEPOINT
database=# select foo;
ERROR:  column "foo" does not exist
LINE 1: select foo;
               ^
database=# ROLLBACK TO hoge;
ROLLBACK
database=# select 1;
 ?column?
----------
        1
(1 row)

database=# COMMIT;
COMMIT

I think 'Attempt to rollback (savepoint) if committing fails' is consistent to the behavior of outer most BEGIN / COMMIT / ROLLBACK

[rustonaut]

I also think "Attempting to rollback the savepoint if releasing it (commit) does fail" is the best solution as it mirrors how top-level commit failure works (at last in postgres, an I guess in other DBs, too).

I also would love it if the transaction manager uses the r2d2::ManageConnection::has_broken hook to check some fast to check invariants, i.e. that transaction depth is 0 and if they are broken drop the connection just to be on the safe side.

fn has_broken(&self, conn: &mut Self::Connection) -> bool {
        let transaction_depth = <TransactionManager<Self::Connection>>
            ::get_transaction_depth(conn.transaction_manager());

        transaction_depth != 0
}

(I think this is currently the only invariant which we can fastly check across all databases/)

[rustonaut]

Just to clarify I'm aware that this problem happens independent of r2d2, it's just that with connection pooling the reach such a bug can have is pretty big.

[torepettersen]

I experienced the same problem even without the nesting, just as @sgrif expected. I was making a test that was validating that a value should be unique and noticed that the following test was failing.

I also reproduced it:

// main.rs
use diesel::pg::PgConnection;
use diesel::prelude::*;

fn main() {
    let conn = PgConnection::establish("postgres://postgres:password@localhost/main").unwrap();
    conn.begin_test_transaction().unwrap();

    let ret = conn.execute("insert into foo (bar) values ('hello world')");
    println!("{:?}", ret);

    match conn.execute("insert into foo (bar) values ('hello world')") {
        Ok(_) => panic!("This query should fail"),
        Err(e) => println!("This should fail with unique constraint violation: {}", e),
    };

    let ret = conn.execute("select * from foo where bar = 'hello world'");
    println!("{:?}", ret);
}

Which gives the following output:

$ cargo run
Ok(1)
This should fail with unique constraint violation: duplicate key value violates unique constraint "foo_bar_key"
Err(DatabaseError(__Unknown, "current transaction is aborted, commands ignored until end of transaction block"))