MIME-Version: 1.0
References: <163714495450.16056.3566953095730878367@malur.postgresql.org>
 <c73a1746-0cd0-6bdd-6b23-3ae0b7c0c582@postgrespro.ru>
 <CA+TgmoandAsE=Y=0UiYfP=Oyv7Fn2m717PWuPgFw8m9jQQbopg@mail.gmail.com>
 <9186127e-8f8f-cbd3-40de-b7bf01e57fb5@postgrespro.ru>
 <CALNJ-vTgp527=NiduNv5Q42pY3zaUnJjTDEf1-=JcX6sZyGFRA@mail.gmail.com>
In-Reply-To: 
 <CALNJ-vTgp527=NiduNv5Q42pY3zaUnJjTDEf1-=JcX6sZyGFRA@mail.gmail.com>
From: Zhihong Yu <zyu@yugabyte.com>
Date: Tue, 11 Oct 2022 10:15:05 -0700
Message-ID: 
 <CALNJ-vRee1RmHgzV=-_B=6AN3L_CCuAjdzej5XxhfHR6iT2K5w@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000038f6bb05eac56cb3"
Archived-At: 
 <https://www.postgresql.org/message-id/CALNJ-vRee1RmHgzV%3D-_B%3D6AN3L_CCuAjdzej5XxhfHR6iT2K5w%40mail.gmail.com>
Precedence: bulk

--00000000000038f6bb05eac56cb3
Content-Type: text/plain; charset="UTF-8"

On Tue, Oct 11, 2022 at 9:58 AM Zhihong Yu <zyu@yugabyte.com> wrote:

>
>
> On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval <d.koval@postgrespro.ru>
> wrote:
>
>> Hi!
>>
>> Fixed couple warnings (for cfbot).
>>
>> --
>> With best regards,
>> Dmitry Koval
>>
>> Postgres Professional: http://postgrespro.com
>
> Hi,
> For v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch:
>
> +       if (equal(name, cmd->name))
> +           /* One new partition can have the same name as merged
> partition. */
> +           isSameName = true;
>
> I think there should be a check before assigning true to isSameName - if
> isSameName is true, that means there are two partitions with this same name.
>
> Cheers
>

Pardon - I see that transformPartitionCmdForMerge() compares the partition
names.
Maybe you can add a comment in ATExecMergePartitions referring to
transformPartitionCmdForMerge() so that people can more easily understand
the logic.

--00000000000038f6bb05eac56cb3
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, Oct 11, 2022 at 9:58 AM Zhiho=
ng Yu &lt;<a href=3D"mailto:zyu@yugabyte.com">zyu@yugabyte.com</a>&gt; wrot=
e:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"l=
tr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote"><div dir=3D"l=
tr" class=3D"gmail_attr">On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval &lt;<=
a href=3D"mailto:d.koval@postgrespro.ru" target=3D"_blank">d.koval@postgres=
pro.ru</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"m=
argin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left=
:1ex">Hi!<br>
<br>
Fixed couple warnings (for cfbot).<br>
<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>Hi,</div><d=
iv>For=C2=A0v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch:</div=
><div><br></div>+ =C2=A0 =C2=A0 =C2=A0 if (equal(name, cmd-&gt;name))<br>+ =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* One new partition can have the same n=
ame as merged partition. */<br><div>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 is=
SameName =3D true;</div><div><br></div><div>I think there should be a check=
 before assigning true to isSameName - if isSameName is true, that means th=
ere are two partitions with this same name.</div><div><br></div><div>Cheers=
=C2=A0</div></div></div></blockquote><div><br></div><div>Pardon - I see tha=
t=C2=A0transformPartitionCmdForMerge() compares the partition names.</div><=
div>Maybe you can add a comment in=C2=A0ATExecMergePartitions referring to =
transformPartitionCmdForMerge() so that people can more easily understand t=
he logic.</div></div></div>

--00000000000038f6bb05eac56cb3--