Fix administer schema_repair() (#8730)

`administer schema_repair()` had a bug where, if any repairs were
done, it set the in-memory schema to be the "reloaded_schema" used to
compare against for mismatches. This schema is equivalent to the
original, except that all of the ids were different.

This meant that any queries that were compiled using that schema would
be broken, and would try to query nonexistent tables.

This only affected the `administer` codepath; running `schema_repair`
as part of patches during version upgrades still worked.

If no uncached queries were compiled before the server was restarted,
this might not be noticed, which I suspect is what happened with
previous uses of `administer schema_repair()`.

Previously schema_repair was not really tested, since it typically
only does anything when a bug resulted in a broken schema.

I've added infrastructure for intentionally breaking the schema (by
UPDATEing the reflection schema), and a couple tests.

Author: msullivan

edb/server/bootstrap.py

@@ -704,7 +704,7 @@ def prepare_repair_patch(
     res = edbcompiler.repair_schema(compilerctx)
     if not res:
         return ""
-    sql, _, _ = res
+    sql, _ = res
 
     return sql.decode('utf-8')
 

edb/server/compiler/ddl.py

@@ -1318,7 +1318,7 @@ def _track(key: str) -> None:
 
 def repair_schema(
     ctx: compiler.CompileContext,
-) -> Optional[tuple[bytes, s_schema.Schema, Any]]:
+) -> Optional[tuple[bytes, Any]]:
     """Repair inconsistencies in the schema caused by bug fixes
 
     Works by comparing the actual current schema to the schema we get
@@ -1386,7 +1386,7 @@ def repair_schema(
         debug.header('Repair Delta Script')
         debug.dump_code(sql, lexer='sql')
 
-    return sql, reloaded_schema, config_ops
+    return sql, config_ops
 
 
 def administer_repair_schema(
@@ -1404,9 +1404,7 @@ def administer_repair_schema(
     res = repair_schema(ctx)
     if not res:
         return dbstate.MaintenanceQuery(sql=b"")
-    sql, new_schema, config_ops = res
-
-    current_tx.update_schema(new_schema)
+    sql, config_ops = res
 
     return dbstate.DDLQuery(
         sql=sql,

tests/test_edgeql_data_migration.py

@@ -12117,6 +12117,10 @@ async def test_edgeql_migration_trigger_shift_01(self):
 class TestEdgeQLDataMigrationNonisolated(EdgeQLDataMigrationTestCase):
     TRANSACTION_ISOLATION = False
 
+    PER_TEST_TEARDOWN = [
+        'reset schema to initial'
+    ]
+
     async def test_edgeql_migration_eq_collections_25(self):
         await self.con.execute(r"""
             START MIGRATION TO {
@@ -12439,6 +12443,127 @@ async def test_edgeql_migration_extension_01(self):
             tb.bag([[3, 1, 4], [5, 6, 0]]),
         )
 
+    async def _test_schema_repair(
+        self, *,
+        schema,
+        setup_script=None,
+        breakage_script,
+        check_breakage=True,
+    ):
+        '''Helper for testing ADMINISTER schema_repair()
+
+        Takes a schema and an optional setup_script and runs them.
+
+        Then runs the breakage_script, which will be run with access
+        to the reflschema and which should break the schema in some
+        way.
+
+        We will then load back the broken schema, verify migrations
+        are broken, run schema_repair, and verify migrations work.
+        '''
+
+        await self.migrate(schema, module='default')
+        if setup_script:
+            await self.con.execute(setup_script)
+
+        # Use a transaction to make sure the configures get rolled back
+        async with self.con.transaction():
+            # Enable poking directly at the schema
+            await self.con.execute('''
+                configure session set __internal_query_reflschema := true;
+                configure session set __internal_no_apply_query_rewrites :=
+                    true;
+            ''')
+
+            # Break the schema
+            await self.con.execute(breakage_script)
+
+        # Do some random configure current database in order to force
+        # a reload of the schema from reflection.
+        await self.con.execute("""
+            configure current database reset __internal_sess_testvalue
+        """)
+
+        # Make sure we succesfully busted the schema.
+        async with self.assertRaisesRegexTx(
+            # Error could be the 'complete' assertion in this test
+            # suite or some failure in the server.
+            (AssertionError, edgedb.EdgeDBError),
+            '',
+        ):
+            await self.migrate(schema, module='default')
+
+        await self.con.execute('''
+            administer schema_repair()
+        ''')
+
+        # Make sure the schema is repaired
+        async with self._run_and_rollback():
+            await self.start_migration(schema, module='default')
+            await self.assert_describe_migration({
+                'complete': True
+            })
+
+        # Make sure objects still can be fetched
+        await self.con.query('''
+            select Object
+        ''')
+
+    async def test_edgeql_migration_schema_repair_01(self):
+        await self._test_schema_repair(
+            schema='''
+                type T {
+                    lol := count(Object);
+                    foo: str;
+                }
+                type S extending T;
+            ''',
+            setup_script='''
+                insert T { foo := "t" };
+                insert S { foo := "s" };
+            ''',
+            breakage_script='''
+                update schema::Property
+                filter .name__internal = 'default::__|foo@default|S'
+                set { required := true };
+            ''',
+        )
+
+        # Check that a query works and the data is still there
+        await self.assert_query_result(
+            '''
+                select T { foo };
+            ''',
+            tb.bag([
+                {'foo': 't'},
+                {'foo': 's'},
+            ])
+        )
+
+    async def test_edgeql_migration_schema_repair_02(self):
+        await self._test_schema_repair(
+            schema='''
+            abstract type Named {
+                index std::fts::index on (
+                    std::fts::with_options(
+                        .name, language := std::fts::Language.eng));
+                required name: std::str;
+            };
+
+            type Project0 extending Named;
+            ''',
+            breakage_script='''
+                update schema::Index
+                filter .name__internal =
+                'default::std|fts|index@default|Project0' ++
+                '@bb75b87f16a631d2bd48c6c660d4b52a8b7274cb'
+                set { name__internal :=
+                    'default::fts|index@default|Project0' ++
+                    '@bb75b87f16a631d2bd48c6c660d4b52a8b7274cb'
+                }
+            ''',
+        )
+
 
 class EdgeQLAIMigrationTestCase(EdgeQLDataMigrationTestCase):
     # AI specific tests, just because creating the extension is so slow,

tests/test_edgeql_ddl.py

@@ -18878,16 +18878,6 @@ async def test_edgeql_ddl_rebase_views_02(self):
             [{}],
         )
 
-    async def test_edgeql_ddl_schema_repair(self):
-        await self.con.execute('''
-            create type Tgt {
-                create property lol := count(Object)
-            }
-        ''')
-        await self.con.execute('''
-            administer schema_repair()
-        ''')
-
     async def test_edgeql_ddl_alias_and_create_set_required(self):
         await self.con.execute(r"""
             create type T;