On Tue, Jun 19, 2018 at 6:16 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > > * As I said upthread, the patch makes code much more simple; I removed all > the changes to setrefs.c added by the partitionwise-join patch. I also > simplified the logic for building a tlist for a child-join rel. The original > PWJ computes attr_needed data even for child rels, and build the tlist for a > child-join by passing to build_joinrel_tlist that data for input child rels > for the child-join. But I think that's redundant, and it's more > straightforward to apply adjust_appendrel_attrs to the parent-join's tlist > to get the child-join's tlist. So, I changed that way, which made > unnecessary all the changes to build_joinrel_tlist and placeholder.c added > by the PWJ patch, so I removed those as well. > > * The patch contains all of the regression tests in the original patch > proposed by Ashutosh.
I have started reviewing the patch. + if (enable_partitionwise_join && rel->top_parent_is_partitioned) + { + build_childrel_tlist(root, rel, childrel, 1, &appinfo); + } Why do we need rel->top_parent_is_partitioned? If a relation is partitioned (if (rel->part_scheme), it's either the top parent or is partition of some other partitioned table. In either case this condition will be true. + /* No work if the child plan is an Append or MergeAppend */ + if (IsA(subplan, Append) || IsA(subplan, MergeAppend)) + return; Why? Probably it's related to the answer to the first question, But I don't see the connection. Given that partition-wise join will be applicable level by level, we need to recurse in adjust_subplan_tlist(). + /* The child plan should be able to do projection */ + Assert(is_projection_capable_plan(subplan)); + Again why? A MergeAppend's subplan could be a Sort plan, which will not be projection capable. This is not a full review. I will continue reviewing it further. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company