Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nwTeM-0001OV-4H for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Jun 2022 19:04:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nwTeK-0007Xf-RZ for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Jun 2022 19:04:56 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nwTeK-0007XV-GP for pgsql-hackers@lists.postgresql.org; Wed, 01 Jun 2022 19:04:56 +0000 Received: from mail-lf1-x12e.google.com ([2a00:1450:4864:20::12e]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1nwTeH-0007NQ-6i for pgsql-hackers@lists.postgresql.org; Wed, 01 Jun 2022 19:04:55 +0000 Received: by mail-lf1-x12e.google.com with SMTP id c19so4296276lfv.5 for ; Wed, 01 Jun 2022 12:04:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yugabyte.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5ZTuuk3siCJNHOFWadkvrAeYqX/Mwk+KgqFofQ57YBE=; b=OeOx9cFk8fUGfC42N+RRLEVYR5ysERxyGHMHtvkvsi84HkdmeWzHhdzFjelk4jVuPc j5O8F62nSsgLxT8HxTGmnt7yAchVeUHDmE8mzNs5j5DvU6DuatFQZoAx5ANcqyx6LEXZ wHtcPoX3ImXJTF59WptTezSNl1Pl4Bh+w/gic= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5ZTuuk3siCJNHOFWadkvrAeYqX/Mwk+KgqFofQ57YBE=; b=x1S60sLLaIydrSiUhQwvVBa+79QVshSZgMbs+YOK7ikjk+xzsSqhBDKsKuLhtB9Q8V aXE0Y31sJtpokbjEvDfG5gbbkkphHgxFTv2/zqJ7vAmmYUBIkWf5y+D4qB8KQgzbOII8 nnJfwgUriU1MdNn+ngWgWhaw6K5ykbabpKqoVbnB/sLjtGYsbeGXOuMzLDBGcHceA93e TGIPi8encs7MaGWFNZUpzZuxQCpXQ8ZisnRMUOXCTY5EHkA4i9gIEAKlw8EkfiY+ULud lHKzJ40wHsmE/jEXNXoGCLr7QSDil+7xuAmitr2BYywtxkHIonKo0EOe4LKNTQuGksu/ 11dA== X-Gm-Message-State: AOAM5312zuEq/8PwyV+KbTdq+SmX1ddUd0fYwq0ylyggipuYB/e6LTIT dVMXdLRaAxOcoAFT0KkSyeC1/m3xBlS2CoJTCLgB6w== X-Google-Smtp-Source: ABdhPJwa4FFsBnoRpqrzkmOjBl8pMBHVJ4yqhOruGpH+7Lyl3vS42tGbsqUonm1AdF6twFNp1wGxtUwTpfY0lhISNJw= X-Received: by 2002:a05:6512:1191:b0:473:c7c3:f6ff with SMTP id g17-20020a056512119100b00473c7c3f6ffmr650442lfr.39.1654110292093; Wed, 01 Jun 2022 12:04:52 -0700 (PDT) MIME-Version: 1.0 References: <163714495450.16056.3566953095730878367@malur.postgresql.org> <28bcc918-ec5d-5287-963f-86c678db9ae0@postgrespro.ru> <81aa663e-9785-5fc1-f711-10626cdeb365@postgrespro.ru> In-Reply-To: <81aa663e-9785-5fc1-f711-10626cdeb365@postgrespro.ru> From: Zhihong Yu Date: Wed, 1 Jun 2022 12:10:22 -0700 Message-ID: Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands To: Dmitry Koval Cc: Matthias van de Meent , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="000000000000614b1805e0679026" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000614b1805e0679026 Content-Type: text/plain; charset="UTF-8" On Wed, Jun 1, 2022 at 11:58 AM Dmitry Koval wrote: > Hi, > > 1) > > 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 > > Thanks, changed. > > 2) > > For transformPartitionCmdForMerge(), nested loop is used to detect > duplicate names. > > If the number of partitions in partcmd->partlist, we should utilize map > to speed up the check. > > I'm not sure what we should utilize map in this case because chance that > number of merging partitions exceed dozens is low. > Is there a function example that uses a map for such a small number of > elements? > > 3) > > For check_parent_values_in_new_partitions(): > > > > + if (!find_value_in_new_partitions(&key->partsupfunc[0], > > + key->partcollation, parts, > nparts, datum, false)) > > + found = false; > > > > It seems we can break out of the loop when found is false. > > We have implicit "break" in "for" construction: > > + for (i = 0; i < boundinfo->ndatums && found; i++) > > I'll change it to explicit "break;" to avoid confusion. > > > Attached patch with the changes described above. > -- > With best regards, > Dmitry Koval > > Postgres Professional: http://postgrespro.com Hi, Thanks for your response. w.r.t. #2, I think using nested loop is fine for now. If, when this feature is merged, some user comes up with long merge list, we can revisit this topic. Cheers --000000000000614b1805e0679026 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Jun 1, 2022 at 11:58 AM Dmitr= y Koval <d.koval@postgrespro.r= u> wrote:
Hi,

1)
> For attachPartTable, the parameter wqueue is missing from comment.
> The parameters of CloneRowTriggersToPartition are called parent and pa= rtition.
> I think it is better to name the parameters to attachPartTable in a si= milar manner.
>
> For struct SplitPartContext, SplitPartitionContext would be better nam= e.
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Store partition contect into list. */ > contect -> context

Thanks, changed.

2)
> For transformPartitionCmdForMerge(), nested loop is used to detect dup= licate names.
> If the number of partitions in partcmd->partlist, we should utilize= map to speed up the check.

I'm not sure what we should utilize map in this case because chance tha= t
number of merging partitions exceed dozens is low.
Is there a function example that uses a map for such a small number of
elements?

3)
> For check_parent_values_in_new_partitions():
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!find_value_in_new_parti= tions(&key->partsupfunc[0],
> +=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=A0key->partcollation, parts, nparts, datum, false))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0found =3D fals= e;
>
> It seems we can break out of the loop when found is false.

We have implicit "break" in "for" construction:

+=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < boundinfo->ndatums &= ;& found; i++)

I'll change it to explicit "break;" to avoid confusion.


Attached patch with the changes described above.
--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com
Hi,
Thanks for your response.=C2=A0

w.r.t. #2, I th= ink using nested loop is fine for now.
If, when this feature is m= erged, some user comes up with long merge list, we can revisit this topic.<= /div>

Cheers
--000000000000614b1805e0679026--