[6.0] Fix exception handling on transaction commit in DB::transaction()

[jansennerd10]

When an exception occurs during commit (which can happen due to serialization failures, dropped connection, etc), DB::transaction() tries to roll back the current transaction. This results in another exception being thrown on rollback because there is no active transaction; the original exception is lost.

This PR modifies DB::transaction() to handle exceptions during commit properly, without trying to roll back. Additionally, it retries the transaction if the commit error was caused by a serialization failure.

This does not modify the existing retry logic, but simply pulls the commit operation out so that errors there can be handled separately. The tests covering the existing logic pass with no modifications.

[mfn]

Uh, SQLSTATE 40001 is database specific .. no? I don't think this is supposed to be in this "shared" handling code.

Why is only this particular error generating this problem and not others?

[jansennerd10]

Actually, no. SQLSTATE error codes are part of the SQL standard. PostgreSQL, MySQL, SQL Server, Oracle, and IBM databases all use SQLSTATE 40001 to report serialization failures. Many databases additionally use their own error codes to provide more detailed information, but the value returned by PDOException::getCode() is the SQLSTATE code.

Any exception occurring during commit would cause the rollback problem. We do need special handling for serialization failures, though, because that is a case where (just like a deadlock) retrying the transaction is the correct course of action.

[mfn]

#TIL , thanks

[staudenmeir]

Can you provide a simple example that demonstrates this issue?

[jansennerd10]

@staudenmeir so it turns out there are two distinct situations where Laravel doesn't handle serialization failures correctly. This PR originally fixed the second one I will describe below. The first one is fixed by abaff32.

Note that the examples use PostgreSQL. They could be modified to work with other DBMSes.

Case 1: Serialization failure occurs on update (fixed: abaff32)
Setup:

CREATE TABLE IF NOT EXISTS pr_29067_table (
	id SERIAL,
	value INT NOT NULL
);

INSERT INTO pr_29067_table ( value ) VALUES ( 2 );

Then run two instances of this code side by side in Tinker:

DB::transaction( function () {
        DB::statement( 'SET TRANSACTION ISOLATION LEVEL SERIALIZABLE' );
        
        DB::table( 'pr_29067_table' )->where( 'id', '=', 1 )->update( [ 'value' => \DB::raw( 'value + 1' ) ] );
        // This line makes sure that the transactions are actually concurrent.
        readline( 'Start another instance of this snippet and then press Enter>' );
    }, 3 );

Expected result (what you get if you run with this PR):
One of the instances immediately ends after pressing Enter. The other automatically runs a second try, which succeeds.

Actual result (what you get if you run with the current release of Laravel):
One of the instances immediately ends after pressing Enter. The other fails with this (or similar) exception:

Illuminate/Database/QueryException with message 'SQLSTATE[40001]: Serialization failure: 7 ERROR:  could not serialize access due to concurrent update (SQL: update "pr_29067_table" set "value" = 3 where "id" = 1)'

Case 2: Serialization failure occurs on commit (fixed: b78bf2d)
This scenario is adapted from https://wiki.postgresql.org/wiki/SSI#Black_and_White
Setup:

create table pr_29067_table2
  (
    id int not null primary key,
    color text not null
  );
insert into pr_29067_table2
  with x(id) as (select generate_series(1,10))
  select id, case when id % 2 = 1 then 'black'
    else 'white' end from x;

Run this code in Tinker:

    DB::transaction( function () {
        DB::statement( 'SET TRANSACTION ISOLATION LEVEL SERIALIZABLE' );
        
        DB::table( 'pr_29067_table2' )->where( 'color', '=', 'black' )->update( [ 'color' => 'white' ] );
        // This line makes sure that the transactions are actually concurrent.
        readline( 'Start the other snippet and then press Enter>' );
    }, 3 );

Before pressing Enter, run this opposite code in another instance of Tinker:

    DB::transaction( function () {
        DB::statement( 'SET TRANSACTION ISOLATION LEVEL SERIALIZABLE' );
        
        DB::table( 'pr_29067_table2' )->where( 'color', '=', 'white' )->update( [ 'color' => 'black' ] );
        // This line makes sure that the transactions are actually concurrent.
        readline( 'Start the other snippet and then press Enter>' );
    }, 3 );

The updates initially succeed; however, PostgreSQL identifies a serialization error when the second transaction tries to commit.

Expected result (what you get if you run with this PR):
One of the instances immediately ends after pressing Enter. The other automatically runs a second try, which succeeds.

Actual result (what you get if you run with the current release of Laravel):
One of the instances immediately ends after pressing Enter. The other fails with this (or similar) exception:

PDOException with message 'There is no active transaction'

Note that this is not the real exception! The real exception was a serialization failure very similar to the one in Case 1. However, because the exception occurred on commit, the transaction was automatically rolled back, leading to the exception of There is no active transaction.

[jansennerd10]

Technically, this may now amount to a breaking change. Let me know if I need to split this into two PRs: I could make one targeting 5.8 to fix the rollback bug, and one targeting 5.9 to handle serialization failures in addition to deadlocks.

[driesvints]

@jansennerd10 I've retargeted this to the master branch because of the breaking changes.

[staudenmeir]

Cosmetic suggestion: We could move return $callbackReturn; in here and remove continue; from the catch block.

[jansennerd10]

@staudenmeir I considered that initially, but I think this way makes the pattern of "if something goes wrong, continue; return at the end" more clear. In fact, while working on this PR I initially forgot to put in the first continue a few lines up, and it took several minutes to figure out why I was failing unit tests. So while your way is slightly more concise, I think this way is more readable and makes the code easier to modify in the future.

[jansennerd10]

@driesvints your review is still marked "changes requested," but all your comments are resolved. Do you have additional issues you'd like me to address?

Changes were made.

[driesvints]

@jansennerd10 sorry, forgot to dismiss it.

[jansennerd10]

I know you guys have a lot to deal with, but my team is waiting on the outcome of this PR to fix issues we're having in our Laravel app. Any idea when there might be an update? Thank you!

[driesvints]

@jansennerd10 even if this PR gets merged soon, the next major release is only coming out in September the earliest.

[jansennerd10]

@driesvints true, but if we know that this issue is going to be fixed in that release we can plan accordingly and use the master branch in our development versions until 5.9 is released. If it won't be fixed then we will have to implement our own workaround.

[driesvints]

@jansennerd10 I wouldn't recommend that. master will be unstable until its final release. Running anything in production with it would be a risk.