MIME-Version: 1.0
References: <163714495450.16056.3566953095730878367@malur.postgresql.org>
 <c73a1746-0cd0-6bdd-6b23-3ae0b7c0c582@postgrespro.ru>
 <CAEze2WhU9Wkn2gD9FC6kbuh1gUdqCmdi9_d62-NeBkPZ_5=eOA@mail.gmail.com>
 <28bcc918-ec5d-5287-963f-86c678db9ae0@postgrespro.ru>
In-Reply-To: <28bcc918-ec5d-5287-963f-86c678db9ae0@postgrespro.ru>
From: Zhihong Yu <zyu@yugabyte.com>
Date: Tue, 31 May 2022 13:43:26 -0700
Message-ID: 
 <CALNJ-vSMinSB=BTPj4zthMAV3N_1t6vO2rVYBt4uiz3dif-_DA@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000006f147705e054bf56"
Archived-At: 
 <https://www.postgresql.org/message-id/CALNJ-vSMinSB%3DBTPj4zthMAV3N_1t6vO2rVYBt4uiz3dif-_DA%40mail.gmail.com>
Precedence: bulk

--0000000000006f147705e054bf56
Content-Type: text/plain; charset="UTF-8"

On Tue, May 31, 2022 at 12:43 PM Dmitry Koval <d.koval@postgrespro.ru>
wrote:

>  >Just out of curiosity, why is SPLIT / MERGE support not included for
>  >HASH partitions? Because sibling partitions can have a different
>  >modulus, you should be able to e.g. split a partition with (modulus,
>  >remainder) of (3, 1) into two partitions with (mod, rem) of (6, 1) and
>  >(6, 4) respectively, with the reverse being true for merge operations,
>  >right?
>
> You are right, SPLIT/MERGE operations can be added for HASH-partitioning
> in the future. But HASH-partitioning is rarer than RANGE- and
> LIST-partitioning and I decided to skip it in the first step.
> Maybe community will say that SPLIT/MERGE commands are not needed... (At
> first step I would like to make sure that it is no true)
>
> P.S. I attached patch with 1-line warning fix (for cfbot).
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com


Hi,
For attachPartTable, the parameter wqueue is missing from comment.
The parameters of CloneRowTriggersToPartition are called parent
and partition. I think it is better to name the parameters to
attachPartTable in a similar manner.

For struct SplitPartContext, SplitPartitionContext would be better name.

+       /* Store partition contect into list. */
contect -> context

Cheers

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Tue, May 31, 2022 at 12:43 PM Dmit=
ry Koval &lt;<a href=3D"mailto:d.koval@postgrespro.ru">d.koval@postgrespro.=
ru</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margi=
n:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex=
">=C2=A0&gt;Just out of curiosity, why is SPLIT / MERGE support not include=
d for<br>
=C2=A0&gt;HASH partitions? Because sibling partitions can have a different<=
br>
=C2=A0&gt;modulus, you should be able to e.g. split a partition with (modul=
us,<br>
=C2=A0&gt;remainder) of (3, 1) into two partitions with (mod, rem) of (6, 1=
) and<br>
=C2=A0&gt;(6, 4) respectively, with the reverse being true for merge operat=
ions,<br>
=C2=A0&gt;right?<br>
<br>
You are right, SPLIT/MERGE operations can be added for HASH-partitioning <b=
r>
in the future. But HASH-partitioning is rarer than RANGE- and <br>
LIST-partitioning and I decided to skip it in the first step.<br>
Maybe community will say that SPLIT/MERGE commands are not needed... (At <b=
r>
first step I would like to make sure that it is no true)<br>
<br>
P.S. I attached patch with 1-line warning fix (for cfbot).<br>
-- <br>
With best regards,<br>
Dmitry Koval<br>
<br>
Postgres Professional: <a href=3D"http://postgrespro.com" rel=3D"noreferrer=
" target=3D"_blank">http://postgrespro.com</a></blockquote><div><br></div><=
div>Hi,</div><div>For=C2=A0attachPartTable, the parameter=C2=A0wqueue is mi=
ssing from comment.</div><div>The parameters of=C2=A0CloneRowTriggersToPart=
ition are called=C2=A0parent and=C2=A0partition. I think it is better to na=
me the parameters to attachPartTable in a similar manner.<br></div><div><br=
></div><div>For struct SplitPartContext, SplitPartitionContext would be bet=
ter name.</div><div><br></div><div>+ =C2=A0 =C2=A0 =C2=A0 /* Store partitio=
n contect into list. */<br></div><div>contect -&gt;=C2=A0context<br></div><=
div><br></div><div>Cheers</div></div></div>

--0000000000006f147705e054bf56--