From 1761ad02ecc59a6ba17d2e0a7e3136a53c3e90f6 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 8 Nov 2026 18:55:04 +0100 Subject: [PATCH v3-18] 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 1359dd424bb..0d616bdb266 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -153,6 +153,7 @@ add_paths_to_joinrel(PlannerInfo *root, extra.mergeclause_list = NIL; extra.sjinfo = sjinfo; extra.param_source_rels = NULL; + extra.ojrelids = NULL; /* * See if the inner relation is provably unique for this outer rel. @@ -278,6 +279,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. @@ -948,11 +971,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; /* @@ -1160,11 +1185,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; /* @@ -1333,11 +1360,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 eef8acf1747..46c45f27ca1 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -3387,6 +3387,9 @@ typedef struct SemiAntiJoinFactors * sjinfo is extra info about special joins for selectivity estimation * semifactors is as shown above (only valid for SEMI/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 */ typedef struct JoinPathExtraData { @@ -3396,6 +3399,7 @@ typedef struct JoinPathExtraData SpecialJoinInfo *sjinfo; SemiAntiJoinFactors semifactors; Relids param_source_rels; + Relids ojrelids; } JoinPathExtraData; /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index be055a23124..aba11c61247 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -6501,6 +6501,41 @@ select 1 from t t1 (2 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 f17c2c5c627..4d2a9302c86 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2295,6 +2295,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