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 <memissemer...@gmail.com> wrote: > On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed <rahilasye...@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 >