Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uymni-00CjDn-EJ for pgsql-hackers@arkaria.postgresql.org; Wed, 17 Sep 2025 07:42:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uymng-005PMM-Af for pgsql-hackers@arkaria.postgresql.org; Wed, 17 Sep 2025 07:42:01 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uymnf-005PME-PA for pgsql-hackers@lists.postgresql.org; Wed, 17 Sep 2025 07:42:00 +0000 Received: from mail-pl1-x631.google.com ([2607:f8b0:4864:20::631]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uymnd-000rUP-2L for pgsql-hackers@lists.postgresql.org; Wed, 17 Sep 2025 07:41:59 +0000 Received: by mail-pl1-x631.google.com with SMTP id d9443c01a7336-2445824dc27so58157885ad.3 for ; Wed, 17 Sep 2025 00:41:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1758094917; x=1758699717; darn=lists.postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=D66lk92P2VTImoaDNjymLCDEvzCZHyB2Kox4U2mHSDE=; b=DY4v5Qgz1BLMMKCkOxI8QwOlJgHnl60H3b6dllF0rvrH8kHe+j0yCtoTgeC3oxWzaM ttxD2HOtXwSEIp7jeZ+pvy2gavWQKuWyt3jcBeNkMPTg3gxfUQkF9CXPfhETrc3hUZp9 a+CGq+KHaiIdkNRepsmKTeiTllvecp2u7IbEhA1VckLl0fMQAwn+gjeUuq1vQ7OqCjgL Yv+kqymkG4Mm0IBVYWbYt4f77wt+N7rfmUOv2Fcf/02YSE36BPgv1/RTgZwdWelr27sl KK3MlrpG/BDTOWgjetB6eE75oSgvHmTRTRcR9sW8n2A9cwvXdUWNQiCm0InhVqH4eCEb XLbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1758094917; x=1758699717; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=D66lk92P2VTImoaDNjymLCDEvzCZHyB2Kox4U2mHSDE=; b=hCwHmcLkbOHeTBxcnikLMNd2laE/djfgFdJIoLV3QRDBPP2mS03qR2HMhSaOtAjQ62 uJtNE86JC/A9ndh725mx7tHdUbcbpOtKGdhpzvQVuFMNleuLXj/zbALDF2UWn3EfrsF8 dPbbuqmJAFpfEffpWkiXSTdendsDg6v0YBYbhs+VU6C84AUXsrn3CtdbVwHnoJPZxh8H 1zX3izmVaWyakG7Nl64vJWhg3gKL9CyDKkthDTb02+PGlbHkt3BNk6J0mY4nLL2lQjKY FKIKC3XeBNIs0VAm5Wbc//wUN9Ow7LN6gObCzHBhGfZrbp3YSXbfSFdSdPUuctl5fSW5 r83A== X-Forwarded-Encrypted: i=1; AJvYcCVst4wZchudIVeOimoUsdAHpXOBTyaycT7DN6rgA0gAm4GGkJwGpnidAj5T8l7YdGKBtaONGbcQArAyKqUU@lists.postgresql.org X-Gm-Message-State: AOJu0Yzdv7+YW7yaiZ/LvC0mGcbQNvdc3JNWsydmDdjuUsqLqM1k7QoH TdFV9JTfH2Pvl7H2WF7K/0BoK+dH6mB5F+mWL8nTv0kE24aPgl/7cYbe X-Gm-Gg: ASbGncvtdiU7C3yRY8kWwKsZk6GsCxKweePsgVzeD4YKaJaiQRVBai9f27mnz4Sonvf ZmIxgcg6H/szp9ZYbY+YNVdPGZmwXw7VRm9HwKbtP2+TZuZatJcGv6w25Rjja0sWnXrG94KpQJL Rt2vzOMyyj7wiQanMyccnnT5S0UxXBuCO5nfHhRtu4Db6neEVioLvGrf5hgUI/WWgKcXxWqwPB6 J2wkU7SlwtpDOIkyaqevOSEb7WOxaCyOwL5nkm01YCUGN9uNFEZg+36xZvcQ+na/HfSw7S2WMyp QBseeYQy5f6lOCvlmeArrNfbwlX0C0JwJq4WyNnY3SBn3kt8030PwgtiYyJLlSMXdGS0bhROfeS o72JI282oSBHyQnsPaNXzuut4sc5h/1Ww X-Google-Smtp-Source: AGHT+IFmp3jbMh/fYRagkhwVEWjP9g/ZR6iaS/C2gK46rqMbqnfrReryPeimTNCprWVTRBPNq0xvXw== X-Received: by 2002:a17:903:298b:b0:25c:76f1:b024 with SMTP id d9443c01a7336-268123806aemr14791305ad.25.1758094916755; Wed, 17 Sep 2025 00:41:56 -0700 (PDT) Received: from smtpclient.apple ([142.171.105.12]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2651d2df15esm91829215ad.45.2025.09.17.00.41.54 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 17 Sep 2025 00:41:56 -0700 (PDT) From: Chao Li Message-Id: <54414932-9E82-47AA-902B-2DED18F07F64@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_A9D5D231-9DFE-4EB6-BE0D-1F1632977337" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands Date: Wed, 17 Sep 2025 15:41:40 +0800 In-Reply-To: <57a94d8a-d7b6-4c5e-9001-be1cd403b172@postgrespro.ru> Cc: jian he , pgsql-hackers@lists.postgresql.org To: Dmitry Koval References: <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org> <2643645f-2a02-43de-9321-c995ab1b7ef8@postgrespro.ru> <57a94d8a-d7b6-4c5e-9001-be1cd403b172@postgrespro.ru> X-Mailer: Apple Mail (2.3826.700.81) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_A9D5D231-9DFE-4EB6-BE0D-1F1632977337 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Sep 17, 2025, at 06:08, Dmitry Koval = wrote: >=20 >=20 > Postgres Professional: = http://postgrespro.com 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); } =20 +/* + * 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: =E2=80=9Cwhen you need delete=E2=80=9D =3D> =E2=80=9Cwhen you need = to delete" =E2=80=9CMust specified=E2=80=9D =3D> =E2=80=9Cmust be specified" =E2=80=9CWe want delete=E2=80=9D =3D> =E2=80=9Cwe want to delete" 2 - 0001 ``` +void +performDeletionCheck(const ObjectAddress *object, + DropBehavior behavior, int = flags) +{ + Relation depRel; + ObjectAddresses *targetObjects; + + Assert(behavior =3D=3D DROP_RESTRICT); + + depRel =3D 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 =3D AT_PASS_MISC; break; + case AT_MergePartitions: + ATSimplePermissions(cmd->subtype, rel, = ATT_PARTITIONED_TABLE); + /* No command-specific prep needed */ + pass =3D AT_PASS_MISC; + break; ``` AT_MergePartitions, AT_DetachPartitionFinalize and AT_DetachPartition do = the same thing, why don=E2=80=99t 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 =3D 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=E2=80=99t 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 =3D NIL; ``` Why this local variable starts with a capital character =E2=80=9CC=E2=80=9D= ? 8 - 0001 ``` + /* Look up the access method for new relation. */ + relamId =3D (parent_rel->rd_rel->relam !=3D InvalidOid) ? = parent_rel->rd_rel->relam : HEAP_TABLE_AM_OID; ``` In this function, =E2=80=9Cparent_rel->rd_rel=E2=80=9D is used in many = places, maybe we can cache it to a local variable. 9 - 0001 ``` + /* Create tuple slot for new partition. */ + srcslot =3D 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 =3D (AlteredTableInfo *) lfirst(ltab); + if (tab->relid =3D=3D RelationGetRelid(newPartRel)) + *wqueue =3D list_delete_cell(*wqueue, ltab); + } ``` Based on the comment of =E2=80=9Cforeach=E2=80=9D, deleting cell while = interacting is unsafe. And nit: =E2=80=9Cneed process=E2=80=9D =3D> =E2=80=9Cneed to process" 11 - 0001 ``` + /* Detach all merged partitions */ + foreach_oid(mergingPartitionOid, mergingPartitions) ``` Should it be =E2=80=9Call merging partitions=E2=80=9D? 12 - 0001 ``` +static void +checkPartition(Relation rel, Oid partRelOid) +{ + Relation partRel; + + partRel =3D table_open(partRelOid, NoLock); + + if (partRel->rd_rel->relkind !=3D 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 =E2=80=9Cif=E2=80=9D can be combined. We are = trying to check If =E2=80=9CpartRel=E2=80=9D is a partition, when a = relation is a partition, its relkind is =E2=80=9Cr=E2=80=9D and = =E2=80=9Crelispartition=E2=80=9D is true. Instead, =E2=80=9Cxx is not a = table=E2=80=9D is a quite confusing message. I would suggest: ``` if (partRel->rd_rel->relkind !=3D 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 =3D table_open(partRelOid, NoLock); ``` We can immediately close the table, as data is stored in partRel = already, we don=E2=80=99t have to defer table close. 14 - 0002 ``` @@ -5283,6 +5290,11 @@ ATPrepCmd(List **wqueue, Relation rel, = AlterTableCmd *cmd, /* No command-specific prep needed */ pass =3D AT_PASS_MISC; break; + case AT_SplitPartition: + ATSimplePermissions(cmd->subtype, rel, = ATT_PARTITIONED_TABLE); + /* No command-specific prep needed */ + pass =3D AT_PASS_MISC; + break; ``` Same as comment 3. 14 - 0002 ``` + foreach(ltab, *wqueue) + { + AlteredTableInfo *tab =3D (AlteredTableInfo *) = lfirst(ltab); + + if (tab->relid =3D=3D RelationGetRelid(pc->partRel)) + *wqueue =3D list_delete_cell(*wqueue, ltab); + } ``` Same as comment 10. 15 - 0002 ``` + /* Scan through the rows. */ + snapshot =3D RegisterSnapshot(GetLatestSnapshot()); + scan =3D 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 =3D = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); + + while (table_scan_getnextslot(scan, ForwardScanDirection, = srcslot)) + { + bool found =3D 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/ --Apple-Mail=_A9D5D231-9DFE-4EB6-BE0D-1F1632977337 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Sep 17, 2025, at 06:08, Dmitry Koval = <d.koval@postgrespro.ru> wrote:


Postgres = Professional: http://postgrespro.com<v57-0001-Implement-ALT= ER-TABLE-.-MERGE-PARTITIONS-.-comma.patch><v57-0002-Implement-ALT= ER-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);
 }
 
