On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat <ashutosh.ba...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
