Re: BUG #15746: cache lookup failed for function in plpgsql block - Mailing list pgsql-bugs

Hello.

At Wed, 10 Apr 2019 03:51:07 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in
<15746-6e0482a4c0f915cb@postgresql.org>
> The following bug has been logged on the website:
> 
> Bug reference:      15746
> Logged by:          Roman Zharkov
> Email address:      r.zharkov@postgrespro.ru
> PostgreSQL version: 10.7
> Operating system:   centos 7, fedora 28
> Description:        
> 
> Hello,
> I found a problem within regression tests. The plpgsql test fails when
> running twice on the same database.
> Here is small script illustrates the problem:
> 
> begin;
>   create function sql_to_date(integer) returns date as $$
>   select $1::text::date
>   $$ language sql immutable strict;
>   create cast (integer as date) with function sql_to_date(integer) as
> assignment;
>   create function cast_invoker(integer) returns date as $$
>   begin
>     return $1;
>   end$$ language plpgsql;
> 
>   select cast_invoker(20150717);
> 
>   drop function cast_invoker(integer);
>   drop function sql_to_date(integer) cascade;
> commit;
> 
> begin;
>   create function sql_to_date(integer) returns date as $$
>   select $1::text::date
>   $$ language sql immutable strict;
>   create cast (integer as date) with function sql_to_date(integer) as
> assignment;
>   create function cast_invoker(integer) returns date as $$
>   begin
>     return $1;
>   end$$ language plpgsql;
> 
>   select cast_invoker(20150717);
> 
>   drop function cast_invoker(integer);
>   drop function sql_to_date(integer) cascade;
> commit;
> 
> Results:
> 
> begin;
>   create function sql_to_date(integer) returns date as $$
>   select $1::text::date
>   $$ language sql immutable strict;
>   create cast (integer as date) with function sql_to_date(integer) as
> assignment;
>   create function cast_invoker(integer) returns date as $$
>   begin
>     return $1;
>   end$$ language plpgsql;
>   select cast_invoker(20150717);
>  cast_invoker
> --------------
>  07-17-2015
> (1 row)
> 
>   drop function cast_invoker(integer);
>   drop function sql_to_date(integer) cascade;
> NOTICE:  drop cascades to cast from integer to date
> commit;
> begin;
>   create function sql_to_date(integer) returns date as $$
>   select $1::text::date
>   $$ language sql immutable strict;
>   create cast (integer as date) with function sql_to_date(integer) as
> assignment;
>   create function cast_invoker(integer) returns date as $$
>   begin
>     return $1;
>   end$$ language plpgsql;
>   select cast_invoker(20150717);
> ERROR:  cache lookup failed for function 16414
> CONTEXT:  PL/pgSQL function cast_invoker(integer) while casting return value
> to function's return type
>   drop function cast_invoker(integer);
> ERROR:  current transaction is aborted, commands ignored until end of
> transaction block
>   drop function sql_to_date(integer) cascade;
> ERROR:  current transaction is aborted, commands ignored until end of
> transaction block
> commit;
> 
> The problem reproduces in the 10, 11 versions.

The cause is stale cast function id in cached expression of
plpgsql. (get_cast_hashentry)
    
Happens since 9.5 to 11.  Once happens, the symptom persists
until session-end. Doesn't happen on 9.4 since it doesn't cache
cast expressions. 12 invalidates cached cast expressions
(04fe805a17).

It seems to me possible that a cast calls a wrong function and
leads to a crash. But I don't come up with a good way to fix
this, but applying a part of the patch 04fe805a17 on 11(.2) seems
working.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7f1f962f60..aec1a68a4b 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5909,6 +5909,57 @@ expression_planner(Expr *expr)
     return (Expr *) result;
 }
 
