open-dsi
/
postgres
mirror of https://github.com/postgres/postgres
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Repair mishandling of cached cast-expression trees in plpgsql.

Browse Source 
In commit 1345cc67bb, I introduced caching
of expressions representing type-cast operations into plpgsql.  However,
I supposed that I could cache both the expression trees and the evaluation
state trees derived from them for the life of the session.  This doesn't
work, because we execute the expressions in plpgsql's simple_eval_estate,
which has an ecxt_per_query_memory that is only transaction-lifespan.
Therefore we can end up putting pointers into the evaluation state tree
that point to transaction-lifespan memory; in particular this happens if
the cast expression calls a SQL-language function, as reported by Geoff
Winkless.

The minimum-risk fix seems to be to treat the state trees the same way
we do for "simple expression" trees in plpgsql, ie create them in the
simple_eval_estate's ecxt_per_query_memory, which means recreating them
once per transaction.

Since I had to introduce bookkeeping overhead for that anyway, I bought
back some of the added cost by sharing the read-only expression trees
across all functions in the session, instead of using a per-function
table as originally.  The simple-expression bookkeeping takes care of
the recursive-usage risk that I was concerned about avoiding before.

At some point we should take a harder look at how all this works,
and see if we can't reduce the amount of tree reinitialization needed.
But that won't happen for 9.5.
pull/14/head
Tom Lane 10 years ago
parent 266e771435
commit 0fc94a5bab
4 changed files with 262 additions and 124 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 290
      src/pl/plpgsql/src/pl_exec.c
  2. 4
      src/pl/plpgsql/src/plpgsql.h
  3. 62
      src/test/regress/expected/plpgsql.out
  4. 30
      src/test/regress/sql/plpgsql.sql

290
src/pl/plpgsql/src/pl_exec.c
Unescape View File

