Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uVaGX-002GHF-BY for pgsql-bugs@arkaria.postgresql.org; Sat, 28 Jun 2025 18:27:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uVaGV-008yM4-DE for pgsql-bugs@arkaria.postgresql.org; Sat, 28 Jun 2025 18:27:04 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uVaGU-008yLw-Om for pgsql-bugs@lists.postgresql.org; Sat, 28 Jun 2025 18:27:03 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1uVaGS-004TgS-1d for pgsql-bugs@lists.postgresql.org; Sat, 28 Jun 2025 18:27:02 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 55SIQw6o2329681; Sat, 28 Jun 2025 14:26:58 -0400 From: Tom Lane To: Alexander Lakhin cc: Richard Guo , pgsql-bugs@lists.postgresql.org Subject: Re: BUG #18953: Planner fails to build plan for complex query with LATERAL references In-reply-to: <6072cd36-8d7b-457a-a7c6-7cf41c05fe6c@gmail.com> References: <18953-1c9883a9d4afeb30@postgresql.org> <1268645.1749765012@sss.pgh.pa.us> <1499514.1749848324@sss.pgh.pa.us> <527134.1750087495@sss.pgh.pa.us> <835659.1750177751@sss.pgh.pa.us> <837b0dce-b2f3-4811-bccf-f42e8985d6e2@gmail.com> <3040798.1750617116@sss.pgh.pa.us> <3886446.1750703041@sss.pgh.pa.us> <1487176.1750963077@sss.pgh.pa.us> <2056150.1751049350@sss.pgh.pa.us> <2075249.1751060239@sss.pgh.pa.us> <6072cd36-8d7b-457a-a7c6-7cf41c05fe6c@gmail.com> Comments: In-reply-to Alexander Lakhin message dated "Sat, 28 Jun 2025 09:00:01 +0300" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <2329666.1751135208.0@sss.pgh.pa.us> Date: Sat, 28 Jun 2025 14:26:58 -0400 Message-ID: <2329680.1751135218@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <2329666.1751135208.1@sss.pgh.pa.us> Alexander Lakhin writes: > 28.06.2025 00:37, Tom Lane wrote: >> ... BTW, looking at these two patches again, I wonder if it'd be >> better to put the hackery for SubLink conversion into >> identify_current_nestloop_params? That'd remove the need to argue >> that we don't have to fix both copies of a nestloop-parameter PHV. > I've discovered a query which fails with an error after a16ef313f with > both v2 patches applied: Thank you! That demonstrates an unrelated oversight in a16ef313f: we need to apply replace_nestloop_params() to the expression of a PHV that is chosen to be a nestloop param. That's because it could contain references to Vars/PHVs that will be supplied by some upper nestloop. In v3 attached, I moved the SubLink hacking into identify_current_nestloop_params as suggested above. But replace_nestloop_params is local to createplan.c, and anyway we can't run it until all of the current nestloop's NLPs have been removed from root->curOuterParams, else it might make invalid replacements. So that has to be done in create_nestloop_plan. I squashed everything into one patch, too. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v3-0001-fix-bug-18953-some-more.patch"; charset="us-ascii" Content-ID: <2329666.1751135208.2@sss.pgh.pa.us> Content-Description: v3-0001-fix-bug-18953-some-more.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimiz= er/plan/createplan.c index 8baf36ba4b7..4d059eba3f0 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root, NestLoop *join_plan; Plan *outer_plan; Plan *inner_plan; + Relids outerrelids; List *tlist =3D build_path_tlist(root, &best_path->jpath.path); List *joinrestrictclauses =3D best_path->jpath.joinrestrictinfo; List *joinclauses; @@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root, outer_plan =3D create_plan_recurse(root, best_path->jpath.outerjoinpath,= 0); = /* For a nestloop, include outer relids in curOuterRels for inner side *= / - root->curOuterRels =3D bms_union(root->curOuterRels, - best_path->jpath.outerjoinpath->parent->relids); + outerrelids =3D best_path->jpath.outerjoinpath->parent->relids; + root->curOuterRels =3D bms_union(root->curOuterRels, outerrelids); = inner_plan =3D create_plan_recurse(root, best_path->jpath.innerjoinpath,= 0); = @@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root, * node, and remove them from root->curOuterParams. */ nestParams =3D identify_current_nestloop_params(root, - best_path->jpath.outerjoinpath); + outerrelids, + PATH_REQ_OUTER((Path *) best_path)); = /* * While nestloop parameters that are Vars had better be available from @@ -4423,32 +4425,57 @@ create_nestloop_plan(PlannerInfo *root, * that are PHVs won't be. In such cases we must add them to the * outer_plan's tlist, since the executor's NestLoopParam machinery * requires the params to be simple outer-Var references to that tlist. + * (This is cheating a little bit, because the outer path's required-out= er + * relids might not be enough to allow evaluating such a PHV. But in + * practice, if we could have evaluated the PHV at the nestloop node, we + * can do so in the outer plan too.) */ outer_tlist =3D outer_plan->targetlist; outer_parallel_safe =3D outer_plan->parallel_safe; foreach(lc, nestParams) { NestLoopParam *nlp =3D (NestLoopParam *) lfirst(lc); + PlaceHolderVar *phv; TargetEntry *tle; = if (IsA(nlp->paramval, Var)) continue; /* nothing to do for simple Vars */ - if (tlist_member((Expr *) nlp->paramval, outer_tlist)) + /* Otherwise it must be a PHV */ + phv =3D castNode(PlaceHolderVar, nlp->paramval); + + if (tlist_member((Expr *) phv, outer_tlist)) continue; /* already available */ = + /* + * It's possible that nestloop parameter PHVs selected to evaluate + * here contain references to surviving root->curOuterParams items + * (that is, they reference values that will be supplied by some + * higher-level nestloop). Those need to be converted to Params now. + */ + phv =3D (PlaceHolderVar *) replace_nestloop_params(root, (Node *) phv); + + /* + * It's not actually necessary to update the NestLoopParam entry, + * because the two PHVs would be equal() anyway and thus setrefs.c + * would still replace the NestLoopParam's PHV with an outer-plan + * reference Var. (This is also why we needn't run + * replace_nestloop_params before checking tlist_member.) But we + * might as well do it for cleanliness. + */ + nlp->paramval =3D (Var *) phv; + /* Make a shallow copy of outer_tlist, if we didn't already */ if (outer_tlist =3D=3D outer_plan->targetlist) outer_tlist =3D list_copy(outer_tlist); /* ... and add the needed expression */ - tle =3D makeTargetEntry((Expr *) copyObject(nlp->paramval), + tle =3D makeTargetEntry((Expr *) copyObject(phv), list_length(outer_tlist) + 1, NULL, true); outer_tlist =3D lappend(outer_tlist, tle); /* ... and track whether tlist is (still) parallel-safe */ if (outer_parallel_safe) - outer_parallel_safe =3D is_parallel_safe(root, - (Node *) nlp->paramval); + outer_parallel_safe =3D is_parallel_safe(root, (Node *) phv); } if (outer_tlist !=3D outer_plan->targetlist) outer_plan =3D change_plan_targetlist(outer_plan, outer_tlist, diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimi= zer/util/paramassign.c index 9836abf9479..4c13c5931b4 100644 --- a/src/backend/optimizer/util/paramassign.c +++ b/src/backend/optimizer/util/paramassign.c @@ -599,38 +599,31 @@ process_subquery_nestloop_params(PlannerInfo *root, = List *subplan_params) } = /* - * Identify any NestLoopParams that should be supplied by a NestLoop plan - * node with the specified lefthand input path. Remove them from the act= ive - * root->curOuterParams list and return them as the result list. + * Identify any NestLoopParams that should be supplied by a NestLoop + * plan node with the specified lefthand rels and required-outer rels. + * Remove them from the active root->curOuterParams list and return + * them as the result list. * - * XXX Here we also hack up the returned Vars and PHVs so that they do no= t - * contain nullingrel sets exceeding what is available from the outer sid= e. - * This is needed if we have applied outer join identity 3, - * (A leftjoin B on (Pab)) leftjoin C on (Pb*c) - * =3D A leftjoin (B leftjoin C on (Pbc)) on (Pab) - * and C contains lateral references to B. It's still safe to apply the - * identity, but the parser will have created those references in the for= m - * "b*" (i.e., with varnullingrels listing the A/B join), while what we w= ill - * have available from the nestloop's outer side is just "b". We deal wi= th - * that here by stripping the nullingrels down to what is available from = the - * outer side according to leftrelids. - * - * That fixes matters for the case of forward application of identity 3. - * If the identity was applied in the reverse direction, we will have - * parameter Vars containing too few nullingrel bits rather than too many= . - * Currently, that causes no problems because setrefs.c applies only a - * subset check to nullingrels in NestLoopParams, but we'd have to work - * harder if we ever want to tighten that check. This is all pretty anno= ying - * because it greatly weakens setrefs.c's cross-check, but the alternativ= e + * Vars and PHVs appearing in the result list must have nullingrel sets + * that could validly appear in the lefthand rel's output. Ordinarily th= at + * would be true already, but if we have applied outer join identity 3, + * there could be more or fewer nullingrel bits in the nodes appearing in + * curOuterParams than are in the nominal leftrelids. We deal with that = by + * forcing their nullingrel sets to include exactly the outer-join relids + * that appear in leftrelids and can null the respective Var or PHV. + * This fix is a bit ad-hoc and intellectually unsatisfactory, because it= 's + * essentially jumping to the conclusion that we've placed evaluation of + * the nestloop parameters correctly, and thus it defeats the intent of t= he + * subsequent nullingrel cross-checks in setrefs.c. But the alternative * seems to be to generate multiple versions of each laterally-parameteri= zed * subquery, which'd be unduly expensive. */ List * -identify_current_nestloop_params(PlannerInfo *root, Path *leftpath) +identify_current_nestloop_params(PlannerInfo *root, + Relids leftrelids, + Relids outerrelids) { List *result; - Relids leftrelids =3D leftpath->parent->relids; - Relids outerrelids =3D PATH_REQ_OUTER(leftpath); Relids allleftrelids; ListCell *cell; = @@ -661,25 +654,58 @@ identify_current_nestloop_params(PlannerInfo *root, = Path *leftpath) bms_is_member(nlp->paramval->varno, leftrelids)) { Var *var =3D (Var *) nlp->paramval; + RelOptInfo *rel =3D root->simple_rel_array[var->varno]; = root->curOuterParams =3D foreach_delete_current(root->curOuterParams, cell); - var->varnullingrels =3D bms_intersect(var->varnullingrels, + var->varnullingrels =3D bms_intersect(rel->nulling_relids, leftrelids); result =3D lappend(result, nlp); } else if (IsA(nlp->paramval, PlaceHolderVar)) { PlaceHolderVar *phv =3D (PlaceHolderVar *) nlp->paramval; - Relids eval_at =3D find_placeholder_info(root, phv)->ph_eval_at; + PlaceHolderInfo *phinfo =3D find_placeholder_info(root, phv); + Relids eval_at =3D phinfo->ph_eval_at; = if (bms_is_subset(eval_at, allleftrelids) && bms_overlap(eval_at, leftrelids)) { root->curOuterParams =3D foreach_delete_current(root->curOuterParams, cell); - phv->phnullingrels =3D bms_intersect(phv->phnullingrels, - leftrelids); + + /* + * Deal with an edge case: if the PHV was pulled up out of a + * subquery and it contains a subquery that was originally + * pushed down from this query level, then that will still be + * represented as a SubLink, because SS_process_sublinks won't + * recurse into outer PHVs, so it didn't get transformed + * during expression preprocessing in the subquery. We need a + * version of the PHV that has a SubPlan, which we can get + * from the current query level's placeholder_list. This is + * quite grotty of course, but dealing with it earlier in the + * handling of subplan params would be just as grotty, and it + * might end up being a waste of cycles if we don't decide to + * treat the PHV as a NestLoopParam. (Perhaps that whole + * mechanism should be redesigned someday, but today is not + * that day.) + */ + if (root->parse->hasSubLinks) + { + phv =3D copyObject(phinfo->ph_var); + + /* + * The ph_var will have empty nullingrels, but that + * doesn't matter since we're about to overwrite + * phv->phnullingrels. Other fields should be OK already. + */ + nlp->paramval =3D (Var *) phv; + } + + phv->phnullingrels =3D + bms_intersect(get_placeholder_nulling_relids(root, phinfo), + leftrelids); + result =3D lappend(result, nlp); } } diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimi= zer/util/placeholder.c index 41a4c81e94a..e1cd00a72fb 100644 --- a/src/backend/optimizer/util/placeholder.c +++ b/src/backend/optimizer/util/placeholder.c @@ -545,3 +545,43 @@ contain_placeholder_references_walker(Node *node, return expression_tree_walker(node, contain_placeholder_references_walke= r, context); } + +/* + * Compute the set of outer-join relids that can null a placeholder. + * + * This is analogous to RelOptInfo.nulling_relids for Vars, but we comput= e it + * on-the-fly rather than saving it somewhere. Currently the value is ne= eded + * at most once per query, so there's little value in doing otherwise. I= f it + * ever gains more widespread use, perhaps we should cache the result in + * PlaceHolderInfo. + */ +Relids +get_placeholder_nulling_relids(PlannerInfo *root, PlaceHolderInfo *phinfo= ) +{ + Relids result =3D NULL; + int relid =3D -1; + + /* + * Form the union of all potential nulling OJs for each baserel included + * in ph_eval_at. + */ + while ((relid =3D bms_next_member(phinfo->ph_eval_at, relid)) > 0) + { + RelOptInfo *rel =3D root->simple_rel_array[relid]; + + /* ignore the RTE_GROUP RTE */ + if (relid =3D=3D root->group_rtindex) + continue; + + if (rel =3D=3D NULL) /* must be an outer join */ + { + Assert(bms_is_member(relid, root->outer_join_rels)); + continue; + } + result =3D bms_add_members(result, rel->nulling_relids); + } + + /* Now remove any OJs already included in ph_eval_at, and we're done. */ + result =3D bms_del_members(result, phinfo->ph_eval_at); + return result; +} diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/p= aramassign.h index d30d20de299..bbf7214289b 100644 --- a/src/include/optimizer/paramassign.h +++ b/src/include/optimizer/paramassign.h @@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(Plan= nerInfo *root, extern void process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params); extern List *identify_current_nestloop_params(PlannerInfo *root, - Path *leftpath); + Relids leftrelids, + Relids outerrelids); extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype, int32 paramtypmod, Oid paramcollation); extern int assign_special_exec_param(PlannerInfo *root); diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/p= laceholder.h index d351045e2e0..db92d8861ba 100644 --- a/src/include/optimizer/placeholder.h +++ b/src/include/optimizer/placeholder.h @@ -30,5 +30,7 @@ extern void add_placeholders_to_joinrel(PlannerInfo *roo= t, RelOptInfo *joinrel, SpecialJoinInfo *sjinfo); extern bool contain_placeholder_references_to(PlannerInfo *root, Node *cl= ause, int relid); +extern Relids get_placeholder_nulling_relids(PlannerInfo *root, + PlaceHolderInfo *phinfo); = #endif /* PLACEHOLDER_H */ diff --git a/src/test/regress/expected/join.out b/src/test/regress/expecte= d/join.out index c292f04fdba..390aabfb34b 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -4119,6 +4119,164 @@ select * from int8_tbl t1 (16 rows) = rollback; +-- ... not that the initial replacement didn't have some bugs too +begin; +create temp table t(i int primary key); +explain (verbose, costs off) +select * from t t1 + left join (select 1 as x, * from t t2(i2)) t2ss on t1.i =3D t2ss.i2 + left join t t3(i3) on false + left join t t4(i4) on t4.i4 > t2ss.x; + QUERY PLAN = +---------------------------------------------------------- + Nested Loop Left Join + Output: t1.i, (1), t2.i2, i3, t4.i4 + -> Nested Loop Left Join + Output: t1.i, t2.i2, (1), i3 + Join Filter: false + -> Hash Left Join + Output: t1.i, t2.i2, (1) + Inner Unique: true + Hash Cond: (t1.i =3D t2.i2) + -> Seq Scan on pg_temp.t t1 + Output: t1.i + -> Hash + Output: t2.i2, (1) + -> Seq Scan on pg_temp.t t2 + Output: t2.i2, 1 + -> Result + Output: i3 + One-Time Filter: false + -> Memoize + Output: t4.i4 + Cache Key: (1) + Cache Mode: binary + -> Index Only Scan using t_pkey on pg_temp.t t4 + Output: t4.i4 + Index Cond: (t4.i4 > (1)) +(25 rows) + +explain (verbose, costs off) +select * from + (select k from + (select i, coalesce(i, j) as k from + (select i from t union all select 0) + join (select 1 as j limit 1) on i =3D j) + right join (select 2 as x) on true + join (select 3 as y) on i is not null + ), + lateral (select k as kl limit 1); + QUERY PLAN = +------------------------------------------------------------------- + Nested Loop + Output: COALESCE(t.i, (1)), ((COALESCE(t.i, (1)))) + -> Limit + Output: 1 + -> Result + Output: 1 + -> Nested Loop + Output: t.i, ((COALESCE(t.i, (1)))) + -> Result + Output: t.i, COALESCE(t.i, (1)) + -> Append + -> Index Only Scan using t_pkey on pg_temp.t + Output: t.i + Index Cond: (t.i =3D (1)) + -> Result + Output: 0 + One-Time Filter: ((1) =3D 0) + -> Limit + Output: ((COALESCE(t.i, (1)))) + -> Result + Output: (COALESCE(t.i, (1))) +(21 rows) + +rollback; +-- PHVs containing SubLinks are quite tricky to get right +explain (verbose, costs off) +select * +from int8_tbl i8 + inner join + (select (select true) as x + from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1 + where i4.f1 =3D 0) ss2 on true + right join (select false as z) ss3 on true, + lateral (select i8.q2 as q2l where x limit 1) ss4 +where i8.q2 =3D 123; + QUERY PLAN = +---------------------------------------------------------------- + Nested Loop + Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2) + InitPlan 1 + -> Result + Output: true + InitPlan 2 + -> Result + Output: true + -> Seq Scan on public.int4_tbl i4 + Output: i4.f1 + Filter: (i4.f1 =3D 0) + -> Nested Loop + Output: i8.q1, i8.q2, (i8.q2) + -> Subquery Scan on ss1 + Output: ss1.y, (InitPlan 1).col1 + -> Limit + Output: NULL::integer + -> Result + Output: NULL::integer + -> Nested Loop + Output: i8.q1, i8.q2, (i8.q2) + -> Seq Scan on public.int8_tbl i8 + Output: i8.q1, i8.q2 + Filter: (i8.q2 =3D 123) + -> Limit + Output: (i8.q2) + -> Result + Output: i8.q2 + One-Time Filter: ((InitPlan 1).col1) +(29 rows) + +explain (verbose, costs off) +select * +from int8_tbl i8 + inner join + (select (select true) as x + from int4_tbl i4, lateral (select 1 as y limit 1) ss1 + where i4.f1 =3D 0) ss2 on true + right join (select false as z) ss3 on true, + lateral (select i8.q2 as q2l where x limit 1) ss4 +where i8.q2 =3D 123; + QUERY PLAN = +---------------------------------------------------------------- + Nested Loop + Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2) + InitPlan 1 + -> Result + Output: true + InitPlan 2 + -> Result + Output: true + -> Limit + Output: NULL::integer + -> Result + Output: NULL::integer + -> Nested Loop + Output: i8.q1, i8.q2, (i8.q2) + -> Seq Scan on public.int4_tbl i4 + Output: i4.f1, (InitPlan 1).col1 + Filter: (i4.f1 =3D 0) + -> Nested Loop + Output: i8.q1, i8.q2, (i8.q2) + -> Seq Scan on public.int8_tbl i8 + Output: i8.q1, i8.q2 + Filter: (i8.q2 =3D 123) + -> Limit + Output: (i8.q2) + -> Result + Output: i8.q2 + One-Time Filter: ((InitPlan 1).col1) +(27 rows) + -- Test proper handling of appendrel PHVs during useless-RTE removal explain (costs off) select * from diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 88d2204e447..f6e7070db65 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1361,6 +1361,52 @@ select * from int8_tbl t1 on true; rollback; = +-- ... not that the initial replacement didn't have some bugs too +begin; +create temp table t(i int primary key); + +explain (verbose, costs off) +select * from t t1 + left join (select 1 as x, * from t t2(i2)) t2ss on t1.i =3D t2ss.i2 + left join t t3(i3) on false + left join t t4(i4) on t4.i4 > t2ss.x; + +explain (verbose, costs off) +select * from + (select k from + (select i, coalesce(i, j) as k from + (select i from t union all select 0) + join (select 1 as j limit 1) on i =3D j) + right join (select 2 as x) on true + join (select 3 as y) on i is not null + ), + lateral (select k as kl limit 1); + +rollback; + +-- PHVs containing SubLinks are quite tricky to get right +explain (verbose, costs off) +select * +from int8_tbl i8 + inner join + (select (select true) as x + from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1 + where i4.f1 =3D 0) ss2 on true + right join (select false as z) ss3 on true, + lateral (select i8.q2 as q2l where x limit 1) ss4 +where i8.q2 =3D 123; + +explain (verbose, costs off) +select * +from int8_tbl i8 + inner join + (select (select true) as x + from int4_tbl i4, lateral (select 1 as y limit 1) ss1 + where i4.f1 =3D 0) ss2 on true + right join (select false as z) ss3 on true, + lateral (select i8.q2 as q2l where x limit 1) ss4 +where i8.q2 =3D 123; + -- Test proper handling of appendrel PHVs during useless-RTE removal explain (costs off) select * from ------- =_aaaaaaaaaa0--