+/*
+ * expression_planner_with_deps
+ *        Perform planner's transformations on a standalone expression,
+ *        returning expression dependency information along with the result.
+ *
+ * This is identical to expression_planner() except that it also returns
+ * information about possible dependencies of the expression, ie identities of
+ * objects whose definitions affect the result.  As in a PlannedStmt, these
+ * are expressed as a list of relation Oids and a list of PlanInvalItems.
+ */
+Expr *
+expression_planner_with_deps(Expr *expr,
+                             List **relationOids,
+                             List **invalItems)
+{
+    Node       *result;
+    PlannerGlobal glob;
+    PlannerInfo root;
+
+    /* Make up dummy planner state so we can use setrefs machinery */
+    MemSet(&glob, 0, sizeof(glob));
+    glob.type = T_PlannerGlobal;
+    glob.relationOids = NIL;
+    glob.invalItems = NIL;
+
+    MemSet(&root, 0, sizeof(root));
+    root.type = T_PlannerInfo;
+    root.glob = &glob;
+
+    /*
+     * Convert named-argument function calls, insert default arguments and
+     * simplify constant subexprs.  Collect identities of inlined functions
+     * and elided domains, too.
+     */
+    result = eval_const_expressions(&root, (Node *) expr);
+
+    /* Fill in opfuncid values if missing */
+    fix_opfuncids(result);
+
+    /*
+     * Now walk the finished expression to find anything else we ought to
+     * record as an expression dependency.
+     */
+    (void) extract_query_dependencies_walker(result, &root);
+
+    *relationOids = glob.relationOids;
+    *invalItems = glob.invalItems;
+
+    return (Expr *) result;
+}
+
 
 /*
  * plan_cluster_use_sort
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 80e6e0da0d..2ed1f65767 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -138,8 +138,7 @@ static List *set_returning_clause_references(PlannerInfo *root,
                                 Plan *topplan,
                                 Index resultRelation,
                                 int rtoffset);
-static bool extract_query_dependencies_walker(Node *node,
-                                  PlannerInfo *context);
+
 
 /*****************************************************************************
  *
@@ -2602,7 +2601,15 @@ extract_query_dependencies(Node *query,
     *hasRowSecurity = glob.dependsOnRole;
 }
 
-static bool
+/*
+ * Tree walker for extract_query_dependencies.
+ *
+ * This is exported so that expression_planner_with_deps can call it on
+ * simple expressions (post-planning, not before planning, in that case).
+ * In that usage, glob.dependsOnRole isn't meaningful, but the relationOids
+ * and invalItems lists are added to as needed.
+ */
+bool
 extract_query_dependencies_walker(Node *node, PlannerInfo *context)
 {
     if (node == NULL)
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 0ad3e3c736..87c82a4a6a 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -57,6 +57,7 @@
 #include "nodes/nodeFuncs.h"
 #include "optimizer/cost.h"
 #include "optimizer/planmain.h"
+#include "optimizer/planner.h"
 #include "optimizer/prep.h"
 #include "parser/analyze.h"
 #include "parser/parsetree.h"
@@ -86,6 +87,10 @@
  * guarantee to save a CachedPlanSource without error.
  */
 static CachedPlanSource *first_saved_plan = NULL;
+/*
+ * This is the head of the backend's list of CachedExpressions.
+ */
+static dlist_head cached_expression_list = DLIST_STATIC_INIT(cached_expression_list);
 
 static void ReleaseGenericPlan(CachedPlanSource *plansource);
 static List *RevalidateCachedQuery(CachedPlanSource *plansource,
@@ -103,7 +108,7 @@ static void ScanQueryForLocks(Query *parsetree, bool acquire);
 static bool ScanQueryWalker(Node *node, bool *acquire);
 static TupleDesc PlanCacheComputeResultDesc(List *stmt_list);
 static void PlanCacheRelCallback(Datum arg, Oid relid);
-static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
+static void PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue);
 static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
 
 
@@ -116,7 +121,7 @@ void
 InitPlanCache(void)
 {
     CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
-    CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+    CacheRegisterSyscacheCallback(PROCOID, PlanCacheObjectCallback, (Datum) 0);
     CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
     CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
     CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
@@ -1450,6 +1455,85 @@ CachedPlanGetTargetList(CachedPlanSource *plansource,
     return FetchStatementTargetList((Node *) pstmt);
 }
 
+/*
+ * GetCachedExpression: construct a CachedExpression for an expression.
+ *
+ * This performs the same transformations on the expression as
+ * expression_planner(), ie, convert an expression as emitted by parse
+ * analysis to be ready to pass to the executor.
+ *
+ * The result is stashed in a private, long-lived memory context.
+ * (Note that this might leak a good deal of memory in the caller's
+ * context before that.)  The passed-in expr tree is not modified.
+ */
+CachedExpression *
+GetCachedExpression(Node *expr)
+{
+    CachedExpression *cexpr;
+    List       *relationOids;
+    List       *invalItems;
+    MemoryContext cexpr_context;
+    MemoryContext oldcxt;
+
+    /*
+     * Pass the expression through the planner, and collect dependencies.
+     * Everything built here is leaked in the caller's context; that's
+     * intentional to minimize the size of the permanent data structure.
+     */
+    expr = (Node *) expression_planner_with_deps((Expr *) expr,
+                                                 &relationOids,
+                                                 &invalItems);
+
+    /*
+     * Make a private memory context, and copy what we need into that.  To
+     * avoid leaking a long-lived context if we fail while copying data, we
+     * initially make the context under the caller's context.
+     */
+    cexpr_context = AllocSetContextCreate(CurrentMemoryContext,
+                                          "CachedExpression",
+                                          ALLOCSET_SMALL_SIZES);
+
+    oldcxt = MemoryContextSwitchTo(cexpr_context);
+
+    cexpr = (CachedExpression *) palloc(sizeof(CachedExpression));
+    cexpr->magic = CACHEDEXPR_MAGIC;
+    cexpr->expr = copyObject(expr);
+    cexpr->is_valid = true;
+    cexpr->relationOids = copyObject(relationOids);
+    cexpr->invalItems = copyObject(invalItems);
+    cexpr->context = cexpr_context;
+
+    MemoryContextSwitchTo(oldcxt);
+
+    /*
+     * Reparent the expr's memory context under CacheMemoryContext so that it
+     * will live indefinitely.
+     */
+    MemoryContextSetParent(cexpr_context, CacheMemoryContext);
+
+    /*
+     * Add the entry to the global list of cached expressions.
+     */
+    dlist_push_tail(&cached_expression_list, &cexpr->node);
+
+    return cexpr;
+}
+
+/*
+ * FreeCachedExpression
+ *        Delete a CachedExpression.
+ */
+void
+FreeCachedExpression(CachedExpression *cexpr)
+{
+    /* Sanity check */
+    Assert(cexpr->magic == CACHEDEXPR_MAGIC);
+    /* Unlink from global list */
+    dlist_delete(&cexpr->node);
+    /* Free all storage associated with CachedExpression */
+    MemoryContextDelete(cexpr->context);
+}
+
 /*
  * QueryListGetPrimaryStmt
  *        Get the "primary" stmt within a list, ie, the one marked canSetTag.
@@ -1710,6 +1794,7 @@ static void
 PlanCacheRelCallback(Datum arg, Oid relid)
 {
     CachedPlanSource *plansource;
+    dlist_iter    iter;
 
     for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
     {
@@ -1759,11 +1844,30 @@ PlanCacheRelCallback(Datum arg, Oid relid)
             }
         }
     }
+
+    /* Likewise check cached expressions */
+    dlist_foreach(iter, &cached_expression_list)
+    {
+        CachedExpression *cexpr = dlist_container(CachedExpression,
+                                                  node, iter.cur);
+
+        Assert(cexpr->magic == CACHEDEXPR_MAGIC);
+
+        /* No work if it's already invalidated */
+        if (!cexpr->is_valid)
+            continue;
+
+        if ((relid == InvalidOid) ? cexpr->relationOids != NIL :
+            list_member_oid(cexpr->relationOids, relid))
+        {
+            cexpr->is_valid = false;
+        }
+    }
 }
 
 /*
- * PlanCacheFuncCallback
- *        Syscache inval callback function for PROCOID cache
+ * PlanCacheObjectCallback
+ *        Syscache inval callback function for PROCOID and TYPEOID caches
  *
  * Invalidate all plans mentioning the object with the specified hash value,
  * or all plans mentioning any member of this cache if hashvalue == 0.
@@ -1772,9 +1876,10 @@ PlanCacheRelCallback(Datum arg, Oid relid)
  * now only user-defined functions are tracked this way.
  */
 static void
-PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
+PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
 {
     CachedPlanSource *plansource;
+    dlist_iter    iter;
 
     for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
     {
@@ -1842,6 +1947,34 @@ PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
             }
         }
     }
