Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1s0Li6-003JnB-86 for pgsql-hackers@arkaria.postgresql.org; Fri, 26 Apr 2024 13:33:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1s0Li3-00Azl5-Gf for pgsql-hackers@arkaria.postgresql.org; Fri, 26 Apr 2024 13:33:52 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1s0Li3-00Azkx-5m for pgsql-hackers@lists.postgresql.org; Fri, 26 Apr 2024 13:33:52 +0000 Received: from mail-ej1-x629.google.com ([2a00:1450:4864:20::629]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1s0Lhz-000B8s-Co for pgsql-hackers@lists.postgresql.org; Fri, 26 Apr 2024 13:33:51 +0000 Received: by mail-ej1-x629.google.com with SMTP id a640c23a62f3a-a4702457ccbso294173766b.3 for ; Fri, 26 Apr 2024 06:33:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714138426; x=1714743226; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ZuBu4nY6LNIG+AQ18VuCc5jMgYsja9/MFa4R/Ybogyg=; b=lf1Ba1gl8inF5fX52vxUwxF0DmzPVAr+QpnrPa7H/X3if1GrbD/FOSD17dzPIs4kj/ YNRvMoOU+0aB8qWNQ1bpTzbdPS9Jx92z+RXovh2p2gGDd5G3dmhzVT9WugsnwGRO9sij kgO/AUgu+HCAi4kZ2oc4VEmiaspHNEYhJNhiPtg/KcIJExeoTbepQCqOIitQ+RA29sRH e4IJTU3H74+Wc93ZHZZywSFuU5nCzIqWtdwK27/vzR1qFtFqyUYvrDtGcAYTT+Udgy7E TRFrvfja3/U5V4Flz9KhOx6rUvWK66HE6Frodjf/wEM3KVZu19sCnKCuFKhUVhBimxy1 SjTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714138426; x=1714743226; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZuBu4nY6LNIG+AQ18VuCc5jMgYsja9/MFa4R/Ybogyg=; b=QS+Py0psKqVOaqKL8qDMgt5deyO4iVFv+kgDFQ70a4YLBFuVGAczv2/ofqou/pbSSV SorMCeqyK5MG9RqS1bzHsHp6fNXUE7PO7D9AhLiHgyA+5MjDtHgvJXNbAbAJB3WffOOn eer3NgV4JYaCpw1DikIsrkl+s1XNpFT6H6xX1eerQFcrUzQDY9nxWXG/5+LN17Te9q80 NBPBwAADFehUjfMM3iy2XHULdjzq2/iT/V0UjauOcqMDjzIXJYQzD4VdtX/4UlYAshIt TOK/wmCl1ds3LpdOiynqKlgy0/3knp3SovWpdj+UspKRfkvK9W30CYiqbe02+Wxp6Fe/ CVUw== X-Forwarded-Encrypted: i=1; AJvYcCU+MjVtLVRdtXuk5/1Yk73NnNIwW6sb4gzvL7ryotsj4XjozBxSFQfSeDBgkKEu+KLKGV/0umNZuTkK5uEYu3Zg1FhYwVHBOKfsUMsfcTAp95+E X-Gm-Message-State: AOJu0YzxZO99VuTnIr6GV61N15mwj3+5SXIGQYYDrxnQ6T4F93beW8Bh CLNtEKym+G4kgkRwOhvyCwiljR9c13xoFWSfbCGNSukRks7SIgybt4KrTk25V8A8I76Nwe7l3Lm 4HjU9ed5n/gzO280Zq/M4CHtBn7+xqbQNLXM= X-Google-Smtp-Source: AGHT+IGGbOB/8v5BGnKXJsFjkgg9q7k265jX2l6x3jgydQOtC7wBIPVKwLViXEuc98UlIirlOOgrgaa+GgSrkJEEIFA= X-Received: by 2002:a17:906:6894:b0:a52:6290:9219 with SMTP id n20-20020a170906689400b00a5262909219mr1848130ejr.38.1714138425947; Fri, 26 Apr 2024 06:33:45 -0700 (PDT) MIME-Version: 1.0 References: <56e0000c-6b39-44ce-90b4-d3c3062e408c@postgrespro.ru> <5dee3937-8e9f-cca4-11fb-737709a92b37@gmail.com> In-Reply-To: From: Pavel Borisov Date: Fri, 26 Apr 2024 17:33:33 +0400 Message-ID: Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands To: Justin Pryzby Cc: Alexander Korotkov , Dmitry Koval , Alexander Lakhin , Robert Haas , pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000f97c770616fff3b7" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000f97c770616fff3b7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Hackers! On Thu, 25 Apr 2024 at 00:26, Justin Pryzby wrote: > On Mon, Apr 22, 2024 at 01:31:48PM +0300, Alexander Korotkov wrote: > > Hi! > > > > On Fri, Apr 19, 2024 at 4:29=E2=80=AFPM Alexander Korotkov > wrote: > > > On Fri, Apr 19, 2024 at 2:26=E2=80=AFAM Dmitry Koval > wrote: > > > > 18.04.2024 19:00, Alexander Lakhin wrote: > > > > > leaves a strange constraint: > > > > > \d+ t* > > > > > Table "public.tp_0" > > > > > ... > > > > > Not-null constraints: > > > > > "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i" > > > > > > > > Thanks! > > > > Attached fix (with test) for this case. > > > > The patch should be applied after patches > > > > v6-0001- ... .patch ... v6-0004- ... .patch > > > > > > I've incorporated this fix with 0001 patch. > > > > > > Also added to the patchset > > > 005 =E2=80=93 tab completion by Dagfinn [1] > > > 006 =E2=80=93 draft fix for table AM issue spotted by Alexander Lakhi= n [2] > > > 007 =E2=80=93 doc review by Justin [3] > > > > > > I'm continuing work on this. > > > > > > Links > > > 1. > https://www.postgresql.org/message-id/87plumiox2.fsf%40wibble.ilmari.org > > > 2. > https://www.postgresql.org/message-id/84ada05b-be5c-473e-6d1c-ebe5dd21b19= 0%40gmail.com > > > 3. > https://www.postgresql.org/message-id/ZiGH0xc1lxJ71ZfB%40pryzbyj2023 > > > > 0001 > > The way we handle name collisions during MERGE PARTITIONS operation is > > reworked by integration of patch [3]. This makes note about commit in > > [2] not relevant. > > This patch also/already fixes the schema issue I reported. Thanks. > > If you wanted to include a test case for that: > > begin; > CREATE SCHEMA s; > CREATE SCHEMA t; > CREATE TABLE p(i int) PARTITION BY RANGE(i); > CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); > CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); > ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if > merging into the same name as an existing partition > \d+ p > ... > Partitions: c1 FOR VALUES FROM (1) TO (3) > > > 0002 > > The persistence of the new partition is copied as suggested in [1]. > > But the checks are in-place, because search_path could influence new > > table persistence. Per review [2], commit message typos are fixed, > > documentation is revised, revised tests to cover schema-qualification, > > usage of search_path. > > Subject: [PATCH v8 2/7] Make new partitions with parent's persistence > during MERGE/SPLIT operations > > This patch adds documentation saying: > + Any indexes, constraints and user-defined row-level triggers that > exist > + in the parent table are cloned on new partitions [...] > > Which is good to say, and addresses part of my message [0] > [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 > > But it doesn't have anything to do with "creating new partitions with > parent's persistence". Maybe there was a merge conflict and the docs > ended up in the wrong patch ? > > Also, defaults, storage options, compression are also copied. As will > be anything else from LIKE. And since anything added in the future will > also be copied, maybe it's better to just say that the tables will be > created the same way as "LIKE .. INCLUDING ALL EXCLUDING ..", or > similar. Otherwise, the next person who adds a new option for LIKE > would have to remember to update this paragraph... > > Also, extended stats objects are currently cloned to new child tables. > But I suggested in [0] that they probably shouldn't be. > > > 007 =E2=80=93 doc review by Justin [3] > > I suggest to drop this patch for now. I'll send some more minor fixes to > docs and code comments once the other patches are settled. > I've looked at the patchset: 0001 Look good. 0002 Also right with docs modification proposed by Justin. 0003: Looks like unused code 5268 datum =3D cmpval ? list_nth(spec->lowerdatums, abs(cmpval)= - 1) : NULL; overridden by 5278 datum =3D list_nth(spec->upperdatums, abs(cmpval) = - 1); and 5290 datum =3D list_nth(spec->upperdatums, abs(cmpval) = - 1); Otherwise - good. 0004: I suggest also getting rid of thee-noun compound words like: salesperson_name. Maybe salesperson -> clerk? Or maybe use the same terms like in pgbench: branches, tellers, accounts, balance. 0005: Good 0006: Patch is right In comments: + New partitions will have the same table access method, + same column names and types as the partitioned table to which they belong. (I'd suggest to remove second "same") Tests are passed. I suppose that it's better to add similar tests for SPLIT/MERGE PARTITION(S) to those covering ATTACH/DETACH PARTITION (e.g.: subscription/t/013_partition.pl and regression tests) Overall, great work! Thanks! Regards, Pavel Borisov, Supabase. --000000000000f97c770616fff3b7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi, Hackers!

On Thu, 25 Apr 2024 at 00:26,= Justin Pryzby <pryzby@telsasoft= .com> wrote:
On Mon, Apr 22, 2024 at 01:31:48PM +0300, Alexander Korotkov wrote:
> Hi!
>
> On Fri, Apr 19, 2024 at 4:29=E2=80=AFPM Alexander Korotkov <aekorotkov@gmail.com= > wrote:
> > On Fri, Apr 19, 2024 at 2:26=E2=80=AFAM Dmitry Koval <d.koval@postgrespro.ru= > wrote:
> > > 18.04.2024 19:00, Alexander Lakhin wrote:
> > > > leaves a strange constraint:
> > > > \d+ t*
> > > >=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 Table "public.tp_0"
> > > > ...
> > > > Not-null constraints:
> > > >=C2=A0 =C2=A0 =C2=A0 "merge-16385-26BCB0-tmp_i_not_= null" NOT NULL "i"
> > >
> > > Thanks!
> > > Attached fix (with test) for this case.
> > > The patch should be applied after patches
> > > v6-0001- ... .patch ... v6-0004- ... .patch
> >
> > I've incorporated this fix with 0001 patch.
> >
> > Also added to the patchset
> > 005 =E2=80=93 tab completion by Dagfinn [1]
> > 006 =E2=80=93 draft fix for table AM issue spotted by Alexander L= akhin [2]
> > 007 =E2=80=93 doc review by Justin [3]
> >
> > I'm continuing work on this.
> >
> > Links
> > 1. https://www.pos= tgresql.org/message-id/87plumiox2.fsf%40wibble.ilmari.org
> > 2. h= ttps://www.postgresql.org/message-id/84ada05b-be5c-473e-6d1c-ebe5dd21b190%4= 0gmail.com
> > 3. https://www.postgre= sql.org/message-id/ZiGH0xc1lxJ71ZfB%40pryzbyj2023
>
> 0001
> The way we handle name collisions during MERGE PARTITIONS operation is=
> reworked by integration of patch [3].=C2=A0 This makes note about comm= it in
> [2] not relevant.

