From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: David Rowley <dgrowleyml@gmail.com>
Subject: Buglets in equivclass.c
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1507895.1601832404.0@sss.pgh.pa.us>
Date: Sun, 04 Oct 2020 13:29:41 -0400
Message-ID: <1508010.1601832581@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1507895.1601832404.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-equivclass-buglets.patch";
	charset="us-ascii"
Content-ID: <1507895.1601832404.2@sss.pgh.pa.us>
Content-Description: fix-equivclass-buglets.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimiz=
er/path/equivclass.c
index b68a5a0ec7..d3d826b790 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -634,12 +634,6 @@ get_eclass_for_sort_expr(PlannerInfo *root,
 	 */
 	expr =3D canonicalize_ec_expression(expr, opcintype, collation);
 =

-	/*
-	 * Get the precise set of nullable relids appearing in the expression.
-	 */
-	expr_relids =3D pull_varnos((Node *) expr);
-	nullable_relids =3D bms_intersect(nullable_relids, expr_relids);
-
 	/*
 	 * Scan through the existing EquivalenceClasses for a match
 	 */
@@ -716,6 +710,12 @@ get_eclass_for_sort_expr(PlannerInfo *root,
 	if (newec->ec_has_volatile && sortref =3D=3D 0) /* should not happen */
 		elog(ERROR, "volatile EquivalenceClass has no sortref");
 =

+	/*
+	 * Get the precise set of nullable relids appearing in the expression.
+	 */
+	expr_relids =3D pull_varnos((Node *) expr);
+	nullable_relids =3D bms_intersect(nullable_relids, expr_relids);
+
 	newem =3D add_eq_member(newec, copyObject(expr), expr_relids,
 						  nullable_relids, false, opcintype);
 =

@@ -1171,9 +1171,9 @@ generate_join_implied_equalities(PlannerInfo *root,
 	}
 =

 	/*
-	 * Get all eclasses in common between inner_rel's relids and outer_relid=
s
+	 * Get all eclasses that mention both inner and outer sides of the join
 	 */
-	matching_ecs =3D get_common_eclass_indexes(root, inner_rel->relids,
+	matching_ecs =3D get_common_eclass_indexes(root, nominal_inner_relids,
 											 outer_relids);
 =

 	i =3D -1;

------- =_aaaaaaaaaa0--