+
+    /* Likewise check cached expressions */
+    dlist_foreach(iter, &cached_expression_list)
+    {
+        CachedExpression *cexpr = dlist_container(CachedExpression,
+                                                  node, iter.cur);
+        ListCell   *lc;
+
+        Assert(cexpr->magic == CACHEDEXPR_MAGIC);
+
+        /* No work if it's already invalidated */
+        if (!cexpr->is_valid)
+            continue;
+
+        foreach(lc, cexpr->invalItems)
+        {
+            PlanInvalItem *item = (PlanInvalItem *) lfirst(lc);
+
+            if (item->cacheId != cacheid)
+                continue;
+            if (hashvalue == 0 ||
+                item->hashValue == hashvalue)
+            {
+                cexpr->is_valid = false;
+                break;
+            }
+        }
+    }
 }
 
 /*
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index a081ca689a..8de97f9399 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -122,5 +122,6 @@ extern void extract_query_dependencies(Node *query,
                            List **relationOids,
                            List **invalItems,
                            bool *hasRowSecurity);
+extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);
 
 #endif                            /* PLANMAIN_H */
diff --git a/src/include/optimizer/planner.h b/src/include/optimizer/planner.h
index 3e733b34ed..902ab403ed 100644
--- a/src/include/optimizer/planner.h
+++ b/src/include/optimizer/planner.h
@@ -55,6 +55,9 @@ extern Path *get_cheapest_fractional_path(RelOptInfo *rel,
                              double tuple_fraction);
 
 extern Expr *expression_planner(Expr *expr);
