aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTom Lane <tgl@sss.pgh.pa.us>2015-08-15 12:00:36 -0400
committerTom Lane <tgl@sss.pgh.pa.us>2015-08-15 12:00:48 -0400
commit83604cc42353b6c0de2a3f3ac31f94759a9326ae (patch)
tree394d0b4dd7611b03594f4b5b41a0cb1a2db62135
parente95126cf048b08d7ff5eb72ec33737e9e27c08f8 (diff)
downloadpostgresql-83604cc42353b6c0de2a3f3ac31f94759a9326ae.tar.gz
postgresql-83604cc42353b6c0de2a3f3ac31f94759a9326ae.zip
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.
-rw-r--r--src/pl/plpgsql/src/pl_exec.c79
-rw-r--r--src/pl/plpgsql/src/plpgsql.h4
-rw-r--r--src/test/regress/expected/plpgsql.out7
-rw-r--r--src/test/regress/sql/plpgsql.sql9
4 files changed, 71 insertions, 28 deletions
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b7f63604e32..8bc5e6f29fd 100644
--- 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 b411e5a8d4d..fd5679c23ef 100644
--- 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 31182db705e..828b4a030bc 100644
--- 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 b697c9a935e..b46439ee709 100644
--- 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 $$