On Wed, May 31, 2017 at 5:53 PM, Beena Emerson <memissemer...@gmail.com> wrote: > Hello, > > PFA the updated patch. > Dependent patch default_partition_v17.patch [1] > This patch adds regression tests and removes the separate function to > get default partition qual. > > > On Mon, May 29, 2017 at 3:22 PM, Jeevan Ladhe > <jeevan.la...@enterprisedb.com> wrote: >> Hi Beena, >> >> I went through your patch, and here are some of my comments: >> >> - For generating a qual for default range partition, instead of scanning for >> all >> the children and collecting all the boundspecs, how about creating and >> negating >> an expression from the lists of lowerdatums and upperdatums in boundinfo. >> > > Unlike list, range partition can be for multiple columns and the > expressions get complicated. I have used the same logic of looping > through different partitions and negating the ORed expr. However, this > is now done under get_qual_for_range. > > >> - Wrong comment: >> + int default_index; /* Index of the default list partition. */ > > Comment is made generic in the dependent patch. > >> >> - Suggested by Robert earlier on default list partitioning thread, instead >> of >> abbreviating is_def/found_def use is(found)_default etc. > > corrected. > >> >> - unrelated change: >> - List *all_parts; >> + List *all_parts; >> > > undone. > >> - typo: partiton should be partition, similar typo at other places too. >> + * A non-default range partiton table does not currently allow partition >> keys >> > > rectified. > >> - Useless hunk for this patch: >> - Oid defid; >> + Oid defid; > > undone. > >> >> - better to use IsA here, instead of doing explicit check: >> + bound->content[i] = (datum->type == T_DefElem) >> + ? RANGE_DATUM_DEFAULT > > Modified > >> >> - It is better to use head of either lowerdatums or upperdatums list to >> verify >> if its a default partition and get rid of the constant PARTITION_DEFAULT >> altogether. >> > > modified this part as necessary. > > >> - In the function get_qual_from_partbound() is_def is always going to be >> false >> for range partition, the function get_qual_for_range can be directly passed >> false instead. >> >> - Following comment for function get_qual_for_range_default() implies that >> this >> function returns bool, but the actually it returns a List. >> + * >> + * If DEFAULT is the only partiton for the table then this returns TRUE. >> + * >> > Updated. > > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315573.html >
Hi Beena, I had a look at the patch from the angle of aesthetics and there are a few cosmetic changes I might suggest. Please have a look at the attached patch and if you agree with those changes you may merge it on your patch. The patch also fixes a couple of more spelling mistakes unrelated to this patch. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
cosmetic_range_default_partition.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers