Re: Default Partition for Range

On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed <rahilasyed90(at)gmail(dot)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