+/*
<= div>+ * 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: =E2=80=9Cwhen = you need delete=E2=80=9D =3D> =E2=80=9Cwhen you need to = delete"

=E2=80=9CMust specified=E2=80=9D =3D> = =E2=80=9Cmust be specified"

=E2=80=9CWe want = delete=E2=80=9D =3D> =E2=80=9Cwe want to = delete"

2 - = 0001
```
+void
+performDeletionCheck(const = ObjectAddress *object,
+ = DropBehavior behavior, int flags)
+{
+ = Relation = depRel;
+ ObjectAddresses = *targetObjects;
+
+ Assert(behavior =3D=3D = DROP_RESTRICT);
+
+ depRel =3D = 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 =3D AT_PASS_MISC;
  = break;
+ case = AT_MergePartitions:
+ = ATSimplePermissions(cmd->subtype, rel, = ATT_PARTITIONED_TABLE);
+ /* No = command-specific prep needed */
+ pass =3D = AT_PASS_MISC;
+ = break;
```

AT_MergePartiti= ons, AT_DetachPartitionFinalize and AT_DetachPartition do the same = thing, why don=E2=80=99t 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 =3D 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=E2=80=99t 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 =3D = NIL;
```

Why this local variable = starts with a capital character = =E2=80=9CC=E2=80=9D?

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

