Richard Guo <[email protected]> writes:
> I was curious whether there are other cases where we can end up with
> duplicate qual clauses, so I added the attached Assert to verify that
> the clauses to be enforced at a join or at a parameterized path's scan
> contain no duplicate rinfo_serial, and the regression tests
> immediately crashed :-O
Hm.
> After a closer look, I don't think anything is wrong in
> deconstruct_distribute_oj_quals. Both variants are needed for clause
> selection at joins, where subbuild_joinrel_restrictlist checks
> required_relids and incompatible_relids against the input relids. In
> the normal join order, [0] is rejected because relid 3 appears in its
> incompatible_relids, and [2] is the one applied; in the commuted
> order, where t3/t4 join is performed below t1/t2 join, [2] is rejected
> because relid 3 in its required_relids is not available, and [0] is
> the one applied.
> But a parameterized path cannot tell them apart. Since outer join 3
> nulls no Var referenced by this clause, the parameterization looks
> exactly the same whether that join is computed below the scan (the
> normal join order, where [2] is the right variant) or above it (the
> commuted order, where [0] is), so the same ParamPathInfo serves both
> orders. For the same reason, either variant is correct in any join
> order, so I think we should just enforce one of them and ignore the
> rest? Thought?
I'm not quite convinced that this is worth spending cycles on.
We have only two example queries that trigger this case, and in
neither one does the actually-selected plan change. I think that's
because the case only occurs with very bizarre join ordering choices
that will lose on cost grounds anyway. Also the proposed assertion
would only catch rather narrow cases where we try to put the same
clause twice in the same place, but not if we put it in two different
places in the plan tree.
Having said that, I looked into the other query that hits the
assertion, which is later on in join.sql:
select ss1.d1 from
tenk1 as t1
inner join tenk1 as t2
on t1.tenthous = t2.ten
inner join
int8_tbl as i8
left join int4_tbl as i4
inner join (select 64::information_schema.cardinal_number as d1
from tenk1 t3,
lateral (select abs(t3.unique1) + random()) ss0(x)
where t3.fivethous < 0) as ss1
on i4.f1 = ss1.d1
on i8.q1 = i4.f1
on t1.tenthous = ss1.d1
where t1.unique1 < i4.f1;
It turns out that this one has nothing to do with clone clauses,
it's that the hacky bit in get_joinrel_parampathinfo to ensure full
enforcement of equivalence classes (lines 1952-2006 in HEAD, dating to
commit 207d5a656) isn't being careful not to add duplicate clauses.
As I said in that commit message, this is a super rare case already,
so it's not surprising nobody noticed. I made a quick-n-dirty patch
for it, attached, but I don't have a test case that visibly exposes
the misbehavior.
regards, tom lane
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 8862004cbad..3c672e277c1 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -1974,6 +1974,12 @@ get_joinrel_parampathinfo(PlannerInfo *root, RelOptInfo *joinrel,
* has nothing that needs to be enforced here, while if the clause can be
* moved into the LHS then it should have been enforced within that path.)
*
+ * In cases where an EC needs to constrain EC members that are newly
+ * computable at this join, it can emit clauses that it already returned
+ * above and we accepted into pclauses. Hence, do a final list-membership
+ * check before accepting more clauses. (Pointer comparison should be
+ * enough to detect duplicates, since ECs cache derived clauses.)
+ *
* Note that we don't need similar processing for ECs whose clause was
* considered to be movable into the LHS, because the LHS can't refer to
* the RHS so there is no comparable ambiguity about what it might
@@ -1998,10 +2004,13 @@ get_joinrel_parampathinfo(PlannerInfo *root, RelOptInfo *joinrel,
Assert(join_clause_is_movable_into(rinfo,
outer_path->parent->relids,
real_outer_and_req));
- if (!join_clause_is_movable_into(rinfo,
- outer_path->parent->relids,
- outer_and_req))
- pclauses = lappend(pclauses, rinfo);
+ if (join_clause_is_movable_into(rinfo,
+ outer_path->parent->relids,
+ outer_and_req))
+ continue; /* drop if movable into LHS */
+ if (list_member_ptr(pclauses, rinfo))
+ continue; /* drop if already accepted */
+ pclauses = lappend(pclauses, rinfo);
}
}
