Assert failure in try_nestloop_path()

Richard Guo <guofenglinux@gmail.com> 于2026年9月1日周二 15:43写道：

I was studying the PHV bug caused by join-removal and ran into an
assertion failure with the query below, which is not related to
join-removal. So start a new thread for it.

create table ta (id int primary key, x int);
create table tb (id int primary key, a_id int, x int);
create table tc (id int, x int);

explain (costs off)
select 1 from ta t1 left join
(select tb.x as bx, 1 as one from ta a2 left join tb on a2.id =
tb.a_id) t2 on true
left join lateral (select tc.x as cnt from tc where tc.id = t2.one
offset 0) t3
on t2.bx = t3.cnt;

TRAP: failed Assert("!have_unsafe_outer_join_ref(root, outerrelids,
inner_paramrels)")

I tested v1 and it fixes the reported assertion failure.

I also noticed that the resulting plan contains a duplicated filter:
QUERY PLAN
-------------------------------------------------------------------
Nested Loop Left Join
-> Seq Scan on ta t1
-> Materialize
-> Nested Loop Left Join
-> Hash Right Join
Hash Cond: (tb.a_id = a2.id)
-> Seq Scan on tb
-> Hash
-> Seq Scan on ta a2
-> Subquery Scan on t3
Filter: ((tb.x = t3.cnt) AND (tb.x = t3.cnt))
-> Seq Scan on tc
Filter: (id = (1))

```
Filter: ((tb.x = t3.cnt) AND (tb.x = t3.cnt))
```

This is not introduced by v1; I can reproduce the same duplicate qual on
pg16.14 as well, so it seems to be a pre-existing issue.

It is probably unrelated to this fix, but I thought it was worth mentioning
since the test case exposes it.

--
Thanks,
Tender Wang

Richard Guo <guofenglinux@gmail.com> writes:

I was studying the PHV bug caused by join-removal and ran into an
assertion failure with the query below, which is not related to
join-removal. So start a new thread for it.
...
Attached is a patch that teaches join_is_legal() to reject a join
whose minimum parameterization includes an outer-join relid, if that
outer join cannot be formed outside the join.

This looks like a good fix to me. Interesting that we've failed
to notice this bug up to now.

regards, tom lane

On Thu, Sep 3, 2026 at 11:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

Attached is a patch that teaches join_is_legal() to reject a join
whose minimum parameterization includes an outer-join relid, if that
outer join cannot be formed outside the join.

This looks like a good fix to me. Interesting that we've failed
to notice this bug up to now.

Thanks for looking! I think part of the reason we've failed to notice
this bug is that such joins used to be rejected by have_dangerous_phv(),
which was only removed in v18

I've pushed this patch and back-patched it to v18.

- Richard

On Thu, Sep 3, 2026 at 6:39 PM Tender Wang <tndrwang@gmail.com> wrote:

I also noticed that the resulting plan contains a duplicated filter:

This is not introduced by v1; I can reproduce the same duplicate qual on
pg16.14 as well, so it seems to be a pre-existing issue.

Right, this is a pre-existing issue, and is exposed by the new test
case.

+            ->  Subquery Scan on t3
+                  Filter: ((b.q1 = t3.cnt) AND (b.q1 = t3.cnt))

I looked into it. What happens here is that outer-join identity 3
permits the join to t3 to commute with the a/b left join, so we
generate multiple clones of "b.q1 = t3.cnt", differing in the
nullingrels of b.q1. Only one of them should be applied in any given
plan. However, when movable join clauses are pushed down into a
parameterized path, we fail to choose among the clones. In this
example, t3's lateral reference to a PlaceHolderVar evaluated at the
a/b join forces every path for t3 to be parameterized by a set that
includes that outer join's relid, making both clones movable into the
scan. The same thing can also happen for clauses moved down into a
parameterized join.

I think we need to fix this. It wastes effort evaluating the same
qual clause repeatedly. What is worse, it applies the clause's
selectivity multiple times, underestimating the result's row count.

Attached is a patch that teaches get_baserel_parampathinfo and
get_joinrel_parampathinfo to check incompatible_relids. A clone
should not be enforced if an outer join it is incompatible with has
already been computed.

Thoughts?

- Richard

Richard Guo <guofenglinux@gmail.com> 于2026年9月4日周五 15:49写道：

On Thu, Sep 3, 2026 at 6:39 PM Tender Wang <tndrwang@gmail.com> wrote:

