From e032e4c7ddd0e1f7865b246ec18944365d4f8614 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 30 Nov 2024 12:42:19 -0500 Subject: [PATCH] Avoid mislabeling of lateral references, redux. As I'd feared, commit 5c9d8636d was still a few bricks shy of a load. We can't just leave pulled-up lateral-reference Vars with no new nullingrels: we have to carefully compute what subset of the to-be-replaced Var's nullingrels apply to them, else we still get "wrong varnullingrels" errors. This is a bit tedious, but it looks like we can use the nullingrel data this patch computes for other purposes, enabling better optimization. We don't want to inject unnecessary plan changes into stable branches though, so leave that idea for a later HEAD-only patch. Patch by me, but thanks to Richard Guo for devising a test case that broke 5c9d8636d, and for preliminary investigation about how to fix it. As before, back-patch to v16. Discussion: https://postgr.es/m/E1tGn4j-0003zi-MP@gemulon.postgresql.org --- src/backend/optimizer/prep/prepjointree.c | 171 +++++++++++++++++++++- src/test/regress/expected/subselect.out | 51 +++++++ src/test/regress/sql/subselect.sql | 15 ++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 233 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 2e0d41a8d1f..2ebd938f6bd 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 joins + * 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_nullingrels, + nullingrel_info *info); /* @@ -1259,10 +1275,16 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, rvcontext.targetlist = subquery->targetList; rvcontext.target_rte = rte; if (rte->lateral) + { rvcontext.relids = get_relids_in_jointree((Node *) subquery->jointree, true, true); - else /* won't need relids */ + rvcontext.nullinfo = get_nullingrels(parse); + } + else /* won't need these values */ + { rvcontext.relids = NULL; + rvcontext.nullinfo = NULL; + } rvcontext.outer_hasSubLinks = &parse->hasSubLinks; rvcontext.varno = varno; /* this flag will be set below, if needed */ @@ -1725,6 +1747,9 @@ is_simple_subquery(PlannerInfo *root, Query *subquery, RangeTblEntry *rte, * such refs to be wrapped in PlaceHolderVars, even when they're below * the nearest outer join? But it's a pretty hokey usage, so not * clear this is worth sweating over.) + * + * If you change this, see also the comments about lateral references + * in pullup_replace_vars_callback(). */ if (lowest_outer_join != NULL) { @@ -1809,7 +1834,8 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte) rvcontext.root = root; rvcontext.targetlist = tlist; rvcontext.target_rte = rte; - rvcontext.relids = NULL; + rvcontext.relids = NULL; /* can't be any lateral references here */ + rvcontext.nullinfo = NULL; rvcontext.outer_hasSubLinks = &parse->hasSubLinks; rvcontext.varno = varno; rvcontext.wrap_non_vars = false; @@ -1971,9 +1997,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 = NULL; + rvcontext.nullinfo = NULL; rvcontext.outer_hasSubLinks = &parse->hasSubLinks; rvcontext.varno = ((RangeTblRef *) jtnode)->rtindex; @@ -2688,9 +2715,52 @@ 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. (This + * corresponds to the fact that the expression will now be + * evaluated at the join level of the Var that we are replacing: + * the lateral references may have bubbled up through fewer outer + * joins than the subquery's Vars have. Per the discussion above, + * we'll still get the right answers.) That relid set could be + * different for different lateral relations, so we have to do + * this work for each one. + * + * (Currently, the restrictions in is_simple_subquery() mean that + * at most we have to remove the lowest outer join's relid from + * the nullingrels of a lateral reference. However, we might + * relax those restrictions someday, so let's do this right.) */ + if (rcon->target_rte->lateral) + { + nullingrel_info *nullinfo = rcon->nullinfo; + Relids lvarnos; + int lvarno; + + /* + * Identify lateral varnos used within newnode. We must do + * this before injecting var->varnullingrels into the tree. + */ + lvarnos = pull_varnos(rcon->root, newnode); + lvarnos = bms_del_members(lvarnos, rcon->relids); + /* For each one, add relevant nullingrels if any */ + lvarno = -1; + while ((lvarno = bms_next_member(lvarnos, lvarno)) >= 0) + { + Relids lnullingrels; + + Assert(lvarno > 0 && lvarno <= nullinfo->rtlength); + lnullingrels = bms_intersect(var->varnullingrels, + nullinfo->nullingrels[lvarno]); + if (!bms_is_empty(lnullingrels)) + newnode = add_nulling_relids(newnode, + bms_make_singleton(lvarno), + lnullingrels); + } + } + + /* Finally, deal with Vars/PHVs of the subquery itself */ newnode = add_nulling_relids(newnode, rcon->relids, var->varnullingrels); @@ -4120,3 +4190,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 relations + * + * 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 = palloc_object(nullingrel_info); + + result->rtlength = list_length(parse->rtable); + result->nullingrels = 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 be + * 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 mutated + * 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 == NULL) + return; + if (IsA(jtnode, RangeTblRef)) + { + int varno = ((RangeTblRef *) jtnode)->rtindex; + + Assert(varno > 0 && varno <= info->rtlength); + info->nullingrels[varno] = upper_nullingrels; + } + else if (IsA(jtnode, FromExpr)) + { + FromExpr *f = (FromExpr *) jtnode; + ListCell *l; + + foreach(l, f->fromlist) + { + get_nullingrels_recurse(lfirst(l), upper_nullingrels, info); + } + } + else if (IsA(jtnode, JoinExpr)) + { + JoinExpr *j = (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 = 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 = 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 = 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/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out index da219aba4c4..6c1fb2bfdbb 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -1797,6 +1797,57 @@ order by t1.ten; 9 | 18000 (10 rows) +explain (verbose, costs off) +select t1.q1, x from + int8_tbl t1 left join + (int8_tbl t2 left join + lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2) + on t1.q2 = t2.q2 +order by 1, 2; + QUERY PLAN +-------------------------------------------------------- + Sort + Output: t1.q1, ((t2.q1 + t3.q1)) + Sort Key: t1.q1, ((t2.q1 + t3.q1)) + -> Hash Left Join + Output: t1.q1, (t2.q1 + t3.q1) + Hash Cond: (t2.q2 = t3.q2) + -> Hash Left Join + Output: t1.q1, t2.q1, t2.q2 + Hash Cond: (t1.q2 = t2.q2) + -> Seq Scan on public.int8_tbl t1 + Output: t1.q1, t1.q2 + -> Hash + Output: t2.q1, t2.q2 + -> Seq Scan on public.int8_tbl t2 + Output: t2.q1, t2.q2 + -> Hash + Output: t3.q1, t3.q2 + -> Seq Scan on public.int8_tbl t3 + Output: t3.q1, t3.q2 +(19 rows) + +select t1.q1, x from + int8_tbl t1 left join + (int8_tbl t2 left join + lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2) + on t1.q2 = t2.q2 +order by 1, 2; + q1 | x +------------------+------------------ + 123 | 246 + 123 | 246 + 123 | 4567890123456912 + 123 | 4567890123456912 + 123 | 9135780246913578 + 4567890123456789 | 246 + 4567890123456789 | 4567890123456912 + 4567890123456789 | 4567890123456912 + 4567890123456789 | 9135780246913578 + 4567890123456789 | 9135780246913578 + 4567890123456789 | 9135780246913578 +(11 rows) + -- -- Tests for CTE inlining behavior -- diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql index 1815a5e6642..e5a562c3f5d 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -924,6 +924,21 @@ select t1.ten, sum(x) from group by t1.ten order by t1.ten; +explain (verbose, costs off) +select t1.q1, x from + int8_tbl t1 left join + (int8_tbl t2 left join + lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2) + on t1.q2 = t2.q2 +order by 1, 2; + +select t1.q1, x from + int8_tbl t1 left join + (int8_tbl t2 left join + lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2) + on t1.q2 = t2.q2 +order by 1, 2; + -- -- Tests for CTE inlining behavior -- diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b54428b38cd..2d4c870423a 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