Re: UNION ALL on partitioned tables won't use indices.

On Sat, Nov 23, 2013 at 01:35:32PM -0500, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)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.

Thanks for the pointer; I found things clearer after reviewing the ECs from
the test case queries from commits dd4134ea and 57664ed2.

> I've not looked at these patches yet, but if they're touching equivclass.c
> at all, I'd guess that it's wrong.

Only PATCH-1 touched equivclass.c.

> 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.

PATCH-4 does approximately that. I agree that's the right general direction.

> > 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 bit annoying...)
>
> 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.

After further study, I agree. It would still be good to understand why that
test case crashed PATCH-1, then ensure that the other patches don't have a
similar lurking bug.

An alternative to extending our ability to pull up UNION ALL subqueries,
having perhaps broader applicability, would be to push down the
possibly-useful sort order to subqueries we can't pull up. We'd sometimes
have two levels of MergeAppend, but that could still handily beat the
explicit-sort plan. In any case, it is indeed a separate topic. For the sake
of the archives, you can get such plans today by manually adding the ORDER BY
to the relevant UNION ALL branch:

EXPLAIN (ANALYZE) (SELECT oid, * FROM pg_proc WHERE protransform = 0 ORDER BY oid)
UNION ALL SELECT oid, * FROM pg_proc ORDER BY 1 LIMIT 5;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------------------
Limit (cost=0.57..1.31 rows=5 width=544) (actual time=0.102..0.155 rows=5 loops=1)
-> Merge Append (cost=0.57..748.25 rows=5084 width=544) (actual time=0.095..0.130 rows=5 loops=1)
Sort Key: pg_proc_1.oid
-> Index Scan using pg_proc_oid_index on pg_proc pg_proc_1 (cost=0.28..332.83 rows=2538 width=544) (actual time=0.058..0.069 rows=3 loops=1)
Filter: ((protransform)::oid = 0::oid)
-> Index Scan using pg_proc_oid_index on pg_proc (cost=0.28..326.47 rows=2546 width=544) (actual time=0.029..0.036 rows=3 loops=1)

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com