Hi, The second bug / assert failure I ran into when running the randomized join generator seems to be hitting a gap in a798660ebe3:
Defend against bogus parameterization of join input paths.
A trivial reproducer for this is:
CREATE TABLE t (a integer, b integer);
CREATE INDEX ON t (a, b);
EXPLAIN
SELECT 1 FROM t t1
WHERE t1.a IN (SELECT t2.a
FROM t t2
LEFT JOIN t t3 ON (t2.a = t3.a)
LEFT JOIN (t t4 LEFT JOIN t t5 ON (t4.a = t5.a))
ON (t3.a = t4.a)
WHERE coalesce(t5.b, 0) = t2.b);
Which trips over this asssert in get_baserel_parampathinfo():
Assert(!bms_overlap(joinrel->relids, required_outer));
This is not an area I know very well, but it seems the defense added by
a798660ebe3 considers only sjinfo->ojrelid. But that does not seem to be
enough for joins pushed down through identity 3. IIUC those are added to
ojrelid only for the join that completes it, and so the sjinfo->ojrelid
may not be entirely "up to date", and the parameterized paths have the
same issue as those rejected by a798660ebe3.
The attached patch fixes that by adding a new JoinPathExtraData field
ojrelids, and calculating it from joinrel/inner/outer like this:
if (sjinfo->ojrelid != 0)
{
extra.ojrelids = bms_difference(joinrel->relids, outerrel->relids);
extra.ojrelids = bms_del_members(extra.ojrelids, innerrel->relids);
extra.ojrelids = bms_add_member(extra.ojrelids, sjinfo->ojrelid);
}
There's a comment in add_paths_to_joinrel explaining why I think this is
correct, but is it? Perhaps I'm forgetting something.
Also, the new field would be a problem for backpatching (the problem
exists since 16), and a new field might break ABI. Perhaps it'd be
better to just calculate it in the handful of places that need it.
regards
--
Tomas Vondra
From 614b03eaf6c6349c0bcdaed38ab1eb19c9b9e7ab Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Sat, 31 Oct 2026 20:29:12 -0400 Subject: [PATCH] Reject join input paths parameterized by pushed-down outer joins. Commit a798660ebe3 taught try_nestloop_path(), try_mergejoin_path() and try_hashjoin_path() to reject an input path whose parameterization includes the outer join being formed at the current join, since a value that is supposed to be nulled by that outer join can't be available before the join is done. However, that considered only sjinfo->ojrelid. When an outer join has been pushed down into the RHS of another one per outer-join identity 3, its relid is added only at the join that completes it, along with the relid of the outer join actually being performed there (see add_outer_joins_to_relids). An input path parameterized by such a pushed-down outer join is just as nonsensical, but we failed to reject it, so we could build a join path whose required_outer overlaps the joinrel's own relids, causing an assertion failure in get_joinrel_parampathinfo(). To fix, determine the set of outer joins computed at the current join, that is sjinfo's own OJ plus any pushed-down OJs completed there, once in add_paths_to_joinrel(), save it in a new JoinPathExtraData field, and check the input paths' parameterization against that whole set. The partial-path variants need no change, since they never accept input paths whose parameterization isn't satisfied by the outer rel. In non-assert builds the bogus paths seem to be harmless apart from wasted planning effort, since a join path parameterized by part of its own relids can never be used in a complete plan. The assertion failure is reachable since v16, so this should be back-patched; but that's also an issue because the new JoinPathExtraData field is ABI break. Alternatively, we can probably compute the ojrelids in the various places needing it. --- src/backend/optimizer/path/joinpath.c | 49 ++++++++++++++++++++++----- src/include/nodes/pathnodes.h | 4 +++ src/test/regress/expected/join.out | 35 +++++++++++++++++++ src/test/regress/sql/join.sql | 15 ++++++++ 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 91bac051fcc..39bdc3f1241 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -150,6 +150,7 @@ add_paths_to_joinrel(PlannerInfo *root, extra.mergeclause_list = NIL; extra.sjinfo = sjinfo; extra.param_source_rels = NULL; + extra.ojrelids = NULL; extra.pgs_mask = joinrel->pgs_mask; /* @@ -310,6 +311,27 @@ add_paths_to_joinrel(PlannerInfo *root, extra.param_source_rels = bms_add_members(extra.param_source_rels, joinrel->lateral_relids); + /* + * Identify the outer joins computed at this join, since it'd be + * nonsensical to use an input path that is parameterized by any of them + * (see try_nestloop_path). + * + * Besides sjinfo's own OJ, this includes any pushed-down OJs that are + * completed at this join per outer-join identity 3; those are the members + * of the joinrel's relids that are in neither input rel. + * + * (We add sjinfo->ojrelid explicitly because it won't be in the joinrel's + * relids if this join has itself been pushed down into the RHS of a lower + * outer join.) If sjinfo has no ojrelid, then no outer joins can be + * computed here; see add_outer_joins_to_relids. + */ + if (sjinfo->ojrelid != 0) + { + extra.ojrelids = bms_difference(joinrel->relids, outerrel->relids); + extra.ojrelids = bms_del_members(extra.ojrelids, innerrel->relids); + extra.ojrelids = bms_add_member(extra.ojrelids, sjinfo->ojrelid); + } + /* * 1. Consider mergejoin paths where both relations must be explicitly * sorted. Skip this if we can't mergejoin. @@ -901,10 +923,13 @@ try_nestloop_path(PlannerInfo *root, * an input path that uses the outer join as part of its parameterization. * (This can happen despite our join order restrictions, since those apply * to what is in an input relation not what its parameters are.) + * + * The same goes for any pushed-down outer joins that are completed at this + * join, so we must check against all of extra->ojrelids, not just + * sjinfo->ojrelid. */ - if (extra->sjinfo->ojrelid != 0 && - (bms_is_member(extra->sjinfo->ojrelid, inner_paramrels) || - bms_is_member(extra->sjinfo->ojrelid, outer_paramrels))) + if (bms_overlap(extra->ojrelids, inner_paramrels) || + bms_overlap(extra->ojrelids, outer_paramrels)) return; /* @@ -1116,10 +1141,13 @@ try_mergejoin_path(PlannerInfo *root, * an input path that uses the outer join as part of its parameterization. * (This can happen despite our join order restrictions, since those apply * to what is in an input relation not what its parameters are.) + * + * The same goes for any pushed-down outer joins that are completed at this + * join, so we must check against all of extra->ojrelids, not just + * sjinfo->ojrelid. */ - if (extra->sjinfo->ojrelid != 0 && - (bms_is_member(extra->sjinfo->ojrelid, PATH_REQ_OUTER(inner_path)) || - bms_is_member(extra->sjinfo->ojrelid, PATH_REQ_OUTER(outer_path)))) + if (bms_overlap(extra->ojrelids, PATH_REQ_OUTER(inner_path)) || + bms_overlap(extra->ojrelids, PATH_REQ_OUTER(outer_path))) return; /* @@ -1290,10 +1318,13 @@ try_hashjoin_path(PlannerInfo *root, * an input path that uses the outer join as part of its parameterization. * (This can happen despite our join order restrictions, since those apply * to what is in an input relation not what its parameters are.) + * + * The same goes for any pushed-down outer joins that are completed at this + * join, so we must check against all of extra->ojrelids, not just + * sjinfo->ojrelid. */ - if (extra->sjinfo->ojrelid != 0 && - (bms_is_member(extra->sjinfo->ojrelid, PATH_REQ_OUTER(inner_path)) || - bms_is_member(extra->sjinfo->ojrelid, PATH_REQ_OUTER(outer_path)))) + if (bms_overlap(extra->ojrelids, PATH_REQ_OUTER(inner_path)) || + bms_overlap(extra->ojrelids, PATH_REQ_OUTER(outer_path))) return; /* diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index ff46a9f7419..62cbfbae932 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -3638,6 +3638,9 @@ typedef struct SemiAntiJoinFactors * semifactors is as shown above (only valid for SEMI/ANTI/RIGHT_SEMI/ * RIGHT_ANTI/inner_unique joins) * param_source_rels are OK targets for parameterization of result paths + * ojrelids are the outer joins computed at this join (sjinfo's own OJ, if + * any, plus any pushed-down OJs completed here); input paths must not + * be parameterized by any of these * pgs_mask is a bitmask of PGS_* constants to limit the join strategy */ typedef struct JoinPathExtraData @@ -3648,6 +3651,7 @@ typedef struct JoinPathExtraData SpecialJoinInfo *sjinfo; SemiAntiJoinFactors semifactors; Relids param_source_rels; + Relids ojrelids; uint64 pgs_mask; } JoinPathExtraData; diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 4ec9291833b..2fb4c1ae043 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7162,6 +7162,41 @@ select 1 from t t1 (3 rows) rollback; +-- +-- check a similar case where the parameterization refers to an outer join +-- that was pushed down per outer-join identity 3 +-- +explain (costs off) +select 1 from tenk1 t1 +where t1.thousand in + (select t2.thousand + from tenk1 t2 + left join tenk1 t3 on t2.thousand = t3.thousand + left join (tenk1 t4 left join tenk1 t5 on t4.thousand = t5.thousand) + on t3.thousand = t4.thousand + where coalesce(t5.tenthous, 0) = t2.tenthous); + QUERY PLAN +-------------------------------------------------------------------------------------- + Hash Right Semi Join + Hash Cond: (t2.thousand = t1.thousand) + -> Hash Left Join + Hash Cond: (t3.thousand = t4.thousand) + Filter: (COALESCE(t5.tenthous, 0) = t2.tenthous) + -> Merge Left Join + Merge Cond: (t2.thousand = t3.thousand) + -> Index Only Scan using tenk1_thous_tenthous on tenk1 t2 + -> Materialize + -> Index Only Scan using tenk1_thous_tenthous on tenk1 t3 + -> Hash + -> Hash Left Join + Hash Cond: (t4.thousand = t5.thousand) + -> Index Only Scan using tenk1_thous_tenthous on tenk1 t4 + -> Hash + -> Index Only Scan using tenk1_thous_tenthous on tenk1 t5 + -> Hash + -> Index Only Scan using tenk1_thous_tenthous on tenk1 t1 +(18 rows) + -- -- check a case in which a PlaceHolderVar forces join order -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 381c57a5a88..730c2f62fb5 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2540,6 +2540,21 @@ select 1 from t t1 rollback; +-- +-- check a similar case where the parameterization refers to an outer join +-- that was pushed down per outer-join identity 3 +-- + +explain (costs off) +select 1 from tenk1 t1 +where t1.thousand in + (select t2.thousand + from tenk1 t2 + left join tenk1 t3 on t2.thousand = t3.thousand + left join (tenk1 t4 left join tenk1 t5 on t4.thousand = t5.thousand) + on t3.thousand = t4.thousand + where coalesce(t5.tenthous, 0) = t2.tenthous); + -- -- check a case in which a PlaceHolderVar forces join order -- -- 2.55.0
crash-minimized.sql
Description: application/sql
