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

Add `DatabaseErrorKind` variants for isolation failures #1612

Open
kestred opened this Issue Apr 4, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@kestred
Contributor

kestred commented Apr 4, 2018

Feature Request

The DatabaseErrorKind enum is intended to container error kind variants describing errors which are commonly recoverable at the application level.

I'd like to have a variant or variants representing when an isolation failure occurs in the database (e.g. an otherwise valid request fails due to a conflicting transaction occurring at the same time; such as commit serializability, dead lock, etc).

In some applications these are frequently recoverable via simple retry logic (ideally implemented with randomized retry falloff), so it should make sense to have variants for them in the enum.

Checklist

  • I have already looked over the issue tracker for similar issues.
@sgrif

This comment has been minimized.

Member

sgrif commented Apr 4, 2018

Seems reasonable. Can you point me at the specific error code and provide a minimal query/code to trigger the error?

@kestred

This comment has been minimized.

Contributor

kestred commented Sep 9, 2018

The error codes that I'm hoping to catch are:

  • 40001 | serialization_failure
  • 40P01 | deadlock_detected

Typically one or the other depending on whether the transaction is constructed with build_transaction().serializable() or not.

@kestred

This comment has been minimized.

Contributor

kestred commented Sep 9, 2018

This error occurs for my use case specifically when two different services were attempting to do something that conflicts, but I've not been successful building a simplified use case.

Non-Working Test (for serialization_failure)

Sourced from the postgres documentation hoping to find a minimal test case --- I'd written this, but I must've gotten something a bit wrong 😓.

#[test]
#[cfg(feature = "postgres")]
fn isolation_error_is_detected() {
    // we need custom and multiple transactions, so we can't use default transaction
    let conn = connection_without_transaction();
    conn.execute(
            "CREATE TABLE IF NOT EXISTS isolation_example (
                id SERIAL PRIMARY KEY,
                class INTEGER NOT NULL,
                value INTEGER NOT NULL
            );",
        )
        .unwrap();
    conn.execute(
            "INSERT INTO isolation_example (class, value) VALUES
                (1, 10),
                (1, 20),
                (2, 100),
                (2, 200)
             ;",
        ).unwrap();

    let other_process = ::std::thread::spawn(|| {
        let other_conn = connection_without_transaction();
        other_conn.build_transaction().serializable().run::<_, result::Error, _>(|| {
            other_conn
                .execute("SELECT SUM(value) AS sum FROM isolation_example WHERE class = 1);")
                .unwrap();

            ::std::thread::sleep(::std::time::Duration::from_millis(5));

            other_conn
                .execute("INSERT INTO isolation_example (class, value) VALUES (2, 30);")
                .unwrap();
            Ok(())
        }).ok();
    });

    let failure = conn.build_transaction().serializable().run::<_, result::Error, _>(|| {
        ::std::thread::sleep(::std::time::Duration::from_millis(2));

        conn.execute("SELECT SUM(value) FROM isolation_example WHERE class = 2;")
            .unwrap();

        other_process.join().unwrap();

        Ok(())
    });
    assert_matches!(failure, Err(DatabaseError(IsolationFailure, _)));

    // clean up because we aren't in a transaction
    conn.execute("DROP TABLE isolation_example;").unwrap();
}

I'll take another crack it at eventually...

@sgrif

This comment has been minimized.

Member

sgrif commented Sep 10, 2018

Calling the error SerializationFailure, as it does not appear to apply to any isolation levels other than SERIALIZABLE, and that is the documented text for this error in the SQLSTATE standard

sgrif added a commit that referenced this issue Sep 10, 2018

Add an explicit error kind for failed `SERIALIZABLE` transactions
"serializable" is the strictest isolation level defined in the SQL
standard. Any two transactions at this isolation level executed
concurrently must have the same results, regardless of which order they
are actually executed in. The standard also defines this as the default
isolation level, but there is no backend I'm aware of that both properly
implements the serializable isolation level, and uses is it as the
default.

We're generally very conservative about adding error kinds to this enum,
and only do so when there's a demonstrable use case for handling it
programatically. I believe this is clearly the case for this error code,
since the correct response to receiving it is almost always to retry the
transaction.

The test for this is pretty straight forward. We create two
transactions. Each one counts half the table, and then inserts a row in
the other half. When run serially, one of the transactions will get 1
for its count, and the other will get 2. Which transaction gets which
result depends on the order they are run, so one of them will fail to
commit. Interestingly, PG is too conservative in determining whether the
dependency exists. If I replace `other_i` with `i`, the transactions are
no longer dependent, but PG still thinks they are.

Currently this error is only detected on PostgreSQL, as that is the only
backend that we support isolation levels in our query builder, and the
semantics of this isolation level are unclear on other backends (SQLite
claims its the default, but the behavior it describes is `REPEATABLE
READ`. MySQL appears to just make your transactions deadlock).

This is the first half of #1612
@sgrif

This comment has been minimized.

Member

sgrif commented Sep 10, 2018

I've cleaned up the test case and opened #1839 for the SERIALIZABLE half of this. I'm holding off on deadlocks for now, since that's much easier to demonstrate across multiple backends (maybe not SQLite >_>), but the SQLSTATE code for it is PG specific

sgrif added a commit that referenced this issue Sep 10, 2018

Add an explicit error kind for failed `SERIALIZABLE` transactions
"serializable" is the strictest isolation level defined in the SQL
standard. Any two transactions at this isolation level executed
concurrently must have the same results, regardless of which order they
are actually executed in. The standard also defines this as the default
isolation level, but there is no backend I'm aware of that both properly
implements the serializable isolation level, and uses is it as the
default.

We're generally very conservative about adding error kinds to this enum,
and only do so when there's a demonstrable use case for handling it
programatically. I believe this is clearly the case for this error code,
since the correct response to receiving it is almost always to retry the
transaction.

The test for this is pretty straight forward. We create two
transactions. Each one counts half the table, and then inserts a row in
the other half. When run serially, one of the transactions will get 1
for its count, and the other will get 2. Which transaction gets which
result depends on the order they are run, so one of them will fail to
commit. Interestingly, PG is too conservative in determining whether the
dependency exists. If I replace `other_i` with `i`, the transactions are
no longer dependent, but PG still thinks they are.

Currently this error is only detected on PostgreSQL, as that is the only
backend that we support isolation levels in our query builder, and the
semantics of this isolation level are unclear on other backends (SQLite
claims its the default, but the behavior it describes is `REPEATABLE
READ`. MySQL appears to just make your transactions deadlock).

This is the first half of #1612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment