Re: Default Partition for Range

Hi Beena,

I have posted the rebased patches[1] for default list partition.
Your patch also needs a rebase.

[1]
https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com

Regards,
Jeevan Ladhe

On Fri, Jul 14, 2017 at 3:28 PM, Beena Emerson <memissemerson@gmail.com>
wrote:

> 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
>