From a798660ebe3ff1feb310db13b957c5cda4c8c50d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 29 Jun 2023 12:12:52 -0400 Subject: [PATCH] Defend against bogus parameterization of join input paths. An outer join cannot be formed using an input path that is parameterized by a value that is supposed to be nulled by the outer join. This is obviously nonsensical, and it could lead to a bad plan being selected; although currently it seems that we'll hit various sanity-check assertions first. I think that such cases were formerly prevented by the delay_upper_joins mechanism, but now that that's gone we need an explicit check. (Perhaps we should avoid generating baserel paths that could lead to this situation in the first place; but it seems like having a defense at the join level would be a good idea anyway.) Richard Guo and Tom Lane, per report from Jaime Casanova Discussion: https://postgr.es/m/CAJKUy5g2uZRrUDZJ8p-=giwcSHVUn0c9nmdxPSY0jF0Ov8VoEA@mail.gmail.com --- src/backend/optimizer/path/joinpath.c | 33 +++++++++++++++++++++++++++ src/test/regress/expected/join.out | 23 +++++++++++++++++++ src/test/regress/sql/join.sql | 22 ++++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index c2f211a60d..f047ad9ba4 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -698,6 +698,17 @@ try_nestloop_path(PlannerInfo *root, Relids inner_paramrels = PATH_REQ_OUTER(inner_path); Relids outer_paramrels = PATH_REQ_OUTER(outer_path); + /* + * 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.) + */ + if (extra->sjinfo->ojrelid != 0 && + (bms_is_member(extra->sjinfo->ojrelid, inner_paramrels) || + bms_is_member(extra->sjinfo->ojrelid, outer_paramrels))) + return; + /* * Paths are parameterized by top-level parents, so run parameterization * tests on the parent relids. @@ -908,6 +919,17 @@ try_mergejoin_path(PlannerInfo *root, return; } + /* + * 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.) + */ + 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)))) + return; + /* * Check to see if proposed path is still parameterized, and reject if the * parameterization wouldn't be sensible. @@ -1054,6 +1076,17 @@ try_hashjoin_path(PlannerInfo *root, Relids required_outer; JoinCostWorkspace workspace; + /* + * 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.) + */ + 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)))) + return; + /* * Check to see if proposed path is still parameterized, and reject if the * parameterization wouldn't be sensible. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 6917faec14..9b8638f286 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5063,6 +5063,29 @@ select 1 from ---------- (0 rows) +-- +-- check a case where we formerly generated invalid parameterized paths +-- +begin; +create temp table t (a int unique); +explain (costs off) +select 1 from t t1 + join lateral (select t1.a from (select 1) foo offset 0) as s1 on true + join + (select 1 from t t2 + inner join (t t3 + left join (t t4 left join t t5 on t4.a = 1) + on t3.a = t4.a) + on false + where t3.a = coalesce(t5.a,1)) as s2 + on true; + QUERY PLAN +-------------------------- + Result + One-Time Filter: false +(2 rows) + +rollback; -- -- 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 55080bec9a..3e5032b04d 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1751,6 +1751,28 @@ select 1 from on false, lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; +-- +-- check a case where we formerly generated invalid parameterized paths +-- + +begin; + +create temp table t (a int unique); + +explain (costs off) +select 1 from t t1 + join lateral (select t1.a from (select 1) foo offset 0) as s1 on true + join + (select 1 from t t2 + inner join (t t3 + left join (t t4 left join t t5 on t4.a = 1) + on t3.a = t4.a) + on false + where t3.a = coalesce(t5.a,1)) as s2 + on true; + +rollback; + -- -- check a case in which a PlaceHolderVar forces join order --