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):