+extern Expr *expression_planner_with_deps(Expr *expr,
+                             List **relationOids,
+                             List **invalItems);
 
 extern Expr *preprocess_phv_expression(PlannerInfo *root, Expr *expr);
 
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ab20aa04b0..dc2b5ba946 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -16,6 +16,7 @@
 #define PLANCACHE_H
 
 #include "access/tupdesc.h"
+#include "lib/ilist.h"
 #include "nodes/params.h"
 #include "utils/queryenvironment.h"
 
@@ -24,6 +25,7 @@ struct RawStmt;
 
 #define CACHEDPLANSOURCE_MAGIC        195726186
 #define CACHEDPLAN_MAGIC            953717834
+#define CACHEDEXPR_MAGIC            838275847
 
 /*
  * CachedPlanSource (which might better have been called CachedQuery)
@@ -143,6 +145,30 @@ typedef struct CachedPlan
     MemoryContext context;        /* context containing this CachedPlan */
 } CachedPlan;
 
+/*
+ * CachedExpression is a low-overhead mechanism for caching the planned form
+ * of standalone scalar expressions.  While such expressions are not usually
+ * subject to cache invalidation events, that can happen, for example because
+ * of replacement of a SQL function that was inlined into the expression.
+ * The plancache takes care of storing the expression tree and marking it
+ * invalid if a cache invalidation occurs, but the caller must notice the
+ * !is_valid status and discard the obsolete expression without reusing it.
+ * We do not store the original parse tree, only the planned expression;
+ * this is an optimization based on the assumption that we usually will not
+ * need to replan for the life of the session.
+ */
+typedef struct CachedExpression
+{
+    int            magic;            /* should equal CACHEDEXPR_MAGIC */
+    Node       *expr;            /* planned form of expression */
+    bool        is_valid;        /* is the expression still valid? */
+    /* remaining fields should be treated as private to plancache.c: */
+    List       *relationOids;    /* OIDs of relations the expr depends on */
+    List       *invalItems;        /* other dependencies, as PlanInvalItems */
+    MemoryContext context;        /* context containing this CachedExpression */
+    dlist_node    node;            /* link in global list of CachedExpressions */
+} CachedExpression;
+
 
 extern void InitPlanCache(void);
 extern void ResetPlanCache(void);
