From 3fdfeabc4871f2314a7e85925120ef9f1b9f3644 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:43:32 +0100 Subject: [PATCH 1/2] Schema Diff: drop the scaffolding default partition after rebuilding a partitioned table (#10301) Rebuilding a partitioned table (e.g. because its partition key changed) generates a script that creates a temporary partitioned table, adds a DEFAULT partition to it purely so the row-copy INSERT doesn't fail on rows that match none of the real partitions, copies the rows across and renames everything into place. The scaffolding DEFAULT partition was never removed, so a source table with no default partition of its own ended up with an extra one in the rebuilt target, and Schema Diff would report the table as different forever after. Worse, if the source table did have a genuine default partition, the generated script tried to create it as well as the scaffolding one, and Postgres only allows a single DEFAULT partition per parent, so applying the script failed outright. get_sql_from_diff() now checks whether the source table already has a default partition and only asks the template to scaffold one when it doesn't; partition_diff.sql only creates that scaffolding partition (and drops it again once the row copy is done) in that case, leaving a genuine source default partition to be carried across, renamed into place, by the normal per-partition rename loop. Added test_schema_diff_partition_default.py, covering both a source table without a default partition (the scaffolding one must be dropped) and one with a genuine default partition (it must survive and the script must not attempt to create two). --- .../schemas/tables/partitions/__init__.py | 15 + .../sql/pg/default/partition_diff.sql | 11 +- .../sql/ppas/default/partition_diff.sql | 11 +- .../test_schema_diff_partition_default.py | 294 ++++++++++++++++++ 4 files changed, 325 insertions(+), 6 deletions(-) create mode 100644 web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py index bffe011e04f..f6097805507 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py @@ -497,6 +497,21 @@ def get_sql_from_diff(self, **kwargs): target_data['orig_name'] = target_data['name'] target_data['name'] = 'temp_partitioned_{0}'.format( secrets.choice(range(1, 9999999))) + + # If the source table already has a default partition of its own, + # it will be (re)created below via source_data['partitions'], so we + # must not scaffold an extra one (Postgres allows only a single + # default partition per parent). We only need the scaffolding + # default partition - dropped again once the data copy is done - + # when the source has no default partition, to prevent the + # row-copy INSERT from failing on rows that don't match any of the + # real partitions. + source_has_default_partition = any( + partition.get('is_default') for partition in + source_data.get('partitions', [])) + target_data['create_scaffolding_default_partition'] = \ + not source_has_default_partition + # For PG/EPAS 11 and above when we copy the data from original # table to temporary table for schema diff, we will have to create # a default partition to prevent the data loss. diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql index ab65e87fb33..b35cc8c01a7 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql @@ -1,15 +1,20 @@ CREATE TABLE IF NOT EXISTS {{conn|qtIdent(data.schema, data.name)}} ( LIKE {{conn|qtIdent(data.schema, data.orig_name)}} INCLUDING ALL ) PARTITION BY {{ data.partition_scheme }}; -{{partition_sql}}{{partition_data.default_partition_header}} +{{partition_sql}}{% if data.create_scaffolding_default_partition %}{{partition_data.default_partition_header}} CREATE TABLE IF NOT EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}} PARTITION OF {{conn|qtIdent(data.schema, data.name)}} DEFAULT; - +{% endif %} INSERT INTO {{conn|qtIdent(data.schema, data.name)}}( {% if data.columns and data.columns|length > 0 %} {% for c in data.columns %} {{c.name}}{% if not loop.last %},{% endif %}{% endfor %}{% endif %}) SELECT {% if data.columns and data.columns|length > 0 %}{% for c in data.columns %}{{c.name}}{% if not loop.last %},{% endif %}{% endfor %}{% endif %} FROM {{conn|qtIdent(data.schema, data.orig_name)}}; - +{% if data.create_scaffolding_default_partition %} +-- The source table has no default partition of its own, so the +-- scaffolding default partition created above (purely to stop the row +-- copy above failing on unmatched rows) is no longer needed. +DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}}; +{% endif %} {% if partition_data.partitions and partition_data.partitions|length > 0 %} {% for part in partition_data.partitions %} DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, part.partition_name)}}; diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql index bfc063c0f79..dae5e41e5f1 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql @@ -1,15 +1,20 @@ CREATE TABLE {{conn|qtIdent(data.schema, data.name)}} ( LIKE {{conn|qtIdent(data.schema, data.orig_name)}} INCLUDING ALL ) PARTITION BY {{ data.partition_scheme }}; -{{partition_sql}}{{partition_data.default_partition_header}} +{{partition_sql}}{% if data.create_scaffolding_default_partition %}{{partition_data.default_partition_header}} CREATE TABLE IF NOT EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}} PARTITION OF {{conn|qtIdent(data.schema, data.name)}} DEFAULT; - +{% endif %} INSERT INTO {{conn|qtIdent(data.schema, data.name)}}( {% if data.columns and data.columns|length > 0 %} {% for c in data.columns %} {{c.name}}{% if not loop.last %},{% endif %}{% endfor %}{% endif %}) SELECT {% if data.columns and data.columns|length > 0 %}{% for c in data.columns %}{{c.name}}{% if not loop.last %},{% endif %}{% endfor %}{% endif %} FROM {{conn|qtIdent(data.schema, data.orig_name)}}; - +{% if data.create_scaffolding_default_partition %} +-- The source table has no default partition of its own, so the +-- scaffolding default partition created above (purely to stop the row +-- copy above failing on unmatched rows) is no longer needed. +DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}}; +{% endif %} {% if partition_data.partitions and partition_data.partitions|length > 0 %} {% for part in partition_data.partitions %} DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, part.partition_name)}}; diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py new file mode 100644 index 00000000000..b3867a685ff --- /dev/null +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py @@ -0,0 +1,294 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Schema Diff tests for partitioned table rebuilds (#10301). + +When Schema Diff has to rebuild a partitioned table (e.g. because a +partition's bounds changed) it creates a temporary partitioned table, adds +a scaffolding DEFAULT partition to it so the row-copy INSERT does not fail +on rows that match none of the real partitions, copies the rows across and +then renames everything into place. Two things must hold once that is +done: + +* If the source table has no default partition of its own, the scaffolding + one must be dropped again, otherwise the rebuilt target ends up with an + extra default partition the source never had. +* If the source table genuinely has a default partition, only one DEFAULT + partition may exist on the temporary table (Postgres allows a single + one), and it must be the real one, not the scaffolding one. +""" + +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_partition_default_diff' + +DDL_SOURCE = """ +CREATE SCHEMA {0}; + +CREATE TABLE {0}.part_no_default ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_no_default_p1 PARTITION OF {0}.part_no_default + FOR VALUES FROM (1) TO (10); +CREATE TABLE {0}.part_no_default_p2 PARTITION OF {0}.part_no_default + FOR VALUES FROM (10) TO (20); + +CREATE TABLE {0}.part_with_default ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_with_default_p1 PARTITION OF {0}.part_with_default + FOR VALUES FROM (1) TO (10); +CREATE TABLE {0}.part_with_default_def PARTITION OF {0}.part_with_default + DEFAULT; +""" + +# The target starts out with different partition bounds so that Schema Diff +# has to rebuild both tables; the default-partition shape of each table +# matches its source counterpart's *before* the fix, i.e. still wrong, +# forcing Schema Diff's generated DDL to correct it. +DDL_TARGET = """ +CREATE SCHEMA {0}; + +CREATE TABLE {0}.part_no_default ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_no_default_p1 PARTITION OF {0}.part_no_default + FOR VALUES FROM (1) TO (5); + +CREATE TABLE {0}.part_with_default ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_with_default_p1 PARTITION OF {0}.part_with_default + FOR VALUES FROM (1) TO (5); +CREATE TABLE {0}.part_with_default_def PARTITION OF {0}.part_with_default + DEFAULT; +""" + + +class SchemaDiffPartitionDefaultTestCase(BaseSocketTestGenerator): + """ This class will test Schema Diff against partitioned table + rebuilds involving default partitions. """ + scenarios = [ + ('Schema diff comparison of partition rebuild default partitions', + dict()) + ] + SOCKET_NAMESPACE = '/schema_diff' + + def setUp(self): + super().setUp() + self.src_database = "db_part_diff_src_%s" % str(uuid.uuid4())[1:8] + self.tar_database = "db_part_diff_tar_%s" % str(uuid.uuid4())[1:8] + + self.src_db_id = utils.create_database(self.server, self.src_database) + self.tar_db_id = utils.create_database(self.server, self.tar_database) + + self.server = parent_node_dict["server"][-1]["server"] + self.server_id = parent_node_dict["server"][-1]["server_id"] + + self.execute_sql(self.src_database, DDL_SOURCE.format(SCHEMA_NAME)) + self.execute_sql(self.tar_database, DDL_TARGET.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 fetch_scalar(self, db_name, sql): + """ + Run a query against one of the test databases and return the + first column of the first row. + + :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']) + pg_cursor = connection.cursor() + pg_cursor.execute(sql) + result = pg_cursor.fetchone()[0] + connection.close() + return int(result) + + def count_default_partitions(self, db_name, table_name): + """ + Count how many DEFAULT partitions the given partitioned table has + in the given database. + """ + return self.fetch_scalar( + db_name, + "SELECT count(*) FROM pg_inherits i " + "JOIN pg_class c ON c.oid = i.inhrelid " + "JOIN pg_class p ON p.oid = i.inhparent " + "WHERE p.relname = '{0}' " + "AND p.relnamespace = '{1}'::regnamespace " + "AND pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT'".format( + table_name, SCHEMA_NAME)) + + def count_partitions(self, db_name, table_name): + """ + Count how many partitions the given partitioned table has in the + given database. + """ + return self.fetch_scalar( + db_name, + "SELECT count(*) FROM pg_inherits i " + "JOIN pg_class p ON p.oid = i.inhparent " + "WHERE p.relname = '{0}' " + "AND p.relnamespace = '{1}'::regnamespace".format( + table_name, SCHEMA_NAME)) + + def compare(self): + """ + Compare the two test databases and return the result. + + :return: List of compared objects + """ + data = { + 'trans_id': self.trans_id, + 'source_sid': self.server_id, + 'source_did': self.src_db_id, + 'target_sid': self.server_id, + 'target_did': self.tar_db_id, + 'ignore_owner': 0, + 'ignore_whitespaces': 0, + 'ignore_tablespace': 0, + 'ignore_grants': 0 + } + self.socket_client.emit('compare_database', data, + namespace=self.SOCKET_NAMESPACE) + received = self.socket_client.get_received(self.SOCKET_NAMESPACE) + response_data = received[-1]['args'][0] + self.assertEqual(received[-1]['name'], "compare_database_success", + response_data) + return response_data + + def find_object(self, response_data, node_type, title): + """ + Pick a single compared object out of the comparison result. + + :param response_data: Result of compare() + :param node_type: Node type, e.g. 'table' + :param title: Object name + :return: The compared object + """ + for diff in response_data: + if diff.get('type') == node_type and diff.get('title') == title: + return diff + + self.fail('{0} {1} was not compared'.format(node_type, title)) + + def runTest(self): + """ This function will test Schema Diff for partition rebuilds + that involve default partitions. """ + 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() + + # --- Source has no default partition of its own. --- + no_default = self.find_object(response_data, 'table', + 'part_no_default') + self.assertEqual(no_default['status'], 'Different') + diff_ddl = no_default['diff_ddl'] + + # Applying the diff must succeed and leave no default partition + # behind, since the source table doesn't have one. + self.execute_sql(self.tar_database, diff_ddl) + self.assertEqual( + self.count_default_partitions(self.tar_database, + 'part_no_default'), 0, + 'Schema Diff left a scaffolding default partition behind on ' + 'a table whose source has no default partition: ' + '{0}'.format(diff_ddl)) + self.assertEqual( + self.count_partitions(self.tar_database, 'part_no_default'), 2) + + # --- Source has a genuine default partition. --- + with_default = self.find_object(response_data, 'table', + 'part_with_default') + self.assertEqual(with_default['status'], 'Different') + diff_ddl = with_default['diff_ddl'] + + # Applying the diff must succeed (it must not try to create two + # DEFAULT partitions on the temporary table) and the real default + # partition must survive. + self.execute_sql(self.tar_database, diff_ddl) + self.assertEqual( + self.count_default_partitions(self.tar_database, + 'part_with_default'), 1, + 'Schema Diff did not preserve the source table\'s genuine ' + 'default partition: {0}'.format(diff_ddl)) + self.assertEqual( + self.count_partitions(self.tar_database, 'part_with_default'), + 2) + + # Re-comparing must now report both tables as identical. + response_data = self.compare() + self.assertEqual( + self.find_object(response_data, 'table', + 'part_no_default')['status'], 'Identical') + self.assertEqual( + self.find_object(response_data, 'table', + 'part_with_default')['status'], 'Identical') + + def tearDown(self): + """This function drops the added databases""" + super().tearDown() + for db_name in (self.src_database, self.tar_database): + connection = utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port'], + self.server['sslmode']) + utils.drop_database(connection, db_name) From 09e8078b4c6aeaac29f9c5cc72cd461773b0cfa0 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:42:52 +0100 Subject: [PATCH 2/2] Schema Diff: fix scaffold naming collision, stop dropping non-empty scaffold default partitions, and skip redundant partition ALTER diff Address CodeRabbit review findings on the partitioned-table rebuild added in #10301: - The scaffolding default partition's name was derived from the original table's name (_default), a deterministic name that can collide with an existing relation. Derive it from the already string.randomised temporary table name instead, matching the collision-avoidance convention already used for the temp partitioned table and its temp partitions. - The scaffolding default partition was unconditionally dropped once the row copy finished. Any row from the source table that fell outside every real partition's bounds landed in that scaffold and was silently destroyed. The generated SQL now only drops the scaffold if it is still empty; otherwise it is kept, so the rows it caught survive as the default partition of the rebuilt table. - Separately, found and fixed a data-loss bug this exposed: whenever a table's partitions differ on both source and target, the generic table-diff also ran its own ALTER-based partition add/remove/detach logic (get_sql_from_table_diff/_check_for_partitions_in_sql) alongside the full-table rebuild path in schema_diff_table_utils.py. Because that generic path detaches a bound-changed partition using its real name before the rebuild's row-copy INSERT ... SELECT runs against the original table, the detached partition's rows were invisible to that copy and got dropped when the rebuild's own cleanup step removed the now-standalone table - causing the run-python-tests-pg/run-feature-tests-pg CI failures on PR #10316. The generic partition diffing is now skipped whenever both sides are partitioned, since the rebuild path already handles every partition difference itself. - Strengthened the regression test: both fixtures now carry real rows (including rows that only fit a DEFAULT partition, and a row outside every rebuilt partition's bounds) so the row-copy and row-routing paths are actually exercised, not just the DDL shape. --- .../schemas/tables/partitions/__init__.py | 7 +- .../schemas/tables/schema_diff_table_utils.py | 14 +++ .../sql/pg/default/partition_diff.sql | 16 ++- .../sql/ppas/default/partition_diff.sql | 16 ++- .../test_schema_diff_partition_default.py | 99 ++++++++++++++++++- 5 files changed, 145 insertions(+), 7 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py index f6097805507..1fe4533447e 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/partitions/__init__.py @@ -514,9 +514,12 @@ def get_sql_from_diff(self, **kwargs): # For PG/EPAS 11 and above when we copy the data from original # table to temporary table for schema diff, we will have to create - # a default partition to prevent the data loss. + # a default partition to prevent the data loss. Derive its name + # from the already-randomised temporary table name (rather than + # the original table's name) so it cannot collide with an + # existing relation. target_data['default_partition_name'] = \ - target_data['orig_name'] + '_default' + target_data['name'] + '_default' # Copy the partition scheme from source to target. if 'partition_scheme' in source_data: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/schema_diff_table_utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/schema_diff_table_utils.py index 74b91420f3b..4dac00edfa5 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/schema_diff_table_utils.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/schema_diff_table_utils.py @@ -322,6 +322,20 @@ def get_sql_from_submodule_diff(self, **kwargs): pk_diff = self.table_constraint_comp(source, target) diff_dict.update(pk_diff) + # When both the source and target tables are partitioned, the + # 'partition' submodule branch below rebuilds the whole table + # (and every one of its partitions) from scratch, handling any + # added, removed or bound-changed partition itself. The generic + # partition add/remove handling in get_sql_from_table_diff (via + # _check_for_partitions_in_sql) must not also run in that case: + # it would detach/recreate the same partitions using their real + # names ahead of the rebuild script, so the rebuild's row-copy + # step (which reads from the original table) would miss rows + # already detached out from under it, silently losing data. + if 'partitions' in diff_dict and \ + source.get('is_partitioned') and target.get('is_partitioned'): + del diff_dict['partitions'] + # Get the difference DDL/DML statements for table target_params['diff_data'] = diff_dict diff = self.get_sql_from_table_diff(**target_params) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql index b35cc8c01a7..4edbc05581f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/pg/default/partition_diff.sql @@ -12,8 +12,20 @@ SELECT {% if data.columns and data.columns|length > 0 %}{% for c in data.columns {% if data.create_scaffolding_default_partition %} -- The source table has no default partition of its own, so the -- scaffolding default partition created above (purely to stop the row --- copy above failing on unmatched rows) is no longer needed. -DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}}; +-- copy above failing on unmatched rows) is dropped, but only if it is +-- still empty. If any rows were routed into it (i.e. rows that don't +-- fall within the bounds of any other partition), it is left in place +-- so that data is not lost; it becomes the default partition of the +-- rebuilt table. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM {{conn|qtIdent(data.schema, data.default_partition_name)}} + ) THEN + DROP TABLE {{conn|qtIdent(data.schema, data.default_partition_name)}}; + END IF; +END; +$$; {% endif %} {% if partition_data.partitions and partition_data.partitions|length > 0 %} {% for part in partition_data.partitions %} diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql index dae5e41e5f1..8a3fccf9861 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/partitions/sql/ppas/default/partition_diff.sql @@ -12,8 +12,20 @@ SELECT {% if data.columns and data.columns|length > 0 %}{% for c in data.columns {% if data.create_scaffolding_default_partition %} -- The source table has no default partition of its own, so the -- scaffolding default partition created above (purely to stop the row --- copy above failing on unmatched rows) is no longer needed. -DROP TABLE IF EXISTS {{conn|qtIdent(data.schema, data.default_partition_name)}}; +-- copy above failing on unmatched rows) is dropped, but only if it is +-- still empty. If any rows were routed into it (i.e. rows that don't +-- fall within the bounds of any other partition), it is left in place +-- so that data is not lost; it becomes the default partition of the +-- rebuilt table. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM {{conn|qtIdent(data.schema, data.default_partition_name)}} + ) THEN + DROP TABLE {{conn|qtIdent(data.schema, data.default_partition_name)}}; + END IF; +END; +$$; {% endif %} {% if partition_data.partitions and partition_data.partitions|length > 0 %} {% for part in partition_data.partitions %} diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py index b3867a685ff..487912df787 100644 --- a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_partition_default.py @@ -22,6 +22,10 @@ * If the source table genuinely has a default partition, only one DEFAULT partition may exist on the temporary table (Postgres allows a single one), and it must be the real one, not the scaffolding one. +* If the target table already holds rows that fall outside every one of + the rebuilt partitions' bounds (so they land in the scaffolding DEFAULT + partition during the copy), those rows must not be silently discarded + when the scaffolding partition would otherwise be dropped. """ import json @@ -54,6 +58,13 @@ FOR VALUES FROM (1) TO (10); CREATE TABLE {0}.part_with_default_def PARTITION OF {0}.part_with_default DEFAULT; + +CREATE TABLE {0}.part_stray_rows ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_stray_rows_p1 PARTITION OF {0}.part_stray_rows + FOR VALUES FROM (1) TO (10); """ # The target starts out with different partition bounds so that Schema Diff @@ -70,6 +81,8 @@ CREATE TABLE {0}.part_no_default_p1 PARTITION OF {0}.part_no_default FOR VALUES FROM (1) TO (5); +INSERT INTO {0}.part_no_default_p1 VALUES (2), (3); + CREATE TABLE {0}.part_with_default ( col1 integer NOT NULL ) PARTITION BY RANGE (col1); @@ -78,6 +91,29 @@ FOR VALUES FROM (1) TO (5); CREATE TABLE {0}.part_with_default_def PARTITION OF {0}.part_with_default DEFAULT; + +-- col1=2 fits the regular partition both before and after the rebuild; +-- col1=50 fits neither the old nor the new bounds of the regular +-- partition, so it must land (and stay) in the genuine DEFAULT partition +-- across the rebuild. +INSERT INTO {0}.part_with_default VALUES (2), (50); + +CREATE TABLE {0}.part_stray_rows ( + col1 integer NOT NULL +) PARTITION BY RANGE (col1); + +CREATE TABLE {0}.part_stray_rows_p1 PARTITION OF {0}.part_stray_rows + FOR VALUES FROM (1) TO (5); +CREATE TABLE {0}.part_stray_rows_def PARTITION OF {0}.part_stray_rows + DEFAULT; + +-- col1=2 fits the rebuilt regular partition's new bounds; col1=500 fits +-- neither the old nor the new bounds of any regular partition, so it is +-- only reachable via a DEFAULT partition. The source table has no +-- DEFAULT partition of its own, so this row can only survive the +-- rebuild if the scaffolding DEFAULT partition is kept instead of being +-- unconditionally dropped once it holds data. +INSERT INTO {0}.part_stray_rows VALUES (2), (500); """ @@ -160,6 +196,14 @@ def count_default_partitions(self, db_name, table_name): "AND pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT'".format( table_name, SCHEMA_NAME)) + def count_rows(self, db_name, table_name): + """ + Count how many rows the given table holds in the given database. + """ + return self.fetch_scalar( + db_name, + "SELECT count(*) FROM {0}.{1}".format(SCHEMA_NAME, table_name)) + def count_partitions(self, db_name, table_name): """ Count how many partitions the given partitioned table has in the @@ -252,6 +296,12 @@ def runTest(self): '{0}'.format(diff_ddl)) self.assertEqual( self.count_partitions(self.tar_database, 'part_no_default'), 2) + # The rows that were copied via the scaffolding default partition + # must still be there once it is dropped. + self.assertEqual( + self.count_rows(self.tar_database, 'part_no_default'), 2, + 'Schema Diff lost rows while rebuilding a table whose source ' + 'has no default partition: {0}'.format(diff_ddl)) # --- Source has a genuine default partition. --- with_default = self.find_object(response_data, 'table', @@ -271,8 +321,48 @@ def runTest(self): self.assertEqual( self.count_partitions(self.tar_database, 'part_with_default'), 2) + # The row that fits the regular partition and the row that only + # ever fit the DEFAULT partition must both survive the rebuild. + self.assertEqual( + self.count_rows(self.tar_database, 'part_with_default'), 2, + 'Schema Diff lost rows while rebuilding a table whose source ' + 'has a genuine default partition: {0}'.format(diff_ddl)) + + # --- Target has rows outside every rebuilt partition's bounds. --- + # The source table has no default partition of its own, so a + # scaffolding one is created purely to let the row-copy INSERT + # succeed. col1=500 in the target doesn't fit any real partition + # in the rebuilt scheme, so it can only be copied via that + # scaffolding partition; it must not be lost when the scaffolding + # partition is cleaned up. + stray_rows = self.find_object(response_data, 'table', + 'part_stray_rows') + self.assertEqual(stray_rows['status'], 'Different') + diff_ddl = stray_rows['diff_ddl'] - # Re-comparing must now report both tables as identical. + self.execute_sql(self.tar_database, diff_ddl) + self.assertEqual( + self.count_rows(self.tar_database, 'part_stray_rows'), 2, + 'Schema Diff silently dropped a row that fell outside every ' + 'rebuilt partition\'s bounds: {0}'.format(diff_ddl)) + self.assertEqual( + self.fetch_scalar( + self.tar_database, + "SELECT count(*) FROM {0}.part_stray_rows " + "WHERE col1 = 500".format(SCHEMA_NAME)), 1, + 'Schema Diff dropped the out-of-bounds row instead of routing ' + 'it to a durable partition: {0}'.format(diff_ddl)) + self.assertEqual( + self.count_default_partitions(self.tar_database, + 'part_stray_rows'), 1, + 'Schema Diff dropped the scaffolding default partition even ' + 'though it still held an out-of-bounds row: {0}'.format( + diff_ddl)) + self.assertEqual( + self.count_partitions(self.tar_database, 'part_stray_rows'), 2) + + # Re-comparing must now report both bounds-only tables as + # identical. response_data = self.compare() self.assertEqual( self.find_object(response_data, 'table', @@ -280,6 +370,13 @@ def runTest(self): self.assertEqual( self.find_object(response_data, 'table', 'part_with_default')['status'], 'Identical') + # part_stray_rows legitimately still differs from its source: the + # target kept a default partition (holding the recovered + # out-of-bounds row) that the source doesn't have. Preserving the + # data takes priority over reporting a false "Identical". + self.assertEqual( + self.find_object(response_data, 'table', + 'part_stray_rows')['status'], 'Different') def tearDown(self): """This function drops the added databases"""