Repair unsafe use of shared typecast-lookup table in plpgsql DO blocks.

DO blocks use private simple_eval_estates to avoid intra-transaction memory
leakage, cf commit c7b849a89.  I had forgotten about that while writing
commit 0fc94a5ba, but it means that expression execution trees created
within a DO block disappear immediately on exiting the DO block, and hence
can't safely be linked into plpgsql's session-wide cast hash table.
To fix, give a DO block a private cast hash table to go with its private
simple_eval_estate.  This is less efficient than one could wish, since
DO blocks can no longer share any cast lookup work with other plpgsql
execution, but it shouldn't be too bad; in any case it's no worse than
what happened in DO blocks before commit 0fc94a5ba.

Per bug #13571 from Feike Steenbergen.  Preliminary analysis by
Oleksandr Shulgin.

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b7f63604e328da82fad0fff293918af92faad9b4..8bc5e6f29fd0537e5442fdf78462c20682bcefee 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -98,6 +98,10 @@ static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
  *
  * The evaluation state trees (cast_exprstate) 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.
  */
 typedef struct                                 /* lookup key for cast info */
 {
@@ -118,8 +122,8 @@ typedef struct                                      /* cast_hash table entry */
        LocalTransactionId cast_lxid;
 } plpgsql_CastHashEntry;
 
-static MemoryContext cast_hash_context = NULL;
-static HTAB *cast_hash = NULL;
+static MemoryContext shared_cast_context = NULL;
+static HTAB *shared_cast_hash = NULL;
 
 /************************************************************
  * Local function forward declarations
@@ -287,7 +291,9 @@ static char *format_preparedparamsdata(PLpgSQL_execstate *estate,
  * difference that this code is aware of is that for a DO block, we want
  * to use a private simple_eval_estate, which is created and passed in by
  * the caller.  For regular functions, pass NULL, which implies using
- * shared_simple_eval_estate.
+ * shared_simple_eval_estate.  (When using a private simple_eval_estate,
+ * we must also use a private cast hashtable, but that's taken care of
+ * within plpgsql_estate_setup.)
  * ----------
  */
 Datum
@@ -3271,6 +3277,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
                                         ReturnSetInfo *rsi,
                                         EState *simple_eval_estate)
 {
+       HASHCTL         ctl;
+
        /* this link will be restored at exit from plpgsql_call_handler */
        func->cur_estate = estate;
 
@@ -3319,11 +3327,44 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
        estate->paramLI->numParams = estate->ndatums;
        estate->params_dirty = false;
 
-       /* set up for use of appropriate simple-expression EState */
+       /* set up for use of appropriate simple-expression EState and cast hash */
        if (simple_eval_estate)
+       {
                estate->simple_eval_estate = simple_eval_estate;
+               /* Private cast hash just lives in function's main context */
+               memset(&ctl, 0, sizeof(ctl));
+               ctl.keysize = sizeof(plpgsql_CastHashKey);
+               ctl.entrysize = sizeof(plpgsql_CastHashEntry);
+               ctl.hcxt = CurrentMemoryContext;
+               estate->cast_hash = hash_create("PLpgSQL private cast cache",
+                                                                               16,             /* start small and extend */
+                                                                               &ctl,
+                                                                         HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+               estate->cast_hash_context = CurrentMemoryContext;
+       }
        else
+       {
                estate->simple_eval_estate = shared_simple_eval_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_MINSIZE,
+                                                                                                  ALLOCSET_DEFAULT_INITSIZE,
+                                                                                                  ALLOCSET_DEFAULT_MAXSIZE);
+                       memset(&ctl, 0, sizeof(ctl));
+                       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);
+               }
+               estate->cast_hash = shared_cast_hash;
+               estate->cast_hash_context = shared_cast_context;
+       }
 
        estate->eval_tuptable = NULL;
        estate->eval_processed = 0;
@@ -6111,32 +6152,12 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
        LocalTransactionId curlxid;
        MemoryContext oldcontext;
 