@ -53,21 +53,6 @@ typedef struct
bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
typedef struct
{
/* NB: we assume this struct contains no padding bytes */
Oid srctype; /* source type for cast */
Oid dsttype; /* destination type for cast */
int32 srctypmod; /* source typmod for cast */
int32 dsttypmod; /* destination typmod for cast */
} plpgsql_CastHashKey;
typedef struct
{
plpgsql_CastHashKey key; /* hash key --- MUST BE FIRST */
ExprState *cast_exprstate; /* cast expression, or NULL if no-op cast */
} plpgsql_CastHashEntry;
/*
* All plpgsql function executions within a single transaction share the same
* executor EState for evaluating "simple" expressions. Each function call
@ -104,6 +89,38 @@ typedef struct SimpleEcontextStackEntry
static EState *shared_simple_eval_estate = NULL;
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/*
* We use a session-wide hash table 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.
*
* The evaluation state trees (cast_exprstate) are managed in the same way as
* simple expressions (i.e., we assume cast expressions are always simple).
*/
typedef struct /* lookup key for cast info */
{
/* NB: we assume this struct contains no padding bytes */
Oid srctype; /* source type for cast */
Oid dsttype; /* destination type for cast */
int32 srctypmod; /* source typmod for cast */
int32 dsttypmod; /* destination typmod for cast */
} plpgsql_CastHashKey;
typedef struct /* cast_hash table entry */
{
plpgsql_CastHashKey key; /* hash key --- MUST BE FIRST */
Expr *cast_expr; /* cast expression, or NULL if no-op cast */
/* The ExprState tree 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 cast_hash_context = NULL;
static HTAB *cast_hash = NULL;
/************************************************************
* Local function forward declarations
************************************************************/
@ -238,8 +255,9 @@ static Datum exec_cast_value(PLpgSQL_execstate *estate,
Datum value, bool *isnull,
Oid valtype, int32 valtypmod,
Oid reqtype, int32 reqtypmod);
static ExprState *get_cast_expression(PLpgSQL_execstate *estate,
Oid srctype, int32 srctypmod, Oid dsttype, int32 dsttypmod);
static plpgsql_CastHashEntry *get_cast_hashentry(PLpgSQL_execstate *estate,
Oid srctype, int32 srctypmod,
Oid dsttype, int32 dsttypmod);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
@ -6043,12 +6061,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
if (valtype != reqtype ||
(valtypmod != reqtypmod && reqtypmod != -1))
{
ExprState *cast_expr;
plpgsql_CastHashEntry *cast_entry;
cast_expr = get_cast_expression(estate,
cast_entry = get_cast_hashentry(estate,
valtype, valtypmod,
reqtype, reqtypmod);
if (cast_expr)
if (cast_entry)
{
ExprContext *econtext = estate->eval_econtext;
MemoryContext oldcontext;
@ -6058,7 +6076,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
econtext->caseValue_datum = value;
econtext->caseValue_isNull = *isnull;
value = ExecEvalExpr(cast_expr, econtext, isnull, NULL);
cast_entry->cast_in_use = true;
value = ExecEvalExpr(cast_entry->cast_exprstate, econtext,
isnull, NULL);
cast_entry->cast_in_use = false;
MemoryContextSwitchTo(oldcontext);
}
@ -6068,46 +6091,44 @@ exec_cast_value(PLpgSQL_execstate *estate,
}
/* ----------
* get_cast_expression Look up how to perform a type cast
*
* Returns an expression evaluation tree based on a CaseTestExpr input,
* or NULL if the cast is a mere no-op relabeling.
* get_cast_hashentry Look up how to perform a type cast
*
* We cache the results of the lookup in a per-function hash table.
* It's tempting to consider using a session-wide hash table instead,
* but that introduces some corner-case questions that probably aren't
* worth dealing with; in particular that re-entrant use of an evaluation
* tree might occur. That would also set in stone the assumption that
* collation isn't important to a cast function.
* Returns a plpgsql_CastHashEntry if an expression has to be evaluated,
* or NULL if the cast is a mere no-op relabeling. If there's work to be
* done, the cast_exprstate field contains an expression evaluation tree
* based on a CaseTestExpr input, and the cast_in_use field should be set
* TRUE while executing it.
* ----------
*/
static ExprState *
get_cast_expression(PLpgSQL_execstate *estate,
Oid srctype, int32 srctypmod, Oid dsttype, int32 dsttypmod)
static plpgsql_CastHashEntry *
get_cast_hashentry(PLpgSQL_execstate *estate,
Oid srctype, int32 srctypmod,
Oid dsttype, int32 dsttypmod)
{
HTAB *cast_hash = estate->func->cast_hash;
plpgsql_CastHashKey cast_key;
plpgsql_CastHashEntry *cast_entry;
bool found;
CaseTestExpr *placeholder;
Node *cast_expr;
ExprState *cast_exprstate;
LocalTransactionId curlxid;
MemoryContext oldcontext;
/* Create the cast-info hash table if we didn't already */
/* 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 = estate->func->fn_cxt;
ctl.hcxt = cast_hash_context;
cast_hash = hash_create("PLpgSQL cast cache",
16, /* start small and extend */
&ctl,
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
estate->func->cast_hash = cast_hash;
}
/* Look for existing entry */
@ -6118,102 +6139,131 @@ get_cast_expression(PLpgSQL_execstate *estate,
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
(void *) &cast_key,
HASH_FIND, NULL);
if (cast_entry)
return cast_entry->cast_exprstate;
/* Construct expression tree for coercion in function's context */
oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt);
if (cast_entry == NULL)
{
/* We've not looked up this coercion before */
Node *cast_expr;
CaseTestExpr *placeholder;
/*
* We use a CaseTestExpr as the base of the coercion tree, since it's very
* cheap to insert the source value for that.
*/
placeholder = makeNode(CaseTestExpr);
placeholder->typeId = srctype;
placeholder->typeMod = srctypmod;
placeholder->collation = get_typcollation(srctype);
if (OidIsValid(estate->func->fn_input_collation) &&
OidIsValid(placeholder->collation))
placeholder->collation = estate->func->fn_input_collation;
/*
* Since we could easily fail (no such coercion), construct a
* temporary coercion expression tree in a short-lived context, then
* if successful copy it to cast_hash_context.
*/
oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
/*
* Apply coercion. We use ASSIGNMENT coercion because that's the closest
* match to plpgsql's historical behavior; in particular, EXPLICIT
* coercion would allow silent truncation to a destination
* varchar/bpchar's length, which we do not want.
*
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
* expects to see that for Const input nodes), so don't call it; we'll
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
* coercing RECORD to some other type, so skip for that too.
*/
if (srctype == UNKNOWNOID || srctype == RECORDOID)
cast_expr = NULL;
else
cast_expr = coerce_to_target_type(NULL,
(Node *) placeholder, srctype,
dsttype, dsttypmod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
-1);
/*
* We use a CaseTestExpr as the base of the coercion tree, since it's
* very cheap to insert the source value for that.
*/
placeholder = makeNode(CaseTestExpr);
placeholder->typeId = srctype;
placeholder->typeMod = srctypmod;
placeholder->collation = get_typcollation(srctype);
/*
* If there's no cast path according to the parser, fall back to using an
* I/O coercion; this is semantically dubious but matches plpgsql's
* historical behavior. We would need something of the sort for UNKNOWN
* literals in any case.
*/
if (cast_expr == NULL)
{
CoerceViaIO *iocoerce = makeNode(CoerceViaIO);
iocoerce->arg = (Expr *) placeholder;
iocoerce->resulttype = dsttype;
iocoerce->resultcollid = InvalidOid;
iocoerce->coerceformat = COERCE_IMPLICIT_CAST;
iocoerce->location = -1;
cast_expr = (Node *) iocoerce;
if (dsttypmod != -1)
/*
* Apply coercion. We use ASSIGNMENT coercion because that's the
* closest match to plpgsql's historical behavior; in particular,
* EXPLICIT coercion would allow silent truncation to a destination
* varchar/bpchar's length, which we do not want.
*
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
* expects to see that for Const input nodes), so don't call it; we'll
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
* coercing RECORD to some other type, so skip for that too.
*/
if (srctype == UNKNOWNOID || srctype == RECORDOID)
cast_expr = NULL;
else
cast_expr = coerce_to_target_type(NULL,
cast_expr, dsttype,
(Node *) placeholder, srctype,
dsttype, dsttypmod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
-1);
}
/* Note: we don't bother labeling the expression tree with collation */
/*
* If there's no cast path according to the parser, fall back to using
* an I/O coercion; this is semantically dubious but matches plpgsql's
* historical behavior. We would need something of the sort for
* UNKNOWN literals in any case.
*/
if (cast_expr == NULL)
{
CoerceViaIO *iocoerce = makeNode(CoerceViaIO);
iocoerce->arg = (Expr *) placeholder;
iocoerce->resulttype = dsttype;
iocoerce->resultcollid = InvalidOid;
iocoerce->coerceformat = COERCE_IMPLICIT_CAST;
iocoerce->location = -1;
cast_expr = (Node *) iocoerce;
if (dsttypmod != -1)
cast_expr = coerce_to_target_type(NULL,
cast_expr, dsttype,
dsttype, dsttypmod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
-1);
}
/* Note: we don't bother labeling the expression tree with collation */
/* Detect whether we have a no-op (RelabelType) coercion */
if (IsA(cast_expr, RelabelType) &&
((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
cast_expr = NULL;
/* Detect whether we have a no-op (RelabelType) coercion */
if (IsA(cast_expr, RelabelType) &&
((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
cast_expr = NULL;
if (cast_expr)
{
/* ExecInitExpr assumes we've planned the expression */
cast_expr = (Node *) expression_planner((Expr *) cast_expr);
/* Create an expression eval state tree for it */
cast_exprstate = ExecInitExpr((Expr *) cast_expr, NULL);
if (cast_expr)
{
/* ExecInitExpr assumes we've planned the expression */
cast_expr = (Node *) expression_planner((Expr *) cast_expr);
/* Now copy the tree into cast_hash_context */
MemoryContextSwitchTo(cast_hash_context);
cast_expr = copyObject(cast_expr);
}
MemoryContextSwitchTo(oldcontext);
/* Now we can fill in a hashtable entry. */
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
(void *) &cast_key,
HASH_ENTER, &found);
Assert(!found); /* wasn't there a moment ago */
cast_entry->cast_expr = (Expr *) cast_expr;
cast_entry->cast_exprstate = NULL;
cast_entry->cast_in_use = false;
cast_entry->cast_lxid = InvalidLocalTransactionId;
}
else
cast_exprstate = NULL;
MemoryContextSwitchTo(oldcontext);
/* Done if we have determined that this is a no-op cast. */
if (cast_entry->cast_expr == NULL)
return NULL;
/*
* Now fill in a hashtable entry. If we fail anywhere up to/including
* this step, we've only leaked some memory in the function context, which
* isn't great but isn't disastrous either.
*/
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
(void *) &cast_key,
HASH_ENTER, &found);
Assert(!found); /* wasn't there a moment ago */
cast_entry->cast_exprstate = cast_exprstate;
* Prepare the expression for execution, if it's not been done already in
* the current transaction; also, if it's marked busy in the current
* transaction, abandon that expression tree and build a new one, so as to
* avoid potential problems with recursive cast expressions and failed
* 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.
*/
curlxid = MyProc->lxid;
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_in_use = false;
cast_entry->cast_lxid = curlxid;
MemoryContextSwitchTo(oldcontext);
}
return cast_exprstate;
return cast_entry;
}
/* ----------

4
src/pl/plpgsql/src/plpgsql.h
Unescape View File

@ -22,7 +22,6 @@
#include "commands/event_trigger.h"
#include "commands/trigger.h"
#include "executor/spi.h"
#include "utils/hsearch.h"
/**********************************************************************
* Definitions
@ -760,9 +759,6 @@ typedef struct PLpgSQL_function
/* function body parsetree */
PLpgSQL_stmt_block *action;
/* table for performing casts needed in this function */
HTAB *cast_hash;
/* these fields change when the function is used */
struct PLpgSQL_execstate *cur_estate;
unsigned long use_count;

62
src/test/regress/expected/plpgsql.out
Unescape View File

@ -4702,6 +4702,68 @@ select error2('public.stuffs');
rollback;
drop function error2(p_name_table text);
drop function error1(text);
-- Test for proper handling of cast-expression caching
create function sql_to_date(integer) returns date as $$
select $1::text::date
$$ language sql immutable strict;
create cast (integer as date) with function sql_to_date(integer) as assignment;
create function cast_invoker(integer) returns date as $$
begin
return $1;
end$$ language plpgsql;
select cast_invoker(20150717);
cast_invoker
--------------
07-17-2015
(1 row)
select cast_invoker(20150718); -- second call crashed in pre-release 9.5
cast_invoker
--------------
07-18-2015
(1 row)
begin;
select cast_invoker(20150717);
cast_invoker
--------------
07-17-2015
(1 row)
select cast_invoker(20150718);
cast_invoker
--------------
07-18-2015
(1 row)
savepoint s1;
select cast_invoker(20150718);
cast_invoker
--------------
07-18-2015
(1 row)
select cast_invoker(-1); -- fails
ERROR: invalid input syntax for type date: "-1"
CONTEXT: SQL function "sql_to_date" statement 1
PL/pgSQL function cast_invoker(integer) while casting return value to function's return type
rollback to savepoint s1;
select cast_invoker(20150719);
cast_invoker
--------------
07-19-2015
(1 row)
select cast_invoker(20150720);
cast_invoker
--------------
07-20-2015
(1 row)
commit;
drop function cast_invoker(integer);
drop function sql_to_date(integer) cascade;
NOTICE: drop cascades to cast from integer to date
-- Test for consistent reporting of error context
create function fail() returns int language plpgsql as $$
begin

30
src/test/regress/sql/plpgsql.sql
Unescape View File

@ -3806,6 +3806,36 @@ rollback;
drop function error2(p_name_table text);
drop function error1(text);
-- Test for proper handling of cast-expression caching
create function sql_to_date(integer) returns date as $$
select $1::text::date
$$ language sql immutable strict;
create cast (integer as date) with function sql_to_date(integer) as assignment;
create function cast_invoker(integer) returns date as $$
begin
return $1;
end$$ language plpgsql;
select cast_invoker(20150717);
select cast_invoker(20150718); -- second call crashed in pre-release 9.5
begin;
select cast_invoker(20150717);
select cast_invoker(20150718);
savepoint s1;
select cast_invoker(20150718);
select cast_invoker(-1); -- fails
rollback to savepoint s1;
select cast_invoker(20150719);
select cast_invoker(20150720);
commit;
drop function cast_invoker(integer);
drop function sql_to_date(integer) cascade;
-- Test for consistent reporting of error context
create function fail() returns int language plpgsql as $$

Write Preview
Loading…
Cancel
Save