Buglets in equivclass.c

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: David Rowley <dgrowleyml(at)gmail(dot)com>
Subject: Buglets in equivclass.c
Date: 2020-10-04 17:29:41
Message-ID: 1508010.1601832581@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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

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?

regards, tom lane

Attachment Content-Type Size
fix-equivclass-buglets.patch text/x-diff 1.5 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-10-04 20:20:05 Re: small cleanup: unify scanstr() functions
Previous Message Alvaro Herrera 2020-10-04 14:10:34 Re: Add Information during standby recovery conflicts