Add a hack to make migration deadlocks less likely in some common cases

edb/pgsql/delta.py

@@ -3047,6 +3047,34 @@ def get_alter_table(
             if table_name is None:
                 self.table_name = tabname
 
+        # HACK: See issue #6304.
+        # There are a lot of opportunities for deadlocks between DDL
+        # (which can take a lot of full locks on things) and
+        # long-running queries.  One place this comes up is with views,
+        # since recreating a view requires a lock. When adding things to
+        # tables, our DDL must modify the table, then modify the view.
+        # This poses a deadlock risk with queries, which nearly always
+        # will access the view before accessing the table (since the table
+        # is how they get to the view).
+        #
+        # Put a hacky and hopefully temporary bandage around this particular
+        # deadlock case by injecting a no-op ALTER VIEW before the first
+        # ALTER of some table.  This ensures that DDL locks the inhview
+        # before the real table, making the lock order match the typical
+        # query lock order.
+        #
+        # By putting the hook to insert the ALTER VIEW here, we cover
+        # both the object and link cases and avoid doing it when the
+        # table is not actually modified (which is important, since
+        # doing that could induce the sort of deadlock we are trying
+        # to avoid).
+        if dbops.AlterTable not in self._multicommands:
+            mod, name = common.get_backend_name(
+                schema, self.scls, aspect='inhview', catenate=False)
+            self.pgops.add(dbops.Query(f'''\
+                ALTER VIEW IF EXISTS {mod}."{name}" SET SCHEMA {mod};
+            '''))
+
         return self._get_multicommand(
             context, dbops.AlterTable, tabname,
             force_new=force_new, manual=manual,

tests/test_edgeql_ddl.py

@@ -16,6 +16,7 @@
 # limitations under the License.
 #
 
+import asyncio
 import decimal
 import json
 import os
@@ -16916,3 +16917,73 @@ async def test_edgeql_ddl_reindex(self):
         await self.con.execute('''
             administer reindex(Object)
         ''')
+
+    async def _deadlock_tester(self, setup, teardown, modification, query):
+        """Deadlock test helper.
+
+        Interleave a long running query, some DDL, and a short running query
+        in a way that has triggered deadlock in the past.
+        (See #6304.)
+        """
+        cons = []
+        con1 = self.con
+        await con1.execute(setup)
+
+        try:
+            for _ in range(2):
+                con = await self.connect(database=self.con.dbname)
+                await con.query('select 1')
+                cons.append(con)
+            con2, con3 = cons
+
+            long_call = asyncio.create_task(con1.query_single(f'''
+                select (count({query}), sys::_sleep(3));
+            '''))
+            await asyncio.sleep(0.5)
+            ddl = asyncio.create_task(con2.execute(modification))
+            await asyncio.sleep(0.5)
+            short_call = con3.query(f'''
+                select {query}
+            ''')
+
+            return await asyncio.gather(long_call, ddl, short_call)
+        finally:
+            await self.con.execute(teardown)
+            for con in cons:
+                await con.aclose()
+
+    async def test_edgeql_ddl_deadlock_01(self):
+        ((cnt, _), _, objs) = await self._deadlock_tester(
+            setup='''
+                create type X;
+                insert X;
+            ''',
+            teardown='''
+                drop type X;
+            ''',
+            modification='''
+                alter type X create property foo -> str;
+            ''',
+            query='X',
+        )
+        self.assertEqual(cnt, 1)
+        self.assertEqual(len(objs), 1)
+
+    async def test_edgeql_ddl_deadlock_02(self):
+        ((cnt, _), _, objs) = await self._deadlock_tester(
+            setup='''
+                create type Y;
+                create type X { create multi link t -> Y; };
+                insert X { t := (insert Y) };
+            ''',
+            teardown='''
+                drop type X;
+                drop type Y;
+            ''',
+            modification='''
+                alter type X alter link t create property foo -> str;
+            ''',
+            query='(Y, Y.<t[is X])',
+        )
+        self.assertEqual(cnt, 1)
+        self.assertEqual(len(objs), 1)