Re: [PATCH] Automatic HASH and LIST partition creation
| От | Nitin Jadhav |
|---|---|
| Тема | Re: [PATCH] Automatic HASH and LIST partition creation |
| Дата | |
| Msg-id | CAMm1aWbsoywpRq9He2PYRXH5iz9KXoYgHtcwQnnbJARe32onNQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] Automatic HASH and LIST partition creation (Michael Paquier <michael@paquier.xyz>) |
| Ответы |
Re: [PATCH] Automatic HASH and LIST partition creation
|
| Список | pgsql-hackers |
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 <pryzby@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
В списке pgsql-hackers по дате отправления: