From 614b03eaf6c6349c0bcdaed38ab1eb19c9b9e7ab Mon Sep 17 00:00:00 2001 From: Tomas Vondra 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