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 35db1bbb1f2..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 @@ -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,42 @@ 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 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 + nextval() default + """ + if not defval: + return None + + match = re.match(r"nextval\('(.+)'::regclass\)$", defval) + return match.group(1).replace("''", "'") 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..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 @@ -20,6 +20,35 @@ 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 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}} AS {{data.serial_seq_create.data_type}}{% 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 %}; + +{### 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{% 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 ###} {% 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 +64,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..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 @@ -20,6 +20,35 @@ 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 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}} AS {{data.serial_seq_create.data_type}}{% 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 %}; + +{### 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{% 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 ###} {% 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 +64,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..a8a4c1d625a --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/test_normalise_serial_column_unit.py @@ -0,0 +1,143 @@ +########################################################################## +# +# 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')), + ('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): + 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.assertEqual(data['serial_seq_create']['data_type'], 'bigint') + 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': []}}) + + 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 ba9edabcbc3..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 @@ -1304,24 +1304,90 @@ 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. + # 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 'defval' in data \ + and data['defval'] != old_col_data.get('defval') + + 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, + # 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'), + '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..14f8571b16b --- /dev/null +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_serial_conversion.py @@ -0,0 +1,353 @@ +########################################################################## +# +# 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 +); + +CREATE TABLE {0}.int_to_smallserial ( + id smallserial 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 +); + +CREATE TABLE {0}.int_to_smallserial ( + id smallint NOT NULL, + val text +); +""" + +# 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 +# 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 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. +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 +$$; +""" + + +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)) + self.execute_sql(self.tar_database, TAR_ROWS.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)) + + @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. """ + 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(')) + + # 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 + # 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 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 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, 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 + # 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""" + 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)