Improve implementation of CRE-stack-flattening in map_variable_attnos().

I (tgl) objected to the obscure implementation introduced in commit
1c497fa72.  This one seems a bit less action-at-a-distance-y, at the
price of repeating a few lines of code.

Improve the comments about what the function is doing, too.

Amit Khandekar, whacked around a bit more by me

Discussion: https://postgr.es/m/CAJ3gD9egYTyHUH0nTMxm8-1m3RvdqEbaTyGC-CUNtYf7tKNDaQ@mail.gmail.com

diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 9290c7f793600b621b929b5ea0462ca053053d56..6579b2446d1832b0265f0f7f16e7ef06c54333ca 100644 (file)
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1203,9 +1203,11 @@ replace_rte_variables_mutator(Node *node,
  * appear in the expression.
  *
  * If the expression tree contains a whole-row Var for the target RTE,
- * *found_whole_row is returned as TRUE.  In addition, if to_rowtype is
- * not InvalidOid, we modify the Var's vartype and insert a ConvertRowTypeExpr
- * to map back to the orignal rowtype.  Callers that don't provide to_rowtype
+ * *found_whole_row is set to TRUE.  In addition, if to_rowtype is
+ * not InvalidOid, we replace the Var with a Var of that vartype, inserting
+ * a ConvertRowTypeExpr to map back to the rowtype expected by the expression.
+ * (Therefore, to_rowtype had better be a child rowtype of the rowtype of the
+ * RTE we're changing references to.)  Callers that don't provide to_rowtype
  * should report an error if *found_row_type is true; we don't do that here
  * because we don't know exactly what wording for the error message would
  * be most appropriate.  The caller will be aware of the context.
@@ -1221,10 +1223,8 @@ typedef struct
        int                     sublevels_up;   /* (current) nesting depth */
        const AttrNumber *attno_map;    /* map array for user attnos */
        int                     map_length;             /* number of entries in attno_map[] */
-       /* Target type when converting whole-row vars */
-       Oid                     to_rowtype;
+       Oid                     to_rowtype;             /* change whole-row Vars to this type */
        bool       *found_whole_row;    /* output flag */
-       bool            coerced_var;    /* var is under ConvertRowTypeExpr */
 } map_variable_attnos_context;
 
 static Node *
@@ -1244,7 +1244,8 @@ map_variable_attnos_mutator(Node *node,
                        Var                *newvar = (Var *) palloc(sizeof(Var));
                        int                     attno = var->varattno;
 
-                       *newvar = *var;
+                       *newvar = *var;         /* initially copy all fields of the Var */
+
                        if (attno > 0)
                        {
                                /* user-defined column, replace attno */
@@ -1259,39 +1260,29 @@ map_variable_attnos_mutator(Node *node,
                                /* whole-row variable, warn caller */
                                *(context->found_whole_row) = true;
 
-                               /* If the callers expects us to convert the same, do so. */
-                               if (OidIsValid(context->to_rowtype))
+                               /* If the caller expects us to convert the Var, do so. */
+                               if (OidIsValid(context->to_rowtype) &&
+                                       context->to_rowtype != var->vartype)
                                {
-                                       /* No support for RECORDOID. */
+                                       ConvertRowtypeExpr *r;
+
+                                       /* This certainly won't work for a RECORD variable. */
                                        Assert(var->vartype != RECORDOID);
 
-                                       /* Don't convert unless necessary. */
-                                       if (context->to_rowtype != var->vartype)
-                                       {
-                                               /* Var itself is converted to the requested type. */
-                                               newvar->vartype = context->to_rowtype;
-
-                                               /*
-                                                * If this var is already under a ConvertRowtypeExpr,
-                                                * we don't have to add another one.
-                                                */
-                                               if (!context->coerced_var)
-                                               {
-                                                       ConvertRowtypeExpr *r;
-
-                                                       /*
-                                                        * And a conversion node on top to convert back to
-                                                        * the original type.
-                                                        */
-                                                       r = makeNode(ConvertRowtypeExpr);
-                                                       r->arg = (Expr *) newvar;
-                                                       r->resulttype = var->vartype;
-                                                       r->convertformat = COERCE_IMPLICIT_CAST;
-                                                       r->location = -1;
-
-                                                       return (Node *) r;
-                                               }
-                                       }
+                                       /* Var itself is changed to the requested type. */
+                                       newvar->vartype = context->to_rowtype;
+
+                                       /*
+                                        * Add a conversion node on top to convert back to the
+                                        * original type expected by the expression.
+                                        */
+                                       r = makeNode(ConvertRowtypeExpr);
+                                       r->arg = (Expr *) newvar;
+                                       r->resulttype = var->vartype;
+                                       r->convertformat = COERCE_IMPLICIT_CAST;
+                                       r->location = -1;
+
+                                       return (Node *) r;
                                }
                        }
                        return (Node *) newvar;
@@ -1301,24 +1292,43 @@ map_variable_attnos_mutator(Node *node,
        else if (IsA(node, ConvertRowtypeExpr))
        {
                ConvertRowtypeExpr *r = (ConvertRowtypeExpr *) node;
+               Var                *var = (Var *) r->arg;
 
                /*
-                * If this is coercing a var (which is typical), convert only the var,
-                * as against adding another ConvertRowtypeExpr over it.
+                * If this is coercing a whole-row Var that we need to convert, then
+                * just convert the Var without adding an extra ConvertRowtypeExpr.
+                * Effectively we're simplifying var::parenttype::grandparenttype into
+                * just var::grandparenttype.  This avoids building stacks of CREs if
+                * this function is applied repeatedly.
                 */
-               if (IsA(r->arg, Var))
+               if (IsA(var, Var) &&
+                       var->varno == context->target_varno &&
+                       var->varlevelsup == context->sublevels_up &&
+                       var->varattno == 0 &&
+                       OidIsValid(context->to_rowtype) &&
+                       context->to_rowtype != var->vartype)
                {
                        ConvertRowtypeExpr *newnode;
+                       Var                *newvar = (Var *) palloc(sizeof(Var));
+
+                       /* whole-row variable, warn caller */
+                       *(context->found_whole_row) = true;
+
+                       *newvar = *var;         /* initially copy all fields of the Var */
+
+                       /* This certainly won't work for a RECORD variable. */
+                       Assert(var->vartype != RECORDOID);
+
+                       /* Var itself is changed to the requested type. */
+                       newvar->vartype = context->to_rowtype;
 
                        newnode = (ConvertRowtypeExpr *) palloc(sizeof(ConvertRowtypeExpr));
-                       *newnode = *r;
-                       context->coerced_var = true;
-                       newnode->arg = (Expr *) map_variable_attnos_mutator((Node *) r->arg, context);
-                       context->coerced_var = false;
+                       *newnode = *r;          /* initially copy all fields of the CRE */
+                       newnode->arg = (Expr *) newvar;
 
                        return (Node *) newnode;
                }
-               /* Else fall through the expression tree mutator */
+               /* otherwise fall through to process the expression normally */
        }
        else if (IsA(node, Query))
        {
@@ -1351,7 +1361,6 @@ map_variable_attnos(Node *node,
        context.map_length = map_length;
        context.to_rowtype = to_rowtype;
        context.found_whole_row = found_whole_row;
-       context.coerced_var = false;
 
        *found_whole_row = false;