Re: UNION ALL on partitioned tables won't use indices. - Mailing list pgsql-hackers

Noah Misch <noah@leadboat.com> writes:
> I'm unclear on the key ideas behind distinguishing em_is_child members from
> ordinary EC members.  src/backend/optimizer/README says "These members are
> *not* full-fledged members of the EquivalenceClass and do not affect the
> class's overall properties at all."  Is that an optimization to avoid futile
> planner work, or is it necessary for correctness?  If the latter, what sort of
> query might give wrong answers if an EquivalenceMember were incorrectly added
> as em_is_child = false?

See commit dd4134ea, which added that text.  IIRC, the key point is that
among "real" EC members, there will never be duplicates: a Var "x.y"
for instance cannot appear in two different ECs, because we'd have merged
the two ECs into one instead.  However, this property does *not* hold for
child EC members.  The problem is that two completely unrelated UNION ALL
child subselects might have, say, constant "1" in their tlists.  This
should not lead to concluding that we can merge ECs that mention their
UNION ALL parent variables.

I've not looked at these patches yet, but if they're touching equivclass.c
at all, I'd guess that it's wrong.  I think what we want is for appendrel
formation to take care of flattening nested appendrels.  The core of the
problem is that pull_up_simple_union_all handles that for nested
UNION ALLs, and expand_inherited_rtentry sees to it by letting
find_all_inheritors do the recursion, but the two cases don't know about
each other.

My gut feeling after two minutes' thought is that the best fix is for
expand_inherited_rtentry to notice when the parent table is already an
appendrel member, and enlarge that appendrel instead of making a new one.
(This depends on the assumption that pull_up_simple_union_all always
happens first, but that seems OK.)  I'm not sure if that concept exactly
corresponds to either PATCH-3 or PATCH-4, but that's the way I'd think
about it.

> However, adding a qual to one of the inheritance queries once again defeated
> MergeAppend with the patches for approaches (2) and (3).

That's an independent limitation, see is_safe_append_member:
    * Also, the child can't have any WHERE quals because there's no place to    * put them in an appendrel.  (This is a
bitannoying...)
 

It'd be nice to fix that, but it's not going to be easy, and it should
be a separate patch IMO.  It's pretty much unrelated to the question at
hand here.

> Approaches (2) and (3) leave the inheritance parent with rte->inh == true
> despite no AppendRelInfo pointing to it as their parent.  Until now,
> expand_inherited_rtentry() has been careful to clear rte->inh in such cases.
> My gut feeling is that we should retain that property; what do you think?

Yes.  IIRC, failing to clear rte->inh doesn't actually break anything,
but it will lead to wasted work later in the planner.

> Finally, the patch should add test cases.

Agreed, at least one example showing that flattening does happen.
        regards, tom lane