Обсуждение: Memory leak in CachememoryContext

Ajit Awekar <ajit.awekar@enterprisedb.com> writes:
> Please find below simple repro for CacheMemoryContext memory leak

Hm, yeah, reproduced here.

> During anonymous block execution in the function "plpgsql_estate_setup", a
> local casting hash table gets created in SPI memory context. When hash
> table look up is performed in "get_cast_hashenty" function if entry is no
> present , memory is allocated in CacheMemoryContext in function
> "GetCachedExpression".At the end of proc execution SPI memory context is
> deleted and hence local hash table gets deleted, but still entries remain
> in Cachemeorycontext.

Yeah, it's from using just a short-lived cast hash table for DO blocks.
I think that was okay when it was written, but when we wheeled the
CachedExpression machinery undeneath it, we created a problem.

> Please find attached(memoryleakfix.patch) to this email.

I don't think this fix is acceptable at all.  A minor problem is that
we can't change the API of GetCachedExpression() in stable branches,
because extensions may be using it.  We could work around that by
making it a wrapper function.  But the big problem is that this patch
destroys the reason for using a CachedExpression in the first place:
because you aren't linking it into cached_expression_list, the plancache
will not detect events that should obsolete the expression.  The
test cases added by 04fe805a1 only cover regular functions, but one
that did domain constraint DDL within a DO block would expose the
shortcoming.

A possible answer is to split plpgsql's cast hash table into two parts.
The lower-level part would contain the hash key, cast_expr and
cast_cexpr fields, and would have session lifespan and be used by
both DO blocks and regular functions.  In this way we'd not leak
CachedExpressions.   The upper-level hash table would contain the
hash key, a link to the relevant lower-level entry, and the
cast_exprstate, cast_in_use, cast_lxid fields.  There would be a
session-lifespan one of these plus one for each DO block, so that
management of the ExprStates still works as it does now for DO blocks.

This could be factored in other ways, and maybe another way would be
simpler.  But the idea is that DO blocks should use persistent
CachedExpressions even though their cast_exprstates are transient.

I've not tried to code this, do you want to?

            regards, tom lane

Ajit Awekar <ajit.awekar@enterprisedb.com> writes:
> I have implemented the approach by splitting the hash table into two parts.
> Please find the attached patch for the same.

I found a few things not to like about this:

* You didn't update the comment describing these hash tables.

* I wasn't really thrilled about renaming the plpgsql_CastHashEntry
typedef, as that seemed to just create uninteresting diff noise.
Also, "SessionCastHashEntry" versus "PrivateCastHashEntry" seems a
very misleading choice of names, since one of the "PrivateCastHashEntry"
hash tables is in fact session-lifespan.  After some thought I left
the "upper" hash table entry type as plpgsql_CastHashEntry so that
code outside the immediate area needn't be affected, and named the
"lower" table cast_expr_hash, with entry type plpgsql_CastExprHashEntry.
I'm not wedded to those names though, if you have a better idea.

(BTW, it's completely reasonable to rename the type as an intermediate
step in making a patch like this, since it ensures you'll examine
every existing usage to choose the right thing to change it to.  But
I generally rename things back afterwards.)

* I didn't like having to do two hashtable lookups during every
call even after we've fully cached the info.  That's easy to avoid
by keeping a link to the associated "lower" hashtable entry in the
"upper" ones.

* You removed the reset of cast_exprstate etc from the code path where
we've just reconstructed the cast_expr.  That's a mistake since it
might allow us to skip rebuilding the derived expression state after
a DDL change.


Also, while looking at this I noticed that we are no longer making
any use of estate->cast_hash_context.  That's not the fault of
your patch; it's another oversight in the one that added the
CachedExpression mechanism.  The compiled expressions used to be
stored in that context, but now the plancache is responsible for
them and we are never putting anything in the cast_hash_context.
So we might as well get rid of that and save 8K of wasted memory.
This allows some simplification in the hashtable setup code too.

In short, I think we need something more like the attached.

(Note to self: we can't remove the cast_hash_context field in
back branches for fear of causing an ABI break for pldebugger.
But we can leave it unused, I think.)

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index e271ae5151..68340251a4 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -136,18 +136,22 @@ static ResourceOwner shared_simple_eval_resowner = NULL;
     MemoryContextAllocZero(get_eval_mcontext(estate), sz)

 /*
- * We use a session-wide hash table for caching cast information.
+ * We use two session-wide hash tables for caching cast information.
  *
- * Once built, the compiled expression trees (cast_expr fields) survive for
- * the life of the session.  At some point it might be worth invalidating
- * those after pg_cast changes, but for the moment we don't bother.
+ * cast_expr_hash entries (of type plpgsql_CastExprHashEntry) hold compiled
+ * expression trees for casts.  These survive for the life of the session and
+ * are shared across all PL/pgSQL functions and DO blocks.  At some point it
+ * might be worth invalidating them after pg_cast changes, but for the moment
+ * we don't bother.
  *
- * The evaluation state trees (cast_exprstate) are managed in the same way as
- * simple expressions (i.e., we assume cast expressions are always simple).
+ * There is a separate hash table shared_cast_hash (with entries of type
+ * plpgsql_CastHashEntry) containing evaluation state trees for these
+ * expressions, which are managed in the same way as simple expressions
+ * (i.e., we assume cast expressions are always simple).
  *
- * As with simple expressions, DO blocks don't use the shared hash table but
- * must have their own.  This isn't ideal, but we don't want to deal with
- * multiple simple_eval_estates within a DO block.
+ * As with simple expressions, DO blocks don't use the shared_cast_hash table
+ * but must have their own evaluation state trees.  This isn't ideal, but we
+ * don't want to deal with multiple simple_eval_estates within a DO block.
  */
 typedef struct                    /* lookup key for cast info */
 {
@@ -158,18 +162,24 @@ typedef struct                    /* lookup key for cast info */
     int32        dsttypmod;        /* destination typmod for cast */
 } plpgsql_CastHashKey;

