On Wed, Mar 22, 2017 at 8:46 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > Here's set of updated patches rebased on > 1148e22a82edc96172fc78855da392b6f0015c88. > > I have fixed all the issues reported till now.
I don't understand why patch 0001 ends up changing every existing test for RELOPT_JOINREL anywhere in the source tree to use IS_JOIN_REL(), yet none of the existing tests for RELOPT_OTHER_MEMBER_REL end up getting changed to use IS_OTHER_REL(). That's very surprising. Some of those tests are essentially checking for something that is going to have a scan plan rather than a join or upper plan, and those tests probably don't need to be modified; for example, the test in set_rel_consider_parallel() is obviously of this type. But others are testing whether we've got some kind of child rel, and those seem like they might need work. Going through a few specific examples: - generate_join_implied_equalities_for_ecs() assumes that any child rel is an other member rel. - generate_join_implied_equalities_broken() assumes that any child rel is an other member rel. - generate_implied_equalities_for_column() set is_child_rel on the assumption that only an other member rel can be a child rel. - eclass_useful_for_merging() assumes that the only kind of child rel is an other member rel. - find_childrel_appendrelinfo() assumes that any child rel is an other member rel. - find_childrel_top_parent() and find_childrel_parents() assume that children must be other member rels and their parents must be baserels. - adjust_appendrel_attrs_multilevel() assumes that children must be other member rels and their parents must be baserels. It's possible that, for various reasons, none of these code paths would ever be reachable by a child join, but it doesn't look likely to me. And even if that's true, some comment updates are probably needed, and maybe some renaming of functions too. In postgres_fdw, get_useful_ecs_for_relation() assumes that any child rel is an other member rel. I'm not sure if we're hoping that partitionwise join will work with postgres_fdw's join pushdown out of the chute, but clearly this would need to be adjusted to have any chance of being right. Some that seem OK: - set_rel_consider_parallel() is fine. - set_append_rel_size() is only going to be called for baserels or their children, so it's fine. - relation_excluded_by_constraints() is only intended to be called on baserels or their children, so it's fine. - check_index_predicates() is only intended to be called on baserels or their children, so it's fine. - query_planner() loops over baserels and their children, so it's fine. Perhaps we could introduce an IS_BASEREL_OR_CHILD() test that could be used in some of these places, just for symmetry. The point is that there are really three questions here: (1) is it some kind of baserel (parent or child)? (2) is it some kind of joinrel (parent or child)? and (3) is it some kind of child (baserel or join)? Right now, both #2 and #3 are tested by just comparing against RELOPT_OTHER_MEMBER_REL, but they become different tests as soon as we add child joinrels. The goal of 0001, IMV, ought to be to try to figure out which of #1, #2, and #3 is being checked in each case and make that clear via use of an appropriate macro. (If is-other-baserel is the real test, then fine, but I bet that's a rare case.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers