> On Sep 17, 2025, at 06:08, Dmitry Koval <[email protected]> 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/
