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 1lafci-0002uM-K2 for pgsql-hackers@arkaria.postgresql.org; Sun, 25 Apr 2021 14:20:36 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lafch-00087G-92 for pgsql-hackers@arkaria.postgresql.org; Sun, 25 Apr 2021 14:20:35 +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 1lafcg-000879-UK for pgsql-hackers@lists.postgresql.org; Sun, 25 Apr 2021 14:20:35 +0000 Received: from mail-lj1-x22d.google.com ([2a00:1450:4864:20::22d]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lafcZ-0003dJ-Jo for pgsql-hackers@lists.postgresql.org; Sun, 25 Apr 2021 14:20:34 +0000 Received: by mail-lj1-x22d.google.com with SMTP id a36so50171458ljq.8 for ; Sun, 25 Apr 2021 07:20:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=OAoj6M0TNkVhjBD6ZuRvidjDIByxJJjs8CPk/V6fea0=; b=vW5LZ/ErXMVOrbQpePGwyEsCp1Cv5U+PC39RbKug+NS4zmMiLDK4J56RFTd1x5Dt1F EYkL/uck6zTvRxG/9z4sTUpK/DnPcsx/q3rVimySR8VfYrQEWgeQ6n8eqae81mx5iyB4 /+p5tg8iYCnJKSl5c0zvZWD5S+4HY/8I/zWeSMH3JttVrJ0V8iTFxDHkHNDqDoHxpLeP mL1J4ywOYkEIQRkDu1GQZWSZuZsa69fEgCX7Kglpw3jgCIPlKP2OvEFEbfaEp37++7iN LqmoPEN0j51djUlclVx6qjUh4mXJDfdBDrycQ+TZF9VVXAdbedjw7bjR4H+aUjrCSGJ2 XIJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=OAoj6M0TNkVhjBD6ZuRvidjDIByxJJjs8CPk/V6fea0=; b=S88AsO6ZWqm20YZEIbFOlGAvECyrsUzziFmj2Wzw1Z8ACXMomRbLC7pAQbDXb00O0i 5XQeLgGG6kd+/GcZ55x+6c/ibGF/x8VmeUKnq+rSxw9O6K8gUqJrdc4Bb+9pf7Gzfz1l WPKQ5tvXCESKW3uYCb9qb3IkbdsYIZBIrQDAlQprFdcBtN9CacDCAvX/oCWJV5ytA3dp QWkvQ7bWPSOrCfigU7FRnvnQv/+m1LTjgTnnBpNhioBpwEJ7VQYJrQBOG861mc2XbFnu Jivxh4nDrbjpx1pUbcO79s8J5h4PUKaEDY0cpxg6Y+rCybJKXV6+HYGwN4r2+f5TO8d3 hjwg== X-Gm-Message-State: AOAM533WmGT8ypbOEu+LJ7ZgfCtazLziPoSO/rycVF7lKwCNfIr05TYX 9sx0pCpTY1UWg8UMsHSBVI9En62uojpiJJ6DtWU= X-Google-Smtp-Source: ABdhPJyS0tZMzvRcTpIlNIQ3+2Q0wzxD2gjpxLV8LjAFBkIe2h4LOBwb88piWiRRgboculDCOljJuRDeDEsoo6ASE9E= X-Received: by 2002:a2e:9957:: with SMTP id r23mr10128674ljj.306.1619360425495; Sun, 25 Apr 2021 07:20:25 -0700 (PDT) MIME-Version: 1.0 References: <20200924032738.GA28585@paquier.xyz> <37f1072a-16ec-8ce8-8044-1fcec85c8ed5@postgrespro.ru> <199124d5-e710-71a6-1836-4653cd668d96@postgrespro.ru> <6ad2294a-cedc-2f3c-13bd-1ceceb56e4f7@postgrespro.ru> <20210302202617.GD29832@telsasoft.com> In-Reply-To: <20210302202617.GD29832@telsasoft.com> From: Nitin Jadhav Date: Sun, 25 Apr 2021 19:49:47 +0530 Message-ID: Subject: Re: [PATCH] Automatic HASH and LIST partition creation To: Justin Pryzby Cc: Pavel Borisov , Thomas Munro , Anastasia Lubennikova , Rahila Syed , Michael Paquier , PostgreSQL Hackers , Fabien COELHO , Robert Haas , Amul Sul Content-Type: multipart/alternative; boundary="000000000000eccf0805c0ccbade" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000eccf0805c0ccbade Content-Type: text/plain; charset="UTF-8" I have reviewed the v4 patch. The patch does not get applied on the latest source. Kindly rebase. However I have found few comments. 1. > +-- must fail because of wrong configuration > +CREATE TABLE tbl_hash_fail (i int) PARTITION BY HASH (i) > +CONFIGURATION (values in (1, 2), (3, 4) default partition tbl_default); Here some of the keywords are mentioned in UPPER CASE (Ex: CREATE TABLE, CONFIGURATION, etc) and some are mentioned in lower case (Ex: values in, default partition, etc). Kindly make it common. I feel making it to UPPER CASE is better. Please take care of this in all the cases. 2. It is better to separate the failure cases and success cases in /src/test/regress/sql/create_table.sql for better readability. All the failure cases can be listed first and then the success cases. 3. > + char *part_relname; > + > + /* > + * Generate partition name in the format: > + * $relname_$partnum > + * All checks of name validity will be made afterwards in DefineRelation() > + */ > + part_relname = psprintf("%s_%d", cxt->relation->relname, i); The assignment can be done directly while declaring the variable. Thanks & Regards, Nitin Jadhav On Wed, Mar 3, 2021 at 1:56 AM Justin Pryzby wrote: > https://commitfest.postgresql.org/32/2694/ > > I don't know what committers will say, but I think that "ALTER TABLE" > might be > the essential thing for this patch to support, not "CREATE". (This is > similar > to ALTER..SET STATISTICS, which is not allowed in CREATE.) > > The reason is that ALTER is what's important for RANGE partitions, which > need > to be created dynamically (for example, to support time-series data > continuously inserting data around 'now'). I assume it's sometimes also > important for LIST. I think this patch should handle those cases better > before > being commited, or else we risk implementing grammar and other user-facing > interface > that fails to handle what's needed into the future (or that's > non-essential). > Even if dynamic creation isn't implemented yet, it seems important to at > least > implement the foundation for setting the configuration to *allow* that in > the > future, in a manner that's consistent with the initial implementation for > "static" partitions. > > ALTER also supports other ideas I mentioned here: > https://www.postgresql.org/message-id/20200706145947.GX4107%40telsasoft.com > > - ALTER .. SET interval (for dynamic/deferred RANGE partitioning) > - ALTER .. SET modulus, for HASH partitioning, in the initial > implementation, > this would allow CREATING paritions, but wouldn't attempt to handle > moving > data if overlapping table already exists: > - Could also set the table-name, maybe by format string; > - Could set "retention interval" for range partitioning; > - Could set if the partitions are themselves partitioned(??) > > I think once you allow setting configuration parameters like this, then you > might have an ALTER command to "effect" them, which would create any static > tables required by the configuration. maybe that'd be automatic, but if > there's an "ALTER .. APPLY PARTITIONS" command (or whatever), maybe in the > future, the command could also be used to "repartition" existing table data > into partitions with more fine/course granularity (modulus, or daily vs > monthly > range, etc). > > -- > Justin > > > --000000000000eccf0805c0ccbade Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I = have reviewed the v4 patch. The patch does not get applied on the latest so= urce. Kindly rebase.=C2=A0
However I have found few comments.

1.
> +-- must fail because of wrong conf= iguration
> +CREATE TABLE tbl_hash_fail (i int) PARTITION BY H= ASH (i)
> +CONFIGURATION (values in (1, 2), (3, 4) default par= tition tbl_default);

Here some of the keywords are= mentioned=C2=A0in UPPER CASE (Ex: CREATE TABLE, CONFIGURATION, etc) and so= me are mentioned in lower case (Ex: values in, default partition, etc). Kin= dly make it common. I feel making it to UPPER CASE is better. Please take c= are of this in all the cases.

2. It is better to s= eparate the failure cases and success cases in /src/test/regress/sql/create= _table.sql for better readability. All the failure cases can be listed firs= t and then the success cases.

3.=C2=A0
<= div>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char *part_relname;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
&g= t; + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Generate partition name in = the format:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* $re= lname_$partnum
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* = All checks of name validity will be made afterwards in DefineRelation()
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 part_relname =3D psprintf("%s_%d&qu= ot;, cxt->relation->relname, i);

The a= ssignment can be done directly while declaring the variable.

=
Thanks & Regards,
Nitin Jadhav

On Wed, Mar 3, 2021 at 1:56 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
https://commitfest.postgresql.org/32/2694/

I don't know what committers will say, but I think that "ALTER TAB= LE" might be
the essential thing for this patch to support, not "CREATE".=C2= =A0 (This is similar
to ALTER..SET STATISTICS, which is not allowed in CREATE.)

The reason is that ALTER is what's important for RANGE partitions, whic= h need
to be created dynamically (for example, to support time-series data
continuously inserting data around 'now').=C2=A0 I assume it's = sometimes also
important for LIST.=C2=A0 I think this patch should handle those cases bett= er before
being commited, or else we risk implementing grammar and other user-facing = interface
that fails to handle what's needed into the future (or that's non-e= ssential).
Even if dynamic creation isn't implemented yet, it seems important to a= t least
implement the foundation for setting the configuration to *allow* that in t= he
future, in a manner that's consistent with the initial implementation f= or
"static" partitions.

ALTER also supports other ideas I mentioned here:
https://www.postgresql.or= g/message-id/20200706145947.GX4107%40telsasoft.com

=C2=A0 - ALTER .. SET interval (for dynamic/deferred RANGE partitioning) =C2=A0 - ALTER .. SET modulus, for HASH partitioning, in the initial implem= entation,
=C2=A0 =C2=A0 this would allow CREATING paritions, but wouldn't attempt= to handle moving
=C2=A0 =C2=A0 data if overlapping table already exists:
=C2=A0 - Could also set the table-name, maybe by format string;
=C2=A0 - Could set "retention interval" for range partitioning; =C2=A0 - Could set if the partitions are themselves partitioned(??)

I think once you allow setting configuration parameters like this, then you=
might have an ALTER command to "effect" them, which would create = any static
tables required by the configuration.=C2=A0 maybe that'd be automatic, = but if
there's an "ALTER .. APPLY PARTITIONS" command (or whatever),= maybe in the
future, the command could also be used to "repartition" existing = table data
into partitions with more fine/course granularity (modulus, or daily vs mon= thly
range, etc).

--
Justin


--000000000000eccf0805c0ccbade--