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.
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.
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.
Similarly, I think relation_excluded_by_constraints() would also
benefit from Assert(IS_SIMPLE_REL(rel)).
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.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
