database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-05-05 09:19:17 +03:00
Code

Avoid mislabeling of lateral references, redux.

Browse Source 
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
This commit is contained in:
Tom Lane 2024-11-30 12:42:19 -05:00
parent 49ae9fd8b7
commit e032e4c7dd
4 changed files with 233 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

171
src/backend/optimizer/prep/prepjointree.c
View File

@ -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));
}

51
src/test/regress/expected/subselect.out
View File

@ -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
--

15
src/test/regress/sql/subselect.sql
View File

@ -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
--

1
src/tools/pgindent/typedefs.list
View File

@ -3665,6 +3665,7 @@ nodeitem
normal_rand_fctx
nsphash_hash
ntile_context
nullingrel_info
numeric
object_access_hook_type
object_access_hook_type_str