Re: Default Partition for Range

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.

2. RANGE_DATUM_FINITE = 0, /* actual datum stored elsewhere */
+ RANGE_DATUM_DEFAULT, /* default */

More elaborative comment?

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.

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.

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.

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

--Testing the boundary conditions like in the above example
following should pass.

postgres=# insert into partr_def1 values (1,10);
INSERT 0 1

Thank you,
Rahila Syed

On Mon, Jul 3, 2017 at 8:00 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Fri, Jun 30, 2017 at 11:56 AM, Beena Emerson <memissemerson(at)gmail(dot)com>
> wrote:
> > Hello Dilip,
> >
> > On Wed, Jun 21, 2017 at 6:27 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> >> On Tue, Jun 20, 2017 at 6:57 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> >>> This is basically crashing in RelationBuildPartitionDesc, so I think
>
> >
> > Thank you for your review and analysis.
> >
> > I have updated the patch.
> > - bound->content is set to RANGE_DATUM_DEFAULT for each of the keys
> > and not just the first one.
> > - Improve the way of handling DEFAULT bounds in
> RelationBuildPartitionDesc,
> >
> > There is a test for multiple column range in alter_table.sql
>
> Thanks for update patch, After this fix segmentation fault is solved.
>
> I have some more comments.
>
> 1.
>
> lower = make_one_range_bound(key, -1, spec->lowerdatums, true);
> upper = make_one_range_bound(key, -1, spec->upperdatums, false);
>
> + /* Default case is not handled here */
> + if (spec->is_default)
> + break;
> +
>
> I think we can move default check just above the make range bound it
> will avoid unnecessary processing.
>
>
> 2.
> RelationBuildPartitionDesc
> {
> ....
>
> /*
> * If either of them has infinite element, we can't equate
> * them. Even when both are infinite, they'd have
> * opposite signs, because only one of cur and prev is a
> * lower bound).
> */
> if (cur->content[j] != RANGE_DATUM_FINITE ||
> prev->content[j] != RANGE_DATUM_FINITE)
> {
> is_distinct = true;
> break;
> }
> After default range partitioning patch the above comment doesn't sound
> very accurate and
> can lead to the confusion, now here we are not only considering
> infinite bounds but
> there is also a possibility that prev bound can be DEFAULT, so
> comments should clearly
> mention that.
>
> 3.
>
> + bound->datums = datums ? (Datum *) palloc0(key->partnatts *
> sizeof(Datum))
> + : NULL;
> bound->content = (RangeDatumContent *) palloc0(key->partnatts *
> sizeof(RangeDatumContent));
> bound->lower = lower;
>
> i = 0;
> +
> + /* datums are NULL for default */
> + if (datums == NULL)
> + for (i = 0; i < key->partnatts; i++)
> + bound->content[i] = RANGE_DATUM_DEFAULT;
>
> To me, it will look cleaner if we keep bound->content=NULL for
> default, instead of allocating memory
> and initializing each element, But it's just a suggestions and you
> can decide whichever way
> seems better to you. Then the other places e.g.
> + if (cur->content[i] == RANGE_DATUM_DEFAULT)
> + {
> + /*
>
> will change like
>
> + if (cur->content == NULL)
> + {
> + /*
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com
>