Fix explicit transaction start and add tests

geldata · Feb 4, 2025 · 956e89c · 956e89c
1 parent 85f807a
commit 956e89c
Show file tree

Hide file tree

Showing 4 changed files with 339 additions and 18 deletions.
diff --git a/edb/ir/statypes.py b/edb/ir/statypes.py
@@ -801,3 +801,13 @@ def get_translation_map(cls) -> Mapping[TransactionIsolationEnum, str]:
             TransactionIsolationEnum.Serializable: "serializable",
             TransactionIsolationEnum.RepeatableRead: "repeatable read",
         }
+
+    def to_qltypes(self) -> qltypes.TransactionIsolationLevel:
+        from edb.edgeql import qltypes
+        match self._val:
+            case TransactionIsolationEnum.Serializable:
+                return qltypes.TransactionIsolationLevel.SERIALIZABLE
+            case TransactionIsolationEnum.RepeatableRead:
+                return qltypes.TransactionIsolationLevel.REPEATABLE_READ
+            case _:
+                raise AssertionError(f"unexpected value: {self._val!r}")
diff --git a/edb/lib/cfg.edgeql b/edb/lib/cfg.edgeql
@@ -191,6 +191,7 @@ ALTER TYPE cfg::AbstractConfig {
     CREATE REQUIRED PROPERTY default_transaction_isolation
         -> sys::TransactionIsolation
     {
+        CREATE ANNOTATION cfg::affects_compilation := 'true';
         CREATE ANNOTATION cfg::backend_setting :=
             '"default_transaction_isolation"';
         CREATE ANNOTATION std::description :=

diff --git a/edb/server/compiler/compiler.py b/edb/server/compiler/compiler.py
@@ -2153,27 +2153,39 @@ def _compile_ql_transaction(
 
         ctx.state.start_tx()
 
-        sqls = 'START TRANSACTION'
+        # Compute the effective isolation level
+        iso_config: statypes.TransactionIsolation = _get_config_val(
+            ctx, "default_transaction_isolation"
+        )
+        default_iso = iso_config.to_qltypes()
         iso = ql.isolation
-        if iso is not None:
-            if (
-                iso is not qltypes.TransactionIsolationLevel.SERIALIZABLE
-                and ql.access is not qltypes.TransactionAccessMode.READ_ONLY
-            ):
-                raise errors.TransactionError(
-                    f"{iso.value} transaction isolation level is only "
-                    "supported in read-only transactions",
-                    span=ql.span,
-                    hint=f"specify READ ONLY access mode",
+        if iso is None:
+            iso = default_iso
+
+        # Compute the effective access mode
+        access = ql.access
+        if access is None:
+            if default_iso is qltypes.TransactionIsolationLevel.SERIALIZABLE:
+                access_mode: statypes.TransactionAccessMode = _get_config_val(
+                    ctx, "default_transaction_access_mode"
                 )
-            sqls += f' ISOLATION LEVEL {iso.value}'
-        if ql.access is None:
-            access_mode: statypes.TransactionAccessMode = _get_config_val(
-                ctx, "default_transaction_access_mode"
+                access = access_mode.to_qltypes()
+            else:
+                access = qltypes.TransactionAccessMode.READ_ONLY
+
+        # Guard against unsupported isolation + access combinations
+        if (
+            iso is not qltypes.TransactionIsolationLevel.SERIALIZABLE
+            and access is not qltypes.TransactionAccessMode.READ_ONLY
+        ):
+            raise errors.TransactionError(
+                f"{iso.value} transaction isolation level is only "
+                "supported in read-only transactions",
+                span=ql.span,
+                hint=f"specify READ ONLY access mode",
             )
-            sqls += f' {access_mode.to_qltypes().value}'
-        else:
-            sqls += f' {ql.access.value}'
+
+        sqls = f'START TRANSACTION ISOLATION LEVEL {iso.value} {access.value}'
         if ql.deferrable is not None:
             sqls += f' {ql.deferrable.value}'
         sqls += ';'

diff --git a/tests/test_server_proto.py b/tests/test_server_proto.py
@@ -2069,6 +2069,304 @@ async def test_server_proto_tx_22(self):
 
         self.assertEqual(await self.con.query_single('SELECT 42'), 42)
 
+    async def test_server_proto_tx_23(self):
+        # Test that default_transaction_isolation is respected
+
+        await self.con.query('''
+            CONFIGURE SESSION
+                SET default_transaction_isolation := 'RepeatableRead';
+        ''')
+
+        try:
+            self.assertEqual(
+                await self.con.query(
+                    'select <str>sys::get_transaction_isolation();',
+                ),
+                ["RepeatableRead"],
+            )
+        finally:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+    async def test_server_proto_tx_24(self):
+        # default_transaction_isolation < Serializable enforces read-only
+
+        await self.con.query('''
+            CONFIGURE SESSION
+                SET default_transaction_isolation := 'RepeatableRead';
+        ''')
+
+        try:
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'cannot execute.*RepeatableRead',
+            ):
+                await self.con.query('''
+                        INSERT Tmp {
+                            tmp := 'aaa'
+                        };
+                    ''')
+        finally:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_25(self):
+        # default_transaction_isolation < Serializable overrides read-write
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_isolation := 'RepeatableRead';
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_access_mode := 'ReadWrite';
+            ''')
+
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'cannot execute.*RepeatableRead',
+            ):
+                await self.con.query('''
+                        INSERT Tmp {
+                            tmp := 'aaa'
+                        };
+                    ''')
+        finally:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_access_mode;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_26(self):
+        # Test that default_transaction_access_mode is respected
+
+        await self.con.query('''
+            CONFIGURE SESSION
+                SET default_transaction_access_mode := 'ReadOnly';
+        ''')
+
+        try:
+            self.assertEqual(
+                await self.con.query(
+                    'select <str>sys::get_transaction_isolation();',
+                ),
+                ["Serializable"],
+            )
+
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'cannot execute.*ReadOnly',
+            ):
+                await self.con.query('''
+                        INSERT Tmp {
+                            tmp := 'aaa'
+                        };
+                    ''')
+        finally:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_access_mode;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_27(self):
+        # Test that START TRANSACTION respects the default isolation
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_isolation := 'RepeatableRead';
+            ''')
+            await self.con.query('''
+                START TRANSACTION;
+            ''')
+
+            self.assertEqual(
+                await self.con.query(
+                    'select <str>sys::get_transaction_isolation();',
+                ),
+                ["RepeatableRead"],
+            )
+
+        finally:
+            await self.con.query(f'''
+                ROLLBACK;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_28(self):
+        # Test that non-serializable START TRANSACTION enforces read-only
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_isolation := 'RepeatableRead';
+            ''')
+            await self.con.query('''
+                START TRANSACTION;
+            ''')
+
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'read-only transaction'):
+
+                await self.con.query('''
+                    INSERT Tmp {
+                        tmp := 'aaa'
+                    };
+                ''')
+        finally:
+            await self.con.query(f'''
+                ROLLBACK;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_29(self):
+        # Test that START TRANSACTION respects default read-only
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_access_mode:= 'ReadOnly';
+            ''')
+            await self.con.query('''
+                START TRANSACTION;
+            ''')
+
+            self.assertEqual(
+                await self.con.query(
+                    'select <str>sys::get_transaction_isolation();',
+                ),
+                ["Serializable"],
+            )
+
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'read-only transaction'):
+
+                await self.con.query('''
+                    INSERT Tmp {
+                        tmp := 'aaa'
+                    };
+                ''')
+        finally:
+            await self.con.query(f'''
+                ROLLBACK;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_access_mode;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_30(self):
+        # Test that non-serializable START TRANSACTION conflicts read-write
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_isolation := 'RepeatableRead';
+            ''')
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'only supported in read-only transactions',
+            ):
+                await self.con.query('''
+                    START TRANSACTION READ WRITE;
+                ''')
+
+        finally:
+            await self.con.query(f'''
+                ROLLBACK;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_isolation;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
+    async def test_server_proto_tx_31(self):
+        # Test that non-serializable START TRANSACTION works fine with the
+        # default read-only
+
+        try:
+            await self.con.query('''
+                CONFIGURE SESSION
+                    SET default_transaction_access_mode:= 'ReadOnly';
+            ''')
+            await self.con.query('''
+                START TRANSACTION ISOLATION REPEATABLE READ;
+            ''')
+
+            self.assertEqual(
+                await self.con.query(
+                    'select <str>sys::get_transaction_isolation();',
+                ),
+                ["RepeatableRead"],
+            )
+
+            with self.assertRaisesRegex(
+                edgedb.TransactionError,
+                'read-only transaction'):
+
+                await self.con.query('''
+                    INSERT Tmp {
+                        tmp := 'aaa'
+                    };
+                ''')
+        finally:
+            await self.con.query(f'''
+                ROLLBACK;
+            ''')
+            await self.con.query('''
+                CONFIGURE SESSION
+                    RESET default_transaction_access_mode;
+            ''')
+
+        self.assertEqual(
+            await self.con.query('SELECT 42'),
+            [42])
+
 
 class TestServerProtoMigration(tb.QueryTestCase):