isoos / postgresql-dart

Dart PostgreSQL driver: supports extended query format, binary protocol and statement reuse.
https://pub.dev/packages/postgres
BSD 3-Clause "New" or "Revised" License
131 stars 37 forks source link

Serializable isolation level transaction throws unexpected PgException for parallel transactions #385

Closed SandPod closed 2 weeks ago

SandPod commented 2 weeks ago

Background

I'm integrating isolation levels for transactions in the Serverpod Framework and wanted to create a test that highlighted the difference between REPEATABLE READ and SERIALIZABLE transaction isolation level.

One way to prove the integration is correct and the transaction isolation levels are respected is to insert two rows in a table. And then do the following operations with two transactions (T1 and T2):

  1. T1 - Read the first row in the table
  2. T2 - Read the second row in the table
  3. T1 - Update the second row in the table
  4. T2 - Update the first row in the table.
  5. Commit both transactions

If this is done using the REPEATABLE READ isolation level the modification is allowed and both entries are updated. If this is done using the SERIALIZABLE isolation level only the first modification is allowed and the second one will be rejected with a 40001 serialization_failure exception.

Validation

To validate the behavior in Postgres I created two SQL scrips and ran them through two separate connections directly towards a database, starting the first script first and then running the second one.

#### Create database ```sql DROP TABLE IF EXISTS t; CREATE TABLE t (id INT PRIMARY KEY, counter INT); INSERT INTO t VALUES (1, 0); INSERT INTO t VALUES (2, 1); ``` #### Repeatable read First transaction ```sql BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; SELECT * from t WHERE id=1; SELECT pg_sleep(5); UPDATE t SET counter = counter + 10 WHERE id=2; COMMIT; ``` Second transaction ```sql BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; SELECT * from t WHERE id=2; UPDATE t SET counter = counter + 20 WHERE id=1; COMMIT; ``` **Result** - Transaction will complete without exception and first row will have value 20 and second row will have value 11. #### Serializable First transaction ```sql BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; SELECT * from t WHERE id=1; SELECT pg_sleep(5); UPDATE t SET counter = counter + 10 WHERE id=2; COMMIT; ``` Second transaction ```sql BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; SELECT * from t WHERE id=2; UPDATE t SET counter = counter + 20 WHERE id=1; -- Optional sleep to simulate a long running transaction -- SELECT pg_sleep(5); COMMIT; ``` **Result** Only one transaction will complete while the other transaction will throw an exception.

Problem

When using the driver, I noticed that if two SERIALIZABLE transactions are allowed to complete in parallel a PgException with the description Session or transaction has already finished, did you forget to await a statement? is thrown instead of the expected ServerException with code 40001.

To capture my findings, I created three tests, one validating the REPEATABLE READ isolation level behavior, one validating the SERIALIZABLE isolation level behavior with special care, and then the last one where the unexpected behavior is exhibited.

Tests are available as a single commit on my branch here: https://github.com/SandPod/postgresql-dart/pull/1

The last test that exhibits the unexpected behavior and is also available here:

Validating test ```dart withPostgresServer('Transaction isolation level', (server) { group('Given two rows in the database and two database connections', () { late Connection conn1; late Connection conn2; setUp(() async { conn1 = await server.newConnection(); conn2 = await server.newConnection(); await conn1.execute('CREATE TABLE t (id INT PRIMARY KEY, counter INT)'); await conn1.execute('INSERT INTO t VALUES (1, 0)'); await conn1.execute('INSERT INTO t VALUES (2, 1)'); }); tearDown(() async { await conn1.execute('DROP TABLE t;'); await conn1.close(); await conn2.close(); }); test( 'when two transactions using repeatable read isolation level' 'reads the row updated by the other transaction' 'then one transaction throws exception ', () async { final c1 = Completer(); final c2 = Completer(); final f1 = Future.microtask( () => conn1.runTx( settings: TransactionSettings( isolationLevel: IsolationLevel.serializable, ), (session) async { await session.execute('SELECT * from t WHERE id=1'); c1.complete(); await c2.future; await session .execute('UPDATE t SET counter = counter + 10 WHERE id=2'); }, ), ); final f2 = Future.microtask( () => conn2.runTx( settings: TransactionSettings( isolationLevel: IsolationLevel.serializable, ), (session) async { await session.execute('SELECT * from t WHERE id=2'); await c1.future; // If we complete both transactions in parallel, we get an unexpected // exception c2.complete(); await session .execute('UPDATE t SET counter = counter + 20 WHERE id=1'); // If we complete the first transaction after the second transaction // the correct exception is thrown // c2.complete(); }, ), ); // This test throws Severity.error Session or transaction has already // finished, did you forget to await a statement? await expectLater( () => Future.wait([f1, f2]), throwsA( isA() .having((e) => e.severity, 'Exception severity', Severity.error) .having((e) => e.code, 'Exception code', '40001'), ), ); }); }); }); ```
isoos commented 2 weeks ago

@SandPod: amazing bug report, thank you! I think I have a vague idea what may be happening here, I will investigate.

isoos commented 2 weeks ago

@SandPod: I've merged #386, which passes on your included test case. I'm wondering: could you please test it again before I publish it? Or I could just publish it...

SandPod commented 2 weeks ago

@isoos My test passes with the fix in #386

Thank you for the quick turnaround on this! 🙏

isoos commented 2 weeks ago

Published as 3.4.2