From 6deffe2c02662f815b04b7b9abd6aacf52a886ed Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:58:10 +0100 Subject: [PATCH 1/7] Schema Diff: complete the SERIAL/integer column conversion script (#10292) Schema Diff compares a SERIAL column by reprojecting it onto the SERIAL pseudo-type, which implies its nextval() default rather than stating it, so the reprojection empties the default before comparison. Once a column genuinely differs in "serialness" from its counterpart, that emptied default was all update.sql had to work from, so converting a plain column to SERIAL produced a script that changed the type and created the owned sequence but never set the column's DEFAULT, leaving the column unusable as a SERIAL. BaseTableView._normalise_serial_column() now distinguishes three cases instead of one: both sides SERIAL (unchanged, drop the emptied default only), becoming SERIAL (recreate the sequence from the default preserved under the new 'serial_defval' key and restore the default once the sequence exists), and leaving SERIAL (drop the default before dropping the now-unused sequence, since PostgreSQL refuses to drop a sequence a column's default still references). update.sql renders the new CREATE/DROP SEQUENCE statements around the existing DEFAULT handling in the right order for both directions, self-contained within the column's own diff so it doesn't depend on Schema Diff's separate, unordered sequence-object comparison. The "leaving SERIAL" case is guarded to require an explicit 'cltype' in the payload, since the same normalisation runs for the ordinary column PUT, where a partial update that only changes a comment or a privilege on an already-SERIAL column carries no 'cltype' at all and must be left alone. --- .../databases/schemas/tables/columns/utils.py | 28 +++ .../templates/columns/sql/16_plus/update.sql | 25 ++ .../templates/columns/sql/default/update.sql | 25 ++ .../test_normalise_serial_column_unit.py | 123 ++++++++++ .../servers/databases/schemas/tables/utils.py | 78 +++++- .../test_schema_diff_serial_conversion.py | 224 ++++++++++++++++++ 6 files changed, 492 insertions(+), 11 deletions(-) create mode 100644 web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py create mode 100644 web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py index 35db1bbb1f2..e0cf80165c3 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py @@ -269,6 +269,14 @@ def reproject_serial_column(col): with, so that callers can emit round-trippable DDL. Columns that are not SERIAL, including ones already reprojected, are left untouched. + The real ``nextval(...)`` expression is kept alongside the emptied + ``defval`` (under ``serial_defval``) rather than discarded, because + Schema Diff needs it back verbatim whenever it finds this column + genuinely differs in "serialness" from its counterpart: converting a + column to or from SERIAL means creating or dropping the sequence + behind it, which the pseudo-type's own implied default can't drive by + itself (#10292). + :param col: Column properties, modified in place :return: The same column """ @@ -280,11 +288,31 @@ def reproject_serial_column(col): col['displaytypname'] = serial_type col['cltype'] = serial_type col['typname'] = serial_type + col['serial_defval'] = col['defval'] col['defval'] = '' return col +def parse_nextval_sequence(defval): + """ + Extract the schema-qualified sequence name out of a ``nextval(...)`` + column default expression, e.g. ``nextval('public.t_id_seq'::regclass)`` + yields ``public.t_id_seq``. The identifier is returned exactly as + PostgreSQL rendered it (already quoted if it needs to be), so callers + should use it verbatim rather than re-quoting it. + + :param defval: A column's default value expression, or None + :return: The schema-qualified sequence name, or None if it is not a + nextval() default + """ + if not defval: + return None + + match = re.match(r"nextval\('(.+)'::regclass\)$", defval) + return match.group(1) if match else None + + @get_template_path def get_formatted_columns(conn, tid, data, other_columns, table_or_type, template_path=None, diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql index 102c1429d63..c97b2c1187f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql @@ -20,6 +20,26 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% if data.col_type_conversion is defined and data.col_type_conversion == False %} -- {% endif %} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} TYPE {{ GET_TYPE.UPDATE_TYPE_SQL(conn, data, o_data) }}{% if data.collspcname and data.collspcname != o_data.collspcname and data.cltype != '"char"' %} COLLATE {{data.collspcname}}{% elif o_data.collspcname and data.cltype != '"char"' %} COLLATE {{o_data.collspcname}}{% endif %}; {% endif %} +{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292) ###} +{% if data.serial_seq_create is defined %} +CREATE SEQUENCE IF NOT EXISTS {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} + + CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} + + INCREMENT {{data.serial_seq_create.increment|int}}{% endif %}{% if data.serial_seq_create.start is not none %} + + START {{data.serial_seq_create.start|int}}{% endif %}{% if data.serial_seq_create.minimum is not none %} + + MINVALUE {{data.serial_seq_create.minimum|int}}{% endif %}{% if data.serial_seq_create.maximum is not none %} + + MAXVALUE {{data.serial_seq_create.maximum|int}}{% endif %}{% if data.serial_seq_create.cache is not none %} + + CACHE {{data.serial_seq_create.cache|int}}{% endif %}; + +ALTER SEQUENCE {{data.serial_seq_create.name}} + OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; + +{% endif %} {### Alter column default value ###} {% if is_view_only and data.defval is defined and data.defval is not none and data.defval != '' and data.defval != o_data.defval %} ALTER VIEW {{conn|qtIdent(data.schema, data.table)}} @@ -35,6 +55,11 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} DROP DEFAULT; +{% endif %} +{### Drop the now-unused sequence a column stops owning by leaving SERIAL; the DEFAULT above must already be gone, or PostgreSQL refuses to drop a sequence still referenced by it (#10292) ###} +{% if data.serial_seq_drop is defined %} +DROP SEQUENCE IF EXISTS {{data.serial_seq_drop}}; + {% endif %} {### Alter column not null value ###} {% if 'attnotnull' in data and data.attnotnull != o_data.attnotnull %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql index 4722d3dd10a..4784d731354 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql @@ -20,6 +20,26 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% if data.col_type_conversion is defined and data.col_type_conversion == False %} -- {% endif %} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} TYPE {{ GET_TYPE.UPDATE_TYPE_SQL(conn, data, o_data) }}{% if data.collspcname and data.collspcname != o_data.collspcname and data.cltype != '"char"' %} COLLATE {{data.collspcname}}{% elif o_data.collspcname and data.cltype != '"char"' %} COLLATE {{o_data.collspcname}}{% endif %}; {% endif %} +{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292) ###} +{% if data.serial_seq_create is defined %} +CREATE SEQUENCE IF NOT EXISTS {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} + + CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} + + INCREMENT {{data.serial_seq_create.increment|int}}{% endif %}{% if data.serial_seq_create.start is not none %} + + START {{data.serial_seq_create.start|int}}{% endif %}{% if data.serial_seq_create.minimum is not none %} + + MINVALUE {{data.serial_seq_create.minimum|int}}{% endif %}{% if data.serial_seq_create.maximum is not none %} + + MAXVALUE {{data.serial_seq_create.maximum|int}}{% endif %}{% if data.serial_seq_create.cache is not none %} + + CACHE {{data.serial_seq_create.cache|int}}{% endif %}; + +ALTER SEQUENCE {{data.serial_seq_create.name}} + OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; + +{% endif %} {### Alter column default value ###} {% if is_view_only and data.defval is defined and data.defval is not none and data.defval != '' and data.defval != o_data.defval %} ALTER VIEW {{conn|qtIdent(data.schema, data.table)}} @@ -35,6 +55,11 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} DROP DEFAULT; +{% endif %} +{### Drop the now-unused sequence a column stops owning by leaving SERIAL; the DEFAULT above must already be gone, or PostgreSQL refuses to drop a sequence still referenced by it (#10292) ###} +{% if data.serial_seq_drop is defined %} +DROP SEQUENCE IF EXISTS {{data.serial_seq_drop}}; + {% endif %} {### Alter column not null value ###} {% if 'attnotnull' in data and data.attnotnull != o_data.attnotnull %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py new file mode 100644 index 00000000000..4dfb0f25874 --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py @@ -0,0 +1,123 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Unit tests for BaseTableView._normalise_serial_column(), covering both +directions of converting a column between a plain integer type and +SERIAL/BIGSERIAL/SMALLSERIAL (#10292), and guarding against the ordinary +(non Schema Diff) column PUT being mistaken for one. +""" + +from pgadmin.browser.server_groups.servers.databases.schemas.tables.utils \ + import BaseTableView +from pgadmin.utils.route import BaseTestGenerator + + +class TestNormaliseSerialColumn(BaseTestGenerator): + """Unit tests for BaseTableView._normalise_serial_column().""" + + scenarios = [ + ('Converting a plain column to SERIAL creates the sequence and ' + 'restores the default', + dict(test_method='test_becoming_serial')), + ('Converting a SERIAL column to plain queues the sequence for ' + 'dropping', + dict(test_method='test_leaving_serial')), + ('A genuine difference on a column that is SERIAL on both sides ' + 'is unaffected', + dict(test_method='test_both_sides_already_serial')), + ('A partial update that never mentions cltype leaves an ' + 'already-SERIAL column alone', + dict(test_method='test_partial_update_without_cltype_is_ignored')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + def test_becoming_serial(self): + # Schema Diff's source column, reprojected as BIGSERIAL, with the + # real nextval() default preserved under 'serial_defval'. + data = { + 'cltype': 'bigserial', 'typname': 'bigserial', + 'serial_defval': "nextval('public.t_id_seq'::regclass)", + 'seqincrement': 1, 'seqstart': 1, 'seqmin': 1, + 'seqmax': 9223372036854775807, 'seqcache': 1, 'seqcycle': False, + } + # The target's current (plain, unreprojected) column. + old_col_data = { + 'cltype': 'integer', 'typname': 'integer', 'defval': None, + 'seqrelid': None, 'defseqrelid': None, 'attidentity': '', + } + + BaseTableView._normalise_serial_column(data, old_col_data) + + self.assertEqual(data['cltype'], 'bigint') + self.assertEqual(data['typname'], 'bigint') + self.assertEqual(data['defval'], + "nextval('public.t_id_seq'::regclass)") + self.assertEqual(data['serial_seq_create']['name'], 'public.t_id_seq') + self.assertEqual(data['serial_seq_create']['increment'], 1) + self.assertNotIn('serial_defval', data) + self.assertNotIn('seqincrement', data) + + def test_leaving_serial(self): + # Schema Diff's source column: a plain integer, never reprojected. + data = {'cltype': 'integer', 'typname': 'integer', 'defval': None} + # The target's current column genuinely is SERIAL. + old_col_data = { + 'cltype': 'integer', 'typname': 'integer', + 'defval': "nextval('public.t_id_seq'::regclass)", + 'seqrelid': 100, 'defseqrelid': 100, 'attidentity': '', + } + + BaseTableView._normalise_serial_column(data, old_col_data) + + self.assertEqual(data['serial_seq_drop'], 'public.t_id_seq') + # The type didn't really change; the default is still queued to + # be dropped by the generic template logic (data['defval'] stays + # None/empty and differs from o_data['defval']). + self.assertEqual(data['cltype'], 'integer') + + def test_both_sides_already_serial(self): + # Both sides are BIGSERIAL; only some other property (a comment, + # say) differs. The reprojection emptied 'defval' on the source + # side; that must not be read as a request to drop the real one, + # and no sequence should be created or dropped. + data = { + 'cltype': 'bigserial', 'typname': 'bigserial', + 'serial_defval': "nextval('public.t_id_seq'::regclass)", + 'seqincrement': 1, + } + old_col_data = { + 'cltype': 'integer', 'typname': 'integer', + 'defval': "nextval('public.t_id_seq'::regclass)", + 'seqrelid': 100, 'defseqrelid': 100, 'attidentity': '', + } + + BaseTableView._normalise_serial_column(data, old_col_data) + + self.assertNotIn('defval', data) + self.assertNotIn('serial_seq_create', data) + self.assertNotIn('serial_seq_drop', data) + self.assertNotIn('seqincrement', data) + + def test_partial_update_without_cltype_is_ignored(self): + # The ordinary column PUT (not Schema Diff) submits only the + # fields the user actually changed - e.g. a privilege - and omits + # 'cltype' entirely when the type itself wasn't touched, even if + # the column already is SERIAL. This must be a complete no-op. + data = {'attacl': {'added': []}} + old_col_data = { + 'cltype': 'integer', 'typname': 'integer', + 'defval': "nextval('public.t_id_seq'::regclass)", + 'seqrelid': 100, 'defseqrelid': 100, 'attidentity': '', + } + + BaseTableView._normalise_serial_column(data, old_col_data) + + self.assertEqual(data, {'attacl': {'added': []}}) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py index ba9edabcbc3..e4c477d19ea 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py @@ -1304,24 +1304,80 @@ def _normalise_serial_column(data, old_col_data): 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. + integer type instead. Three cases follow, distinguished by whether + each side is genuinely SERIAL (owns the sequence its own nextval() + default points at): + + * Both sides are SERIAL: the only differences are things like a + comment or NOT NULL, so drop the default the reprojection + emptied (it is not a request to drop the real one) and leave the + owned sequence to be compared as the object it is in its own + right. + * The column is becoming SERIAL: the sequence behind it does not + exist on the other side yet, so it must be created, and its + owner column given back its real ``nextval()`` default, before + `ALTER COLUMN ... SET DEFAULT` can reference it at all. + * The column is leaving SERIAL: PostgreSQL refuses to drop a + sequence that a column's default still references, so the + now-unused sequence must be dropped only once that default is + gone (#10292). :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: + becomes_serial = cltype in column_utils.UNDERLYING_SERIAL_TYPES + was_serial = column_utils.is_serial_column(old_col_data) + + # A column can only be "leaving SERIAL" when this update actually + # says something about its type at all. This function also runs + # for the ordinary (non-diff) column PUT, where a partial update + # that never mentions cltype (e.g. changing only a comment or a + # privilege on an already-SERIAL column) carries no 'cltype' key + # whatsoever, and is not a request to change the type, however + # SERIAL the column already is; unlike Schema Diff's columns, + # which always carry the full properties and so always have one. + leaving_serial = was_serial and not becomes_serial \ + and 'cltype' in data + + if not becomes_serial and not leaving_serial: 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) + serial_defval = None + if becomes_serial: + data['cltype'] = column_utils.UNDERLYING_SERIAL_TYPES[cltype] + if data.get('typname') == cltype: + data['typname'] = data['cltype'] + serial_defval = data.pop('serial_defval', None) + + if becomes_serial and was_serial: + # The reprojection emptied the nextval() default on both + # sides; that is not a request to drop it. + data.pop('defval', None) + elif becomes_serial and not was_serial: + # Genuinely becoming SERIAL: recreate the sequence the + # reprojection's own default was emptied from, and restore + # that default so it can be set once the sequence exists. + data['defval'] = serial_defval or '' + seq_name = column_utils.parse_nextval_sequence(serial_defval) + if seq_name: + data['serial_seq_create'] = { + 'name': seq_name, + 'increment': data.get('seqincrement'), + 'start': data.get('seqstart'), + 'minimum': data.get('seqmin'), + 'maximum': data.get('seqmax'), + 'cache': data.get('seqcache'), + 'cycled': data.get('seqcycle'), + } + elif leaving_serial: + # Genuinely leaving SERIAL: the generic default handling + # below already drops the (empty, non-serial) default, so + # queue the now-unused sequence to be dropped afterwards. + seq_name = column_utils.parse_nextval_sequence( + old_col_data.get('defval')) + if seq_name: + data['serial_seq_drop'] = seq_name # Sequence options ride along with a column because it owns a # sequence, but ALTER COLUMN only accepts them for identity diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py new file mode 100644 index 00000000000..ea380037dba --- /dev/null +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -0,0 +1,224 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Schema Diff tests for converting a column between a plain integer type +and SERIAL/BIGSERIAL/SMALLSERIAL (#10292). + +Schema Diff already detects that such a column differs, but the generated +script used to stop halfway: converting a plain column to SERIAL changed +the column's type and (separately) created the owned sequence, without +ever setting the column's DEFAULT to nextval(...), so the column never +actually became usable as a SERIAL. Converting a SERIAL column back to +plain needs its DROP DEFAULT and the sequence's DROP SEQUENCE issued in +that order, since PostgreSQL refuses to drop a sequence that a column's +default still references. +""" + +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_conversion' + +SRC_DDL = """ +CREATE SCHEMA {0}; + +CREATE TABLE {0}.int_to_serial ( + id bigserial NOT NULL, + val text +); + +CREATE TABLE {0}.serial_to_int ( + id integer NOT NULL, + val text +); +""" + +TAR_DDL = """ +CREATE SCHEMA {0}; + +CREATE TABLE {0}.int_to_serial ( + id integer NOT NULL, + val text +); + +CREATE TABLE {0}.serial_to_int ( + id bigserial NOT NULL, + val text +); +""" + + +class SchemaDiffSerialConversionTestCase(BaseSocketTestGenerator): + """ This class tests converting a column between plain integer and + SERIAL in both directions. """ + scenarios = [ + ('Schema diff comparison converting between integer and SERIAL', + dict()) + ] + SOCKET_NAMESPACE = '/schema_diff' + + def setUp(self): + super().setUp() + self.src_database = "db_serial_conv_src_%s" % str(uuid.uuid4())[1:8] + self.tar_database = "db_serial_conv_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, SRC_DDL.format(SCHEMA_NAME)) + self.execute_sql(self.tar_database, TAR_DDL.format(SCHEMA_NAME)) + + 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 converting a column between integer + and SERIAL, in both directions. """ + 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() + + # Forward: target's plain integer column must become BIGSERIAL, + # which means the ALTER script must also create the owned + # sequence and set the column's DEFAULT to nextval() against it. + int_to_serial = self.find_object(response_data, 'table', + 'int_to_serial') + self.assertEqual(int_to_serial['status'], 'Different') + fwd_ddl = int_to_serial['diff_ddl'] + self.assertIn('TYPE bigint', fwd_ddl) + self.assertIn('CREATE SEQUENCE', fwd_ddl) + self.assertIn('SET DEFAULT nextval(', fwd_ddl) + # The sequence must be created before the column can default to + # nextval() against it. + self.assertLess(fwd_ddl.index('CREATE SEQUENCE'), + fwd_ddl.index('SET DEFAULT nextval(')) + + # Reverse: target's BIGSERIAL column must become plain integer, + # which means the ALTER script must drop the column's DEFAULT + # before it drops the now-unused sequence (PostgreSQL refuses to + # drop a sequence a column's default still references). + serial_to_int = self.find_object(response_data, 'table', + 'serial_to_int') + self.assertEqual(serial_to_int['status'], 'Different') + rev_ddl = serial_to_int['diff_ddl'] + self.assertIn('DROP DEFAULT', rev_ddl) + self.assertIn('DROP SEQUENCE', rev_ddl) + self.assertLess(rev_ddl.index('DROP DEFAULT'), + rev_ddl.index('DROP SEQUENCE')) + + # Applying both must succeed, and must settle the differences, + # including the underlying sequence objects. + self.execute_sql(self.tar_database, fwd_ddl) + self.execute_sql(self.tar_database, rev_ddl) + + response_data = self.compare() + for title in ('int_to_serial', 'serial_to_int'): + self.assertEqual( + self.find_object(response_data, 'table', title)['status'], + 'Identical') + + # The forward conversion must have made the column a genuine + # SERIAL: an insert omitting it must now succeed. + self.execute_sql( + self.tar_database, + "INSERT INTO {0}.int_to_serial (val) VALUES ('x')".format( + SCHEMA_NAME)) + + 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) From 9e6e53543ca08af831385765474144d82e79e4dd Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:17:37 +0100 Subject: [PATCH 2/7] fix: decode escaped quotes in sequence names and stop shadowing conflicts in SERIAL conversion (#10292) parse_nextval_sequence() left doubled single quotes undecoded when a sequence name itself contained a quote (e.g. "id'seq"), producing a wrong identifier when spliced verbatim into CREATE/ALTER/DROP SEQUENCE DDL rather than back into a string literal. CREATE SEQUENCE IF NOT EXISTS in the generated conversion script could also silently skip an existing, unrelated relation of the same name (without checking it is even a sequence), after which the unconditional ALTER SEQUENCE ... OWNED BY would reassign ownership of that unrelated object. Dropping IF NOT EXISTS makes a name collision fail loudly instead. --- .../tests/test_parse_nextval_sequence_unit.py | 60 +++++++++++++++++++ .../databases/schemas/tables/columns/utils.py | 17 +++++- .../templates/columns/sql/16_plus/update.sql | 4 +- .../templates/columns/sql/default/update.sql | 4 +- 4 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_parse_nextval_sequence_unit.py diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_parse_nextval_sequence_unit.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_parse_nextval_sequence_unit.py new file mode 100644 index 00000000000..87754450457 --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_parse_nextval_sequence_unit.py @@ -0,0 +1,60 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Unit tests for parse_nextval_sequence(), covering the schema-qualified +identifier it extracts out of a column's ``nextval(...)`` default, and in +particular the SQL string-literal quote-doubling PostgreSQL applies when +the sequence name itself contains a single quote (#10318). +""" + +from pgadmin.browser.server_groups.servers.databases.schemas.tables.\ + columns.utils import parse_nextval_sequence +from pgadmin.utils.route import BaseTestGenerator + + +class TestParseNextvalSequence(BaseTestGenerator): + """Unit tests for parse_nextval_sequence().""" + + scenarios = [ + ('No default value returns None', + dict(test_method='test_none_defval')), + ('A non-nextval default returns None', + dict(test_method='test_non_nextval_defval')), + ('A plain schema-qualified sequence name is extracted verbatim', + dict(test_method='test_plain_sequence_name')), + ('A sequence name containing a single quote has the doubled ' + 'quote decoded back to one', + dict(test_method='test_quoted_sequence_name_with_embedded_quote')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + def test_none_defval(self): + self.assertIsNone(parse_nextval_sequence(None)) + + def test_non_nextval_defval(self): + self.assertIsNone(parse_nextval_sequence('1')) + + def test_plain_sequence_name(self): + seq_name = parse_nextval_sequence( + "nextval('public.t_id_seq'::regclass)") + self.assertEqual(seq_name, 'public.t_id_seq') + + def test_quoted_sequence_name_with_embedded_quote(self): + # PostgreSQL renders the sequence "id'seq" as the double-quoted + # identifier "id'seq", and then - because the whole thing is the + # argument of a string literal - doubles the embedded single + # quote: nextval('public."id''seq"'::regclass). The extracted + # identifier must have that doubling undone, since it is spliced + # verbatim into CREATE SEQUENCE / ALTER SEQUENCE DDL rather than + # back into a string literal. + seq_name = parse_nextval_sequence( + 'nextval(\'public."id\'\'seq"\'::regclass)') + self.assertEqual(seq_name, 'public."id\'seq"') diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py index e0cf80165c3..9fc809d83e7 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py @@ -299,8 +299,19 @@ def parse_nextval_sequence(defval): Extract the schema-qualified sequence name out of a ``nextval(...)`` column default expression, e.g. ``nextval('public.t_id_seq'::regclass)`` yields ``public.t_id_seq``. The identifier is returned exactly as - PostgreSQL rendered it (already quoted if it needs to be), so callers - should use it verbatim rather than re-quoting it. + PostgreSQL would render it as a bare identifier (already quoted if it + needs to be), so callers should use it verbatim rather than + re-quoting it. + + PostgreSQL renders the argument to ``::regclass`` as a string literal, + so any single quote that is part of the identifier itself (e.g. a + sequence named ``id'seq``, which the server prints as the + double-quoted identifier ``"id'seq"``) is doubled per standard SQL + string-literal escaping: ``nextval('public."id''seq"'::regclass)``. + That doubling has to be undone before the extracted text is usable + outside of a string literal, i.e. spliced directly into + ``CREATE SEQUENCE``/``ALTER SEQUENCE`` DDL, or the doubled quote would + be read back as two literal characters instead of one (#10318). :param defval: A column's default value expression, or None :return: The schema-qualified sequence name, or None if it is not a @@ -310,7 +321,7 @@ def parse_nextval_sequence(defval): return None match = re.match(r"nextval\('(.+)'::regclass\)$", defval) - return match.group(1) if match else None + return match.group(1).replace("''", "'") if match else None @get_template_path diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql index c97b2c1187f..affd48944e0 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql @@ -20,9 +20,9 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% if data.col_type_conversion is defined and data.col_type_conversion == False %} -- {% endif %} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} TYPE {{ GET_TYPE.UPDATE_TYPE_SQL(conn, data, o_data) }}{% if data.collspcname and data.collspcname != o_data.collspcname and data.cltype != '"char"' %} COLLATE {{data.collspcname}}{% elif o_data.collspcname and data.cltype != '"char"' %} COLLATE {{o_data.collspcname}}{% endif %}; {% endif %} -{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292) ###} +{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292). IF NOT EXISTS is deliberately not used here: it would silently skip an existing, unrelated relation of the same name (without checking it is even a sequence), and the unconditional ALTER SEQUENCE ... OWNED BY below would then reassign ownership of that unrelated object instead of failing loudly (#10318). ###} {% if data.serial_seq_create is defined %} -CREATE SEQUENCE IF NOT EXISTS {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} +CREATE SEQUENCE {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql index 4784d731354..60b66593246 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql @@ -20,9 +20,9 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% if data.col_type_conversion is defined and data.col_type_conversion == False %} -- {% endif %} ALTER COLUMN {% if data.name %}{{conn|qtTypeIdent(data.name)}}{% else %}{{conn|qtTypeIdent(o_data.name)}}{% endif %} TYPE {{ GET_TYPE.UPDATE_TYPE_SQL(conn, data, o_data) }}{% if data.collspcname and data.collspcname != o_data.collspcname and data.cltype != '"char"' %} COLLATE {{data.collspcname}}{% elif o_data.collspcname and data.cltype != '"char"' %} COLLATE {{o_data.collspcname}}{% endif %}; {% endif %} -{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292) ###} +{### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292). IF NOT EXISTS is deliberately not used here: it would silently skip an existing, unrelated relation of the same name (without checking it is even a sequence), and the unconditional ALTER SEQUENCE ... OWNED BY below would then reassign ownership of that unrelated object instead of failing loudly (#10318). ###} {% if data.serial_seq_create is defined %} -CREATE SEQUENCE IF NOT EXISTS {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} +CREATE SEQUENCE {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} From 824d569e2b31c7c9ee42cc3e991a4439af21f970 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 14:43:22 +0100 Subject: [PATCH 3/7] Create a SERIAL conversion's sequence with the column's own integer type (#10292) A bare CREATE SEQUENCE is always bigint, but the sequence PostgreSQL creates for a SERIAL or SMALLSERIAL column is integer or smallint respectively, so converting a plain column to either of those produced a sequence of the wrong type. Emit AS with the column's underlying integer type, and extend the Schema Diff test with a smallint to SMALLSERIAL conversion that checks the resulting sequence's type. --- .../templates/columns/sql/16_plus/update.sql | 2 +- .../templates/columns/sql/default/update.sql | 2 +- .../test_normalise_serial_column_unit.py | 1 + .../servers/databases/schemas/tables/utils.py | 4 ++ .../test_schema_diff_serial_conversion.py | 41 ++++++++++++++++++- 5 files changed, 47 insertions(+), 3 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql index affd48944e0..eb24e0ab102 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql @@ -22,7 +22,7 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% endif %} {### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292). IF NOT EXISTS is deliberately not used here: it would silently skip an existing, unrelated relation of the same name (without checking it is even a sequence), and the unconditional ALTER SEQUENCE ... OWNED BY below would then reassign ownership of that unrelated object instead of failing loudly (#10318). ###} {% if data.serial_seq_create is defined %} -CREATE SEQUENCE {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} +CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data_type}}{% if data.serial_seq_create.cycled %} CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql index 60b66593246..146cf1a9022 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql @@ -22,7 +22,7 @@ ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.table)}} {% endif %} {### Create the sequence a column becoming SERIAL needs, before its default below can reference it (#10292). IF NOT EXISTS is deliberately not used here: it would silently skip an existing, unrelated relation of the same name (without checking it is even a sequence), and the unconditional ALTER SEQUENCE ... OWNED BY below would then reassign ownership of that unrelated object instead of failing loudly (#10318). ###} {% if data.serial_seq_create is defined %} -CREATE SEQUENCE {{data.serial_seq_create.name}}{% if data.serial_seq_create.cycled %} +CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data_type}}{% if data.serial_seq_create.cycled %} CYCLE{% endif %}{% if data.serial_seq_create.increment is not none %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py index 4dfb0f25874..6f489a27ac3 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py @@ -62,6 +62,7 @@ def test_becoming_serial(self): "nextval('public.t_id_seq'::regclass)") self.assertEqual(data['serial_seq_create']['name'], 'public.t_id_seq') self.assertEqual(data['serial_seq_create']['increment'], 1) + self.assertEqual(data['serial_seq_create']['data_type'], 'bigint') self.assertNotIn('serial_defval', data) self.assertNotIn('seqincrement', data) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py index e4c477d19ea..0b44add686e 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py @@ -1363,6 +1363,10 @@ def _normalise_serial_column(data, old_col_data): if seq_name: data['serial_seq_create'] = { 'name': seq_name, + # A bare CREATE SEQUENCE is always bigint, whereas the + # sequence a SERIAL/SMALLSERIAL owns matches the + # column's own integer type. + 'data_type': data['cltype'], 'increment': data.get('seqincrement'), 'start': data.get('seqstart'), 'minimum': data.get('seqmin'), diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py index ea380037dba..bb7e359bb15 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -42,6 +42,11 @@ id integer NOT NULL, val text ); + +CREATE TABLE {0}.int_to_smallserial ( + id smallserial NOT NULL, + val text +); """ TAR_DDL = """ @@ -56,6 +61,28 @@ id bigserial NOT NULL, val text ); + +CREATE TABLE {0}.int_to_smallserial ( + id smallint NOT NULL, + val text +); +""" + +# Fails the batch unless the sequence a SMALLSERIAL conversion created has +# the same smallint type PostgreSQL itself gives a SMALLSERIAL's sequence, +# rather than the bigint a bare CREATE SEQUENCE defaults to. +CHECK_SMALLSERIAL_SEQ_TYPE = """ +DO $$ +BEGIN + IF (SELECT s.seqtypid::regtype::text + FROM pg_catalog.pg_sequence s + WHERE s.seqrelid = pg_catalog.pg_get_serial_sequence( + '{0}.int_to_smallserial', 'id')::regclass) + IS DISTINCT FROM 'smallint' THEN + RAISE EXCEPTION 'SMALLSERIAL sequence is not smallint'; + END IF; +END +$$; """ @@ -180,6 +207,14 @@ def runTest(self): self.assertLess(fwd_ddl.index('CREATE SEQUENCE'), fwd_ddl.index('SET DEFAULT nextval(')) + # The same for SMALLSERIAL, whose sequence must be smallint. + int_to_smallserial = self.find_object(response_data, 'table', + 'int_to_smallserial') + self.assertEqual(int_to_smallserial['status'], 'Different') + small_ddl = int_to_smallserial['diff_ddl'] + self.assertIn('AS smallint', small_ddl) + self.assertIn('SET DEFAULT nextval(', small_ddl) + # Reverse: target's BIGSERIAL column must become plain integer, # which means the ALTER script must drop the column's DEFAULT # before it drops the now-unused sequence (PostgreSQL refuses to @@ -197,9 +232,13 @@ def runTest(self): # including the underlying sequence objects. self.execute_sql(self.tar_database, fwd_ddl) self.execute_sql(self.tar_database, rev_ddl) + self.execute_sql(self.tar_database, small_ddl) + self.execute_sql(self.tar_database, + CHECK_SMALLSERIAL_SEQ_TYPE.format(SCHEMA_NAME)) response_data = self.compare() - for title in ('int_to_serial', 'serial_to_int'): + for title in ('int_to_serial', 'serial_to_int', + 'int_to_smallserial'): self.assertEqual( self.find_object(response_data, 'table', title)['status'], 'Identical') From 6cde5c30c5c3ad9746aae7e0706f0e7f21d691c2 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 15:36:40 +0100 Subject: [PATCH 4/7] Move a converted SERIAL column's new sequence past its existing values (#10292) Converting a populated plain column to SERIAL created its sequence at its START value, so the next insert omitting the column reused a value the column already held. Set the new sequence to the column's current MAX (MIN for a descending sequence) once it exists, leaving it at START when the column holds no values, and have the Schema Diff test convert a populated column and check the next generated value follows on. --- .../templates/columns/sql/16_plus/update.sql | 6 ++++ .../templates/columns/sql/default/update.sql | 6 ++++ .../test_schema_diff_serial_conversion.py | 30 +++++++++++++++---- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql index eb24e0ab102..bbd17ea83bb 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql @@ -39,6 +39,12 @@ CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data ALTER SEQUENCE {{data.serial_seq_create.name}} OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; +{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; an empty column leaves the sequence at its START ###} +{% set serial_last = ('MIN(' if data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} +SELECT setval({{data.serial_seq_create.name|qtLiteral(conn)}}, {{serial_last}}) + FROM {{conn|qtIdent(data.schema, data.table)}} + HAVING {{serial_last}} IS NOT NULL; + {% endif %} {### Alter column default value ###} {% if is_view_only and data.defval is defined and data.defval is not none and data.defval != '' and data.defval != o_data.defval %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql index 146cf1a9022..082763b57d2 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql @@ -39,6 +39,12 @@ CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data ALTER SEQUENCE {{data.serial_seq_create.name}} OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; +{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; an empty column leaves the sequence at its START ###} +{% set serial_last = ('MIN(' if data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} +SELECT setval({{data.serial_seq_create.name|qtLiteral(conn)}}, {{serial_last}}) + FROM {{conn|qtIdent(data.schema, data.table)}} + HAVING {{serial_last}} IS NOT NULL; + {% endif %} {### Alter column default value ###} {% if is_view_only and data.defval is defined and data.defval is not none and data.defval != '' and data.defval != o_data.defval %} diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py index bb7e359bb15..9c26bb086f9 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -68,6 +68,26 @@ ); """ +# Rows the target's plain column already holds before it becomes SERIAL. +TAR_ROWS = """ +INSERT INTO {0}.int_to_serial (id, val) VALUES (1, 'a'), (5, 'b'); +""" + +# Fails the batch unless the converted column's new sequence carries on +# past the values it already held, rather than starting again at 1. +CHECK_INT_TO_SERIAL_NEXT_ID = """ +DO $$ +DECLARE + new_id bigint; +BEGIN + INSERT INTO {0}.int_to_serial (val) VALUES ('x') RETURNING id INTO new_id; + IF new_id <> 6 THEN + RAISE EXCEPTION 'expected the next id to be 6, got %', new_id; + END IF; +END +$$; +""" + # Fails the batch unless the sequence a SMALLSERIAL conversion created has # the same smallint type PostgreSQL itself gives a SMALLSERIAL's sequence, # rather than the bigint a bare CREATE SEQUENCE defaults to. @@ -108,6 +128,7 @@ def setUp(self): self.execute_sql(self.src_database, SRC_DDL.format(SCHEMA_NAME)) self.execute_sql(self.tar_database, TAR_DDL.format(SCHEMA_NAME)) + self.execute_sql(self.tar_database, TAR_ROWS.format(SCHEMA_NAME)) def execute_sql(self, db_name, sql): """ @@ -244,11 +265,10 @@ def runTest(self): 'Identical') # The forward conversion must have made the column a genuine - # SERIAL: an insert omitting it must now succeed. - self.execute_sql( - self.tar_database, - "INSERT INTO {0}.int_to_serial (val) VALUES ('x')".format( - SCHEMA_NAME)) + # SERIAL: an insert omitting it must now succeed, and must not + # reuse a value the column already held. + self.execute_sql(self.tar_database, + CHECK_INT_TO_SERIAL_NEXT_ID.format(SCHEMA_NAME)) def tearDown(self): """This function drops the added databases""" From 95c51cf74cad3fe7bece4f45c22ae3b64caf6700 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 16:06:50 +0100 Subject: [PATCH 5/7] Only move a converted SERIAL column's sequence forward from its START (#10292) Setting the new sequence to the column's MAX (or MIN) whatever it held could move it backwards past its START, or abort the script with an out-of-bounds error when the column only held values below MINVALUE, such as a 0. Only call setval() once that value has reached START in the sequence's direction, and cover the below-MINVALUE case in the Schema Diff test. --- .../templates/columns/sql/16_plus/update.sql | 9 ++++++--- .../templates/columns/sql/default/update.sql | 9 ++++++--- .../test_schema_diff_serial_conversion.py | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql index bbd17ea83bb..f25271b9e1e 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/16_plus/update.sql @@ -39,11 +39,14 @@ CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data ALTER SEQUENCE {{data.serial_seq_create.name}} OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; -{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; an empty column leaves the sequence at its START ###} -{% set serial_last = ('MIN(' if data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} +{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; a column that is empty, or holds nothing at or past START, leaves the sequence at its START (setval() below START would also go backwards, or out of bounds below MINVALUE) ###} +{% set serial_desc = data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 %} +{% set serial_last = ('MIN(' if serial_desc else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} SELECT setval({{data.serial_seq_create.name|qtLiteral(conn)}}, {{serial_last}}) FROM {{conn|qtIdent(data.schema, data.table)}} - HAVING {{serial_last}} IS NOT NULL; + HAVING {{serial_last}} IS NOT NULL{% if data.serial_seq_create.start is not none %} + + AND {{serial_last}} {{ '<=' if serial_desc else '>=' }} {{data.serial_seq_create.start|int}}{% endif %}; {% endif %} {### Alter column default value ###} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql index 082763b57d2..38a72f03b87 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/update.sql @@ -39,11 +39,14 @@ CREATE SEQUENCE {{data.serial_seq_create.name}} AS {{data.serial_seq_create.data ALTER SEQUENCE {{data.serial_seq_create.name}} OWNED BY {{conn|qtIdent(data.schema)}}.{{conn|qtIdent(data.table)}}.{% if data.name %}{{conn|qtIdent(data.name)}}{% else %}{{conn|qtIdent(o_data.name)}}{% endif %}; -{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; an empty column leaves the sequence at its START ###} -{% set serial_last = ('MIN(' if data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} +{### Move the new sequence past any values the column already holds, or the next insert omitting the column would reuse one of them; a column that is empty, or holds nothing at or past START, leaves the sequence at its START (setval() below START would also go backwards, or out of bounds below MINVALUE) ###} +{% set serial_desc = data.serial_seq_create.increment is not none and data.serial_seq_create.increment|int < 0 %} +{% set serial_last = ('MIN(' if serial_desc else 'MAX(') ~ conn|qtIdent(data.name or o_data.name) ~ ')' %} SELECT setval({{data.serial_seq_create.name|qtLiteral(conn)}}, {{serial_last}}) FROM {{conn|qtIdent(data.schema, data.table)}} - HAVING {{serial_last}} IS NOT NULL; + HAVING {{serial_last}} IS NOT NULL{% if data.serial_seq_create.start is not none %} + + AND {{serial_last}} {{ '<=' if serial_desc else '>=' }} {{data.serial_seq_create.start|int}}{% endif %}; {% endif %} {### Alter column default value ###} diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py index 9c26bb086f9..0a58a1e4c6f 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -71,6 +71,7 @@ # Rows the target's plain column already holds before it becomes SERIAL. TAR_ROWS = """ INSERT INTO {0}.int_to_serial (id, val) VALUES (1, 'a'), (5, 'b'); +INSERT INTO {0}.int_to_smallserial (id, val) VALUES (0, 'a'); """ # Fails the batch unless the converted column's new sequence carries on @@ -88,6 +89,22 @@ $$; """ +# Fails the batch unless the SMALLSERIAL column, which held nothing past +# its sequence's START (only a 0, below MINVALUE 1), still starts at 1. +CHECK_INT_TO_SMALLSERIAL_NEXT_ID = """ +DO $$ +DECLARE + new_id smallint; +BEGIN + INSERT INTO {0}.int_to_smallserial (val) VALUES ('x') + RETURNING id INTO new_id; + IF new_id <> 1 THEN + RAISE EXCEPTION 'expected the next id to be 1, got %', new_id; + END IF; +END +$$; +""" + # Fails the batch unless the sequence a SMALLSERIAL conversion created has # the same smallint type PostgreSQL itself gives a SMALLSERIAL's sequence, # rather than the bigint a bare CREATE SEQUENCE defaults to. @@ -269,6 +286,8 @@ def runTest(self): # reuse a value the column already held. self.execute_sql(self.tar_database, CHECK_INT_TO_SERIAL_NEXT_ID.format(SCHEMA_NAME)) + self.execute_sql(self.tar_database, + CHECK_INT_TO_SMALLSERIAL_NEXT_ID.format(SCHEMA_NAME)) def tearDown(self): """This function drops the added databases""" From f31ea7c4259294478e908ac6d48d0ffeea0a833f Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 17:36:40 +0100 Subject: [PATCH 6/7] Keep a SERIAL column's sequence when only its type changes (#10292) Widening a SERIAL column to bigint in the table dialog sends only the new cltype, which was enough to treat the column as leaving SERIAL and emit DROP SEQUENCE without DROP DEFAULT. PostgreSQL refuses that drop whilst the column's nextval() default still references the sequence, so the change could not be saved. A column now only leaves SERIAL when its default is also replaced or dropped. --- .../test_normalise_serial_column_unit.py | 19 ++++ .../test_table_serial_type_change_msql.py | 101 ++++++++++++++++++ .../servers/databases/schemas/tables/utils.py | 8 +- 3 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_table_serial_type_change_msql.py diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py index 6f489a27ac3..a8a4c1d625a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py @@ -34,6 +34,9 @@ class TestNormaliseSerialColumn(BaseTestGenerator): ('A partial update that never mentions cltype leaves an ' 'already-SERIAL column alone', dict(test_method='test_partial_update_without_cltype_is_ignored')), + ('A partial update that only changes the type of a SERIAL column ' + 'keeps its sequence', + dict(test_method='test_type_change_without_defval_keeps_serial')), ] def runTest(self): @@ -122,3 +125,19 @@ def test_partial_update_without_cltype_is_ignored(self): BaseTableView._normalise_serial_column(data, old_col_data) self.assertEqual(data, {'attacl': {'added': []}}) + + def test_type_change_without_defval_keeps_serial(self): + # Widening a SERIAL column to bigint in the table dialog sends + # only the new 'cltype'. With its nextval() default untouched the + # column is still SERIAL, so its sequence must not be queued for + # dropping (PostgreSQL would refuse whilst the default uses it). + data = {'cltype': 'bigint'} + old_col_data = { + 'cltype': 'integer', 'typname': 'integer', + 'defval': "nextval('public.t_id_seq'::regclass)", + 'seqrelid': 100, 'defseqrelid': 100, 'attidentity': '', + } + + BaseTableView._normalise_serial_column(data, old_col_data) + + self.assertEqual(data, {'cltype': 'bigint'}) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_table_serial_type_change_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_table_serial_type_change_msql.py new file mode 100644 index 00000000000..c85e7aee4be --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_table_serial_type_change_msql.py @@ -0,0 +1,101 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Changing only the integer type of a SERIAL column from the table dialog +(a partial update carrying 'cltype' but no 'defval') must keep the column +SERIAL: it must not queue its sequence to be dropped, which PostgreSQL +would refuse anyway whilst the column's default still references it +(#10292). +""" + +import json +import uuid + +from pgadmin.browser.server_groups.servers.databases.schemas.tests import \ + utils as schema_utils +from pgadmin.browser.server_groups.servers.databases.tests import utils as \ + database_utils +from pgadmin.utils.route import BaseTestGenerator +from regression import parent_node_dict +from regression.python_test_utils import test_utils as utils +from . import utils as tables_utils + + +class TableSerialTypeChangeMsqlTestCase(BaseTestGenerator): + """Widen a SERIAL column to bigint through the table msql endpoint.""" + url = '/browser/table/msql/' + + scenarios = [ + ('Changing only the type of a SERIAL column keeps its sequence', + dict()), + ] + + def setUp(self): + self.db_name = parent_node_dict["database"][-1]["db_name"] + schema_info = parent_node_dict["schema"][-1] + self.server_id = schema_info["server_id"] + self.db_id = schema_info["db_id"] + db_con = database_utils.connect_database(self, utils.SERVER_GROUP, + self.server_id, self.db_id) + if not db_con['data']["connected"]: + raise Exception("Could not connect to database to add a table.") + + self.schema_id = schema_info["schema_id"] + self.schema_name = schema_info["schema_name"] + if not schema_utils.verify_schemas(self.server, self.db_name, + self.schema_name): + raise Exception("Could not find the schema to add a table.") + + # The default table has "id serial" as its first column. + self.table_name = "test_serial_type_%s" % (str(uuid.uuid4())[1:8]) + self.table_id = tables_utils.create_table(self.server, self.db_name, + self.schema_name, + self.table_name) + + def _query(self, sql, fetch=True): + connection = utils.get_db_connection(self.db_name, + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port'], + self.server['sslmode']) + try: + pg_cursor = connection.cursor() + pg_cursor.execute(sql) + result = pg_cursor.fetchone() if fetch else None + connection.commit() + return result + finally: + connection.close() + + def runTest(self): + data = {'columns': json.dumps( + {'changed': [{'attnum': 1, 'cltype': 'bigint'}]})} + response = tables_utils.api_get_msql(self, data) + self.assertEqual(response.status_code, 200) + sql = json.loads(response.data.decode('utf-8'))['data'] + + self.assertIn('TYPE bigint', sql) + self.assertNotIn('DROP SEQUENCE', sql) + self.assertNotIn('DROP DEFAULT', sql) + + # The generated script must apply, and leave the column a bigint + # that still defaults from, and owns, its sequence. + self._query(sql, fetch=False) + type_name, seq = self._query( + "SELECT a.atttypid::regtype::text, " + "pg_catalog.pg_get_serial_sequence('{0}.{1}', 'id') " + "FROM pg_catalog.pg_attribute a " + "WHERE a.attrelid = '{0}.{1}'::regclass " + "AND a.attname = 'id'".format(self.schema_name, self.table_name)) + self.assertEqual(type_name, 'bigint') + self.assertIsNotNone(seq) + + def tearDown(self): + database_utils.disconnect_database(self, self.server_id, self.db_id) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py index 0b44add686e..8803b5b8a38 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py @@ -1337,8 +1337,14 @@ def _normalise_serial_column(data, old_col_data): # whatsoever, and is not a request to change the type, however # SERIAL the column already is; unlike Schema Diff's columns, # which always carry the full properties and so always have one. + # Nor is a change of type alone (e.g. widening integer to bigint + # in the table dialog, which sends no 'defval'): the column only + # stops being SERIAL once its nextval() default is replaced or + # dropped, and until then PostgreSQL would refuse to drop the + # sequence that default still references. leaving_serial = was_serial and not becomes_serial \ - and 'cltype' in data + and 'cltype' in data and 'defval' in data \ + and data['defval'] != old_col_data.get('defval') if not becomes_serial and not leaving_serial: return From 2f0d304fe4072dc407d60f6ba98e1445ccdae38c Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 17:36:40 +0100 Subject: [PATCH 7/7] Test the whole Schema Diff script for a SERIAL conversion (#10292) Apply every differing object's DDL, ordered as Generate Script orders it, in one transaction, so that the column diffs' own CREATE/DROP SEQUENCE are checked against the owned sequences' separate Source Only / Target Only rows as well. --- .../test_schema_diff_serial_conversion.py | 67 ++++++++++++++++--- 1 file changed, 59 insertions(+), 8 deletions(-) diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py index 0a58a1e4c6f..14f8571b16b 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -208,6 +208,50 @@ def find_object(self, response_data, node_type, title): self.fail('{0} {1} was not compared'.format(node_type, title)) + @staticmethod + def generate_full_script(response_data): + """ + Build the script Schema Diff's "Generate Script" produces for every + differing object, following computeDependLevels() and + generateFinalScript() in static/js/components/SchemaDiffCompare.jsx: + anything another object depends on is written first, and objects + on the same level keep the comparison's own order. + + :param response_data: Result of compare() + :return: The script, wrapped in a single transaction + """ + rows = [diff for diff in response_data + if diff.get('status') != 'Identical'] + by_oid = {diff['oid']: idx for idx, diff in enumerate(rows) + if diff.get('oid') is not None} + dependents = {} + for idx, diff in enumerate(rows): + for dep in diff.get('dependencies') or []: + if dep.get('oid') in by_oid: + dependents.setdefault(by_oid[dep['oid']], []).append(idx) + + levels = {} + + def level_of(idx, resolving=frozenset()): + if idx in levels: + return levels[idx] + if idx in resolving: + return 1 + level = 1 + for dependent in dependents.get(idx, []): + level = max(level, + level_of(dependent, resolving | {idx}) + 1) + levels[idx] = level + return level + + buckets = {} + for idx, diff in enumerate(rows): + buckets.setdefault(level_of(idx), []).append(diff['diff_ddl']) + + return 'BEGIN;\n' + ''.join( + '\n'.join(buckets[level]) + '\n\n' + for level in sorted(buckets, reverse=True)) + 'END;' + def runTest(self): """ This function will test converting a column between integer and SERIAL, in both directions. """ @@ -266,20 +310,27 @@ def runTest(self): self.assertLess(rev_ddl.index('DROP DEFAULT'), rev_ddl.index('DROP SEQUENCE')) - # Applying both must succeed, and must settle the differences, - # including the underlying sequence objects. - self.execute_sql(self.tar_database, fwd_ddl) - self.execute_sql(self.tar_database, rev_ddl) - self.execute_sql(self.tar_database, small_ddl) + # Applying the whole script, which also carries the owned + # sequences' own Source Only / Target Only rows, must succeed in a + # single transaction and settle every difference, so the column + # diffs' CREATE/DROP SEQUENCE must not collide with those rows. + self.execute_sql(self.tar_database, + self.generate_full_script(response_data)) self.execute_sql(self.tar_database, CHECK_SMALLSERIAL_SEQ_TYPE.format(SCHEMA_NAME)) response_data = self.compare() - for title in ('int_to_serial', 'serial_to_int', - 'int_to_smallserial'): + for node_type, title in ( + ('table', 'int_to_serial'), ('table', 'serial_to_int'), + ('table', 'int_to_smallserial'), + ('sequence', 'int_to_serial_id_seq'), + ('sequence', 'int_to_smallserial_id_seq')): self.assertEqual( - self.find_object(response_data, 'table', title)['status'], + self.find_object(response_data, node_type, title)['status'], 'Identical') + self.assertFalse(any( + diff.get('title') == 'serial_to_int_id_seq' + for diff in response_data)) # The forward conversion must have made the column a genuine # SERIAL: an insert omitting it must now succeed, and must not