I also noticed that the resulting plan contains a duplicated filter:

This is not introduced by v1; I can reproduce the same duplicate qual on
pg16.14 as well, so it seems to be a pre-existing issue.

Right, this is a pre-existing issue, and is exposed by the new test
case.

+            ->  Subquery Scan on t3
+                  Filter: ((b.q1 = t3.cnt) AND (b.q1 = t3.cnt))

I looked into it. What happens here is that outer-join identity 3
permits the join to t3 to commute with the a/b left join, so we
generate multiple clones of "b.q1 = t3.cnt", differing in the
nullingrels of b.q1. Only one of them should be applied in any given
plan. However, when movable join clauses are pushed down into a
parameterized path, we fail to choose among the clones. In this
example, t3's lateral reference to a PlaceHolderVar evaluated at the
a/b join forces every path for t3 to be parameterized by a set that
includes that outer join's relid, making both clones movable into the
scan. The same thing can also happen for clauses moved down into a
parameterized join.

Yes, I learned the code and got the same conclusion.

I think we need to fix this. It wastes effort evaluating the same
qual clause repeatedly. What is worse, it applies the clause's
selectivity multiple times, underestimating the result's row count.

Agree

Attached is a patch that teaches get_baserel_parampathinfo and
get_joinrel_parampathinfo to check incompatible_relids. A clone
should not be enforced if an outer join it is incompatible with has
already been computed.

Thoughts?

The patch looks good to me.

One minor comment about the wording:
...
A clone clause must not be enforced here if an outer join it is
incompatible with has already been computed ...
...

I found "an outer join it is incompatible with has ..." a bit difficult
to parse. Would it be clearer to add "that" here?
...
A clone clause must not be enforced here if an outer join that it is
incompatible with has already been computed ...
...

--
Thanks,
Tender Wang

On Fri, Sep 4, 2026 at 10:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yup, clearly an oversight. Your fix LGTM. I'm not sure that we
should push it into stable branches: it might destabilize queries that
people are happy with, and we don't have field complaints about it.
But v19 should still be fair game at this point.

Thanks for reviewing! I agree that back-patching into stable
branches isn't worth the risk of plan changes, given the lack of field
complaints. I've pushed this to master and v19.

- Richard

On Mon, Sep 7, 2026 at 12:03 PM Richard Guo <guofenglinux@gmail.com> wrote:

Thanks for reviewing! I agree that back-patching into stable
branches isn't worth the risk of plan changes, given the lack of field
complaints. I've pushed this to master and v19.

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

One regression query that trips the Assert is:

explain (costs off)
select * from onek t1
left join onek t2 on t1.unique1 = t2.unique1
left join onek t3 on t2.unique1 = t3.unique1
left join onek t4 on t3.unique1 = t4.unique1 and t2.unique2 = t4.unique2;

It crashes in get_baserel_parampathinfo for base rel t4, whose
joininfo contains four clauses:

[0]: and [2] are two clone variants of "t3.unique1 = t4.unique1". Since the commuting outer join (relid 3) nulls no Var referenced by this clause, the two variants are textually identical, differing only in required_relids and incompatible_relids.
[1]: serial=4 clause_relids={2 6} required={2 4 5 6} incompatible={3 7}
[2]: serial=3 clause_relids={4 5 6} required={2 3 4 5 6} incompatible={7}
[3]: serial=4 clause_relids={2 3 6} required={2 3 4 5 6} incompatible={7}

[0]: and [2] are two clone variants of "t3.unique1 = t4.unique1". Since the commuting outer join (relid 3) nulls no Var referenced by this clause, the two variants are textually identical, differing only in required_relids and incompatible_relids.
Since the commuting outer join (relid 3) nulls no Var referenced by
this clause, the two variants are textually identical, differing only
in required_relids and incompatible_relids.

When t4 is probed with required_outer = {4, 5}, joinrelids is {4 5 6}.
Both variants are movable into the scan, and neither one's
incompatible_relids overlaps joinrelids, so both end up in
ppi_clauses.