This patch also/already fixes the schema issue I reported.=C2=A0 Thanks.
If you wanted to include a test case for that:

begin;
CREATE SCHEMA s;
CREATE SCHEMA t;
CREATE TABLE p(i int) PARTITION BY RANGE(i);
CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2);
CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3);
ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if mer= ging into the same name as an existing partition
\d+ p
...
Partitions: c1 FOR VALUES FROM (1) TO (3)

> 0002
> The persistence of the new partition is copied as suggested in [1]. > But the checks are in-place, because search_path could influence new > table persistence.=C2=A0 Per review [2], commit message typos are fixe= d,
> documentation is revised, revised tests to cover schema-qualification,=
> usage of search_path.

Subject: [PATCH v8 2/7] Make new partitions with parent's persistence d= uring MERGE/SPLIT operations

This patch adds documentation saying:
+=C2=A0 =C2=A0 =C2=A0 Any indexes, constraints and user-defined row-level t= riggers that exist
+=C2=A0 =C2=A0 =C2=A0 in the parent table are cloned on new partitions [...= ]

Which is good to say, and addresses part of my message [0]
[0] ZiJW1g2nbQs9ekwK@pryzbyj2023

But it doesn't have anything to do with "creating new partitions w= ith
parent's persistence".=C2=A0 Maybe there was a merge conflict and = the docs
ended up in the wrong patch ?

Also, defaults, storage options, compression are also copied.=C2=A0 As will=
be anything else from LIKE.=C2=A0 And since anything added in the future wi= ll
also be copied, maybe it's better to just say that the tables will be created the same way as "LIKE .. INCLUDING ALL EXCLUDING ..", or<= br> similar.=C2=A0 Otherwise, the next person who adds a new option for LIKE would have to remember to update this paragraph...

Also, extended stats objects are currently cloned to new child tables.
But I suggested in [0] that they probably shouldn't be.

> 007 =E2=80=93 doc review by Justin [3]

I suggest to drop this patch for now.=C2=A0 I'll send some more minor f= ixes to
docs and code comments once the other patches are settled.
=
I've looked at the patchset:

0001 Look go= od.
0002 Also right with docs modification proposed by Justin.
0003:
Looks like unused code
5268 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 datum =3D cmpval ? list_nth(spec->lowerdatu= ms, abs(cmpval) - 1) : NULL;
overridden by
5278 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 datum = =3D list_nth(spec->upperdatums, abs(cmpval) - 1);
and
5290 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 datum =3D list_nth(spec->upperdatums, abs(cmpval) - 1);
=

Otherwise - good.

0004:
<= div>I suggest also getting rid of thee-noun compound words like: salesperso= n_name. Maybe salesperson -> clerk? Or maybe use the same terms like in = pgbench:=C2=A0branches, tellers, accounts, balance.

0005: Good
0006: Patch is right
In comments:
+ =C2=A0 =C2=A0 =C2=A0New partitions will have the same table access = method,
+ =C2=A0 =C2=A0 =C2=A0same column names and types as the partiti= oned table to which they belong.
(I'd suggest to remove s= econd "same")

Tests are passed. I suppos= e that it's better to add similar tests for SPLIT/MERGE PARTITION(S)=C2= =A0 to those covering ATTACH/DETACH PARTITION (e.g.: subscription/t/013_partition.pl and regression tests)

Overall, great work! Thanks!

Regards,
Pavel Borisov,
Supabase.

=


--000000000000f97c770616fff3b7--