Re: Significant Execution Time Difference Between PG13.14 and PG16.4 for Query on information_schema Tables. - Mailing list pgsql-hackers

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;