Re: Memory leak in CachememoryContext

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 */