It seems to me that something is wrong somewhere.

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]and [2] are two clone variants of "t3.unique1 = t4.unique1". Since the commuting outer join (relid 3) nulls no Var referenced by this clause, the two variants are textually identical, differing only in required_relids and incompatible_relids. is rejected because relid 3 appears in its
incompatible_relids, and [2]serial=3 clause_relids={4 5 6} required={2 3 4 5 6} incompatible={7} is the one applied; in the commuted
order, where t3/t4 join is performed below t1/t2 join, [2]serial=3 clause_relids={4 5 6} required={2 3 4 5 6} incompatible={7} is rejected
because relid 3 in its required_relids is not available, and [0]and [2] are two clone variants of "t3.unique1 = t4.unique1". Since the commuting outer join (relid 3) nulls no Var referenced by this clause, the two variants are textually identical, differing only in required_relids and incompatible_relids. 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]serial=3 clause_relids={4 5 6} required={2 3 4 5 6} incompatible={7} is the right variant) or above it (the
commuted order, where [0]and [2] are two clone variants of "t3.unique1 = t4.unique1". Since the commuting outer join (relid 3) nulls no Var referenced by this clause, the two variants are textually identical, differing only in required_relids and incompatible_relids. 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?

- Richard

Richard Guo <guofenglinux@gmail.com> 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

On Tue, Sep 8, 2026 at 3:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Hmm, I don't think bizarre join orderings are required. The duplicate
arises whenever the qual pushed down into the parameterized path is
not affected by the commuting outer join, which seems quite a common
case. Here is an example where the duplicate shows up in the selected
plan, with the joins performed just in syntactic order:

explain (costs off)
select * from onek t1
left join onek t2 on t1.unique1 = t2.unique1
left join onek t3 on t2.unique1 = t3.unique1
left join onek t4 on t3.unique1 = t4.unique1 and t3.ten = t4.ten + 0
and t2.unique2 = t4.unique2 + 0
where t1.unique1 < 1;

-> Index Scan using onek_unique1 on onek t4
Index Cond: (unique1 = t3.unique1)
Filter: ((t3.ten = (ten + 0)) AND (t3.ten = (ten + 0)))

Both clone variants of "t3.ten = t4.ten + 0" are enforced at the
scan.

Even when the duplicate is not visible in the final plan, the clause's
selectivity is still applied multiple times, underestimating the row
count, which can easily lead to worse plan choices in bigger queries.

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.

Right. The assertion is only meant as cheap tripwires at the spots
where we select among clones, not as full coverage.

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.

Agreed. I had arrived at the same conclusion. Your fix makes sense
and I think we should apply it. I don't have a test case that exposes
the misbehavior either.

- Richard

On Tue, Sep 8, 2026 at 3:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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;

Looking at the plan of this query, I found a duplicate clause in the
tree, and this time we put it in two different places, so my proposed
Assert cannot catch it (just as you mentioned).

Nested Loop
Output: (64)::information_schema.cardinal_number
Join Filter: (t1.tenthous =
((64)::information_schema.cardinal_number)::integer)
-> Seq Scan on public.tenk1 t3
...
-> Index Scan using tenk1_thous_tenthous on public.tenk1 t1
Index Cond: (t1.tenthous =
(((64)::information_schema.cardinal_number))::integer)

I can reproduce this issue with a simpler query:

set from_collapse_limit to 1;

explain (costs off)
select * from int4_tbl t1,
lateral (select * from tenk1 t2,
lateral (select t2.ten as x offset 0) s0
join tenk1 t3 on t3.unique2 = t1.f1
where t3.unique1 = t2.hundred + s0.x) ss1;
QUERY PLAN
---------------------------------------------------------------------
Nested Loop
Join Filter: (t3.unique2 = t1.f1)
-> Nested Loop
Join Filter: (t3.unique1 = (t2.hundred + (t2.ten)))
-> Seq Scan on tenk1 t2
-> Nested Loop
-> Result
-> Index Scan using tenk1_unique1 on onek t3
Index Cond: (unique1 = (t2.hundred + (t2.ten)))
-> Materialize
-> Seq Scan on int4_tbl t1
(11 rows)

The condition "t3.unique1 = t2.hundred + s0.x" is enforced twice:
once as t3's index condition, and again as a join filter one level
up.

What happens here is that there are two RestrictInfos for this
condition. The first is the original qual (serial 1): when it is
recognized as an equivalence condition, it is absorbed into an EC and
stored there as a source clause, with parent_ec NULL. The second
(serial 4) is created when we build index paths for t3:
generate_implied_equalities_for_column asks create_join_clause for a
clause equating the index column t3.unique1 to the EC's other member,
passing parent_ec = ec to mark it as a potentially redundant join
clause.

In the selected plan, the serial-4 clause is enforced as t3's index
condition, and the serial-1 qual is handed back by
generate_join_implied_equalities at the t2/{s0,t3} join. The
redundancy should then be removed by create_nestloop_path, which drops
join clauses already enforced within the parameterized inner path, but
it matches them by rinfo_serial, so 1 does not match 4, and the
condition is enforced twice.

I think a quick fix is to make create_join_clause copy the
rinfo_serial from an existing clause that connects the same two
members with the opposite parent_ec marking. The two clauses are
really the same condition, and sharing the serial number allows
create_nestloop_path to detect the redundancy. Please see attached.

- Richard

On Tue, Sep 8, 2026 at 12:13 PM Richard Guo <guofenglinux@gmail.com> wrote:

Hmm, I don't think bizarre join orderings are required. The duplicate
arises whenever the qual pushed down into the parameterized path is
not affected by the commuting outer join, which seems quite a common
case. Here is an example where the duplicate shows up in the selected
plan, with the joins performed just in syntactic order:

... and here is a patch to fix it.

Now we have three patches fixing three duplicate-clause issues.

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

... and here is a patch to fix it.

I think the find_clause_by_serial tests are just clutter, really.
Also, why is the last code hunk in get_joinrel_parampathinfo
commented out?

regards, tom lane

On Tue, Sep 8, 2026 at 11:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think the find_clause_by_serial tests are just clutter, really.

Yeah, we can drop it, along with the two Assert(equal(...)) checks
that used it. I'd added it only to increase my confidence that I
understood the situation correctly.

Also, why is the last code hunk in get_joinrel_parampathinfo
commented out?

Because without your drop-redundant-equivalence-clauses patch the
clause-recovery pass for dropped ECs re-adds a clause and trips that
Assert (the cardinal_number query). It needs to be un-commented once
your fix gets applied.

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

On Tue, Sep 8, 2026 at 11:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, why is the last code hunk in get_joinrel_parampathinfo
commented out?

Because without your drop-redundant-equivalence-clauses patch the
clause-recovery pass for dropped ECs re-adds a clause and trips that
Assert (the cardinal_number query). It needs to be un-commented once
your fix gets applied.

Ah. Do you want to just merge those patches and push them as one?
I think we want HEAD-and-19-only for all these changes.

regards, tom lane

On Tue, Sep 8, 2026 at 11:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Ah. Do you want to just merge those patches and push them as one?
I think we want HEAD-and-19-only for all these changes.

Yes. I've merged the clone-dedup fix and your
drop-redundant-equivalence-clauses fix into a single patch (0001
attached), removed the find_clause_by_serial cross-checks, and
un-commented the assertion in get_joinrel_parampathinfo, which now
holds with your fix in place.

I've kept the create_join_clause change as a separate patch (0002),
since it's a distinct issue. There the trouble is that two
RestrictInfos for the same condition end up with different
rinfo_serials, rather than that we enforce multiple clones of the
same clause.

I plan to push both to HEAD and v19 but no further if there are no
objections.

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

Yes. I've merged the clone-dedup fix and your
drop-redundant-equivalence-clauses fix into a single patch (0001
attached), removed the find_clause_by_serial cross-checks, and
un-commented the assertion in get_joinrel_parampathinfo, which now
holds with your fix in place.

The one nit I can find to pick with this is that I don't really like
the description of the clauses of concern as "textually identical".
That makes it sound like surface syntax details (like number of
spaces or presence of an unnecessary table qualifier) matter.
Perhaps say "parse-tree identical", or "equal()"? That seems a bit
jargony but I have no better ideas.

I've kept the create_join_clause change as a separate patch (0002),
since it's a distinct issue. There the trouble is that two
RestrictInfos for the same condition end up with different
rinfo_serials, rather than that we enforce multiple clones of the
same clause.

I wonder if we are going to push the rinfo_serial mechanism too far.
But that's just a vague feeling of discomfort, I can't say there is
anything wrong with fixing it this way.

I plan to push both to HEAD and v19 but no further if there are no
objections.

I wonder if we should leave the Assert bits out of v19. The intent to
have hard prevention of duplicate clauses is a brand new aspiration,
and I have pretty much no faith that no such cases remain. I don't
really want to commit to fixing all such cases in v19.

regards, tom lane