Make pg_get_expr() more bulletproof.

Since this function is defined to accept pg_node_tree values, it could
get applied to any nodetree that can appear in a cataloged pg_node_tree
column.  Some such cases can't be supported --- for example, its API
doesn't allow providing referents for more than one relation --- but
we should try to throw a user-facing error rather than an internal
error when encountering such a case.

In support of this, extend expression_tree_walker/mutator to be sure
they'll work on any such node tree (which basically means adding
support for relpartbound node types).  That allows us to run pull_varnos
and check for the case of multiple relations before we start processing
the tree.  The alternative of changing the low-level error thrown for an
out-of-range varno isn't appealing, because that could mask actual bugs
in other usages of ruleutils.

Per report from Justin Pryzby.  This is basically cosmetic, so no
back-patch.

Discussion: https://postgr.es/m/20211219205422.GT17618@telsasoft.com

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 7d1a3aff8c1b2f979e532ca25b26ca57b245b42f..acc17da7177b80b6612dd13c6168837a2f4b131d 100644 (file)
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2201,6 +2201,26 @@ expression_tree_walker(Node *node,
                    return true;
            }
            break;
+       case T_PartitionBoundSpec:
+           {
+               PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+
+               if (walker(pbs->listdatums, context))
+                   return true;
+               if (walker(pbs->lowerdatums, context))
+                   return true;
+               if (walker(pbs->upperdatums, context))
+                   return true;
+           }
+           break;
+       case T_PartitionRangeDatum:
+           {
+               PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+
+               if (walker(prd->value, context))
+                   return true;
+           }
+           break;
        case T_List:
            foreach(temp, (List *) node)
            {
@@ -3092,6 +3112,28 @@ expression_tree_mutator(Node *node,
                return (Node *) newnode;
            }
            break;
+       case T_PartitionBoundSpec:
+           {
+               PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+               PartitionBoundSpec *newnode;
+
+               FLATCOPY(newnode, pbs, PartitionBoundSpec);
+               MUTATE(newnode->listdatums, pbs->listdatums, List *);
+               MUTATE(newnode->lowerdatums, pbs->lowerdatums, List *);
+               MUTATE(newnode->upperdatums, pbs->upperdatums, List *);
+               return (Node *) newnode;
+           }
+           break;
+       case T_PartitionRangeDatum:
+           {
+               PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+               PartitionRangeDatum *newnode;
+
+               FLATCOPY(newnode, prd, PartitionRangeDatum);
+               MUTATE(newnode->value, prd->value, Node *);
+               return (Node *) newnode;
+           }
+           break;
        case T_List:
            {
                /*

diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
index ac048beea09eb67be8e191106140b6a75bd44e94..a0543b7f47b388f254db033fa5131585fec7a0dc 100644 (file)
--- a/src/backend/optimizer/util/var.c
+++ b/src/backend/optimizer/util/var.c
@@ -88,6 +88,9 @@ static Relids alias_relid_set(Query *query, Relids relids);
  *     Create a set of all the distinct varnos present in a parsetree.
  *     Only varnos that reference level-zero rtable entries are considered.
  *
+ * "root" can be passed as NULL if it is not necessary to process
+ * PlaceHolderVars.
+ *
  * NOTE: this is used on not-yet-planned expressions.  It may therefore find
  * bare SubLinks, and if so it needs to recurse into them to look for uplevel
  * references to the desired rtable level! But when we find a completed
@@ -168,9 +171,13 @@ pull_varnos_walker(Node *node, pull_varnos_context *context)
        /*
         * If a PlaceHolderVar is not of the target query level, ignore it,
         * instead recursing into its expression to see if it contains any
-        * vars that are of the target level.
+        * vars that are of the target level.  We'll also do that when the
+        * caller doesn't pass a "root" pointer.  (We probably shouldn't see
+        * PlaceHolderVars at all in such cases, but if we do, this is a
+        * reasonable behavior.)
         */
-       if (phv->phlevelsup == context->sublevels_up)
+       if (phv->phlevelsup == context->sublevels_up &&
+           context->root != NULL)
        {
            /*
             * Ideally, the PHV's contribution to context->varnos is its

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 039f897331c863336fa3f7f397810c5913522920..63b85566822f827adb30941a9372f9de9d32afa5 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2561,6 +2561,12 @@ decompile_column_index_array(Datum column_index_array, Oid relId,
  * the one specified by the second parameter.  This is sufficient for
  * partial indexes, column default expressions, etc.  We also support
  * Var-free expressions, for which the OID can be InvalidOid.
+ *
+ * We expect this function to work, or throw a reasonably clean error,
+ * for any node tree that can appear in a catalog pg_node_tree column.
+ * Query trees, such as those appearing in pg_rewrite.ev_action, are
+ * not supported.  Nor are expressions in more than one relation, which
+ * can appear in places like pg_rewrite.ev_qual.
  * ----------
  */
 Datum
@@ -2622,11 +2628,13 @@ static text *
 pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
 {
    Node       *node;
+   Node       *tst;
+   Relids      relids;
    List       *context;
    char       *exprstr;
    char       *str;
 
-   /* Convert input TEXT object to C string */
+   /* Convert input pg_node_tree (really TEXT) object to C string */
    exprstr = text_to_cstring(expr);
 
    /* Convert expression to node tree */
@@ -2634,6 +2642,40 @@ pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
 
    pfree(exprstr);
 
+   /*
+    * Throw error if the input is a querytree rather than an expression tree.
+    * While we could support queries here, there seems no very good reason
+    * to.  In most such catalog columns, we'll see a List of Query nodes, or
+    * even nested Lists, so drill down to a non-List node before checking.
+    */
+   tst = node;
+   while (tst && IsA(tst, List))
+       tst = linitial((List *) tst);
+   if (tst && IsA(tst, Query))
+       ereport(ERROR,
+               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("input is a query, not an expression")));
+
+   /*
+    * Throw error if the expression contains Vars we won't be able to
+    * deparse.
+    */
+   relids = pull_varnos(NULL, node);
+   if (OidIsValid(relid))
+   {
+       if (!bms_is_subset(relids, bms_make_singleton(1)))
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("expression contains variables of more than one relation")));
+   }
+   else
+   {
+       if (!bms_is_empty(relids))
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("expression contains variables")));
+   }
+
    /* Prepare deparse context if needed */
    if (OidIsValid(relid))
        context = deparse_context_for(relname, relid);