Schema Diff: fix false SERIAL/BIGSERIAL differences and invalid ALTER SQL (#10248)

Schema Diff compares tables whose serial columns have been reverse-engineered back onto the SERIAL/BIGSERIAL/SMALLSERIAL pseudo-type, and that reprojection left two problems behind for any column declared as SERIAL. First, the raw oid of the sequence behind the column's nextval() default (defseqrelid) was still being compared, and two independently created databases assign that sequence completely different oids, so structurally identical columns were reported as different; it is now ignored alongside its sibling seqrelid. Second, the SQL generated for a serial column that genuinely does differ read the column's old properties straight from the catalogue without the same reprojection, so the real integer type and the real nextval() default were compared against the pseudo-type and its emptied default. Any genuine difference, such as a comment or a NOT NULL, therefore also rendered an invalid ALTER COLUMN ... TYPE bigserial, a spurious DROP DEFAULT, and a set of sequence options that only an identity column accepts. The changed column is now normalised back onto the type that ALTER COLUMN can actually be given, whilst the sequence it owns is left to the sequence object's own comparison. Covered by a unit test for the column comparison and by a Schema Diff test that applies the generated SQL to the target and re-compares. Fixes #10236
2026-09-03 20:52:57 -05:00 · 2026-08-17 14:58:21 +01:00
parent 549d71a203
commit 01e5bf0993
5 changed files with 400 additions and 39 deletions
@@ -224,6 +224,67 @@ def parse_options_for_column(db_variables):
    return variables_lst


+# The integer types a SERIAL declaration reverse-engineers into, mapped to
+# the pseudo-type that produced them, and the reverse mapping for callers
+# that need the real, alterable type behind a reprojected column.
+SERIAL_TYPES = {
+    'integer': 'serial',
+    'smallint': 'smallserial',
+    'bigint': 'bigserial'
+}
+
+UNDERLYING_SERIAL_TYPES = {v: k for k, v in SERIAL_TYPES.items()}
+
+
+def is_serial_column(col):
+    """
+    Report whether a column is the reverse-engineered form of a SERIAL
+    declaration, which is the case only when it owns the very sequence
+    that its own nextval() default references.
+
+    Ownership comes from pg_depend (deptype='a', surfaced by
+    properties.sql as ``seqrelid``), whilst the default's own dependency
+    (deptype='n', from the pg_attrdef entry) is surfaced as
+    ``defseqrelid``. The two are independent, so a column may own one
+    sequence whilst defaulting from another, and only the case where both
+    point at the same sequence is a SERIAL. Identity columns carry an
+    internal dependency too, but never a nextval() default, and are
+    excluded via attidentity regardless. Issues #9896, #10100, #10101.
+
+    :param col: Column properties, as returned by properties.sql
+    :return: True when the column is a SERIAL/SMALLSERIAL/BIGSERIAL
+    """
+    defval = col.get('defval', '') or ''
+
+    return bool(col.get('seqrelid')) and defval.startswith("nextval('") \
+        and not col.get('attidentity') \
+        and col.get('seqrelid') == col.get('defseqrelid') \
+        and col.get('typname') in SERIAL_TYPES
+
+
+def reproject_serial_column(col):
+    """
+    Rewrite a column that owns the sequence behind its nextval() default
+    back into the SERIAL/SMALLSERIAL/BIGSERIAL pseudo-type it was declared
+    with, so that callers can emit round-trippable DDL. Columns that are
+    not SERIAL, including ones already reprojected, are left untouched.
+
+    :param col: Column properties, modified in place
+    :return: The same column
+    """
+    if not is_serial_column(col):
+        return col
+
+    serial_type = SERIAL_TYPES[col['typname']]
+
+    col['displaytypname'] = serial_type
+    col['cltype'] = serial_type
+    col['typname'] = serial_type
+    col['defval'] = ''
+
+    return col
+
+
@get_template_path
 def get_formatted_columns(conn, tid, data, other_columns,
                          table_or_type, template_path=None,
@@ -271,44 +332,7 @@ def get_formatted_columns(conn, tid, data, other_columns,
                    other_col['inheritedfrom']

        if with_serial:
-            # A column is SERIAL only when it genuinely owns the sequence
-            # referenced by its DEFAULT. properties.sql LEFT JOINs
-            # pg_depend (a sequence's pg_class depending on this column's
-            # pg_attribute) and surfaces the owned sequence oid as
-            # ``seqrelid`` - that dependency is the authoritative
-            # ownership signal. Relying on it (instead of guessing the
-            # ``<table>_<col>_seq`` name) keeps detection correct after a
-            # table/column/sequence rename and never rewrites an
-            # unrelated column whose default happens to match the guessed
-            # name (#10100, #10101). Identity columns carry an internal
-            # dependency too, but never a nextval() default, and are
-            # additionally excluded via attidentity.
-            #
-            # Ownership and default are independent dependencies in
-            # PostgreSQL: a column can own one sequence (pg_depend
-            # deptype='a', -> ``seqrelid``) while its DEFAULT's nextval()
-            # call names a completely different one (pg_depend deptype='n'
-            # from the pg_attrdef entry, -> ``defseqrelid``). SERIAL only
-            # applies when both dependencies point at the same sequence, so
-            # this split-ownership/default case keeps its original,
-            # explicit nextval() default instead of being rewritten.
-            defval = col.get('defval', '') or ''
-
-            if col.get('seqrelid') and defval.startswith("nextval('") \
-                    and not col.get('attidentity') \
-                    and col.get('seqrelid') == col.get('defseqrelid') \
-                    and col['typname'] in ('integer', 'smallint', 'bigint'):
-
-                serial_type = {
-                    'integer': 'serial',
-                    'smallint': 'smallserial',
-                    'bigint': 'bigserial'
-                }[col['typname']]
-
-                col['displaytypname'] = serial_type
-                col['cltype'] = serial_type
-                col['typname'] = serial_type
-                col['defval'] = ''
+            reproject_serial_column(col)

    data['columns'] = all_columns

@@ -24,8 +24,19 @@ class SchemaDiffTableCompare(SchemaDiffObjectCompare):
                            'rows_cnt', 'hastoasttable', 'relhassubclass',
                            'relacl_str', 'setting']

+    # 'seqrelid' (the sequence a column owns) and 'defseqrelid' (the
+    # sequence referenced by the column's nextval() DEFAULT) are compared
+    # against EACH OTHER by get_formatted_columns() to reproject a column
+    # as SERIAL/BIGSERIAL/SMALLSERIAL (see columns/utils.py, #9896/#10100/
+    # #10101) -- but the resulting raw sequence OIDs are otherwise
+    # meaningless across two independently-created databases, even when
+    # both sides have an identical SERIAL column. Without ignoring
+    # 'defseqrelid' here too, Schema Diff falsely reports such columns (and
+    # therefore their whole table) as different, and generates an invalid
+    # `ALTER COLUMN ... TYPE bigserial` statement despite both sides having
+    # the exact same reprojected cltype.
    column_keys_to_ignore = ['atttypid', 'edit_types', 'elemoid', 'seqrelid',
-                             'indkey', 'seqtypid']
+                             'indkey', 'seqtypid', 'defseqrelid']

    constraint_keys_to_ignore = ['relname', 'nspname', 'parent_tbl',
                                 'attrelid', 'adrelid', 'fknsp', 'confrelid',
@@ -0,0 +1,83 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2026, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+"""Unit tests for SchemaDiffTableCompare's column comparison, verifying
+that a SERIAL/BIGSERIAL column's raw sequence OID ('defseqrelid') does not
+cause Schema Diff to report a false-positive difference (#10236).
+"""
+
+from pgadmin.browser.server_groups.servers.databases.schemas.tables.\
+    schema_diff_table_utils import SchemaDiffTableCompare
+from pgadmin.utils.route import BaseTestGenerator
+
+
+def _make_bigserial_column(defseqrelid, **overrides):
+    """A column dict as returned by get_formatted_columns() for a
+    genuine, already-reprojected BIGSERIAL column."""
+    defaults = dict(
+        name='adl_id', cltype='bigserial', typname='bigserial',
+        atttypid=20, attlen=8, attnum=1, elemoid=20, seqtypid=20,
+        indkey=None, seqrelid=defseqrelid, defseqrelid=defseqrelid,
+        defval='', attnotnull=True, attacl=[],
+    )
+    defaults.update(overrides)
+    return defaults
+
+
+class TestSchemaDiffSerialColumnIgnore(BaseTestGenerator):
+    """Unit tests for SchemaDiffTableCompare.compare_target_cols()."""
+
+    scenarios = [
+        ('Identical BIGSERIAL columns with differing sequence OIDs are '
+         'not flagged as different',
+         dict(test_method='test_differing_defseqrelid_not_flagged')),
+        ('A genuinely different column is still flagged as different',
+         dict(test_method='test_genuine_difference_still_flagged')),
+    ]
+
+    def runTest(self):
+        getattr(self, self.test_method)()
+
+    def test_differing_defseqrelid_not_flagged(self):
+        # Two independently-created databases will assign different raw
+        # OIDs to each table's owned sequence, even for structurally
+        # identical BIGSERIAL columns. That OID difference alone must not
+        # cause the column (and thus the table) to be reported as
+        # different, and must not trigger an invalid
+        # `ALTER COLUMN ... TYPE bigserial` in the generated diff SQL.
+        source = _make_bigserial_column(defseqrelid=16482)
+        target_cols = [_make_bigserial_column(defseqrelid=98213)]
+
+        added = []
+        updated = []
+        SchemaDiffTableCompare.compare_target_cols(
+            source, target_cols, added, updated)
+
+        self.assertEqual(added, [])
+        self.assertEqual(updated, [])
+        # The matching target column must have been consumed.
+        self.assertEqual(target_cols, [])
+
+    def test_genuine_difference_still_flagged(self):
+        # A real difference (here, NOT NULL toggled) on an otherwise
+        # identical BIGSERIAL column must still be detected, proving the
+        # fix only suppresses the OID noise and doesn't mask real diffs.
+        source = _make_bigserial_column(defseqrelid=16482, attnotnull=True)
+        target_cols = [
+            _make_bigserial_column(defseqrelid=98213, attnotnull=False)
+        ]
+
+        added = []
+        updated = []
+        SchemaDiffTableCompare.compare_target_cols(
+            source, target_cols, added, updated)
+
+        self.assertEqual(len(updated), 1)
+        self.assertEqual(updated[0]['name'], 'adl_id')
+        self.assertEqual(added, [])
@@ -1284,6 +1284,52 @@ class BaseTableView(PGChildNodeView, BasePartitionTable, VacuumSettings):
                        self.double_newline
        return column_sql

+    @staticmethod
+    def _normalise_serial_column(data, old_col_data):
+        """
+        Reconcile a column that has been reprojected as SERIAL/SMALLSERIAL/
+        BIGSERIAL with the raw catalogue properties of its old self, so that
+        update.sql renders only the genuine changes.
+
+        Schema Diff compares tables fetched with with_serial_cols=True, so a
+        column declared as SERIAL reaches us carrying the pseudo-type as its
+        cltype and an emptied default, whilst its old properties are read
+        straight from the catalogue and carry the underlying integer type
+        along with the real nextval() default. Left alone, that asymmetry
+        made any serial column with a genuine difference (a comment, a NOT
+        NULL, a privilege) also render an invalid
+        `ALTER COLUMN ... TYPE bigserial`, a spurious DROP DEFAULT, and a
+        set of sequence options that only an identity column accepts
+        (#10236).
+
+        The pseudo-type is shorthand for a declaration rather than a type
+        ALTER COLUMN can be given, so compare and alter the underlying
+        integer type instead, drop the default the reprojection emptied,
+        and leave the owned sequence to be compared as the object it is in
+        its own right.
+
+        :param data: The changed column, modified in place
+        :param old_col_data: Properties of the column as it stands now
+        """
+        cltype = data.get('cltype')
+        if cltype not in column_utils.UNDERLYING_SERIAL_TYPES:
+            return
+
+        data['cltype'] = column_utils.UNDERLYING_SERIAL_TYPES[cltype]
+        if data.get('typname') == cltype:
+            data['typname'] = data['cltype']
+
+        # The reprojection emptied the nextval() default; that is not a
+        # request to drop it.
+        data.pop('defval', None)
+
+        # Sequence options ride along with a column because it owns a
+        # sequence, but ALTER COLUMN only accepts them for identity
+        # columns; the sequence itself is compared as a separate object.
+        for key in ('seqincrement', 'seqstart', 'seqmin', 'seqmax',
+                    'seqcache', 'seqcycle'):
+            data.pop(key, None)
+
    def _check_for_column_update(self, columns, data, column_sql, tid):
        # Here we will be needing previous properties of column
        # so that we can compare & update it
@@ -1318,6 +1364,8 @@ class BaseTableView(PGChildNodeView, BasePartitionTable, VacuumSettings):
                    DataTypeReader.parse_type_name(
                        old_col_data['cltype'])

+                self._normalise_serial_column(c, old_col_data)
+
                # Sql for alter column
                if c.get('inheritedfrom', None) is None and \
                        c.get('inheritedfromtable', None) is None:
@@ -0,0 +1,195 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2026, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+"""Schema Diff tests for SERIAL/BIGSERIAL/SMALLSERIAL columns (#10236).
+
+Two independently created databases assign different OIDs to the sequence
+that a SERIAL column owns, and the column is reprojected onto the SERIAL
+pseudo-type before it is compared. Neither of those may leak into the
+result: a structurally identical serial column must compare as identical,
+and a serial column with a genuine difference must produce SQL that
+PostgreSQL will actually accept, which rules out the pseudo-type appearing
+in ALTER COLUMN ... TYPE, the reprojection's emptied default being read as
+a request to drop the default, and the owned sequence's parameters being
+altered through the column.
+"""
+
+import json
+import secrets
+import uuid
+
+from pgadmin.utils.route import BaseSocketTestGenerator
+from regression import parent_node_dict
+from regression.python_test_utils import test_utils as utils
+
+SCHEMA_NAME = 'test_serial_diff'
+
+DDL = """
+CREATE SCHEMA {0};
+
+CREATE TABLE {0}.serial_identical (
+    id bigserial NOT NULL,
+    val text,
+    CONSTRAINT serial_identical_pkey PRIMARY KEY (id)
+);
+
+CREATE TABLE {0}.serial_changed (
+    id bigserial NOT NULL,
+    val text,
+    CONSTRAINT serial_changed_pkey PRIMARY KEY (id)
+);
+
+COMMENT ON COLUMN {0}.serial_changed.id IS '{1} side';
+"""
+
+
+class SchemaDiffSerialColumnTestCase(BaseSocketTestGenerator):
+    """ This class will test Schema Diff against SERIAL columns. """
+    scenarios = [
+        ('Schema diff comparison of SERIAL columns', dict())
+    ]
+    SOCKET_NAMESPACE = '/schema_diff'
+
+    def setUp(self):
+        super().setUp()
+        self.src_database = "db_serial_diff_src_%s" % str(uuid.uuid4())[1:8]
+        self.tar_database = "db_serial_diff_tar_%s" % str(uuid.uuid4())[1:8]
+
+        self.src_db_id = utils.create_database(self.server, self.src_database)
+        self.tar_db_id = utils.create_database(self.server, self.tar_database)
+
+        self.server = parent_node_dict["server"][-1]["server"]
+        self.server_id = parent_node_dict["server"][-1]["server_id"]
+
+        self.execute_sql(self.src_database, DDL.format(SCHEMA_NAME, 'source'))
+        self.execute_sql(self.tar_database, DDL.format(SCHEMA_NAME, 'target'))
+
+    def execute_sql(self, db_name, sql):
+        """
+        Run a statement batch against one of the test databases.
+
+        :param db_name: Database to run against
+        :param sql: SQL to execute
+        """
+        connection = utils.get_db_connection(db_name,
+                                             self.server['username'],
+                                             self.server['db_password'],
+                                             self.server['host'],
+                                             self.server['port'],
+                                             self.server['sslmode'])
+        old_isolation_level = connection.isolation_level
+        utils.set_isolation_level(connection, 0)
+        pg_cursor = connection.cursor()
+        pg_cursor.execute(sql)
+        utils.set_isolation_level(connection, old_isolation_level)
+        connection.commit()
+        connection.close()
+
+    def compare(self):
+        """
+        Compare the two test databases and return the result.
+
+        :return: List of compared objects
+        """
+        data = {
+            'trans_id': self.trans_id,
+            'source_sid': self.server_id,
+            'source_did': self.src_db_id,
+            'target_sid': self.server_id,
+            'target_did': self.tar_db_id,
+            'ignore_owner': 0,
+            'ignore_whitespaces': 0,
+            'ignore_tablespace': 0,
+            'ignore_grants': 0
+        }
+        self.socket_client.emit('compare_database', data,
+                                namespace=self.SOCKET_NAMESPACE)
+        received = self.socket_client.get_received(self.SOCKET_NAMESPACE)
+        response_data = received[-1]['args'][0]
+        self.assertEqual(received[-1]['name'], "compare_database_success",
+                         response_data)
+        return response_data
+
+    def find_object(self, response_data, node_type, title):
+        """
+        Pick a single compared object out of the comparison result.
+
+        :param response_data: Result of compare()
+        :param node_type: Node type, e.g. 'table'
+        :param title: Object name
+        :return: The compared object
+        """
+        for diff in response_data:
+            if diff.get('type') == node_type and diff.get('title') == title:
+                return diff
+
+        self.fail('{0} {1} was not compared'.format(node_type, title))
+
+    def runTest(self):
+        """ This function will test Schema Diff for SERIAL columns. """
+        self.trans_id = str(secrets.choice(range(1, 99999)))
+        response = self.tester.get(
+            'schema_diff/initialize/{}'.format(self.trans_id))
+        self.assertEqual(response.status_code, 200)
+
+        received = self.socket_client.get_received(self.SOCKET_NAMESPACE)
+        self.assertEqual(received[0]['name'], 'connected')
+
+        self.tester.post(
+            'schema_diff/server/connect/{}'.format(self.server_id),
+            data=json.dumps({'password': self.server['db_password']}),
+            content_type='html/json')
+        self.tester.post('schema_diff/database/connect/{0}/{1}'.format(
+            self.server_id, self.src_db_id))
+        self.tester.post('schema_diff/database/connect/{0}/{1}'.format(
+            self.server_id, self.tar_db_id))
+
+        response_data = self.compare()
+
+        # The sequence a serial column owns is assigned a different oid in
+        # each database, which says nothing about the column itself.
+        identical = self.find_object(response_data, 'table',
+                                     'serial_identical')
+        self.assertEqual(identical['status'], 'Identical',
+                         'Identical BIGSERIAL columns were reported as {0}: '
+                         '{1}'.format(identical['status'],
+                                      identical.get('diff_ddl')))
+
+        # A genuine difference is still reported, but the SQL for it must
+        # not carry the pseudo-type, drop the serial's default, or try to
+        # alter the owned sequence through the column.
+        changed = self.find_object(response_data, 'table', 'serial_changed')
+        self.assertEqual(changed['status'], 'Different')
+
+        diff_ddl = changed['diff_ddl']
+        self.assertIn('COMMENT ON COLUMN', diff_ddl)
+        for invalid in ('TYPE bigserial', 'DROP DEFAULT', 'SET INCREMENT'):
+            self.assertNotIn(invalid, diff_ddl,
+                             'Schema Diff generated invalid SQL for a '
+                             'BIGSERIAL column: {0}'.format(diff_ddl))
+
+        # Applying it must succeed, and must settle the difference.
+        self.execute_sql(self.tar_database, diff_ddl)
+
+        response_data = self.compare()
+        self.assertEqual(
+            self.find_object(response_data, 'table',
+                             'serial_changed')['status'], 'Identical')
+
+    def tearDown(self):
+        """This function drops the added databases"""
+        super().tearDown()
+        for db_name in (self.src_database, self.tar_database):
+            connection = utils.get_db_connection(self.server['db'],
+                                                 self.server['username'],
+                                                 self.server['db_password'],
+                                                 self.server['host'],
+                                                 self.server['port'],
+                                                 self.server['sslmode'])
+            utils.drop_database(connection, db_name)