Hi, Hackers! On Thu, 25 Apr 2024 at 00:26, Justin Pryzby <pry...@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 PM Alexander Korotkov <aekorot...@gmail.com> > wrote: > > > On Fri, Apr 19, 2024 at 2:26 AM Dmitry Koval <d.ko...@postgrespro.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@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.
