Re: [HACKERS] Default Partition for Range

On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> Hello Beena,
>
> Thanks for the updated patch. It passes the basic tests which I performed.
>
> Few comments:
> 1. In   check_new_partition_bound(),
>
>> /* Default case is not handled here */
>>                if (spec->is_default)
>>                    break;
> The default partition check here can be performed in common for both range
> and list partition.

Removed the check here.

>
> 2.     RANGE_DATUM_FINITE = 0,     /* actual datum stored elsewhere */
> +   RANGE_DATUM_DEFAULT,        /* default */
>
> More elaborative comment?

I am not sure what to add. Do you have something in mind?

>
> 3.  Inside make_one_range_bound(),
>
>>+
>>+   /* datums are NULL for default */
>>+   if (datums == NULL)
>>+       for (i = 0; i < key->partnatts; i++)
>>+           bound->content[i] = RANGE_DATUM_DEFAULT;
>>+
> IMO, returning bound at this stage will make code clearer as further
> processing
> does not happen for default partition.

Done.

>
> 4.
> @@ -549,6 +568,7 @@ RelationBuildPartitionDesc(Relation rel)
>                                                 sizeof(RangeDatumContent
> *));
>                     boundinfo->indexes = (int *) palloc((ndatums + 1) *
>                                                         sizeof(int));
> +                   boundinfo->default_index = -1;
> This should also be done commonly for both default list and range partition.

Removed the line here. Common allocation is done by Jeevan's patch.

>
> 5.
>               if (!spec->is_default && partdesc->nparts > 0)
>                 {
>                     ListCell   *cell;
>
>                     Assert(boundinfo &&
>                            boundinfo->strategy == PARTITION_STRATEGY_LIST &&
>                            (boundinfo->ndatums > 0 ||
>                             partition_bound_accepts_nulls(boundinfo) ||
>                             partition_bound_has_default(boundinfo)));
> The assertion condition partition_bound_has_default(boundinfo) is rendered
> useless
> because of if condition change and can be removed perhaps.

This cannot be removed.
This check is required when this code is run for a non-default
partition and default is the only existing partition.

>
> 6. I think its better to have following  regression tests coverage
>
> --Testing with inserting one NULL and other non NULL values for multicolumn
> range partitioned table.
>
> postgres=# CREATE TABLE range_parted (
> postgres(#     a int,
> postgres(#     b int
> postgres(# ) PARTITION BY RANGE (a, b);
> CREATE TABLE
> postgres=#
> postgres=# CREATE TABLE part1 (
> postgres(#     a int NOT NULL CHECK (a = 1),
> postgres(#     b int NOT NULL CHECK (b >= 1 AND b <= 10)
> postgres(# );
> CREATE TABLE
>
> postgres=# ALTER TABLE range_parted ATTACH PARTITION part1 FOR VALUES FROM
> (1, 1) TO (1, 10);
> ALTER TABLE
> postgres=# CREATE TABLE partr_def1 PARTITION OF range_parted DEFAULT;
> CREATE TABLE
>
> postgres=# insert into range_parted values (1,NULL);
> INSERT 0 1
> postgres=# insert into range_parted values (NULL,9);
> INSERT 0 1

added.

--

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company