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 1tH8aK-003pGs-DF for pgsql-committers@arkaria.postgresql.org; Fri, 29 Nov 2024 21:31:32 +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 1tH8aG-00Azjj-HY for pgsql-committers@arkaria.postgresql.org; Fri, 29 Nov 2024 21:31:29 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tH8aG-00Azjb-8g for pgsql-committers@lists.postgresql.org; Fri, 29 Nov 2024 21:31:29 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tH8aE-000D9U-13 for pgsql-committers@lists.postgresql.org; Fri, 29 Nov 2024 21:31:28 +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 4ATLVOwH590236; Fri, 29 Nov 2024 16:31:24 -0500 From: Tom Lane To: Richard Guo cc: pgsql-committers@lists.postgresql.org Subject: Re: pgsql: Avoid mislabeling of lateral references when pulling up a subque In-reply-to: <530989.1732907849@sss.pgh.pa.us> References: <397371.1732844670@sss.pgh.pa.us> <530989.1732907849@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 29 Nov 2024 14:17:29 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <590168.1732915859.0@sss.pgh.pa.us> Date: Fri, 29 Nov 2024 16:31:24 -0500 Message-ID: <590235.1732915884@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: <590168.1732915859.1@sss.pgh.pa.us> I wrote: > The ideas I'd been toying with last night involved a pre-scan over > the join tree to calculate the potential nullingrels of each leaf RTE > (same idea as RelOptInfo.nulling_relids, but of course we don't have > any RelOptInfos yet). That seems painful though because we'd have to > update the data structure somehow after each subquery pullup. I realized that we can make that work by doing the pre-calculation at the start of each pull_up_simple_subquery. We only have to do it for subqueries marked LATERAL, so this doesn't seem too horribly inefficient. Draft patch attached. I didn't spend effort on devising test cases, but it works for your example. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v2-0001-Fix-wrong-varnullingrels.patch"; charset="us-ascii" Content-ID: <590168.1732915859.2@sss.pgh.pa.us> Content-Description: v2-0001-Fix-wrong-varnullingrels.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optim= izer/prep/prepjointree.c index 2e0d41a8d1..ff9c1cd24e 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -42,6 +42,17 @@ #include "rewrite/rewriteManip.h" = = +typedef struct nullingrel_info +{ + /* + * For each leaf RTE, nullingrels[rti] is the set of relids of outer joi= ns + * that potentially null that RTE. + */ + Relids *nullingrels; + /* Length of range table (maximum index in nullingrels[]) */ + int rtlength; /* used only for assertion checks */ +} nullingrel_info; + typedef struct pullup_replace_vars_context { PlannerInfo *root; @@ -49,6 +60,8 @@ typedef struct pullup_replace_vars_context RangeTblEntry *target_rte; /* RTE of subquery */ Relids relids; /* relids within subquery, as numbered after * pullup (set only if target_rte->lateral) */ + nullingrel_info *nullinfo; /* per-RTE nullingrel info (set only if + * target_rte->lateral) */ bool *outer_hasSubLinks; /* -> outer query's hasSubLinks */ int varno; /* varno of subquery */ bool wrap_non_vars; /* do we need all non-Var outputs to be PHVs? */ @@ -142,6 +155,9 @@ static void substitute_phv_relids(Node *node, static void fix_append_rel_relids(PlannerInfo *root, int varno, Relids subrelids); static Node *find_jointree_node_for_rel(Node *jtnode, int relid); +static nullingrel_info *get_nullingrels(Query *parse); +static void get_nullingrels_recurse(Node *jtnode, Relids upper_nullingrel= s, + nullingrel_info *info); = = /* @@ -1259,10 +1275,16 @@ pull_up_simple_subquery(PlannerInfo *root, Node *j= tnode, RangeTblEntry *rte, rvcontext.targetlist =3D subquery->targetList; rvcontext.target_rte =3D rte; if (rte->lateral) + { rvcontext.relids =3D get_relids_in_jointree((Node *) subquery->jointree= , true, true); - else /* won't need relids */ + rvcontext.nullinfo =3D get_nullingrels(parse); + } + else /* won't need these fields */ + { rvcontext.relids =3D NULL; + rvcontext.nullinfo =3D NULL; + } rvcontext.outer_hasSubLinks =3D &parse->hasSubLinks; rvcontext.varno =3D varno; /* this flag will be set below, if needed */ @@ -1809,7 +1831,8 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnod= e, RangeTblEntry *rte) rvcontext.root =3D root; rvcontext.targetlist =3D tlist; rvcontext.target_rte =3D rte; - rvcontext.relids =3D NULL; + rvcontext.relids =3D NULL; /* can't be any lateral references here */ + rvcontext.nullinfo =3D NULL; rvcontext.outer_hasSubLinks =3D &parse->hasSubLinks; rvcontext.varno =3D varno; rvcontext.wrap_non_vars =3D false; @@ -1971,9 +1994,10 @@ pull_up_constant_function(PlannerInfo *root, Node *= jtnode, /* * Since this function was reduced to a Const, it doesn't contain any * lateral references, even if it's marked as LATERAL. This means we - * don't need to fill relids. + * don't need to fill relids or nullinfo. */ rvcontext.relids =3D NULL; + rvcontext.nullinfo =3D NULL; = rvcontext.outer_hasSubLinks =3D &parse->hasSubLinks; rvcontext.varno =3D ((RangeTblRef *) jtnode)->rtindex; @@ -2688,9 +2712,42 @@ pullup_replace_vars_callback(Var *var, { /* * There should be Vars/PHVs within the expression that we can - * modify. Per above discussion, modify only Vars/PHVs of the - * subquery, not lateral references. + * modify. Vars/PHVs of the subquery should have the full + * var->varnullingrels added to them, but if there are lateral + * references within the expression, those must be marked with + * only the nullingrels that potentially apply to them. That + * could be different for different lateral relations, so we have + * to do this work for each one. */ + if (rcon->target_rte->lateral) + { + nullingrel_info *nullinfo =3D rcon->nullinfo; + Relids lvarnos; + int lvarno; + + /* + * Identify lateral varnos used within newnode. We must do + * this before injecting var->varnullingrels into the tree. + */ + lvarnos =3D pull_varnos(rcon->root, newnode); + lvarnos =3D bms_del_members(lvarnos, rcon->relids); + /* For each one, add relevant nullingrels if any */ + lvarno =3D -1; + while ((lvarno =3D bms_next_member(lvarnos, lvarno)) >=3D 0) + { + Relids lnullingrels; + + Assert(lvarno > 0 && lvarno <=3D nullinfo->rtlength); + lnullingrels =3D bms_intersect(var->varnullingrels, + nullinfo->nullingrels[lvarno]); + if (!bms_is_empty(lnullingrels)) + newnode =3D add_nulling_relids(newnode, + bms_make_singleton(lvarno), + lnullingrels); + } + } + + /* Finally, deal with Vars/PHVs of the subquery itself */ newnode =3D add_nulling_relids(newnode, rcon->relids, var->varnullingrels); @@ -4120,3 +4177,94 @@ find_jointree_node_for_rel(Node *jtnode, int relid) (int) nodeTag(jtnode)); return NULL; } + +/* + * get_nullingrels: collect info about which outer joins null which relat= ions + * + * The result struct contains, for each leaf relation used in the query, + * the set of relids of outer joins that potentially null that rel. + */ +static nullingrel_info * +get_nullingrels(Query *parse) +{ + nullingrel_info *result =3D palloc_object(nullingrel_info); + + result->rtlength =3D list_length(parse->rtable); + result->nullingrels =3D palloc0_array(Relids, result->rtlength + 1); + get_nullingrels_recurse((Node *) parse->jointree, NULL, result); + return result; +} + +/* + * Recursive guts of get_nullingrels(). + * + * Note: at any recursion level, the passed-down upper_nullingrels must b= e + * treated as a constant, but it can be stored directly into *info + * if we're at leaf level. Upper recursion levels do not free their muta= ted + * copies of the nullingrels, because those are probably referenced by + * at least one leaf rel. + */ +static void +get_nullingrels_recurse(Node *jtnode, Relids upper_nullingrels, + nullingrel_info *info) +{ + if (jtnode =3D=3D NULL) + return; + if (IsA(jtnode, RangeTblRef)) + { + int varno =3D ((RangeTblRef *) jtnode)->rtindex; + + Assert(varno > 0 && varno <=3D info->rtlength); + info->nullingrels[varno] =3D upper_nullingrels; + } + else if (IsA(jtnode, FromExpr)) + { + FromExpr *f =3D (FromExpr *) jtnode; + ListCell *l; + + foreach(l, f->fromlist) + { + get_nullingrels_recurse(lfirst(l), upper_nullingrels, info); + } + } + else if (IsA(jtnode, JoinExpr)) + { + JoinExpr *j =3D (JoinExpr *) jtnode; + Relids local_nullingrels; + + switch (j->jointype) + { + case JOIN_INNER: + get_nullingrels_recurse(j->larg, upper_nullingrels, info); + get_nullingrels_recurse(j->rarg, upper_nullingrels, info); + break; + case JOIN_LEFT: + case JOIN_SEMI: + case JOIN_ANTI: + local_nullingrels =3D bms_add_member(bms_copy(upper_nullingrels), + j->rtindex); + get_nullingrels_recurse(j->larg, upper_nullingrels, info); + get_nullingrels_recurse(j->rarg, local_nullingrels, info); + break; + case JOIN_FULL: + local_nullingrels =3D bms_add_member(bms_copy(upper_nullingrels), + j->rtindex); + get_nullingrels_recurse(j->larg, local_nullingrels, info); + get_nullingrels_recurse(j->rarg, local_nullingrels, info); + break; + case JOIN_RIGHT: + local_nullingrels =3D bms_add_member(bms_copy(upper_nullingrels), + j->rtindex); + get_nullingrels_recurse(j->larg, local_nullingrels, info); + get_nullingrels_recurse(j->rarg, upper_nullingrels, info); + break; + default: + elog(ERROR, "unrecognized join type: %d", + (int) j->jointype); + break; + } + } + else + elog(ERROR, "unrecognized node type: %d", + (int) nodeTag(jtnode)); +} diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedef= s.list index b54428b38c..2d4c870423 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3665,6 +3665,7 @@ nodeitem normal_rand_fctx nsphash_hash ntile_context +nullingrel_info numeric object_access_hook_type object_access_hook_type_str ------- =_aaaaaaaaaa0--