On Tue, Jun 19, 2018 at 6:16 PM, Etsuro Fujita
<fujita.etsuro@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