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 <[email protected]> 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
>
>
>
