From bfd332b3fda5c73e28c05b7ba0aac6cf053cdf00 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 12 Jun 2023 10:01:26 -0400 Subject: [PATCH] Fix "wrong varnullingrels" for subquery nestloop parameters. If we apply outer join identity 3 when relation C is a subquery having lateral references to relation B, then the lateral references within C continue to bear the original syntactically-correct varnullingrels marks, but that won't match what is available from the outer side of the nestloop. Compensate for that in process_subquery_nestloop_params(). This is a slightly hacky fix, but we certainly don't want to re-plan C in toto for each possible outer join order, so there's not a lot of better alternatives. Richard Guo and Tom Lane, per report from Markus Winand Discussion: https://postgr.es/m/DFBB2D25-DE97-49CA-A60E-07C881EA59A7@winand.at --- src/backend/optimizer/plan/setrefs.c | 8 ++++-- src/backend/optimizer/util/paramassign.c | 36 +++++++++++++++++++----- src/test/regress/expected/join.out | 18 ++++++++++++ src/test/regress/sql/join.sql | 7 +++++ 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 1ca26baa259..3585a703fbd 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -2289,9 +2289,11 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset) * the outer-join level at which they are used, Vars seen in the * NestLoopParam expression may have nullingrels that are just a * subset of those in the Vars actually available from the outer - * side. Not checking this exactly is a bit grotty, but the work - * needed to make things match up perfectly seems well out of - * proportion to the value. + * side. Another case that can cause that to happen is explained + * in the comments for process_subquery_nestloop_params. Not + * checking this exactly is a bit grotty, but the work needed to + * make things match up perfectly seems well out of proportion to + * the value. */ nlp->paramval = (Var *) fix_upper_expr(root, (Node *) nlp->paramval, diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c index 66534c0a782..e94f7e7563d 100644 --- a/src/backend/optimizer/util/paramassign.c +++ b/src/backend/optimizer/util/paramassign.c @@ -421,8 +421,26 @@ replace_nestloop_param_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv) * provide these values. This differs from replace_nestloop_param_var in * that the PARAM_EXEC slots to use have already been determined. * + * An additional complication is that the subplan_params may contain + * nullingrel markers that need adjustment. This occurs if we have applied + * outer join identity 3, + * (A leftjoin B on (Pab)) leftjoin C on (Pb*c) + * = A leftjoin (B leftjoin C on (Pbc)) on (Pab) + * and C is a subquery containing lateral references to B. It's still safe + * to apply the identity, but the parser will have created those references + * in the form "b*" (i.e., with varnullingrels listing the A/B join), while + * what we will have available from the nestloop's outer side is just "b". + * We deal with that here by stripping the nullingrels down to what is + * available from the outer side according to root->curOuterRels. + * That fixes matters for the case of forward application of identity 3. + * If the identity was applied in the reverse direction, we will have + * subplan_params 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. + * * Note that we also use root->curOuterRels as an implicit parameter for - * sanity checks. + * sanity checks and nullingrel adjustments. */ void process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params) @@ -449,17 +467,19 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params) nlp = (NestLoopParam *) lfirst(lc2); if (nlp->paramno == pitem->paramId) { - Assert(equal(var, nlp->paramval)); /* Present, so nothing to do */ break; } } if (lc2 == NULL) { - /* No, so add it */ + /* No, so add it after adjusting its nullingrels */ + var = copyObject(var); + var->varnullingrels = bms_intersect(var->varnullingrels, + root->curOuterRels); nlp = makeNode(NestLoopParam); nlp->paramno = pitem->paramId; - nlp->paramval = copyObject(var); + nlp->paramval = var; root->curOuterParams = lappend(root->curOuterParams, nlp); } } @@ -480,17 +500,19 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params) nlp = (NestLoopParam *) lfirst(lc2); if (nlp->paramno == pitem->paramId) { - Assert(equal(phv, nlp->paramval)); /* Present, so nothing to do */ break; } } if (lc2 == NULL) { - /* No, so add it */ + /* No, so add it after adjusting its nullingrels */ + phv = copyObject(phv); + phv->phnullingrels = bms_intersect(phv->phnullingrels, + root->curOuterRels); nlp = makeNode(NestLoopParam); nlp->paramno = pitem->paramId; - nlp->paramval = (Var *) copyObject(phv); + nlp->paramval = (Var *) phv; root->curOuterParams = lappend(root->curOuterParams, nlp); } } diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index d04648df3fd..4999c99f3bc 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2589,6 +2589,24 @@ on t2.q2 = 123; -> Seq Scan on int8_tbl t5 (12 rows) +explain (costs off) +select * from int8_tbl t1 + left join int8_tbl t2 on true + left join lateral + (select * from int8_tbl t3 where t3.q1 = t2.q1 offset 0) s + on t2.q1 = 1; + QUERY PLAN +------------------------------------------- + Nested Loop Left Join + -> Seq Scan on int8_tbl t1 + -> Materialize + -> Nested Loop Left Join + Join Filter: (t2.q1 = 1) + -> Seq Scan on int8_tbl t2 + -> Seq Scan on int8_tbl t3 + Filter: (q1 = t2.q1) +(8 rows) + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 0308258a917..56ca759772b 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -514,6 +514,13 @@ select * from int8_tbl t1 left join left join int8_tbl t5 on t2.q1 = t5.q1 on t2.q2 = 123; +explain (costs off) +select * from int8_tbl t1 + left join int8_tbl t2 on true + left join lateral + (select * from int8_tbl t3 where t3.q1 = t2.q1 offset 0) s + on t2.q1 = 1; + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys