Hi Jeevan, On Tue, Aug 22, 2017 at 7:53 AM, Jeevan Ladhe <jeevan.la...@enterprisedb.com> wrote: > > Hi Beena, > > On Thu, Aug 17, 2017 at 4:59 PM, Beena Emerson <memissemer...@gmail.com> > wrote: >> >> PFA the patch rebased over v25 patches of default list partition [1] > > > Thanks for rebasing. > > Range partition review:
Thank you for your review. > > 3. > @@ -2396,6 +2456,8 @@ make_one_range_bound(PartitionKey key, int index, List > *datums, bool lower) > ListCell *lc; > int i; > > + Assert(datums != NULL); > + > bound = (PartitionRangeBound *) palloc0(sizeof(PartitionRangeBound)); > bound->index = index; > bound->datums = (Datum *) palloc0(key->partnatts * sizeof(Datum)); > > I am not really convinced, why should we have above Assert. The function should never be called for default partition where datums = NULL. > > 4. > static List * > -get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec) > +get_qual_for_range(Relation parent, PartitionBoundSpec *spec, > + bool for_default) > { > > The addition and the way flag ‘for_default’ is being used is very confusing. > At this moment I am not able to think about a alternate solution to the > way you have used this flag. But will try and see if I can come up with > an alternate approach. Well, we need to skip the get_range_nulltest while collecting the qual of other partition to make one for default. We could skip this flag and remove the NullTest from the qual returned for each partition but I am not sure if thats a good way to go about it. -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers