Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

> On Sep 17, 2025, at 06:08, Dmitry Koval <d.koval@postgrespro.ru> wrote:
> 
> 
> Postgres Professional: http://postgrespro.com<v57-0001-Implement-ALTER-TABLE-.-MERGE-PARTITIONS-.-comma.patch><v57-0002-Implement-ALTER-TABLE-.-SPLIT-PARTITION-.-comman.patch>


1 - 0001
```
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -319,6 +319,56 @@ performDeletion(const ObjectAddress *object,
 	table_close(depRel, RowExclusiveLock);
 }
 
+/*
+ * performDeletionCheck: Check whether a specific object can be safely deleted.
+ * This function does not perform any deletion; instead, it raises an error
+ * if the object cannot be deleted due to existing dependencies.
+ *
+ * It can be useful when you need delete some objects later.  See comments in

+ * The behavior must specified as DROP_RESTRICT.


+	/*
+	 * Construct a list of objects we want delete later (ie, the given object
```

Nit: “when you need delete” => “when you need to delete"

“Must specified” => “must be specified"

“We want delete” => “we want to delete"

2 - 0001
```
+void
+performDeletionCheck(const ObjectAddress *object,
+					 DropBehavior behavior, int flags)
+{
+	Relation	depRel;
+	ObjectAddresses *targetObjects;
+
+	Assert(behavior == DROP_RESTRICT);
+
+	depRel = table_open(DependRelationId, RowExclusiveLock);
```

This function looks only performing read-only checks, why do we need RowExclusiveLock? Is AccessShareLock good enough?

3 - 0001
```
@@ -5272,6 +5278,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			/* No command-specific prep needed */
 			pass = AT_PASS_MISC;
 			break;
+		case AT_MergePartitions:
+			ATSimplePermissions(cmd->subtype, rel, ATT_PARTITIONED_TABLE);
+			/* No command-specific prep needed */
+			pass = AT_PASS_MISC;
+			break;
```

AT_MergePartitions, AT_DetachPartitionFinalize and AT_DetachPartition do the same thing, why don’t combine them together?

I see the previous code combine multiple cases if they do the same thing right in this switch:

        case AT_EnableRule:     /* ENABLE/DISABLE RULE variants */
        case AT_EnableAlwaysRule:
        case AT_EnableReplicaRule:
        case AT_DisableRule:
        case AT_AddOf:          /* OF */
        case AT_DropOf:         /* NOT OF */
        case AT_EnableRowSecurity:
        case AT_DisableRowSecurity:
        case AT_ForceRowSecurity:
        case AT_NoForceRowSecurity:
            ATSimplePermissions(cmd->subtype, rel,
                                ATT_TABLE | ATT_PARTITIONED_TABLE);
            /* These commands never recurse */
            /* No command-specific prep needed */
            pass = AT_PASS_MISC;
            break;

4 - 0001
```
+	/* Attach a new partition to the partitioned table. */
+	attachPartitionTable(wqueue, rel, attachrel, cmd->bound);
```

I think the comment can be removed as the function name has clearly described what it is doing.

5 - 0001
```
+static void
+attachPartitionTable(List **wqueue, Relation rel, Relation attachrel, PartitionBoundSpec *bound)
```

I think bound can be const: const PartitionBoundSpec *bound, to indicate read-only on bound.

Of course, you need to also update StorePartitionBound() to make bound also const, as StorePartitionBound() doesn’t update bound as well.

I tried to make them const in my local, and the build passed.

6 - 0001
```
+
+
+/*
+ * buildExpressionExecutionStates: build the needed expression execution states
```

Here seems an extra empty line is added.

6 - 0001
```
+			case CONSTR_CHECK:
+
+				/*
+				 * We already expanded virtual expression in
+				 * createTableConstraints.
+				 */
```

Nit: an unneeded empty line.

7 - 0001
```
+static void
+createTableConstraints(List **wqueue, AlteredTableInfo *tab,
+					   Relation parent_rel, Relation newRel)
+{
+	TupleDesc	tupleDesc;
+	TupleConstr *constr;
+	AttrMap    *attmap;
+	AttrNumber	parent_attno;
+	int			ccnum;
+	List	   *Constraints = NIL;
```

Why this local variable starts with a capital character “C”?

8 - 0001
```
+	/* Look up the access method for new relation. */
+	relamId = (parent_rel->rd_rel->relam != InvalidOid) ? parent_rel->rd_rel->relam : HEAP_TABLE_AM_OID;
```

