Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Hi, Hackers!

On Thu, 25 Apr 2024 at 00:26, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:

> On Mon, Apr 22, 2024 at 01:31:48PM +0300, Alexander Korotkov wrote:
> > Hi!
> >
> > On Fri, Apr 19, 2024 at 4:29 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
> wrote:
> > > On Fri, Apr 19, 2024 at 2:26 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)ru>
> 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 – tab completion by Dagfinn [1]
> > > 006 – draft fix for table AM issue spotted by Alexander Lakhin [2]
> > > 007 – 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-ebe5dd21b190%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(at)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 – 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 = cmpval ? list_nth(spec->lowerdatums, abs(cmpval) -
1) : NULL;
overridden by
5278 datum = list_nth(spec->upperdatums, abs(cmpval) -
1);
and
5290 datum = 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.