Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables - Mailing list pgsql-hackers
From | Ashutosh Bapat |
---|---|
Subject | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables |
Date | |
Msg-id | CAFjFpRfOPsNRK=VcA8Zw74=OVDYOHSOBZRr4mWBRNBhVOpHsUw@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables
(Thomas Munro <thomas.munro@enterprisedb.com>)
|
List | pgsql-hackers |
On Thu, Aug 3, 2017 at 7:01 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jul 31, 2017 at 9:07 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> Forgot the patch set. Here it is. > > The commit message for 0005 isn't really accurate given that it > follows 0004. I think you could just flatten 0005 and 0006 into one > patch. Earlier, there was some doubt about the approach for expanding multi-level partitioned table's inheritance hierarchy. So, I had separated all multi-level partition related changes into patches by themselves, collocating them with their respective single level partition peers. I thought that would make the reviews easier while leaving the possibility of committing single-level partition-wise support before multi-level partition-wise join support. From your previous replies, it seems that you are fine with the multi-level partitioned hierarchy expansion, so it may be committed along-with other patches. So, I have squashed those two patches together. Similarly I have squashed pairs 0008-0009 and 0012-0013. Those dealt with similar issues for single-level partitioned and multi-level partitioned tables. > > Reviewing those together: > > - Existing code does partdesc = RelationGetPartitionDesc(relation) but > this has got it as part_desc. Seems better to be consistent. > Likewise existing variables for PartitionKey are key or partkey, not > part_key. Done. > > - get_relation_partition_info has a useless trailing return. > Done. > - Instead of adding nparts, boundinfo, and part_oids to RelOptInfo, > how about just adding partdesc? Seems cleaner. nparts and boundinfo apply to any kind of relation simple, join or upper but part_oids applies only to simple relations. So, I have split those members and added them in respective sections. Do you still think that we should add PartitionDesc as a single member? Similar to your suggestion of changing name of part_key to partkey, should we rename part_scheme as partscheme, part_rels as partrels and part_oids as partoids? > > - pkexprs seems like a potentially confusing name, since PK is widely > used to mean "primary key" but here you mean "partition key". Maybe > partkeyexprs. agreed. Done. PartitionKey structure has member partexprs for partition keys which are expressions. I have used the same name instread of pkexprs. > > - build_simple_rel's matching algorithm is O(n^2). We may have talked > about this problem before... If root->append_rel_list has AppendRelInfos in the same order as the partition bounds, we could reduce this to O(n). That expansion option is being discussed in [1]. Once we commit it, I will change the code to make it O(n). Right now, we can not rely on the order of AppendRelInfos in root->append_rel_list. > > - This patch introduces some bits that are not yet used, like > nullable_pkexprs, We could fix that by adding that member in 0008. IIRC, earlier you had complained about declaring a structure in one patch and adding members to it in the subsequent patches, so I just added all members in the same patch. BTW, I have renamed that member to nullable_partexprs to be consistent with change to pkexpers. > or even the code to set the partition scheme for > joinrels. I think perhaps some of that logic should be moved from > 0008 to here - e.g. the initial portion of > build_joinrel_partition_info. Setting part_scheme for joinrel should really be part of the patch which actually implements partition-wise join. That will keep all the partition-wise join implementation together. 0005 and 0006 really just introduce PartitionScheme for base relation. I think PartitionScheme and other partitioning properties for base relation are useful for something else like partition-wise aggregation on base relation. So, we may want to commit those two patches separately. If you want, we could squash the partition scheme and partition-wise join implementation together. [1] https://www.postgresql.org/message-id/0118a1f2-84bb-19a7-b906-dec040a206f2@lab.ntt.co.jp Updated patches attached. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
pgsql-hackers by date: