MIME-Version: 1.0
References: 
 <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <CACJufxE6mU7Za2X5Z4Cs9jEmgfjTYPy_ChiVXND5ZEZ3xM665g@mail.gmail.com>
 <7e1c67b8-ceba-4a04-964a-853eedeb02cc@postgrespro.ru>
 <CACJufxHakPGgUHfe4uThY2u_Hzq3YOP9m4B3gjZUTUm2mbVryA@mail.gmail.com>
 <CACJufxHo0u=PzgCn88VZEW0mtB=JopchqBTESudUMH+HCyiTpg@mail.gmail.com>
 <0b3907b4-83b9-4873-9feb-2b60b29cda33@postgrespro.ru>
 <CACJufxFTiAiN_27FzAE60Hm5yQ124EJ+b1dpNDstcSdCApFzLw@mail.gmail.com>
 <4f19550b-eb41-4649-8bf5-566dc020a720@postgrespro.ru>
 <CACJufxHjpdeDtOsmz9TyGX7jFU73LBEjX=JQpRQdAvDadT0coA@mail.gmail.com>
 <df72c242-a099-4d11-b02e-8ee8474c0455@postgrespro.ru>
 <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>
In-Reply-To: <e2f434bf-daaa-42b5-a4f0-094e5b41de32@postgrespro.ru>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Tue, 29 Jul 2025 12:38:22 +0300
Message-ID: 
 <CAPpHfdsW5_rFLwTfkYuG42nX5RbJCVhaC2vkn8p5GsT8Lseuyw@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="00000000000006c970063b0e2ced"
Archived-At: 
 <https://www.postgresql.org/message-id/CAPpHfdsW5_rFLwTfkYuG42nX5RbJCVhaC2vkn8p5GsT8Lseuyw%40mail.gmail.com>
Precedence: bulk

--00000000000006c970063b0e2ced
Content-Type: text/plain; charset="UTF-8"

Hi, Dmitry!

I went through the patches. Both of them applied with a small conflict in
the parallel_schedule, which is easy to resolve.

+               ExecClearTuple(insertslot);
+
+               memcpy(insertslot->tts_values, srcslot->tts_values,
+                      sizeof(Datum) * srcslot->tts_nvalid);
+               memcpy(insertslot->tts_isnull, srcslot->tts_isnull,
+                      sizeof(bool) * srcslot->tts_nvalid);
+
+               ExecStoreVirtualTuple(insertslot);

Similar fragments are present in SplitPartitionMoveRows()
and MergePartitionsMoveRows().  Do you think we could use ExecCopySlot()
(or similar) instead?

The presence of the same name in the split partition list is checked with
equal() (similar concern was already raised about the merge case [1]).  If
one of the names is schema-qualified, the error message is different.
Could we make them the same?

# alter table part_test split partition part_test_1_2 into (partition
part_test_1 for values in (1), partition part_test_1 for values in (2));
ERROR:  name "part_test_1" is already used
LINE 1: ...artition part_test_1 for values in (1), partition part_test_...
                                                             ^
Time: 1.194 ms
# alter table part_test split partition part_test_1_2 into (partition
part_test_1 for values in (1), partition public.part_test_1 for values in
(2));
ERROR:  relation "part_test_1" already exists
Time: 6.187 ms

+       /* Search partition for current slot srcslot. */
+       foreach(listptr, partContexts)
+       {
+           pc = (SplitPartitionContext *) lfirst(listptr);
+
+           /* skip DEFAULT partition */
+           if (pc->partqualstate && ExecCheck(pc->partqualstate, econtext))
+           {
+               found = true;
+               break;
+           }
+       }

I see we're searching for a partition to place each row using the
sequential application of partition constraints.  I concerned if this could
be exhausting when the number of new partitions is large.  Could we use
something like binary search here?

New split/drop commands do the full reorganization of the involved
partitions.  As Robert previously stated [2], there are other possible
strategies.  While they are hard to implement, I don't think we need them
in the initial version.  But I think it's worth mentioning in the docs that
we're completely rewriting the involved partitions.  And this is not
recommended to use for merging very big partitions with small ones, and
splitting a small fraction of rows out of a very big partition.

Both patches could use pgindent run.

Links
1.
https://www.postgresql.org/message-id/CACJufxHHnJm6Jb2YQpuRU1RX__tO%3DJJNJ5%3DEUMuzif_KNxGd9A%40mail.gmail.com
2.
https://www.postgresql.org/message-id/CA%2BTgmoY0%3DbT_xBP8csR%3DMFE%3DFxGE2n2-me2-31jBOgEcLvW7ug%40mail.gmail.com

------
Regards,
Alexander Korotkov
Supabase

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

<div dir=3D"ltr">Hi, Dmitry!<br><br>I went through the patches. Both of the=
m applied with a small conflict in the parallel_schedule, which is easy to =
resolve.<br><br><font face=3D"monospace">+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 ExecClearTuple(insertslot);<br>+<br>+ =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 memcpy(insertslot-&gt;tts_values, srcslot-&=
gt;tts_values,<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0sizeof(Datum) * srcslot-&gt;tts_nvalid);<br>+ =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 memcpy(insertslot-&gt;tts_isnull,=
 srcslot-&gt;tts_isnull,<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sizeof(bool) * srcslot-&gt;tts_nvalid);<br>+=
<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ExecStoreVirtualTupl=
e(insertslot);<br></font><br>Similar fragments are present in SplitPartitio=
nMoveRows() and=C2=A0MergePartitionsMoveRows().=C2=A0 Do you think we could=
 use=C2=A0ExecCopySlot() (or similar) instead?<div><br></div><div>The prese=
nce of the same name in the split partition list is checked with equal() (s=
imilar concern was already raised about the merge case [1]).=C2=A0 If one o=
f the names is schema-qualified, the error message is different.=C2=A0 Coul=
d we make them the same?=C2=A0=C2=A0</div><div><br></div><div># alter table=
 part_test split partition part_test_1_2 into (partition part_test_1 for va=
lues in (1), partition part_test_1 for values in (2));<br>ERROR: =C2=A0name=
 &quot;part_test_1&quot; is already used<br>LINE 1: ...artition part_test_1=
 for values in (1), partition part_test_...<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^<br>Time: 1.194 ms<br># alter tab=
le part_test split partition part_test_1_2 into (partition part_test_1 for =
values in (1), partition public.part_test_1 for values in (2));<br>ERROR: =
=C2=A0relation &quot;part_test_1&quot; already exists<br>Time: 6.187 ms<br>=
<br><font face=3D"monospace">+ =C2=A0 =C2=A0 =C2=A0 /* Search partition for=
 current slot srcslot. */<br>+ =C2=A0 =C2=A0 =C2=A0 foreach(listptr, partCo=
ntexts)<br>+ =C2=A0 =C2=A0 =C2=A0 {<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 pc =3D (SplitPartitionContext *) lfirst(listptr);<br>+<br>+ =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 /* skip DEFAULT partition */<br>+ =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 if (pc-&gt;partqualstate &amp;&amp; ExecCheck(pc-&gt;partqua=
lstate, econtext))<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {<br>+ =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 found =3D true;<br>+ =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 }<br>+ =C2=A0 =C2=A0 =C2=A0 }<br></font><br>I see we&#39;re sear=
ching for a partition to place each row using the sequential application=C2=
=A0of partition constraints.=C2=A0 I concerned if this could be exhausting =
when the number of new partitions is large.=C2=A0 Could we use something li=
ke binary search here?</div><div><br></div><div>New split/drop commands do =
the full reorganization of the involved partitions.=C2=A0 As Robert previou=
sly stated [2], there are other possible strategies.=C2=A0 While they are h=
ard to implement, I don&#39;t think we need them in the initial version.=C2=
=A0 But I think it&#39;s worth mentioning=C2=A0in the docs that we&#39;re c=
ompletely rewriting the involved partitions.=C2=A0 And this is not recommen=
ded to use for merging very big partitions with small ones, and splitting a=
 small fraction of rows out of a=C2=A0very big partition.</div><div><br></d=
iv><div>Both patches could use pgindent run.</div><div><br>Links</div><div>=
1.=C2=A0<a href=3D"https://www.postgresql.org/message-id/CACJufxHHnJm6Jb2YQ=
puRU1RX__tO%3DJJNJ5%3DEUMuzif_KNxGd9A%40mail.gmail.com">https://www.postgre=
sql.org/message-id/CACJufxHHnJm6Jb2YQpuRU1RX__tO%3DJJNJ5%3DEUMuzif_KNxGd9A%=
40mail.gmail.com</a></div><div>2.=C2=A0<a href=3D"https://www.postgresql.or=
g/message-id/CA%2BTgmoY0%3DbT_xBP8csR%3DMFE%3DFxGE2n2-me2-31jBOgEcLvW7ug%40=
mail.gmail.com">https://www.postgresql.org/message-id/CA%2BTgmoY0%3DbT_xBP8=
csR%3DMFE%3DFxGE2n2-me2-31jBOgEcLvW7ug%40mail.gmail.com</a></div><div><br>-=
-----<br>Regards,<br>Alexander Korotkov<br>Supabase</div></div>

--00000000000006c970063b0e2ced--