Re: Partition-wise join for join between (declaratively) partitioned tables

On Tue, Mar 28, 2017 at 10:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> I have gone through the patch, and it looks good to me. Here's the set
>> of patches with this patch included. Fixed the testcase failures.
>> Rebased the patchset on de4da168d57de812bb30d359394b7913635d21a9.
>
> This version of 0001 looks much better to me, but I still have some concerns.
>
> I think we should also introduce IS_UPPER_REL() at the same time, for
> symmetry and because partitionwise aggregate will need it, and use it
> in place of direct tests against RELOPT_UPPER_REL.

Ok. Done. I introduced IS_JOIN_REL and IS_OTHER_REL only to simplify
the tests for child-joins. But now we have grown this patch with
IS_SIMPLE_REL() and IS_UPPER_REL(). That has introduced changes
unrelated to partition-wise join. But I am happy with the way the code
looks now with all IS_*_REL() macros. If we delay this commit, some
more usages of bare RELOPT_* would creep in the code. To avoid that,
we may want to commit these changes in v10.

>
> I think it would make sense to change the test in deparseFromExpr() to
> check for IS_JOIN_REL() || IS_SIMPLE_REL().  There's no obvious reason
> why that shouldn't be OK, and it would remove the last direct test
> against RELOPT_JOINREL in the tree, and it will probably need to be
> changed for partitionwise aggregate anyway.

Done. However, we need another assertion to make sure than an "other"
upper rel has an "other" rel as scanrel. That can be added when
partition-wise aggregate, which would introduce "other" upper rels, is
implemented.

>
> Could set_append_rel_size Assert(IS_SIMPLE_REL(rel))?  I notice that
> you did this in some other places such as
> generate_implied_equalities_for_column(), and I like that.  If for
> some reason that's not going to work, then it's doubtful whether
> Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL) is going to
> survive either.

Done. Also modified prologue of that function to explicitly say simple
"append relation" since we can have join "append relations" and upper
"append relations" with partition-wise operations.

>
> Similarly, I think relation_excluded_by_constraints() would also
> benefit from Assert(IS_SIMPLE_REL(rel)).

Done.

>
> Why not set top_parent_relids earlier, when actually creating the
> RelOptInfo?  I think you could just change build_simple_rel() so that
> instead of passing RelOptKind reloptkind, you instead pass RelOptInfo
> *parent.  I think postponing that work until set_append_rel_size()
> just introduces possible bugs resulting from it not being set early
> enough.

Done.

>
> Apart from the above, I think 0001 is in good shape.
>
> Regarding 0002, I think the parts that involve factoring out
> find_param_path_info() are uncontroversial.  Regarding the changes to
> adjust_appendrel_attrs(), my main question is whether we wouldn't be
> better off using an array representation rather than a List
> representation.  In other words, this function could take PlannerInfo
> *root, Node *node, int nappinfos, AppendRelInfo **appinfos.  Existing
> callers doing adjust_appendrel_attrs(root, whatever, appinfo) could
> just do adjust_appendrel_attrs(root, whatever, 1, &appinfo), not
> needing to allocate.  To make this work, adjust_child_relids() and
> find_appinfos_by_relids() would need to be adjusted to use a similar
> argument-passing convention.  I suspect this makes iterating over the
> AppendRelInfos mildly faster, too, apart from the memory savings.

Done.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company