In this = function, =E2=80=9Cparent_rel->rd_rel=E2=80=9D is used in many = places, maybe we can cache it to a local = variable.

9 - = 0001
```
+ /* Create tuple slot for = new partition. */
+ srcslot =3D = 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 =3D (AlteredTableInfo *) lfirst(ltab);
+ = if (tab->relid =3D=3D = RelationGetRelid(newPartRel))
+ *wqueue =3D = list_delete_cell(*wqueue, ltab);
+ = }
```

Based on the comment of = =E2=80=9Cforeach=E2=80=9D, deleting cell while interacting is = unsafe.

And nit: =E2=80=9Cneed process=E2=80= =9D =3D> =E2=80=9Cneed to process"

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

Should it be = =E2=80=9Call merging partitions=E2=80=9D?

12 - = 0001
```
+static = void
+checkPartition(Relation rel, Oid = partRelOid)
+{
+ Relation = partRel;
+
+ partRel =3D = table_open(partRelOid, NoLock);
+
+ if = (partRel->rd_rel->relkind !=3D 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 =E2=80=9Cif=E2=80=9D can be combined. We are trying to check = If =E2=80=9CpartRel=E2=80=9D is a partition, when a relation is a = partition, its relkind is =E2=80=9Cr=E2=80=9D and =E2=80=9Crelispartition=E2= =80=9D is true. Instead, =E2=80=9Cxx is not a table=E2=80=9D is a quite = confusing message. I would = suggest:

```
if = (partRel->rd_rel->relkind !=3D 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 =3D = table_open(partRelOid, = NoLock);
```

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

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

Same as = comment 3.

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

Same as comment = 10.

15 - = 0002
```
+ /* Scan through the rows. = */
+ = snapshot =3D = RegisterSnapshot(GetLatestSnapshot());
+ scan =3D = 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 =3D = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
=
+ = while (table_scan_getnextslot(scan, ForwardScanDirection, = srcslot))
+ {
+ = bool = found =3D 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/




= --Apple-Mail=_A9D5D231-9DFE-4EB6-BE0D-1F1632977337--