MIME-Version: 1.0
References: 
 <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmail.com>
 <CAPpHfdt_Apbzjx-q2moST3VyNq9Shx8ByGxTF-aqBs3Gaf_n-Q@mail.gmail.com>
 <CA+TgmoY0=bT_xBP8csR=MFE=FxGE2n2-me2-31jBOgEcLvW7ug@mail.gmail.com>
 <CAPpHfdtp3_H_Rp1ubVYGUnnF0fXbam7+OOG_EMnWos1x2a_3Zg@mail.gmail.com>
 <859476bf-3cb0-455e-b093-b8ab5ef17f0e@postgrespro.ru>
 <CA+TgmoZux8hzQ0=1-Rvigm9VuojW0aHiGSzL+G_g43qyrLGiSg@mail.gmail.com>
 <f56c654f-f152-4dc8-b221-4d4484bed612@postgrespro.ru>
 <CAPpHfdtXDuBaHscJn-wada6Kaf3fxmS4k-1TTVhpS+Kx=uveyw@mail.gmail.com>
 <4e2764f7-f6be-478f-8d7f-f921dbd10f5f@postgrespro.ru>
 <CAPpHfdv2V2tsvVn5LYyrJkzmPA==vbdCEKiT8j9yaaiDTfaQtA@mail.gmail.com>
 <ff30dc7e-ebca-42a2-9d49-a66c9a3c7ee5@postgrespro.ru>
 <CALDaNm3V5DQSbZGxhYH+3MMMm-YdgOg+RdJ-ni6D0TZx_=Xd7w@mail.gmail.com>
 <7fdaf3e8-4c05-4739-bee6-98acc068d1c7@postgrespro.ru>
 <CAPpHfduH83+CW1dBppFVjxauPFGC+-ux+Y7wB0koZJCs029SXA@mail.gmail.com>
 <4433b492-9e97-4334-96d9-ddaaa5083099@postgrespro.ru>
In-Reply-To: <4433b492-9e97-4334-96d9-ddaaa5083099@postgrespro.ru>
From: jian he <jian.universality@gmail.com>
Date: Wed, 4 Jun 2025 18:45:32 +0800
Message-ID: 
 <CACJufxEi_JVg+=_BSS0o+TYUE_Hq+jmga_yxxuKLSGbQiAnZDg@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxEi_JVg%2B%3D_BSS0o%2BTYUE_Hq%2Bjmga_yxxuKLSGbQiAnZDg%40mail.gmail.com>
Precedence: bulk

On Wed, Jun 4, 2025 at 4:53=E2=80=AFAM Dmitry Koval <d.koval@postgrespro.ru=
> wrote:
> Added some changes to documentation.
> Patches are attached to the email.
>

hi.
I haven't touched v39-0002 yet.
The following are reviews of v39-0001.

+CREATE INDEX sales_range_sales_date_idx ON sales_range USING btree
(sales_date);
+INSERT INTO sales_range VALUES (1,  'May',      1000, '2022-01-31');
+INSERT INTO sales_range VALUES (2,  'Smirnoff', 500,  '2022-02-10');
+INSERT INTO sales_range VALUES (3,  'Ford',     2000, '2022-04-30');
you can put it into one INSERT. like
INSERT INTO sales_range VALUES (1,  'May',      1000, '2022-01-31'),
(1,  'May',      1000, '2022-01-31');
which can make the regress test faster.
(apply the logic to other places in src/test/regress/sql/partition_merge.sq=
l)

+ key =3D RelationGetPartitionKey(parent);
+ strategy =3D get_partition_strategy(key);
+
+ if (strategy =3D=3D PARTITION_STRATEGY_HASH)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partition of hash-partitioned table cannot be merged")));
This error case doesn't seem to have a related test, and adding one
would be great.

per
https://www.postgresql.org/docs/current/error-message-reporting.html
"The extra parentheses were required before PostgreSQL version 12, but
are now optional."
so now you can remove the extra parentheses.
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partition of hash-partitioned table cannot be merged"));


+ if (!partRel->rd_rel->relispartition)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a partition",
+ RelationGetRelationName(partRel))));
+
+ if (get_partition_parent(partRelOid, false) !=3D RelationGetRelid(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("relation \"%s\" is not a partition of relation \"%s\"",
+ RelationGetRelationName(partRel),
+ RelationGetRelationName(rel))));
we can make the first error message like the second one.
errmsg("\"%s\" is not a partition of \"%s\"....)


+ case AT_MergePartitions:
+ {
+ PartitionCmd *partcmd =3D (PartitionCmd *) cmd->def;
+
+ if (list_length(partcmd->partlist) < 2)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("list of new partitions should contain at least two items")));
This also seems to have no tests.
adding a dummy one should be ok.


We added List *partlist into PartitionCmd
typedef struct PartitionCmd
{
    NodeTag        type;
    RangeVar   *name;            /* name of partition to attach/detach */
    PartitionBoundSpec *bound;    /* FOR VALUES, if attaching */
    List       *partlist;        /* list of partitions, for MERGE/SPLIT
                                 * PARTITION command */
    bool        concurrent;
} PartitionCmd;
then in src/backend/parser/gram.y
we should use
cmd->partlist =3D NIL;
instead of
cmd->partlist =3D NULL;
We also need comments explaining PartitionCmd.name
meaning for ALTER TABLE MERGE PARTITIONS INTO?


transformPartitionCmdForMerge
+ partOid =3D RangeVarGetRelid(name, NoLock, false);
here "NoLock" seems not right?
For example, after "RangeVarGetRelid(name, NoLock, false);"
before checkPartition, I drop the table test.t in another session.
i got the following error:
ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022, test.t) INTO sales_feb_mar_apr2022;
ERROR:  could not open relation with OID 18164