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

On Wed, Mar 22, 2017 at 8:46 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)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