Re: Buglets in equivclass.c

On Mon, 5 Oct 2020 at 06:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> While hacking on a patch that touches equivclass.c, I came across
> a couple of things that seemed wrong, and are fixed by the attached
> proposed patch.
>
> First, get_eclass_for_sort_expr() computes expr_relids and nullable_relids
> too soon. This is a waste of a not-really-trivial number of cycles in
> the common cases where it finds an existing eclass or is told not to
> make a new one. More subtly, the bitmapsets are computed in the caller's
> context. If we do use them, they will be attached to an EquivalenceClass
> that lives in the potentially-longer-lived root->planner_cxt, allowing
> the EC's pointers to them to become dangling. This would be a live bug
> if get_eclass_for_sort_expr() could be called with create_it = true during
> GEQO planning. So far as I can find, it is not; but both its API spec and
> its internal comments certainly give the impression that that's allowed.

hmm, yeah, it seems fairly low-risk to be moving that down to after we
switch the memory context. Seems like the sort of thing that could
easily become an actual bug one day.

> Second, generate_join_implied_equalities() uses inner_rel->relids to
> look up relevant eclasses, but given the surrounding code it seems like
> it ought to be using nominal_inner_relids. The code accidentally works
> because a child RelOptInfo will always have exactly the same
> eclass_indexes as its top parent; but if it did not, we'd risk either
> missing some relevant eclasses or hitting the assertion that claims
> that all the eclasses we find overlap nominal_join_relids. (I noticed
> this while speculating that maybe we needn't bother maintaining
> eclass_indexes for child RelOptInfos. This code is one place that
> would fail if we didn't.)

Oops. That certainly should be using nominal_inner_relids rather than
innerrel->relids. I agree it's not a live bug since child ECs are just
a copy of their parents, currently.

> I'm unsure whether to back-patch either of these. They both seem to be
> just latent bugs so far as the core code is concerned, but the first one
> in particular seems like something that could bite extension code.
> Thoughts?

That's a good question. I'm leaning towards backpatching both of them.
The 2nd is new as of v13, so it does not seem unreasonable that
someone has just not yet stumbled on it with some extension that adds
extra child ECs. I can imagine a use case for that, by getting rid of
needless equality quals that duplicate the partition constraint. The
fix for the first just seems neater/faster/correct, so I can't really
see any reason not to backpatch it.

David