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 -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
default_range_partition_v2.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