Thread: BUG #15746: cache lookup failed for function in plpgsql block
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. regards, Roman Zharkov
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. */
Re: BUG #15746: cache lookup failed for function in plpgsql block
From
r.zharkov@postgrespro.ru
Date:
On 2019-04-11 19:11, Kyotaro HORIGUCHI wrote: > Hello. > > 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. Hello, Thank you! The patch works. -- regards, Roman Zharkov
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: > It seems to me possible that a cast calls a wrong function and > leads to a crash. I don't see a good reason to think that a crash is possible. > 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. FWIW, I intentionally did not back-patch 04fe805a17. I judged that before the domain-related changes in that patch, such cases wouldn't come up often enough to justify inserting poorly-tested code into the back branches. I still think that. regards, tom lane