MIME-Version: 1.0
References: 
 <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <CACJufxGSwvUwu2gyZfTzv=3TJ8Zu4=Kq-GB_pg3x5_5dpwYQkA@mail.gmail.com>
 <da035c5a-bd58-45ea-938d-71857c981147@postgrespro.ru>
 <CACJufxHM0sD8opy2hUxXLcdY3CQOCaMfsQtJs7yF3TS2YxSqKg@mail.gmail.com>
 <195b67ee-ef41-4451-9396-844442eef1a4@postgrespro.ru>
 <CACJufxEWtK3utKjd7yMxRTGE54dJ8B+uigqvt1ocA4Q1mQ8qiQ@mail.gmail.com>
 <e2f434bf-daaa-42b5-a4f0-094e5b41de32@postgrespro.ru>
 <CAPpHfdsW5_rFLwTfkYuG42nX5RbJCVhaC2vkn8p5GsT8Lseuyw@mail.gmail.com>
 <a18a7555-ba1c-4dee-a05d-0a53cd858a89@postgrespro.ru>
 <CACJufxGEa6ZxP6d5VjZ6-Rpx=+u9zEE9ioRZRqQ=d_HYY6+daQ@mail.gmail.com>
 <CACJufxGh=L6YPr37Y4t+ZfReBeWdv8aiNEk7VoyhYyP7d6L-4w@mail.gmail.com>
 <CACJufxE4o-m9Eu0OX33aUgfTVT3p7b3u242-CCJaOpWUaFhS=A@mail.gmail.com>
 <CACJufxEH45Rtkn=n9hjakeKEkH7ou8domh+iQHMp+18Wq_L-4g@mail.gmail.com>
 <e5ecd88e-4087-49cc-8c72-26c6c01f760b@postgrespro.ru>
 <CACJufxGWCVf5r9kE-z6MyR2b+wkaU15Q5m2tKz4cvBhYX3-x1g@mail.gmail.com>
 <f1c96833-6d37-46ac-ae9b-6d48ec55dd4c@postgrespro.ru>
In-Reply-To: <f1c96833-6d37-46ac-ae9b-6d48ec55dd4c@postgrespro.ru>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Mon, 15 Sep 2025 11:03:54 +0300
Message-ID: 
 <CAPpHfdshkf0C2h09S00TVB8h3x8UT+GZyQ6LK2WGg1hOjnNE4g@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: jian he <jian.universality@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000009d9e41063ed2721e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAPpHfdshkf0C2h09S00TVB8h3x8UT%2BGZyQ6LK2WGg1hOjnNE4g%40mail.gmail.com>
Precedence: bulk

--0000000000009d9e41063ed2721e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Dmitry.

On Mon, Sep 1, 2025 at 2:04=E2=80=AFPM Dmitry Koval <d.koval@postgrespro.ru=
> wrote:
> Hi!
> Thank you for the notes and patch!

Some additional notes from me.

1) src/backend/parser/parse_utilcmd.c includes are not alphabetically
ordered here
+#include "partitioning/partdesc.h"
+#include "partitioning/partbounds.h"

2) There is unicode dash in the comment of ATExecMergePartitions() here.  I
suggest we should stick to ascii.

+   /*
+    * Check ownership of merged partitions =E2=80=94 partitions with diffe=
rent
+    * owners cannot be merged. Also, collect the OIDs of these partitions
+    * during the check.
+    */

3) Regarding 17bcf4f545, I see btnamecmp() is collation-aware.  Should we
also specify COLLATE "C" every time we do "ORDER BY relname"?

4) This comment sounds misleading.  Probably it should say "are contained".

+/*
+ * check_parent_values_in_new_partitions
+ *
+ * (function for BY LIST partitioning)
+ *
+ * Checks that all values of split partition (with Oid partOid) contains
in new
+ * partitions.
+ *

5) Given what latter items say, I think the 1. should say "The DEFAULT
partition must be at most one."

/*
 * check_partitions_for_split
 *
 * Checks new partitions for SPLIT PARTITIONS command:
 * 1. DEFAULT partition should be one.

6) Regarding the isolation tests.  I see we are exercising INSERTs and
intra-partition UPDATEs.  Should we also try some cross-partition UPDATEs?

------
Regards,
Alexander Korotkov
Supabase

--0000000000009d9e41063ed2721e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Dmitry.<br><br>On Mon, Sep 1, 2025 at =
2:04=E2=80=AFPM Dmitry Koval &lt;<a href=3D"mailto:d.koval@postgrespro.ru" =
target=3D"_blank">d.koval@postgrespro.ru</a>&gt; wrote:<br>&gt; Hi!<br>&gt;=
 Thank you for the notes and patch!<br><br>Some additional notes from me.<b=
r><br>1) src/backend/parser/parse_utilcmd.c includes are not alphabetically=
 ordered here<br><font face=3D"monospace">+#include &quot;partitioning/part=
desc.h&quot;<br>+#include &quot;partitioning/partbounds.h&quot;<br></font><=
br>2) There is unicode dash in the comment of=C2=A0ATExecMergePartitions() =
here.=C2=A0 I suggest we should stick to ascii.<br><font face=3D"monospace"=
><br></font><div><font face=3D"monospace">+ =C2=A0 /*<br>+ =C2=A0 =C2=A0* C=
heck ownership of merged partitions =E2=80=94 partitions with different<br>=
+ =C2=A0 =C2=A0* owners cannot be merged. Also, collect the OIDs of these p=
artitions<br>+ =C2=A0 =C2=A0* during the check.<br>+ =C2=A0 =C2=A0*/<br></f=
ont><br></div><div>3) Regarding=C2=A017bcf4f545, I see btnamecmp() is colla=
tion-aware.=C2=A0 Should we also specify COLLATE &quot;C&quot; every time w=
e do &quot;ORDER BY relname&quot;?<br><br></div><div>4) This comment sounds=
 misleading.=C2=A0 Probably it should say &quot;are contained&quot;.</div><=
div><br></div><div><font face=3D"monospace">+/*<br>+ * check_parent_values_=
in_new_partitions<br>+ *<br>+ * (function for BY LIST partitioning)<br>+ *<=
br>+ * Checks that all values of split partition (with Oid partOid) contain=
s in new<br>+ * partitions.<br>+ *<br></font><br>5) Given what latter items=
 say, I think the 1. should say &quot;The DEFAULT partition must be at most=
 one.&quot;<br><br><font face=3D"monospace">/*<br>=C2=A0* check_partitions_=
for_split<br>=C2=A0*<br>=C2=A0* Checks new partitions for SPLIT PARTITIONS =
command:<br>=C2=A0* 1. DEFAULT partition should be one.<br></font><br>6) Re=
garding the isolation tests.=C2=A0 I see we are exercising INSERTs and intr=
a-partition UPDATEs.=C2=A0 Should we also try some cross-partition UPDATEs?=
</div><div><br></div><div>------<br>Regards,<br>Alexander Korotkov<br>Supab=
ase</div></div>
</div>

--0000000000009d9e41063ed2721e--