In this function, “parent_rel->rd_rel” is used in many places, maybe we can cache it to a local variable.

9 - 0001
```
+		/* Create tuple slot for new partition. */
+		srcslot = table_slot_create(mergingPartition, NULL);
```

This comment is quite confusing. Can you rewording to something like:

```
/* Create a source tuple slot for the partition being merged. */
```

10 - 0001
```
+	/*
+	 * We don't need process this newPartRel since we already processed in
+	 * here, so delete the ALTER TABLE queue of it.
+	 */
+	foreach(ltab, *wqueue)
+	{
+		tab = (AlteredTableInfo *) lfirst(ltab);
+		if (tab->relid == RelationGetRelid(newPartRel))
+			*wqueue = list_delete_cell(*wqueue, ltab);
+	}
```

Based on the comment of “foreach”, deleting cell while interacting is unsafe.

And nit: “need process” => “need to process"

11 - 0001
```
+	/* Detach all merged partitions */
+	foreach_oid(mergingPartitionOid, mergingPartitions)
```

Should it be “all merging partitions”?

12 - 0001
```
+static void
+checkPartition(Relation rel, Oid partRelOid)
+{
+	Relation	partRel;
+
+	partRel = table_open(partRelOid, NoLock);
+
+	if (partRel->rd_rel->relkind != RELKIND_RELATION)
+		ereport(ERROR,
+				errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				errmsg("\"%s\" is not a table", RelationGetRelationName(partRel)),
+				errhint("ALTER TABLE ... MERGE PARTITIONS can only merge partitions don't have sub-partitions"));
+
+	if (!partRel->rd_rel->relispartition)
+		ereport(ERROR,
+				errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				errmsg("\"%s\" is not a partition of partitioned table \"%s\"",
+					   RelationGetRelationName(partRel), RelationGetRelationName(rel)),
+				errhint("ALTER TABLE ... MERGE PARTITIONS can only merge partitions don't have sub-partitions"));
```

I think the first two “if” can be combined. We are trying to check If “partRel” is a partition, when a relation is a partition, its relkind is “r” and “relispartition” is true. Instead, “xx is not a table” is a quite confusing message. I would suggest:

```
if (partRel->rd_rel->relkind != RELKIND_RELATION || !partRel->rd_rel->relispartition)
		ereport(ERROR,
				errcode(ERRCODE_WRONG_OBJECT_TYPE),
				errmsg("\"%s\" is not a partition of partitioned table \"%s\"",
					   RelationGetRelationName(partRel), RelationGetRelationName(rel)),
				errhint("ALTER TABLE ... MERGE PARTITIONS can only merge partitions don't have sub-partitions"));
```

13 - 0001
```
+static void
+checkPartition(Relation rel, Oid partRelOid)
+{
+	Relation	partRel;
+
+	partRel = table_open(partRelOid, NoLock);
```

We can immediately close the table, as data is stored in partRel already, we don’t have to defer table close.

14 - 0002
```
@@ -5283,6 +5290,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			/* No command-specific prep needed */
 			pass = AT_PASS_MISC;
 			break;
+		case AT_SplitPartition:
+			ATSimplePermissions(cmd->subtype, rel, ATT_PARTITIONED_TABLE);
+			/* No command-specific prep needed */
+			pass = AT_PASS_MISC;
+			break;
```

Same as comment 3.

14 - 0002
```
+	foreach(ltab, *wqueue)
+	{
+		AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
+
+		if (tab->relid == RelationGetRelid(pc->partRel))
+			*wqueue = list_delete_cell(*wqueue, ltab);
+	}
```

Same as comment 10.

15 - 0002
```
+	/* Scan through the rows. */
+	snapshot = RegisterSnapshot(GetLatestSnapshot());
+	scan = table_beginscan(splitRel, snapshot, 0, NULL);
+
+	/*
+	 * Switch to per-tuple memory context and reset it for each tuple
+	 * produced, so we don't leak memory.
+	 */
+	oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+	while (table_scan_getnextslot(scan, ForwardScanDirection, srcslot))
+	{
+		bool		found = false;
+		TupleTableSlot *insertslot;
```

For move rows, the logic of merge partitions and split partition are quite similar. Only difference is that merge partitions takes a fixed dest partition, but split partition use a logic to determine target partition. Maybe we can add a common function to reduce the duplicate code.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/