-typedef struct                    /* cast_hash table entry */
+typedef struct                    /* cast_expr_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 */
+} plpgsql_CastExprHashEntry;
+
+typedef struct                    /* cast_hash table entry */
+{
+    plpgsql_CastHashKey key;    /* hash key --- MUST BE FIRST */
+    plpgsql_CastExprHashEntry *cast_centry; /* link to matching expr entry */
     /* 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 */
     LocalTransactionId cast_lxid;
 } plpgsql_CastHashEntry;

-static MemoryContext shared_cast_context = NULL;
+static HTAB *cast_expr_hash = NULL;
 static HTAB *shared_cast_hash = NULL;

 /*
@@ -4037,7 +4047,6 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
                                         16, /* start small and extend */
                                         &ctl,
                                         HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
-        estate->cast_hash_context = CurrentMemoryContext;
     }
     else
     {
@@ -4045,20 +4054,27 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
         /* Create the session-wide cast-info hash table if we didn't already */
         if (shared_cast_hash == NULL)
         {
-            shared_cast_context = AllocSetContextCreate(TopMemoryContext,
-                                                        "PLpgSQL cast info",
-                                                        ALLOCSET_DEFAULT_SIZES);
             ctl.keysize = sizeof(plpgsql_CastHashKey);
             ctl.entrysize = sizeof(plpgsql_CastHashEntry);
-            ctl.hcxt = shared_cast_context;
             shared_cast_hash = hash_create("PLpgSQL cast cache",
                                            16,    /* start small and extend */
                                            &ctl,
-                                           HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+                                           HASH_ELEM | HASH_BLOBS);
         }
         estate->cast_hash = shared_cast_hash;
-        estate->cast_hash_context = shared_cast_context;
     }
+
+    /* Create the session-wide cast-expression hash if we didn't already */
+    if (cast_expr_hash == NULL)
+    {
+        ctl.keysize = sizeof(plpgsql_CastHashKey);
+        ctl.entrysize = sizeof(plpgsql_CastExprHashEntry);
+        cast_expr_hash = hash_create("PLpgSQL cast expressions",
+                                     16,    /* start small and extend */
+                                     &ctl,
+                                     HASH_ELEM | HASH_BLOBS);
+    }
+
     /* likewise for the simple-expression resource owner */
     if (simple_eval_resowner)
         estate->simple_eval_resowner = simple_eval_resowner;
@@ -7768,6 +7784,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
 {
     plpgsql_CastHashKey cast_key;
     plpgsql_CastHashEntry *cast_entry;
+    plpgsql_CastExprHashEntry *expr_entry;
     bool        found;
     LocalTransactionId curlxid;
     MemoryContext oldcontext;
@@ -7781,10 +7798,28 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
                                                        &cast_key,
                                                        HASH_ENTER, &found);
     if (!found)                    /* initialize if new entry */
-        cast_entry->cast_cexpr = NULL;
+    {
+        /* We need a second lookup to see if a cast_expr_hash entry exists */
+        expr_entry = (plpgsql_CastExprHashEntry *) hash_search(cast_expr_hash,
+                                                               &cast_key,
+                                                               HASH_ENTER,
+                                                               &found);
+        if (!found)                /* initialize if new expr entry */
+            expr_entry->cast_cexpr = NULL;
+
+        cast_entry->cast_centry = expr_entry;
+        cast_entry->cast_exprstate = NULL;
+        cast_entry->cast_in_use = false;
+        cast_entry->cast_lxid = InvalidLocalTransactionId;
+    }
+    else
+    {
+        /* Use always-valid link to avoid a second hash lookup */
+        expr_entry = cast_entry->cast_centry;
+    }

