From aa2f6b0040139931b73431fc29d20258c96fa4fc Mon Sep 17 00:00:00 2001 From: Dave Page Date: Mon, 17 Aug 2026 15:43:48 +0100 Subject: [PATCH 1/3] Schema Diff: make the regression test assert its generated script, and fix what that found 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 --- .../templates/casts/sql/default/nodes.sql | 2 +- .../schemas/foreign_tables/__init__.py | 47 ++++++++- .../functions/pg/sql/default/node.sql | 2 +- .../functions/ppas/sql/default/node.sql | 2 +- .../databases/schemas/sequences/__init__.py | 47 +++++++++ .../sequences/sql/15_plus/update.sql | 3 + .../sequences/sql/default/update.sql | 3 + .../databases/schemas/types/__init__.py | 23 +++++ .../templates/types/pg/sql/default/nodes.sql | 2 +- .../types/ppas/sql/default/nodes.sql | 2 +- web/pgadmin/tools/schema_diff/__init__.py | 8 ++ .../tests/test_schema_diff_comp.py | 97 +++++++++++++++---- web/pgadmin/tools/schema_diff/tests/utils.py | 55 ++++++++++- 13 files changed, 265 insertions(+), 28 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/casts/templates/casts/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/casts/templates/casts/sql/default/nodes.sql index e044e4bd784..489ad91d344 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/casts/templates/casts/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/casts/templates/casts/sql/default/nodes.sql @@ -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 diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py index 99cd1dcc327..bef8aef1b3f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py @@ -1194,6 +1194,8 @@ def get_sql(self, **kwargs): # 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 @@ def get_sql(self, **kwargs): 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 @@ def _modify_column_data(data, tmp_columns): :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): diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/pg/sql/default/node.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/pg/sql/default/node.sql index dfc141f4f85..c50456aa028 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/pg/sql/default/node.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/pg/sql/default/node.sql @@ -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 diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/ppas/sql/default/node.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/ppas/sql/default/node.sql index 2b2dd8107e6..408b9bd29c4 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/ppas/sql/default/node.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/templates/functions/ppas/sql/default/node.sql @@ -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 diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py index ff018a2c310..81512330fa6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py @@ -628,6 +628,50 @@ def msql(self, gid, sid, did, scid, seid=None): 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 +711,9 @@ def get_SQL(self, gid, sid, did, data, scid, seid=None, 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 diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/15_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/15_plus/update.sql index 74ac6ea6c83..e879c5c71fa 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/15_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/15_plus/update.sql @@ -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 %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/default/update.sql index 04c7eb0014b..e7db7f8e917 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/templates/sequences/sql/default/update.sql @@ -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 %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/__init__.py index ef9f26118da..4af93d2ec77 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/__init__.py @@ -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 @@ def fetch_objects_to_compare(self, sid, did, scid): 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 diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/pg/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/pg/sql/default/nodes.sql index 9469379f530..727733a87d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/pg/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/pg/sql/default/nodes.sql @@ -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; diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/ppas/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/ppas/sql/default/nodes.sql index 9469379f530..727733a87d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/ppas/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/types/ppas/sql/default/nodes.sql @@ -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; diff --git a/web/pgadmin/tools/schema_diff/__init__.py b/web/pgadmin/tools/schema_diff/__init__.py index 21086d5abc3..d45dc6d6f9a 100644 --- a/web/pgadmin/tools/schema_diff/__init__.py +++ b/web/pgadmin/tools/schema_diff/__init__.py @@ -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) diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_comp.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_comp.py index adbc6e5f434..83767fda0d0 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_comp.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_comp.py @@ -15,7 +15,7 @@ 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 @@ def restore_backup(self): 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 @@ def compare(self): 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 @@ def runTest(self): 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 @@ def runTest(self): 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) - - 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) + + # 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() + 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""" diff --git a/web/pgadmin/tools/schema_diff/tests/utils.py b/web/pgadmin/tools/schema_diff/tests/utils.py index b226513aa46..e6b63a14fa1 100644 --- a/web/pgadmin/tools/schema_diff/tests/utils.py +++ b/web/pgadmin/tools/schema_diff/tests/utils.py @@ -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,58 @@ 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: + try: + pg_cursor = connection.cursor() + pg_cursor.execute(sql) + pg_cursor.close() + applied.append(label) + except Exception as e: + failed.append((label, sql, str(e))) + + 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): From fa867889e916521638f8767c228366da9db77766 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 1 Sep 2026 11:30:52 +0100 Subject: [PATCH 2/3] 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. --- .../schemas/tables/partitions/__init__.py | 14 ++++++++++++-- web/pgadmin/utils/__init__.py | 14 +++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py index bffe011e04f..c9096591b04 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py @@ -492,6 +492,16 @@ def get_sql_from_diff(self, **kwargs): 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 @@ def get_sql_from_diff(self, **kwargs): '-- 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) diff --git a/web/pgadmin/utils/__init__.py b/web/pgadmin/utils/__init__.py index 0a57d7e6c0f..5fed5d66380 100644 --- a/web/pgadmin/utils/__init__.py +++ b/web/pgadmin/utils/__init__.py @@ -61,6 +61,17 @@ def register(self, app, options): 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 @@ def create_module_preference(): 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) From 447eec1fa245ee23eb32ae1c270546a385cf7709 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 1 Sep 2026 12:09:23 +0100 Subject: [PATCH 3/3] 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. --- .../servers/databases/schemas/sequences/__init__.py | 5 ++++- web/pgadmin/tools/schema_diff/tests/utils.py | 8 +++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py index 81512330fa6..f99531aa956 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/sequences/__init__.py @@ -540,7 +540,10 @@ def update(self, gid, sid, did, scid, seid): 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) diff --git a/web/pgadmin/tools/schema_diff/tests/utils.py b/web/pgadmin/tools/schema_diff/tests/utils.py index e6b63a14fa1..ef10d6632c1 100644 --- a/web/pgadmin/tools/schema_diff/tests/utils.py +++ b/web/pgadmin/tools/schema_diff/tests/utils.py @@ -106,13 +106,19 @@ def apply_sql_chunks(server, db_name, chunks): while pending: failed = [] for label, sql in pending: + pg_cursor = None try: pg_cursor = connection.cursor() pg_cursor.execute(sql) - pg_cursor.close() 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()