-       /* Create the session-wide cast-info hash table if we didn't already */
-       if (cast_hash == NULL)
-       {
-               HASHCTL         ctl;
-
-               cast_hash_context = AllocSetContextCreate(TopMemoryContext,
-                                                                                                 "PLpgSQL cast info",
-                                                                                                 ALLOCSET_DEFAULT_MINSIZE,
-                                                                                                 ALLOCSET_DEFAULT_INITSIZE,
-                                                                                                 ALLOCSET_DEFAULT_MAXSIZE);
-               memset(&ctl, 0, sizeof(ctl));
-               ctl.keysize = sizeof(plpgsql_CastHashKey);
-               ctl.entrysize = sizeof(plpgsql_CastHashEntry);
-               ctl.hcxt = cast_hash_context;
-               cast_hash = hash_create("PLpgSQL cast cache",
-                                                               16,             /* start small and extend */
-                                                               &ctl,
-                                                               HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
-       }
-
        /* Look for existing entry */
        cast_key.srctype = srctype;
        cast_key.dsttype = dsttype;
        cast_key.srctypmod = srctypmod;
        cast_key.dsttypmod = dsttypmod;
-       cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
+       cast_entry = (plpgsql_CastHashEntry *) hash_search(estate->cast_hash,
                                                                                                           (void *) &cast_key,
                                                                                                           HASH_FIND, NULL);
 
@@ -6221,7 +6242,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
                        cast_expr = (Node *) expression_planner((Expr *) cast_expr);
 
                        /* Now copy the tree into cast_hash_context */
-                       MemoryContextSwitchTo(cast_hash_context);
+                       MemoryContextSwitchTo(estate->cast_hash_context);
 
                        cast_expr = copyObject(cast_expr);
                }
@@ -6229,7 +6250,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
                MemoryContextSwitchTo(oldcontext);
 
                /* Now we can fill in a hashtable entry. */
-               cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
+               cast_entry = (plpgsql_CastHashEntry *) hash_search(estate->cast_hash,
                                                                                                                   (void *) &cast_key,
                                                                                                                 HASH_ENTER, &found);
                Assert(!found);                 /* wasn't there a moment ago */
@@ -6251,7 +6272,9 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
         * executions.  (We will leak some memory intra-transaction if that
         * happens a lot, but we don't expect it to.)  It's okay to update the
         * hash table with the new tree because all plpgsql functions within a
-        * given transaction share the same simple_eval_estate.
+        * given transaction share the same simple_eval_estate.  (Well, regular
+        * functions do; DO blocks have private simple_eval_estates, and private
+        * cast hash tables to go with them.)
         */
        curlxid = MyProc->lxid;
        if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use)

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index b411e5a8d4dee2cc30d8b54b4c44b1af82af50f8..fd5679c23ef779b7d7e855494fc79a9afc303bbb 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -801,6 +801,10 @@ typedef struct PLpgSQL_execstate
        /* EState to use for "simple" expression evaluation */
        EState     *simple_eval_estate;
 
+       /* Lookup table to use for executing type casts */
+       HTAB       *cast_hash;
+       MemoryContext cast_hash_context;
+
        /* temporary state for results from evaluation of query or expr */
        SPITupleTable *eval_tuptable;
        uint32          eval_processed;

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 31182db705e58cc51cb4646bb9e41c93de976471..828b4a030bc91a65f89d390609f48e8d6b8dadf5 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -4764,6 +4764,13 @@ commit;
 drop function cast_invoker(integer);
 drop function sql_to_date(integer) cascade;
 NOTICE:  drop cascades to cast from integer to date
+-- Test handling of cast cache inside DO blocks
+-- (to check the original crash case, this must be a cast not previously
+-- used in this session)
+begin;
+do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
+do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
+end;
 -- Test for consistent reporting of error context
 create function fail() returns int language plpgsql as $$
 begin

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index b697c9a935e347531076fdffe8f83a613a3d027d..b46439ee709ee63f98c6c22afb90d8b65aebcdd2 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -3836,6 +3836,15 @@ commit;
 drop function cast_invoker(integer);
 drop function sql_to_date(integer) cascade;
 
+-- Test handling of cast cache inside DO blocks
+-- (to check the original crash case, this must be a cast not previously
+-- used in this session)
+
+begin;
+do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
+do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
+end;
+
 -- Test for consistent reporting of error context
 
 create function fail() returns int language plpgsql as $$