I wrote:
> We didn't see this particular behavior before 2489d76c49 because
> pullup_replace_vars avoided inserting a PHV:
> * If it contains a Var of the subquery being pulled up, and
> * does not contain any non-strict constructs, then it's
> * certainly nullable so we don't need to insert a
> * PlaceHolderVar.
> I dropped that case in 2489d76c49 because now we need to attach
> nullingrels to the expression. You could imagine attaching the
> nullingrels to the contained Var(s) instead of putting a PHV on top,
> but that seems like a mess and I'm not quite sure it's semantically
> the same. In any case it wouldn't fix adjacent cases where there is
> a non-strict construct in the subquery output expression.
I realized that actually we do have the mechanism for making that
work: we could apply add_nulling_relids to the expression, if it
meets those same conditions. This is a kluge really, but it would
restore the status quo ante in a fairly localized fashion that
seems like it might be safe enough to back-patch into v16.
Here's a WIP patch that does it like that. One problem with it
is that it requires rcon->relids to be calculated in cases where
we didn't need that before, which is probably not *that* expensive
but it's annoying. If we go forward with this, I'm thinking about
changing add_nulling_relids' API contract to say "if target_relid
is NULL then all level-zero Vars/PHVs are modified", so that we
don't need that relid set in non-LATERAL cases.
The other problem with this is that it breaks one test case in
memoize.sql: a query that formerly generated a memoize plan
now does not use memoize. I am not sure why not --- does that
mean anything to you?
regards, tom lane
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 969e257f70..3a12a52440 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -48,7 +48,7 @@ typedef struct pullup_replace_vars_context
List *targetlist; /* tlist of subquery being pulled up */
RangeTblEntry *target_rte; /* RTE of subquery */
Relids relids; /* relids within subquery, as numbered after
- * pullup (set only if target_rte->lateral) */
+ * pullup */
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? */
@@ -1163,11 +1163,8 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.root = root;
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.relids = NULL;
+ rvcontext.relids = get_relids_in_jointree((Node *) subquery->jointree,
+ true, true);
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
/* this flag will be set below, if needed */
@@ -1713,7 +1710,7 @@ 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; /* XXX */
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
rvcontext.wrap_non_vars = false;
@@ -1877,7 +1874,7 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
* lateral references, even if it's marked as LATERAL. This means we
* don't need to fill relids.
*/
- rvcontext.relids = NULL;
+ rvcontext.relids = NULL; /* XXX */
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = ((RangeTblRef *) jtnode)->rtindex;
@@ -2490,14 +2487,48 @@ pullup_replace_vars_callback(Var *var,
else
wrap = false;
}
+ else if (rcon->wrap_non_vars)
+ {
+ /* Caller told us to wrap all non-Vars in a PlaceHolderVar */
+ wrap = true;
+ }
else
{
/*
- * Must wrap, either because we need a place to insert
- * varnullingrels or because caller told us to wrap
- * everything.
+ * If the node contains Var(s) or PlaceHolderVar(s) of the
+ * subquery being pulled up, and does not contain any
+ * non-strict constructs, then instead of adding a PHV on top
+ * we can add the required nullingrels to those Vars/PHVs.
+ * (This is fundamentally a generalization of the above cases
+ * for bare Vars and PHVs.)
+ *
+ * This test is somewhat expensive, but it avoids pessimizing
+ * the plan in cases where the nullingrels get removed again
+ * later by outer join reduction.
+ *
+ * This analysis could be tighter: in particular, a non-strict
+ * construct hidden within a lower-level PlaceHolderVar is not
+ * reason to add another PHV. But for now it doesn't seem
+ * worth the code to be more exact.
+ *
+ * For a LATERAL subquery, we have to check the actual var
+ * membership of the node, but if it's non-lateral then any
+ * level-zero var must belong to the subquery.
*/
- wrap = true;
+ if ((rcon->target_rte->lateral ?
+ bms_overlap(pull_varnos(rcon->root, newnode),
+ rcon->relids) :
+ contain_vars_of_level(newnode, 0)) &&
+ !contain_nonstrict_functions(newnode))
+ {
+ /* No wrap needed */
+ wrap = false;
+ }
+ else
+ {
+ /* Else wrap it in a PlaceHolderVar */
+ wrap = true;
+ }
}
if (wrap)
@@ -2522,7 +2553,7 @@ pullup_replace_vars_callback(Var *var,
if (var->varlevelsup > 0)
IncrementVarSublevelsUp(newnode, var->varlevelsup, 0);
- /* Propagate any varnullingrels into the replacement Var or PHV */
+ /* Propagate any varnullingrels into the replacement expression */
if (var->varnullingrels != NULL)
{
if (IsA(newnode, Var))
@@ -2542,7 +2573,15 @@ pullup_replace_vars_callback(Var *var,
var->varnullingrels);
}
else
- elog(ERROR, "failed to wrap a non-Var");
+ {
+ /* There should be lower-level Vars/PHVs we can modify */
+ newnode = add_nulling_relids(newnode,
+ rcon->relids,
+ var->varnullingrels);
+ /* Assert we did put the varnullingrels into the expression */
+ Assert(bms_is_subset(var->varnullingrels,
+ pull_varnos(rcon->root, newnode)));
+ }
}
return newnode;
