From: Alexander Korotkov Date: Wed, 22 May 2024 23:21:00 +0000 (+0300) Subject: Fix the name collision detection in MERGE/SPLIT partition operations X-Git-Tag: REL_17_BETA2~123 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=3a82c689fd1be9bdf9d60135e5db7d352c051269;p=postgresql.git Fix the name collision detection in MERGE/SPLIT partition operations Both MERGE and SPLIT partition operations support the case when the name of the new partition matches the name of the existing partition to be merged/split. But the name collision detection doesn't always work as intended. The SPLIT partition operation finds the namespace to search for an existing partition without taking into account the parent's persistence. The MERGE partition operation checks for the name collision with simple equal() on RangeVar's simply ignoring the search_path. This commit fixes this behavior as follows. 1. The SPLIT partition operation now finds the namespace to search for an existing partition according to the parent's persistence. 2. The MERGE partition operation now checks for the name collision similarly to the SPLIT partition operation using RangeVarGetAndCheckCreationNamespace() and get_relname_relid(). Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/86b4f1e3-0b5d-315c-9225-19860d64d685%40gmail.com Author: Dmitry Koval, Alexander Korotkov --- diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 313c782cae2..7a063ca8ae0 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -20409,6 +20409,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, * against concurrent drop, and mark stmt->relation as * RELPERSISTENCE_TEMP if a temporary namespace is selected. */ + sps->name->relpersistence = rel->rd_rel->relpersistence; namespaceId = RangeVarGetAndCheckCreationNamespace(sps->name, NoLock, NULL); @@ -20601,6 +20602,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, ListCell *listptr; List *mergingPartitionsList = NIL; Oid defaultPartOid; + Oid namespaceId; + Oid existingRelid; /* * Lock all merged partitions, check them and create list with partitions @@ -20617,13 +20620,48 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, */ mergingPartition = table_openrv(name, AccessExclusiveLock); - /* - * Checking that two partitions have the same name was before, in - * function transformPartitionCmdForMerge(). - */ - if (equal(name, cmd->name)) + /* Store a next merging partition into the list. */ + mergingPartitionsList = lappend(mergingPartitionsList, + mergingPartition); + } + + /* + * Look up the namespace in which we are supposed to create the partition, + * check we have permission to create there, lock it against concurrent + * drop, and mark stmt->relation as RELPERSISTENCE_TEMP if a temporary + * namespace is selected. + */ + cmd->name->relpersistence = rel->rd_rel->relpersistence; + namespaceId = + RangeVarGetAndCheckCreationNamespace(cmd->name, NoLock, NULL); + + /* + * Check if this name is already taken. This helps us to detect the + * situation when one of the merging partitions has the same name as the + * new partition. Otherwise, this would fail later on anyway but catching + * this here allows us to emit a nicer error message. + */ + existingRelid = get_relname_relid(cmd->name->relname, namespaceId); + + if (OidIsValid(existingRelid)) + { + Relation sameNamePartition = NULL; + + foreach_ptr(RelationData, mergingPartition, mergingPartitionsList) { - /* One new partition can have the same name as merged partition. */ + if (RelationGetRelid(mergingPartition) == existingRelid) + { + sameNamePartition = mergingPartition; + break; + } + } + + if (sameNamePartition) + { + /* + * The new partition has the same name as one of merging + * partitions. + */ char tmpRelName[NAMEDATALEN]; /* Generate temporary name. */ @@ -20635,7 +20673,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, * in the future because we're going to eventually drop the * existing partition anyway. */ - RenameRelationInternal(RelationGetRelid(mergingPartition), + RenameRelationInternal(RelationGetRelid(sameNamePartition), tmpRelName, false, false); /* @@ -20644,10 +20682,12 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, */ CommandCounterIncrement(); } - - /* Store a next merging partition into the list. */ - mergingPartitionsList = lappend(mergingPartitionsList, - mergingPartition); + else + { + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_TABLE), + errmsg("relation \"%s\" already exists", cmd->name->relname))); + } } /* Detach all merged partitions. */ diff --git a/src/test/regress/expected/partition_merge.out b/src/test/regress/expected/partition_merge.out index 6361732d104..9c67a4a8b15 100644 --- a/src/test/regress/expected/partition_merge.out +++ b/src/test/regress/expected/partition_merge.out @@ -214,7 +214,8 @@ INSERT INTO sales_range VALUES (13, 'Gandi', 377, '2022-01-09'); INSERT INTO sales_range VALUES (14, 'Smith', 510, '2022-05-04'); -- Merge partitions (include DEFAULT partition) into partition with the same -- name -ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_mar2022, sales_others) INTO sales_others; +ALTER TABLE sales_range MERGE PARTITIONS + (sales_jan2022, sales_mar2022, partitions_merge_schema.sales_others) INTO sales_others; select * from sales_others; salesperson_id | salesperson_name | sales_amount | sales_date ----------------+------------------+--------------+------------ diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out index 14c4f97c9ff..d08eb4770ba 100644 --- a/src/test/regress/expected/partition_split.out +++ b/src/test/regress/expected/partition_split.out @@ -1547,6 +1547,14 @@ REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_alice; REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_bob; DROP ROLE regress_partition_split_alice; DROP ROLE regress_partition_split_bob; +-- Split partition of a temporary table when one of the partitions after +-- split has the same name as the partition being split +CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); +CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (2); +ALTER TABLE t SPLIT PARTITION tp_0 INTO + (PARTITION tp_0 FOR VALUES FROM (0) TO (1), + PARTITION tp_1 FOR VALUES FROM (1) TO (2)); +DROP TABLE t; RESET search_path; -- DROP SCHEMA partition_split_schema; diff --git a/src/test/regress/sql/partition_merge.sql b/src/test/regress/sql/partition_merge.sql index 5a741efa09b..56249732002 100644 --- a/src/test/regress/sql/partition_merge.sql +++ b/src/test/regress/sql/partition_merge.sql @@ -140,7 +140,8 @@ INSERT INTO sales_range VALUES (14, 'Smith', 510, '2022-05-04'); -- Merge partitions (include DEFAULT partition) into partition with the same -- name -ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_mar2022, sales_others) INTO sales_others; +ALTER TABLE sales_range MERGE PARTITIONS + (sales_jan2022, sales_mar2022, partitions_merge_schema.sales_others) INTO sales_others; select * from sales_others; diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql index 70d70499ec6..d9e2359cb76 100644 --- a/src/test/regress/sql/partition_split.sql +++ b/src/test/regress/sql/partition_split.sql @@ -931,6 +931,15 @@ REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_bob; DROP ROLE regress_partition_split_alice; DROP ROLE regress_partition_split_bob; +-- Split partition of a temporary table when one of the partitions after +-- split has the same name as the partition being split +CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); +CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (2); +ALTER TABLE t SPLIT PARTITION tp_0 INTO + (PARTITION tp_0 FOR VALUES FROM (0) TO (1), + PARTITION tp_1 FOR VALUES FROM (1) TO (2)); +DROP TABLE t; + RESET search_path; --