From cfaf4f3ab3784daa73078160851aa688a94d661f Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 8 Nov 2026 18:55:04 +0100 Subject: [PATCH v3-master] Reject join input paths parameterized by pushed-down outer joins. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit a798660ebe3 taught try_nestloop_path(), try_mergejoin_path() and try_hashjoin_path() to reject an input path with a nonsensical parameterization. But that check has a gap, making it possible to hit the assertion in get_joinrel_parampathinfo() added by 5b7b5518d0ea. We need to reject all paths 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. That check considered only sjinfo->ojrelid, but that's insufficient because of joins pushed down into the RHS per outer-join identity 3. That relid is added only at the join that completes it, with the relid of the outer join performed there (see add_outer_joins_to_relids). This omission allowed building paths parameterized by a pushed-down outer join, which is still nonsensical. In non-assert builds this is mostly harmless - we still reject the path, just a bit later. To fix, determine the full set of outer joins computed at the current join. That includes sjinfo's own OJ plus any pushed-down OJs completed there. We do this once in add_paths_to_joinrel(), and 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. Backpatch to 16, where 2489d76c490 made Vars outer-join-aware and 9df8f903eb6 introduced the delayed addition of pushed-down outer join relids. The checks added by a798660ebe3 did not cover this case. The new JoinPathExtraData field needs to go at the end of the struct, to avoid an ABI break. It's allocated only in add_paths_to_joinrel (on stack), and only passed through a pointer, so this should be safe. Author: Tomas Vondra Reviewed-by: Tender Wang Reviewed-by: Álvaro Herrera Backpatch-through: 16 Discussion: https://postgr.es/m/72d81c95-60fc-48bb-b5e2-e3346c17dd11@vondra.me --- src/backend/optimizer/path/joinpath.c | 53 +++++++++++++++++++++------ src/include/nodes/pathnodes.h | 4 ++ src/test/regress/expected/join.out | 35 ++++++++++++++++++ src/test/regress/sql/join.sql | 15 ++++++++ 4 files changed, 95 insertions(+), 12 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 9d9e6fad6a1..beb2856dd21 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -154,6 +154,7 @@ add_paths_to_joinrel(PlannerInfo *root, extra.sjinfo = sjinfo; extra.param_source_rels = NULL; extra.pgs_mask = joinrel->pgs_mask; + extra.ojrelids = NULL; /* * Give extensions a chance to take control. In particular, an extension @@ -313,6 +314,28 @@ 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. @@ -1026,11 +1049,13 @@ try_nestloop_path(PlannerInfo *root, * If we are forming an outer join at this join, it's nonsensical to use * 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.) + * 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; /* @@ -1241,11 +1266,13 @@ try_mergejoin_path(PlannerInfo *root, * If we are forming an outer join at this join, it's nonsensical to use * 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.) + * 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; /* @@ -1415,11 +1442,13 @@ try_hashjoin_path(PlannerInfo *root, * If we are forming an outer join at this join, it's nonsensical to use * 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.) + * 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 ce22529bec0..2ba1dbfb67f 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -3642,6 +3642,9 @@ typedef struct SemiAntiJoinFactors * RIGHT_ANTI/inner_unique joins) * param_source_rels are OK targets for parameterization of result paths * pgs_mask is a bitmask of PGS_* constants to limit the join strategy + * 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 */ typedef struct JoinPathExtraData { @@ -3652,6 +3655,7 @@ typedef struct JoinPathExtraData SemiAntiJoinFactors semifactors; Relids param_source_rels; uint64 pgs_mask; + Relids ojrelids; } JoinPathExtraData; /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index b41f2157490..a4b2c8da0f9 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7248,6 +7248,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 eb7135efe8b..72184c326d4 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2577,6 +2577,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