-    if (cast_entry->cast_cexpr == NULL ||
-        !cast_entry->cast_cexpr->is_valid)
+    if (expr_entry->cast_cexpr == NULL ||
+        !expr_entry->cast_cexpr->is_valid)
     {
         /*
          * We've not looked up this coercion before, or we have but the cached
@@ -7797,10 +7832,10 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
         /*
          * Drop old cached expression if there is one.
          */
-        if (cast_entry->cast_cexpr)
+        if (expr_entry->cast_cexpr)
         {
-            FreeCachedExpression(cast_entry->cast_cexpr);
-            cast_entry->cast_cexpr = NULL;
+            FreeCachedExpression(expr_entry->cast_cexpr);
+            expr_entry->cast_cexpr = NULL;
         }

         /*
@@ -7881,9 +7916,11 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
             ((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
             cast_expr = NULL;

-        /* Now we can fill in the hashtable entry. */
-        cast_entry->cast_cexpr = cast_cexpr;
-        cast_entry->cast_expr = (Expr *) cast_expr;
+        /* Now we can fill in the expression hashtable entry. */
+        expr_entry->cast_cexpr = cast_cexpr;
+        expr_entry->cast_expr = (Expr *) cast_expr;
+
+        /* Be sure to reset the exprstate hashtable entry, too. */
         cast_entry->cast_exprstate = NULL;
         cast_entry->cast_in_use = false;
         cast_entry->cast_lxid = InvalidLocalTransactionId;
@@ -7892,7 +7929,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
     }

     /* Done if we have determined that this is a no-op cast. */
-    if (cast_entry->cast_expr == NULL)
+    if (expr_entry->cast_expr == NULL)
         return NULL;

     /*
@@ -7911,7 +7948,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
     if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use)
     {
         oldcontext = MemoryContextSwitchTo(estate->simple_eval_estate->es_query_cxt);
-        cast_entry->cast_exprstate = ExecInitExpr(cast_entry->cast_expr, NULL);
+        cast_entry->cast_exprstate = ExecInitExpr(expr_entry->cast_expr, NULL);
         cast_entry->cast_in_use = false;
         cast_entry->cast_lxid = curlxid;
         MemoryContextSwitchTo(oldcontext);
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index c40471bb89..2b4bcd1dbe 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1076,7 +1076,6 @@ typedef struct PLpgSQL_execstate

     /* lookup table to use for executing type casts */
     HTAB       *cast_hash;
-    MemoryContext cast_hash_context;

     /* memory context for statement-lifespan temporary values */
     MemoryContext stmt_mcontext;    /* current stmt context, or NULL if none */

On 2023-Apr-21, Tom Lane wrote:

> (Note to self: we can't remove the cast_hash_context field in
> back branches for fear of causing an ABI break for pldebugger.
> But we can leave it unused, I think.)

Hmm, we can leave it unused in our code, but it still needs to be
initialized to some valid memory context anyway; otherwise hypothetical
code that uses it would still crash.  This seems halfway obvious, but
since the submitted patch doesn't have this part, I thought better to
point it out.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> (Note to self: we can't remove the cast_hash_context field in
>> back branches for fear of causing an ABI break for pldebugger.
>> But we can leave it unused, I think.)

> Hmm, we can leave it unused in our code, but it still needs to be
> initialized to some valid memory context anyway; otherwise hypothetical
> code that uses it would still crash.

I think we want that to happen, actually, because it's impossible
to guess what such hypothetical code needs the context to be.
As things stand now, that field points to a long-lived context
in some cases and a short-lived one in others.  We risk either
data structure corruption or a session-lifespan memory leak
if we guess about such usage ... which really shouldn't exist
anyway.

            regards, tom lane

I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> Hmm, we can leave it unused in our code, but it still needs to be
>> initialized to some valid memory context anyway; otherwise hypothetical
>> code that uses it would still crash.

> I think we want that to happen, actually, because it's impossible
> to guess what such hypothetical code needs the context to be.

I guess we could have the back branches continue to create a
shared_cast_context and just not use it in core.  Seems rather
expensive for a very hypothetical compatibility measure, though.

            regards, tom lane

On 2023-Apr-24, Tom Lane wrote:

> I wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> >> Hmm, we can leave it unused in our code, but it still needs to be
> >> initialized to some valid memory context anyway; otherwise hypothetical
> >> code that uses it would still crash.
> 
> > I think we want that to happen, actually, because it's impossible
> > to guess what such hypothetical code needs the context to be.
> 
> I guess we could have the back branches continue to create a
> shared_cast_context and just not use it in core.  Seems rather
> expensive for a very hypothetical compatibility measure, though.

I think a session-long memory leak is not so bad, compared to a possible
crash.  However, after looking at the code again, as well as pldebugger
and plpgsql_check, I agree that there's no point in doing anything other
than keeping the field there.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Apr-24, Tom Lane wrote:
>> I guess we could have the back branches continue to create a
>> shared_cast_context and just not use it in core.  Seems rather
>> expensive for a very hypothetical compatibility measure, though.

> I think a session-long memory leak is not so bad, compared to a possible
> crash.  However, after looking at the code again, as well as pldebugger
> and plpgsql_check, I agree that there's no point in doing anything other
> than keeping the field there.

Yeah, I can't see any plausible reason for outside code to be using
that field (and I don't see any evidence in Debian Code Search that
anyone is).  I'll push it like this then.  Thanks for looking!

            regards, tom lane