Commit 83604cc4 authored by Tom Lane's avatar Tom Lane

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 c7b849a8.  I had forgotten about that while writing
commit 0fc94a5b, 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 0fc94a5b.

Per bug #13571 from Feike Steenbergen.  Preliminary analysis by
Oleksandr Shulgin.
parent e95126cf
...@@ -98,6 +98,10 @@ static SimpleEcontextStackEntry *simple_econtext_stack = NULL; ...@@ -98,6 +98,10 @@ static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
* *
* The evaluation state trees (cast_exprstate) are managed in the same way as * The evaluation state trees (cast_exprstate) are managed in the same way as
* simple expressions (i.e., we assume cast expressions are always simple). * 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 */ typedef struct /* lookup key for cast info */
{ {
...@@ -118,8 +122,8 @@ typedef struct /* cast_hash table entry */ ...@@ -118,8 +122,8 @@ typedef struct /* cast_hash table entry */
LocalTransactionId cast_lxid; LocalTransactionId cast_lxid;
} plpgsql_CastHashEntry; } plpgsql_CastHashEntry;
static MemoryContext cast_hash_context = NULL; static MemoryContext shared_cast_context = NULL;
static HTAB *cast_hash = NULL; static HTAB *shared_cast_hash = NULL;
/************************************************************ /************************************************************
* Local function forward declarations * Local function forward declarations
...@@ -287,7 +291,9 @@ static char *format_preparedparamsdata(PLpgSQL_execstate *estate, ...@@ -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 * 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 * to use a private simple_eval_estate, which is created and passed in by
* the caller. For regular functions, pass NULL, which implies using * 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 Datum
...@@ -3271,6 +3277,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, ...@@ -3271,6 +3277,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
ReturnSetInfo *rsi, ReturnSetInfo *rsi,
EState *simple_eval_estate) EState *simple_eval_estate)
{ {
HASHCTL ctl;
/* this link will be restored at exit from plpgsql_call_handler */ /* this link will be restored at exit from plpgsql_call_handler */
func->cur_estate = estate; func->cur_estate = estate;
...@@ -3319,11 +3327,44 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, ...@@ -3319,11 +3327,44 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
estate->paramLI->numParams = estate->ndatums; estate->paramLI->numParams = estate->ndatums;
estate->params_dirty = false; 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) if (simple_eval_estate)
{
estate->simple_eval_estate = 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 else
{
estate->simple_eval_estate = shared_simple_eval_estate; 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_tuptable = NULL;
estate->eval_processed = 0; estate->eval_processed = 0;
...@@ -6111,32 +6152,12 @@ get_cast_hashentry(PLpgSQL_execstate *estate, ...@@ -6111,32 +6152,12 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
LocalTransactionId curlxid; LocalTransactionId curlxid;
MemoryContext oldcontext; 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 */ /* Look for existing entry */
cast_key.srctype = srctype; cast_key.srctype = srctype;
cast_key.dsttype = dsttype; cast_key.dsttype = dsttype;
cast_key.srctypmod = srctypmod; cast_key.srctypmod = srctypmod;
cast_key.dsttypmod = dsttypmod; cast_key.dsttypmod = dsttypmod;
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash, cast_entry = (plpgsql_CastHashEntry *) hash_search(estate->cast_hash,
(void *) &cast_key, (void *) &cast_key,
HASH_FIND, NULL); HASH_FIND, NULL);
...@@ -6221,7 +6242,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate, ...@@ -6221,7 +6242,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
cast_expr = (Node *) expression_planner((Expr *) cast_expr); cast_expr = (Node *) expression_planner((Expr *) cast_expr);
/* Now copy the tree into cast_hash_context */ /* Now copy the tree into cast_hash_context */
MemoryContextSwitchTo(cast_hash_context); MemoryContextSwitchTo(estate->cast_hash_context);
cast_expr = copyObject(cast_expr); cast_expr = copyObject(cast_expr);
} }
...@@ -6229,7 +6250,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate, ...@@ -6229,7 +6250,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);
/* Now we can fill in a hashtable entry. */ /* 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, (void *) &cast_key,
HASH_ENTER, &found); HASH_ENTER, &found);
Assert(!found); /* wasn't there a moment ago */ Assert(!found); /* wasn't there a moment ago */
...@@ -6251,7 +6272,9 @@ get_cast_hashentry(PLpgSQL_execstate *estate, ...@@ -6251,7 +6272,9 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
* executions. (We will leak some memory intra-transaction if that * 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 * 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 * 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; curlxid = MyProc->lxid;
if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use) if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use)
......
...@@ -801,6 +801,10 @@ typedef struct PLpgSQL_execstate ...@@ -801,6 +801,10 @@ typedef struct PLpgSQL_execstate
/* EState to use for "simple" expression evaluation */ /* EState to use for "simple" expression evaluation */
EState *simple_eval_estate; 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 */ /* temporary state for results from evaluation of query or expr */
SPITupleTable *eval_tuptable; SPITupleTable *eval_tuptable;
uint32 eval_processed; uint32 eval_processed;
......
...@@ -4764,6 +4764,13 @@ commit; ...@@ -4764,6 +4764,13 @@ commit;
drop function cast_invoker(integer); drop function cast_invoker(integer);
drop function sql_to_date(integer) cascade; drop function sql_to_date(integer) cascade;
NOTICE: drop cascades to cast from integer to date 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 -- Test for consistent reporting of error context
create function fail() returns int language plpgsql as $$ create function fail() returns int language plpgsql as $$
begin begin
......
...@@ -3836,6 +3836,15 @@ commit; ...@@ -3836,6 +3836,15 @@ commit;
drop function cast_invoker(integer); drop function cast_invoker(integer);
drop function sql_to_date(integer) cascade; 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 -- Test for consistent reporting of error context
create function fail() returns int language plpgsql as $$ create function fail() returns int language plpgsql as $$
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment