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 <pry...@telsasoft.com> 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 > > >