@@ -182,4 +208,7 @@ extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource,
               QueryEnvironment *queryEnv);
 extern void ReleaseCachedPlan(CachedPlan *plan, bool useResOwner);
 
+extern CachedExpression *GetCachedExpression(Node *expr);
+extern void FreeCachedExpression(CachedExpression *cexpr);
+
 #endif                            /* PLANCACHE_H */
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 8f8f7efe44..ecb8e79baa 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -152,6 +152,7 @@ typedef struct                    /* cast_hash table entry */
 {
     plpgsql_CastHashKey key;    /* hash key --- MUST BE FIRST */
     Expr       *cast_expr;        /* cast expression, or NULL if no-op cast */
+    CachedExpression *cast_cexpr;    /* cached expression backing the above */
     /* ExprState is valid only when cast_lxid matches current LXID */
     ExprState  *cast_exprstate; /* expression's eval tree */
     bool        cast_in_use;    /* true while we're executing eval tree */
@@ -7527,18 +7528,35 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
     cast_key.dsttypmod = dsttypmod;
     cast_entry = (plpgsql_CastHashEntry *) hash_search(estate->cast_hash,
                                                        (void *) &cast_key,
-                                                       HASH_FIND, NULL);
+                                                       HASH_ENTER, &found);
 
-    if (cast_entry == NULL)
+    if (!found)                    /* initialize if new entry */
+        cast_entry->cast_cexpr = NULL;
+
+    if (cast_entry->cast_cexpr == NULL ||
+        !cast_entry->cast_cexpr->is_valid)
     {
-        /* We've not looked up this coercion before */
+        /*
+         * We've not looked up this coercion before, or we have but the cached
+         * expression has been invalidated.
+         */
         Node       *cast_expr;
+        CachedExpression *cast_cexpr;
         CaseTestExpr *placeholder;
 
+        /*
+         * Drop old cached expression if there is one.
+         */
+        if (cast_entry->cast_cexpr)
+        {
+            FreeCachedExpression(cast_entry->cast_cexpr);
+            cast_entry->cast_cexpr = NULL;
+        }
+
         /*
          * Since we could easily fail (no such coercion), construct a
          * temporary coercion expression tree in the short-lived
-         * eval_mcontext, then if successful copy it to cast_hash_context.
+         * eval_mcontext, then if successful save it as a CachedExpression.
          */
         oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
 
@@ -7599,33 +7617,23 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
 
         /* Note: we don't bother labeling the expression tree with collation */
 
+        /* Plan the expression and build a CachedExpression */
+        cast_cexpr = GetCachedExpression(cast_expr);
+        cast_expr = cast_cexpr->expr;
+
         /* Detect whether we have a no-op (RelabelType) coercion */
         if (IsA(cast_expr, RelabelType) &&
             ((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
             cast_expr = NULL;
 
-        if (cast_expr)
-        {
-            /* ExecInitExpr assumes we've planned the expression */
-            cast_expr = (Node *) expression_planner((Expr *) cast_expr);
-
-            /* Now copy the tree into cast_hash_context */
-            MemoryContextSwitchTo(estate->cast_hash_context);
-
-            cast_expr = copyObject(cast_expr);
-        }
-
-        MemoryContextSwitchTo(oldcontext);
-
-        /* Now we can fill in a hashtable entry. */
-        cast_entry = (plpgsql_CastHashEntry *) hash_search(estate->cast_hash,
-                                                           (void *) &cast_key,
-                                                           HASH_ENTER, &found);
-        Assert(!found);            /* wasn't there a moment ago */
+        /* Now we can fill in the hashtable entry. */
+        cast_entry->cast_cexpr = cast_cexpr;
         cast_entry->cast_expr = (Expr *) cast_expr;
         cast_entry->cast_exprstate = NULL;
         cast_entry->cast_in_use = false;
         cast_entry->cast_lxid = InvalidLocalTransactionId;
+
+        MemoryContextSwitchTo(oldcontext);
     }
 
     /* Done if we have determined that this is a no-op cast. */