Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

> On Sep 17, 2025, at 06:08, Dmitry Koval <d(dot)koval(at)postgrespro(dot)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/