Skip to content

Commit 9a5f369

Browse files
tglsfdctglsfdc
committed
Repair mishandling of cached cast-expression trees in plpgsql.
In commit 1345cc6, 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.
1 parent eb3b93b commit 9a5f369

File tree

4 files changed

+262
-124
lines changed

4 files changed

+262
-124
lines changed

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 170 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -53,21 +53,6 @@ typedef struct
5353
bool *freevals; /* which arguments are pfree-able */
5454
} PreparedParamsData;
5555

56-
typedef struct
57-
{
58-
/* NB: we assume this struct contains no padding bytes */
59-
Oid srctype; /* source type for cast */
60-
Oid dsttype; /* destination type for cast */
61-
int32 srctypmod; /* source typmod for cast */
62-
int32 dsttypmod; /* destination typmod for cast */
63-
} plpgsql_CastHashKey;
64-
65-
typedef struct
66-
{
67-
plpgsql_CastHashKey key; /* hash key --- MUST BE FIRST */
68-
ExprState *cast_exprstate; /* cast expression, or NULL if no-op cast */
69-
} plpgsql_CastHashEntry;
70-
7156
/*
7257
* All plpgsql function executions within a single transaction share the same
7358
* executor EState for evaluating "simple" expressions. Each function call
@@ -104,6 +89,38 @@ typedef struct SimpleEcontextStackEntry
10489
static EState *shared_simple_eval_estate = NULL;
10590
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
10691

92+
/*
93+
* We use a session-wide hash table for caching cast information.
94+
*
95+
* Once built, the compiled expression trees (cast_expr fields) survive for
96+
* the life of the session. At some point it might be worth invalidating
97+
* those after pg_cast changes, but for the moment we don't bother.
98+
*
99+
* The evaluation state trees (cast_exprstate) are managed in the same way as
100+
* simple expressions (i.e., we assume cast expressions are always simple).
101+
*/
102+
typedef struct /* lookup key for cast info */
103+
{
104+
/* NB: we assume this struct contains no padding bytes */
105+
Oid srctype; /* source type for cast */
106+
Oid dsttype; /* destination type for cast */
107+
int32 srctypmod; /* source typmod for cast */
108+
int32 dsttypmod; /* destination typmod for cast */
109+
} plpgsql_CastHashKey;
110+
111+
typedef struct /* cast_hash table entry */
112+
{
113+
plpgsql_CastHashKey key; /* hash key --- MUST BE FIRST */
114+
Expr *cast_expr; /* cast expression, or NULL if no-op cast */
115+
/* The ExprState tree is valid only when cast_lxid matches current LXID */
116+
ExprState *cast_exprstate; /* expression's eval tree */
117+
bool cast_in_use; /* true while we're executing eval tree */
118+
LocalTransactionId cast_lxid;
119+
} plpgsql_CastHashEntry;
120+
121+
static MemoryContext cast_hash_context = NULL;
122+
static HTAB *cast_hash = NULL;
123+
107124
/************************************************************
108125
* Local function forward declarations
109126
************************************************************/
@@ -236,8 +253,9 @@ static Datum exec_cast_value(PLpgSQL_execstate *estate,
236253
Datum value, bool *isnull,
237254
Oid valtype, int32 valtypmod,
238255
Oid reqtype, int32 reqtypmod);
239-
static ExprState *get_cast_expression(PLpgSQL_execstate *estate,
240-
Oid srctype, int32 srctypmod, Oid dsttype, int32 dsttypmod);
256+
static plpgsql_CastHashEntry *get_cast_hashentry(PLpgSQL_execstate *estate,
257+
Oid srctype, int32 srctypmod,
258+
Oid dsttype, int32 dsttypmod);
241259
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
242260
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
243261
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
@@ -5946,12 +5964,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
59465964
if (valtype != reqtype ||
59475965
(valtypmod != reqtypmod && reqtypmod != -1))
59485966
{
5949-
ExprState *cast_expr;
5967+
plpgsql_CastHashEntry *cast_entry;
59505968

5951-
cast_expr = get_cast_expression(estate,
5969+
cast_entry = get_cast_hashentry(estate,
59525970
valtype, valtypmod,
59535971
reqtype, reqtypmod);
5954-
if (cast_expr)
5972+
if (cast_entry)
59555973
{
59565974
ExprContext *econtext = estate->eval_econtext;
59575975
MemoryContext oldcontext;
@@ -5961,7 +5979,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
59615979
econtext->caseValue_datum = value;
59625980
econtext->caseValue_isNull = *isnull;
59635981

5964-
value = ExecEvalExpr(cast_expr, econtext, isnull, NULL);
5982+
cast_entry->cast_in_use = true;
5983+
5984+
value = ExecEvalExpr(cast_entry->cast_exprstate, econtext,
5985+
isnull, NULL);
5986+
5987+
cast_entry->cast_in_use = false;
59655988

59665989
MemoryContextSwitchTo(oldcontext);
59675990
}
@@ -5971,46 +5994,44 @@ exec_cast_value(PLpgSQL_execstate *estate,
59715994
}
59725995

59735996
/* ----------
5974-
* get_cast_expression Look up how to perform a type cast
5975-
*
5976-
* Returns an expression evaluation tree based on a CaseTestExpr input,
5977-
* or NULL if the cast is a mere no-op relabeling.
5997+
* get_cast_hashentry Look up how to perform a type cast
59785998
*
5979-
* We cache the results of the lookup in a per-function hash table.
5980-
* It's tempting to consider using a session-wide hash table instead,
5981-
* but that introduces some corner-case questions that probably aren't
5982-
* worth dealing with; in particular that re-entrant use of an evaluation
5983-
* tree might occur. That would also set in stone the assumption that
5984-
* collation isn't important to a cast function.
5999+
* Returns a plpgsql_CastHashEntry if an expression has to be evaluated,
6000+
* or NULL if the cast is a mere no-op relabeling. If there's work to be
6001+
* done, the cast_exprstate field contains an expression evaluation tree
6002+
* based on a CaseTestExpr input, and the cast_in_use field should be set
6003+
* TRUE while executing it.
59856004
* ----------
59866005
*/
5987-
static ExprState *
5988-
get_cast_expression(PLpgSQL_execstate *estate,
5989-
Oid srctype, int32 srctypmod, Oid dsttype, int32 dsttypmod)
6006+
static plpgsql_CastHashEntry *
6007+
get_cast_hashentry(PLpgSQL_execstate *estate,
6008+
Oid srctype, int32 srctypmod,
6009+
Oid dsttype, int32 dsttypmod)
59906010
{
5991-
HTAB *cast_hash = estate->func->cast_hash;
59926011
plpgsql_CastHashKey cast_key;
59936012
plpgsql_CastHashEntry *cast_entry;
59946013
bool found;
5995-
CaseTestExpr *placeholder;
5996-
Node *cast_expr;
5997-
ExprState *cast_exprstate;
6014+
LocalTransactionId curlxid;
59986015
MemoryContext oldcontext;
59996016

6000-
/* Create the cast-info hash table if we didn't already */
6017+
/* Create the session-wide cast-info hash table if we didn't already */
60016018
if (cast_hash == NULL)
60026019
{
60036020
HASHCTL ctl;
60046021

6022+
cast_hash_context = AllocSetContextCreate(TopMemoryContext,
6023+
"PLpgSQL cast info",
6024+
ALLOCSET_DEFAULT_MINSIZE,
6025+
ALLOCSET_DEFAULT_INITSIZE,
6026+
ALLOCSET_DEFAULT_MAXSIZE);
60056027
memset(&ctl, 0, sizeof(ctl));
60066028
ctl.keysize = sizeof(plpgsql_CastHashKey);
60076029
ctl.entrysize = sizeof(plpgsql_CastHashEntry);
6008-
ctl.hcxt = estate->func->fn_cxt;
6030+
ctl.hcxt = cast_hash_context;
60096031
cast_hash = hash_create("PLpgSQL cast cache",
60106032
16, /* start small and extend */
60116033
&ctl,
60126034
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
6013-
estate->func->cast_hash = cast_hash;
60146035
}
60156036

60166037
/* Look for existing entry */
@@ -6021,102 +6042,131 @@ get_cast_expression(PLpgSQL_execstate *estate,
60216042
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
60226043
(void *) &cast_key,
60236044
HASH_FIND, NULL);
6024-
if (cast_entry)
6025-
return cast_entry->cast_exprstate;
60266045

6027-
/* Construct expression tree for coercion in function's context */
6028-
oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt);
6046+
if (cast_entry == NULL)
6047+
{
6048+
/* We've not looked up this coercion before */
6049+
Node *cast_expr;
6050+
CaseTestExpr *placeholder;
60296051

6030-
/*
6031-
* We use a CaseTestExpr as the base of the coercion tree, since it's very
6032-
* cheap to insert the source value for that.
6033-
*/
6034-
placeholder = makeNode(CaseTestExpr);
6035-
placeholder->typeId = srctype;
6036-
placeholder->typeMod = srctypmod;
6037-
placeholder->collation = get_typcollation(srctype);
6038-
if (OidIsValid(estate->func->fn_input_collation) &&
6039-
OidIsValid(placeholder->collation))
6040-
placeholder->collation = estate->func->fn_input_collation;
6052+
/*
6053+
* Since we could easily fail (no such coercion), construct a
6054+
* temporary coercion expression tree in a short-lived context, then
6055+
* if successful copy it to cast_hash_context.
6056+
*/
6057+
oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
60416058

6042-
/*
6043-
* Apply coercion. We use ASSIGNMENT coercion because that's the closest
6044-
* match to plpgsql's historical behavior; in particular, EXPLICIT
6045-
* coercion would allow silent truncation to a destination
6046-
* varchar/bpchar's length, which we do not want.
6047-
*
6048-
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
6049-
* expects to see that for Const input nodes), so don't call it; we'll
6050-
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
6051-
* coercing RECORD to some other type, so skip for that too.
6052-
*/
6053-
if (srctype == UNKNOWNOID || srctype == RECORDOID)
6054-
cast_expr = NULL;
6055-
else
6056-
cast_expr = coerce_to_target_type(NULL,
6057-
(Node *) placeholder, srctype,
6058-
dsttype, dsttypmod,
6059-
COERCION_ASSIGNMENT,
6060-
COERCE_IMPLICIT_CAST,
6061-
-1);
6059+
/*
6060+
* We use a CaseTestExpr as the base of the coercion tree, since it's
6061+
* very cheap to insert the source value for that.
6062+
*/
6063+
placeholder = makeNode(CaseTestExpr);
6064+
placeholder->typeId = srctype;
6065+
placeholder->typeMod = srctypmod;
6066+
placeholder->collation = get_typcollation(srctype);
60626067

6063-
/*
6064-
* If there's no cast path according to the parser, fall back to using an
6065-
* I/O coercion; this is semantically dubious but matches plpgsql's
6066-
* historical behavior. We would need something of the sort for UNKNOWN
6067-
* literals in any case.
6068-
*/
6069-
if (cast_expr == NULL)
6070-
{
6071-
CoerceViaIO *iocoerce = makeNode(CoerceViaIO);
6072-
6073-
iocoerce->arg = (Expr *) placeholder;
6074-
iocoerce->resulttype = dsttype;
6075-
iocoerce->resultcollid = InvalidOid;
6076-
iocoerce->coerceformat = COERCE_IMPLICIT_CAST;
6077-
iocoerce->location = -1;
6078-
cast_expr = (Node *) iocoerce;
6079-
if (dsttypmod != -1)
6068+
/*
6069+
* Apply coercion. We use ASSIGNMENT coercion because that's the
6070+
* closest match to plpgsql's historical behavior; in particular,
6071+
* EXPLICIT coercion would allow silent truncation to a destination
6072+
* varchar/bpchar's length, which we do not want.
6073+
*
6074+
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
6075+
* expects to see that for Const input nodes), so don't call it; we'll
6076+
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
6077+
* coercing RECORD to some other type, so skip for that too.
6078+
*/
6079+
if (srctype == UNKNOWNOID || srctype == RECORDOID)
6080+
cast_expr = NULL;
6081+
else
60806082
cast_expr = coerce_to_target_type(NULL,
6081-
cast_expr, dsttype,
6083+
(Node *) placeholder, srctype,
60826084
dsttype, dsttypmod,
60836085
COERCION_ASSIGNMENT,
60846086
COERCE_IMPLICIT_CAST,
60856087
-1);
6086-
}
60876088

6088-
/* Note: we don't bother labeling the expression tree with collation */
6089+
/*
6090+
* If there's no cast path according to the parser, fall back to using
6091+
* an I/O coercion; this is semantically dubious but matches plpgsql's
6092+
* historical behavior. We would need something of the sort for
6093+
* UNKNOWN literals in any case.
6094+
*/
6095+
if (cast_expr == NULL)
6096+
{
6097+
CoerceViaIO *iocoerce = makeNode(CoerceViaIO);
6098+
6099+
iocoerce->arg = (Expr *) placeholder;
6100+
iocoerce->resulttype = dsttype;
6101+
iocoerce->resultcollid = InvalidOid;
6102+
iocoerce->coerceformat = COERCE_IMPLICIT_CAST;
6103+
iocoerce->location = -1;
6104+
cast_expr = (Node *) iocoerce;
6105+
if (dsttypmod != -1)
6106+
cast_expr = coerce_to_target_type(NULL,
6107+
cast_expr, dsttype,
6108+
dsttype, dsttypmod,
6109+
COERCION_ASSIGNMENT,
6110+
COERCE_IMPLICIT_CAST,
6111+
-1);
6112+
}
6113+
6114+
/* Note: we don't bother labeling the expression tree with collation */
60896115

6090-
/* Detect whether we have a no-op (RelabelType) coercion */
6091-
if (IsA(cast_expr, RelabelType) &&
6092-
((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
6093-
cast_expr = NULL;
6116+
/* Detect whether we have a no-op (RelabelType) coercion */
6117+
if (IsA(cast_expr, RelabelType) &&
6118+
((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
6119+
cast_expr = NULL;
60946120

6095-
if (cast_expr)
6096-
{
6097-
/* ExecInitExpr assumes we've planned the expression */
6098-
cast_expr = (Node *) expression_planner((Expr *) cast_expr);
6099-
/* Create an expression eval state tree for it */
6100-
cast_exprstate = ExecInitExpr((Expr *) cast_expr, NULL);
6121+
if (cast_expr)
6122+
{
6123+
/* ExecInitExpr assumes we've planned the expression */
6124+
cast_expr = (Node *) expression_planner((Expr *) cast_expr);
6125+
6126+
/* Now copy the tree into cast_hash_context */
6127+
MemoryContextSwitchTo(cast_hash_context);
6128+
6129+
cast_expr = copyObject(cast_expr);
6130+
}
6131+
6132+
MemoryContextSwitchTo(oldcontext);
6133+
6134+
/* Now we can fill in a hashtable entry. */
6135+
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
6136+
(void *) &cast_key,
6137+
HASH_ENTER, &found);
6138+
Assert(!found); /* wasn't there a moment ago */
6139+
cast_entry->cast_expr = (Expr *) cast_expr;
6140+
cast_entry->cast_exprstate = NULL;
6141+
cast_entry->cast_in_use = false;
6142+
cast_entry->cast_lxid = InvalidLocalTransactionId;
61016143
}
6102-
else
6103-
cast_exprstate = NULL;
61046144

6105-
MemoryContextSwitchTo(oldcontext);
6145+
/* Done if we have determined that this is a no-op cast. */
6146+
if (cast_entry->cast_expr == NULL)
6147+
return NULL;
61066148

61076149
/*
6108-
* Now fill in a hashtable entry. If we fail anywhere up to/including
6109-
* this step, we've only leaked some memory in the function context, which
6110-
* isn't great but isn't disastrous either.
6111-
*/
6112-
cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
6113-
(void *) &cast_key,
6114-
HASH_ENTER, &found);
6115-
Assert(!found); /* wasn't there a moment ago */
6116-
6117-
cast_entry->cast_exprstate = cast_exprstate;
6150+
* Prepare the expression for execution, if it's not been done already in
6151+
* the current transaction; also, if it's marked busy in the current
6152+
* transaction, abandon that expression tree and build a new one, so as to
6153+
* avoid potential problems with recursive cast expressions and failed
6154+
* executions. (We will leak some memory intra-transaction if that
6155+
* happens a lot, but we don't expect it to.) It's okay to update the
6156+
* hash table with the new tree because all plpgsql functions within a
6157+
* given transaction share the same simple_eval_estate.
6158+
*/
6159+
curlxid = MyProc->lxid;
6160+
if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use)
6161+
{
6162+
oldcontext = MemoryContextSwitchTo(estate->simple_eval_estate->es_query_cxt);
6163+
cast_entry->cast_exprstate = ExecInitExpr(cast_entry->cast_expr, NULL);
6164+
cast_entry->cast_in_use = false;
6165+
cast_entry->cast_lxid = curlxid;
6166+
MemoryContextSwitchTo(oldcontext);
6167+
}
61186168

6119-
return cast_exprstate;
6169+
return cast_entry;
61206170
}
61216171

61226172
/* ----------

‎src/pl/plpgsql/src/plpgsql.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include "commands/event_trigger.h"
2323
#include "commands/trigger.h"
2424
#include "executor/spi.h"
25-
#include "utils/hsearch.h"
2625

2726
/**********************************************************************
2827
* Definitions
@@ -756,9 +755,6 @@ typedef struct PLpgSQL_function
756755
PLpgSQL_datum **datums;
757756
PLpgSQL_stmt_block *action;
758757

759-
/* table for performing casts needed in this function */
760-
HTAB *cast_hash;
761-
762758
/* these fields change when the function is used */
763759
struct PLpgSQL_execstate *cur_estate;
764760
unsigned long use_count;

0 commit comments

Comments
 (0)