Schema Diff: make the regression test assert its generated script, and fix what that found (#10305)

The Schema Diff comparison test wrapped applying its generated script, and the comparison that follows it, in a bare `except Exception` that discarded both. It reported a pass whatever the script did, which is why it printed `syntax error at or near ")"` on every run whilst claiming two tests passed. It now fails when an object's SQL does not apply, and when applying the lot leaves the two databases different, with a short list of the differences that are known not to settle yet so that the list cannot quietly rot. Objects are applied one at a time and retried rather than as a single script, because the script is no longer ordered by dependency (#10295), so an object can fail purely because something it needs comes later on; retrying tells that apart from SQL that is simply wrong. Turning the assertions on found the following, each of which is fixed here: * A range type being dropped and recreated because its kind changed lost its subtype, because directory_diff() drops a plain value that only one side of the comparison has, and rendered `CREATE TYPE ... AS RANGE ()`. Once that was fixed it wrote the catalogue's `-` placeholder out as `CANONICAL = -`, which the reverse-engineered SQL path already avoids. Both are now handled where the comparison data is built (#10304). * The constructor functions, casts and multirange types that PostgreSQL creates for a range type were compared as though a user had written them, so the script tried to recreate objects that come into being with their parent type: 47 of 151 objects in the test's fixtures were these. Internal dependencies are now excluded alongside extension ones, matching what pg_dump does. * Recreating a foreign table declared any column that also differed twice, because a changed column was appended to the table's existing columns rather than replacing the entry already there (#10297). * Raising a sequence's MINVALUE above the value it currently sits at, or lowering MAXVALUE below it, generated a statement PostgreSQL rejects outright, taking every other change to that sequence with it. Such a change is now accompanied by the RESTART it requires (#10298). * A foreign table column added by Schema Diff lost its collation, because get_columns.sql calls it collname whilst the column templates render collspcname (#10300). * A comparison that threw part way through emitted its failure and then reported success as well, handing the client a fraction of the databases as though it were a complete result (#10303). Two differences remain listed as known: a rebuilt partitioned table keeps the default partition used as scaffolding for the data copy (#10301), and CREATE OR REPLACE wraps a function body in newlines, leaving a whitespace-only difference (#10302). Fixes #10293 * Stop the partition rebuild script copying from a temporary table Asserting that the generated script actually applies turned up a partitioned table rebuild whose script refers to a relation that has never existed, along the lines of: CREATE TABLE schema.temp_partitioned_2 ( LIKE schema.temp_partitioned_1 INCLUDING ALL ) PARTITION BY RANGE (col1); so that applying it fails with 'relation "schema.temp_partitioned_1" does not exist'. There are two things going on here, and both are fixed. PgAdminModule.register() is called once per application instance, whilst the blueprint objects themselves are module level singletons, so each sub-class that appends its sub-modules from its own register() (most of them do, TableModule included) leaves a duplicate entry behind every time a second application is created in the same process. Nothing in production notices, because production creates a single app, but the regression suite creates several, and anything walking self.submodules then does its work once per duplicate: with four copies of the partition sub-module, get_sql_from_submodule_diff generated the same partition rebuild four times over. self.submodules is now de-duplicated as the blueprint registers, and parentmodules likewise only gains an entry it does not already hold. The second call was only harmful because PartitionsView.get_sql_from_diff stashed its temporary names on the caller's own dictionaries, replacing the table's real name with a temporary one, so a subsequent call read the first call's temporary name back as the original. It now works on copies and leaves the comparison data it is handed alone, which makes it safe to call more than once regardless of how it is reached. * Take a mutable copy of request.form, and close cursors on a failed chunk Two things picked up in review. Sequence update() passed request.form straight through to get_SQL(), and request.form is immutable, so as soon as _add_restart_for_new_bounds had a restart to add (which is the whole point of it) a form encoded update raised TypeError. It now takes a dict copy, as the msql path already effectively did. apply_sql_chunks() left the cursor open when a statement failed, and a failed statement is retried on the next pass, so a run could accumulate one open cursor per attempt. The cursor is now closed in a finally, whether the statement worked or not.
2026-11-01 07:48:03 -06:00 · 2026-09-07 16:35:13 +05:30
1 parent bd7cde5851
commit 8c6fd211a7
@@ -24,6 +24,6 @@
    {% endif %}
    {% if schema_diff %}
        AND CASE WHEN (SELECT COUNT(*) FROM pg_catalog.pg_depend
-            WHERE objid = ca.oid AND deptype = 'e') > 0 THEN FALSE ELSE TRUE END
+            WHERE objid = ca.oid AND deptype IN ('e', 'i')) > 0 THEN FALSE ELSE TRUE END
    {% endif %}
    ORDER BY st.typname, tt.typname
@@ -1194,6 +1194,8 @@ class ForeignTableView(BaseTableView, DataTypeReader,
                # Parse the data coming from client
                data = column_utils.parse_format_columns(data, mode='edit')

+                ForeignTableView._normalise_column_collation(data['columns'])
+
                columns = data['columns']
                column_sql = '\n'

@@ -1229,6 +1231,31 @@ class ForeignTableView(BaseTableView, DataTypeReader,
                                  conn=self.conn)
            return sql, data['name']

+    @staticmethod
+    def _normalise_column_collation(columns):
+        """
+        Give a column's collation the name the column templates expect.
+
+        The dialog calls a column's collation ``collspcname``, and that is
+        what foreign_table_columns' create and update templates render,
+        whilst get_columns.sql calls it ``collname``, which is what Schema
+        Diff hands us. Without reconciling the two, a column added or
+        retyped by Schema Diff silently loses its collation and the two
+        databases stay different however many times the script is applied
+        (#10300).
+
+        :param columns: The column difference, modified in place
+        """
+        # A create carries a plain list of columns; only an update carries
+        # the added/changed/deleted difference this applies to.
+        if not isinstance(columns, dict):
+            return
+
+        for action in ('added', 'changed'):
+            for column in columns.get(action) or []:
+                if not column.get('collspcname') and column.get('collname'):
+                    column['collspcname'] = column['collname']
+
    def _check_for_column_delete(self, columns, data, column_sql):
        # If column(s) is/are deleted
        if 'deleted' in columns:
@@ -1826,15 +1853,31 @@ class ForeignTableView(BaseTableView, DataTypeReader,
        :param data: Data for columns.
        :param tmp_columns: tmp_columns list.
        """
+        def index_of(name):
+            for index, column in enumerate(tmp_columns):
+                if column.get('name') == name:
+                    return index
+            return None
+
        if 'added' in data['columns']:
            for item in data['columns']['added']:
                tmp_columns.append(item)
        if 'changed' in data['columns']:
+            # tmp_columns holds the table as it stands, so a changed column
+            # is already in it in its old form and has to be replaced;
+            # appending it would declare the column twice and PostgreSQL
+            # would reject the recreated table outright (#10297).
            for item in data['columns']['changed']:
-                tmp_columns.append(item)
+                index = index_of(item.get('name'))
+                if index is None:
+                    tmp_columns.append(item)
+                else:
+                    tmp_columns[index] = item
        if 'deleted' in data['columns']:
            for item in data['columns']['deleted']:
-                tmp_columns.remove(item)
+                index = index_of(item.get('name'))
+                if index is not None:
+                    tmp_columns.pop(index)

    @staticmethod
    def _modify_constraints_data(data):
@@ -19,7 +19,7 @@ WHERE
 {% endif %}
 {% if schema_diff %}
    AND CASE WHEN (SELECT COUNT(*) FROM pg_catalog.pg_depend
-        WHERE objid = pr.oid AND deptype = 'e') > 0 THEN FALSE ELSE TRUE END
+        WHERE objid = pr.oid AND deptype IN ('e', 'i')) > 0 THEN FALSE ELSE TRUE END
 {% endif %}
    AND typname NOT IN ('trigger', 'event_trigger')
 ORDER BY
@@ -20,7 +20,7 @@ WHERE
 {% endif %}
 {% if schema_diff %}
    AND CASE WHEN (SELECT COUNT(*) FROM pg_catalog.pg_depend
-        WHERE objid = pr.oid AND deptype = 'e') > 0 THEN FALSE ELSE TRUE END
+        WHERE objid = pr.oid AND deptype IN ('e', 'i')) > 0 THEN FALSE ELSE TRUE END
 {% endif %}
    AND typname NOT IN ('trigger', 'event_trigger')
 ORDER BY
@@ -540,7 +540,10 @@ class SequenceView(PGChildNodeView, SchemaDiffObjectCompare):
        Returns:

        """
-        data = request.form if request.form else json.loads(
+        # request.form is immutable, and get_SQL adds a 'restart' of its
+        # own when the new bounds have left the sequence outside them, so
+        # take a mutable copy of it.
+        data = dict(request.form) if request.form else json.loads(
            request.data
        )
        sql, _ = self.get_SQL(gid, sid, did, data, scid, seid)
@@ -628,6 +631,50 @@ class SequenceView(PGChildNodeView, SchemaDiffObjectCompare):
            status=200
        )

+    def _add_restart_for_new_bounds(self, data, old_data):
+        """
+        Ask for the sequence to be repositioned when the bounds being set
+        would leave it outside them.
+
+        PostgreSQL will not raise a sequence's MINVALUE above, or lower its
+        MAXVALUE below, the value the sequence currently sits at: it
+        rejects the whole statement with "RESTART value (n) cannot be less
+        than MINVALUE (m)", so every other change in it is lost too.
+        Repositioning onto the nearest value the new bounds allow is the
+        only way such a change can be applied, so ask for it rather than
+        generating a statement that cannot run (#10298). A sequence already
+        within its new bounds is left where it is, because handing out
+        values that have been used already would be worse than either.
+
+        :param data: The change being applied, modified in place
+        :param old_data: The sequence as it stands
+        """
+        minimum = data.get('minimum')
+        maximum = data.get('maximum')
+
+        if minimum is None and maximum is None:
+            return
+
+        current = data.get('current_value')
+        if current is None:
+            sql = render_template(
+                "/".join([self.template_path, 'get_def.sql']),
+                data=old_data, conn=self.conn
+            )
+            status, res = self.conn.execute_dict(sql)
+            if not status or not res['rows']:
+                return
+
+            current = res['rows'][0]['last_value']
+
+        if current is None:
+            return
+
+        if minimum is not None and int(minimum) > int(current):
+            data['restart'] = int(minimum)
+        elif maximum is not None and int(maximum) < int(current):
+            data['restart'] = int(maximum)
+
    def get_SQL(self, gid, sid, did, data, scid, seid=None,
                add_not_exists_clause=False):
        """
@@ -667,6 +714,9 @@ class SequenceView(PGChildNodeView, SchemaDiffObjectCompare):
            for arg in required_args:
                if arg not in data:
                    data[arg] = old_data[arg]
+
+            self._add_restart_for_new_bounds(data, old_data)
+
            sql = render_template(
                "/".join([self.template_path, self._UPDATE_SQL]),
                data=data, o_data=old_data, conn=self.conn
@@ -41,6 +41,9 @@ ALTER SEQUENCE IF EXISTS {{ conn|qtIdent(o_data.schema, data.name) }}
 {% if data.maximum is defined %}
 {% set defquery = defquery+'\n    MAXVALUE '+data.maximum|string %}
 {% endif %}
+{% if data.restart is defined %}
+{% set defquery = defquery+'\n    RESTART '+data.restart|string %}
+{% endif %}
 {% if data.cache is defined %}
 {% set defquery = defquery+'\n    CACHE '+data.cache|string %}
 {% endif %}
@@ -36,6 +36,9 @@ SELECT setval({{ seqname|qtLiteral(conn) }}, {{ data.current_value }}, false);
 {% if data.maximum is defined %}
 {% set defquery = defquery+'\n    MAXVALUE '+data.maximum|string %}
 {% endif %}
+{% if data.restart is defined %}
+{% set defquery = defquery+'\n    RESTART '+data.restart|string %}
+{% endif %}
 {% if data.cache is defined %}
 {% set defquery = defquery+'\n    CACHE '+data.cache|string %}
 {% endif %}
@@ -492,6 +492,16 @@ class PartitionsView(BaseTableView, DataTypeReader, SchemaDiffObjectCompare):
        target_data = kwargs['target_data'] if 'target_data' in kwargs \
            else None

+        # Work on copies throughout. The caller owns these dictionaries
+        # (they are its view of the compared objects), so stashing the
+        # temporary names on them would leave the table's real name
+        # replaced by a temporary one, and a second call for the same
+        # table would then generate a script that copies from a relation
+        # that never existed.
+        target_data = dict(target_data)
+        source_partitions = [dict(partition) for partition in
+                             source_data.get('partitions', [])]
+
        # Store the original name and create a temporary name for
        # the partitioned(base) table.
        target_data['orig_name'] = target_data['name']
@@ -517,11 +527,11 @@ class PartitionsView(BaseTableView, DataTypeReader, SchemaDiffObjectCompare):
            '-- matches the inserted data.'

        # Create temporary name for partitions
-        for item in source_data['partitions']:
+        for item in source_partitions:
            item['temp_partition_name'] = 'partition_{0}'.format(
                secrets.choice(range(1, 9999999)))

-        partition_data['partitions'] = source_data['partitions']
+        partition_data['partitions'] = source_partitions

        partition_sql = self.get_partitions_sql(partition_data,
                                                schema_diff=True)
@@ -224,6 +224,18 @@ class TypeView(PGChildNodeView, DataTypeReader, SchemaDiffObjectCompare):
                      'schema', 'oid-2', 'type_acl', 'rngcollation', 'attnum',
                      'typowner']

+    # A range type carries its subtype, collation and support functions as
+    # plain values, which types of every other kind have no equivalent of at
+    # all. directory_diff() silently drops a value that only one side has,
+    # so comparing a range against a type of another kind would lose the
+    # subtype and leave nothing to render but `CREATE TYPE ... AS RANGE ()`
+    # when the type has to be dropped and recreated. Defining the keys on
+    # both sides keeps them in the difference (#10304).
+    range_keys_to_normalise = ['rngsubtype', 'typname', 'rngmultirangetype',
+                               'collname', 'rngsubopc', 'opcname',
+                               'rngcanonical', 'rngsubdiff_proc',
+                               'rngsubdiff']
+
    def check_precondition(f):
        """
        This function will behave as a decorator which will checks
@@ -1587,6 +1599,17 @@ class TypeView(PGChildNodeView, DataTypeReader, SchemaDiffObjectCompare):
        for row in rset['rows']:
            status, data = self._fetch_properties(scid, row['oid'])
            if status:
+                # The catalogue writes '-' where a type has no support
+                # function, which is not something that can be handed back
+                # to CREATE TYPE; the reverse-engineered SQL path drops it
+                # the same way before rendering (#10304).
+                for key, value in data.items():
+                    if value == '-':
+                        data[key] = None
+
+                for key in self.range_keys_to_normalise:
+                    data.setdefault(key, None)
+
                res[row['name']] = data

        return res
@@ -14,6 +14,6 @@ WHERE t.typtype != 'd' AND t.typname NOT LIKE E'\\_%' AND t.typnamespace = {{sci
 {% endif %}
 {% if schema_diff %}
    AND CASE WHEN (SELECT COUNT(*) FROM pg_catalog.pg_depend
-        WHERE objid = t.oid AND deptype = 'e') > 0 THEN FALSE ELSE TRUE END
+        WHERE objid = t.oid AND deptype IN ('e', 'i')) > 0 THEN FALSE ELSE TRUE END
 {% endif %}
 ORDER BY t.typname;
@@ -14,6 +14,6 @@ WHERE t.typtype != 'd' AND t.typname NOT LIKE E'\\_%' AND t.typnamespace = {{sci
 {% endif %}
 {% if schema_diff %}
    AND CASE WHEN (SELECT COUNT(*) FROM pg_catalog.pg_depend
-        WHERE objid = t.oid AND deptype = 'e') > 0 THEN FALSE ELSE TRUE END
+        WHERE objid = t.oid AND deptype IN ('e', 'i')) > 0 THEN FALSE ELSE TRUE END
 {% endif %}
 ORDER BY t.typname;
@@ -637,8 +637,12 @@ def compare_database(params):

    except Exception as e:
        app.logger.exception(e)
+        # Reporting success as well would hand the client a comparison
+        # that stopped part way through as though it were complete
+        # (#10303).
        socketio.emit('compare_database_failed', str(e),
                      namespace=SOCKETIO_NAMESPACE, to=request.sid)
+        return

    socketio.emit('compare_database_success', comparison_result,
                  namespace=SOCKETIO_NAMESPACE, to=request.sid)
@@ -702,8 +706,12 @@ def compare_schema(params):

    except Exception as e:
        app.logger.exception(e)
+        # As above: a partial comparison must not be reported as a
+        # successful one (#10303).
        socketio.emit('compare_schema_failed', str(e),
                      namespace=SOCKETIO_NAMESPACE, to=request.sid)
+        return
+
    socketio.emit('compare_schema_success', comparison_result,
                  namespace=SOCKETIO_NAMESPACE, to=request.sid)

@@ -15,7 +15,7 @@ import secrets
 from pgadmin.utils.route import BaseTestGenerator, BaseSocketTestGenerator
 from regression import parent_node_dict
 from regression.python_test_utils import test_utils as utils
-from .utils import restore_schema
+from .utils import apply_sql_chunks, restore_schema
 from pgadmin.utils.versioned_template_loader import \
    get_version_mapping_directories

@@ -29,6 +29,19 @@ class SchemaDiffTestCase(BaseSocketTestGenerator):
    ]
    SOCKET_NAMESPACE = '/schema_diff'

+    # Objects that the generated script is known not to settle yet, each
+    # with the issue that covers it. The test fails on anything outside
+    # this list, and also fails when something on it starts working, so
+    # that the list cannot quietly rot.
+    KNOWN_DIFFERENCES = {
+        # Rebuilding a partitioned table leaves its scaffolding default
+        # partition behind.
+        'table table_for_partition_1': 10301,
+        # CREATE OR REPLACE wraps the body in newlines, leaving a
+        # whitespace-only difference.
+        'procedure proc1(IN arg1 bigint)': 10302,
+    }
+
    def setUp(self):
        super().setUp()
        self.src_database = "db_schema_diff_src_%s" % str(uuid.uuid4())[1:8]
@@ -63,13 +76,13 @@ class SchemaDiffTestCase(BaseSocketTestGenerator):
            raise FileNotFoundError(
                '{} file does not exists'.format(tar_sql_path))

-        status, self.src_schema_id = restore_schema(
+        status, self.src_schema_id, _ = restore_schema(
            self.server, self.src_database, self.schema_name, src_sql_path)
        if not status:
            print("Failed to restore schema on source database.")
            return False

-        status, self.tar_schema_id = restore_schema(
+        status, self.tar_schema_id, _ = restore_schema(
            self.server, self.tar_database, self.schema_name, tar_sql_path)
        if not status:
            print("Failed to restore schema on target database.")
@@ -124,6 +137,15 @@ class SchemaDiffTestCase(BaseSocketTestGenerator):
        self.socket_client.emit('compare_database', data,
                                namespace=self.SOCKET_NAMESPACE)
        received = self.socket_client.get_received(self.SOCKET_NAMESPACE)
+
+        # A comparison that throws part way through still reports the
+        # objects it managed to get through, so watching only for the
+        # success message would quietly assert against a fraction of the
+        # databases.
+        failures = [message['args'][0] for message in received
+                    if message['name'] == 'compare_database_failed']
+        self.assertEqual(failures, [], 'The comparison failed')
+
        response_data = received[-1]['args'][0]
        self.assertEqual(received[-1]['name'], "compare_database_success",
                         response_data)
@@ -161,7 +183,10 @@ class SchemaDiffTestCase(BaseSocketTestGenerator):
            str(secrets.choice(range(1, 99999)))))
        file_obj = open(diff_file, 'a')

+        chunks = []
+
        for diff in response_data:
+            ddl = None
            if diff['status'] == 'Identical':
                src_obj_oid = diff['source_oid']
                tar_obj_oid = diff['target_oid']
@@ -186,24 +211,60 @@ class SchemaDiffTestCase(BaseSocketTestGenerator):
                    response = self.tester.get(url)

                    self.assertEqual(response.status_code, 200)
-                    response_data = json.loads(response.data.decode('utf-8'))
-                    file_obj.write(response_data['diff_ddl'])
+                    ddl_response = json.loads(response.data.decode('utf-8'))
+                    ddl = ddl_response['diff_ddl']
            elif 'diff_ddl' in diff:
-                file_obj.write(diff['diff_ddl'])
+                ddl = diff['diff_ddl']
+
+            if ddl and ddl.strip():
+                file_obj.write(ddl)
+                chunks.append(('{0} {1}'.format(diff['type'], diff['title']),
+                               ddl))

        file_obj.close()
-        try:
-            restore_schema(self.server, self.tar_database, self.schema_name,
-                           diff_file)

-            os.remove(diff_file)
+        # Every object's SQL has to be valid, and applying the lot has to
+        # leave the two databases identical. Anything else is a bug in the
+        # SQL we generate, so it fails the test rather than being discarded
+        # the way it was before #10293. The script is left on disk when it
+        # does fail, since it is the evidence of what went wrong.
+        #
+        # The objects go in one at a time and are retried, rather than as a
+        # single script, because Schema Diff no longer orders the script it
+        # generates by dependency (#10295), so an object can fail purely
+        # because something it needs comes later on. Retrying tells that
+        # apart from SQL that is simply wrong; once #10295 is fixed this
+        # can go back to applying the script in one go.
+        _, failed = apply_sql_chunks(self.server, self.tar_database, chunks)
+        if failed:
+            self.fail(
+                'The SQL generated for {0} of {1} object(s) never applied:'
+                '\n{2}\nThe script has been left at {3}'.format(
+                    len(failed), len(chunks),
+                    '\n'.join('  {0}: {1}'.format(label, error)
+                              for label, _, error in failed),
+                    diff_file))

-            response_data = self.compare()
-            for diff in response_data:
-                self.assertEqual(diff['status'], 'Identical')
-        except Exception as e:
-            if os.path.exists(diff_file):
-                os.remove(diff_file)
+        response_data = self.compare()
+        not_identical = {'{0} {1}'.format(diff['type'], diff['title'])
+                         for diff in response_data
+                         if diff['status'] != 'Identical'}
+
+        unexpected = not_identical - set(self.KNOWN_DIFFERENCES)
+        if unexpected:
+            self.fail('Applying the generated script left {0} object(s) '
+                      'unexpectedly different: {1}\nThe script has been '
+                      'left at {2}'.format(len(unexpected),
+                                           ', '.join(sorted(unexpected)),
+                                           diff_file))
+
+        settled = set(self.KNOWN_DIFFERENCES) - not_identical
+        if settled:
+            self.fail('{0} settles now that the generated script has been '
+                      'applied, so it should come off '
+                      'KNOWN_DIFFERENCES'.format(', '.join(sorted(settled))))
+
+        os.remove(diff_file)

    def tearDown(self):
        """This function drop the added database"""
@@ -22,7 +22,7 @@ def restore_schema(server, db_name, schema_name, sql_path):
    :param db_name:
    :param schema_name:
    :param sql_path:
-    :return:
+    :return: (status, schema oid, error message when it failed)
    """
    schema_id = None
    try:
@@ -70,9 +70,64 @@ def restore_schema(server, db_name, schema_name, sql_path):
        connection.close()
    except Exception as e:
        print(str(e))
-        return False, schema_id
+        return False, schema_id, str(e)

-    return True, schema_id
+    return True, schema_id, None
+
+
+def apply_sql_chunks(server, db_name, chunks):
+    """
+    Apply each object's SQL in turn against the given database, retrying
+    whatever fails until a pass makes no further progress, and report what
+    is left over.
+
+    Retrying is what separates SQL that is simply wrong from SQL that only
+    failed because Schema Diff wrote it before something it depends on
+    (#10295): the former never applies however many passes it is given.
+
+    :param server: server details
+    :param db_name: database to apply the SQL to
+    :param chunks: list of (label, sql) pairs, in the generated order
+    :return: (labels applied, [(label, sql, error)] that never applied)
+    """
+    connection = utils.get_db_connection(db_name,
+                                         server['username'],
+                                         server['db_password'],
+                                         server['host'],
+                                         server['port'],
+                                         server['sslmode']
+                                         )
+    utils.set_isolation_level(connection, 0)
+    connection.autocommit = True
+
+    applied = []
+    pending = list(chunks)
+
+    while pending:
+        failed = []
+        for label, sql in pending:
+            pg_cursor = None
+            try:
+                pg_cursor = connection.cursor()
+                pg_cursor.execute(sql)
+                applied.append(label)
+            except Exception as e:
+                failed.append((label, sql, str(e)))
+            finally:
+                # A statement that failed is retried on the next pass, so
+                # leaving its cursor open would accumulate one per attempt
+                # for the length of the run.
+                if pg_cursor is not None:
+                    pg_cursor.close()
+
+        if len(failed) == len(pending):
+            connection.close()
+            return applied, failed
+
+        pending = [(label, sql) for label, sql, _ in failed]
+
+    connection.close()
+    return applied, []


 def create_schema(server, db_name, schema_name):
@@ -61,6 +61,17 @@ class PgAdminModule(Blueprint):
        sub-modules at once.
        """

+        # Sub-classes populate self.submodules from their own register(),
+        # but the blueprint objects themselves are module level singletons,
+        # so registering one against a second application instance (which
+        # happens whenever more than one app is created in a single
+        # process, as the regression suite does) would otherwise leave a
+        # duplicate entry behind for every sub-module. Anything that walks
+        # self.submodules then does its work once per duplicate; in Schema
+        # Diff's case that means generating the same DDL several times
+        # over.
+        self.submodules = list(dict.fromkeys(self.submodules))
+
        super().register(app, options)

        def create_module_preference():
@@ -77,7 +88,8 @@ class PgAdminModule(Blueprint):
        app.register_before_app_start(create_module_preference)

        for module in self.submodules:
-            module.parentmodules.append(self)
+            if self not in module.parentmodules:
+                module.parentmodules.append(self)
            if app.blueprints.get(module.name) is None:
                app.register_blueprint(module)
                app.register_logout_hook(module)