Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit0fc94a5

Browse files
committed
Repair mishandling of cached cast-expression trees in plpgsql.
In commit1345cc6, I introduced cachingof expressions representing type-cast operations into plpgsql. However,I supposed that I could cache both the expression trees and the evaluationstate trees derived from them for the life of the session. This doesn'twork, 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 treethat point to transaction-lifespan memory; in particular this happens ifthe cast expression calls a SQL-language function, as reported by GeoffWinkless.The minimum-risk fix seems to be to treat the state trees the same waywe do for "simple expression" trees in plpgsql, ie create them in thesimple_eval_estate's ecxt_per_query_memory, which means recreating themonce per transaction.Since I had to introduce bookkeeping overhead for that anyway, I boughtback some of the added cost by sharing the read-only expression treesacross all functions in the session, instead of using a per-functiontable as originally. The simple-expression bookkeeping takes care ofthe 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 parent266e771 commit0fc94a5

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-
typedefstruct
57-
{
58-
/* NB: we assume this struct contains no padding bytes */
59-
Oidsrctype;/* source type for cast */
60-
Oiddsttype;/* destination type for cast */
61-
int32srctypmod;/* source typmod for cast */
62-
int32dsttypmod;/* destination typmod for cast */
63-
}plpgsql_CastHashKey;
64-
65-
typedefstruct
66-
{
67-
plpgsql_CastHashKeykey;/* 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
staticEState*shared_simple_eval_estate=NULL;
10590
staticSimpleEcontextStackEntry*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+
typedefstruct/* lookup key for cast info */
103+
{
104+
/* NB: we assume this struct contains no padding bytes */
105+
Oidsrctype;/* source type for cast */
106+
Oiddsttype;/* destination type for cast */
107+
int32srctypmod;/* source typmod for cast */
108+
int32dsttypmod;/* destination typmod for cast */
109+
}plpgsql_CastHashKey;
110+
111+
typedefstruct/* cast_hash table entry */
112+
{
113+
plpgsql_CastHashKeykey;/* 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+
boolcast_in_use;/* true while we're executing eval tree */
118+
LocalTransactionIdcast_lxid;
119+
}plpgsql_CastHashEntry;
120+
121+
staticMemoryContextcast_hash_context=NULL;
122+
staticHTAB*cast_hash=NULL;
123+
107124
/************************************************************
108125
* Local function forward declarations
109126
************************************************************/
@@ -238,8 +255,9 @@ static Datum exec_cast_value(PLpgSQL_execstate *estate,
238255
Datumvalue,bool*isnull,
239256
Oidvaltype,int32valtypmod,
240257
Oidreqtype,int32reqtypmod);
241-
staticExprState*get_cast_expression(PLpgSQL_execstate*estate,
242-
Oidsrctype,int32srctypmod,Oiddsttype,int32dsttypmod);
258+
staticplpgsql_CastHashEntry*get_cast_hashentry(PLpgSQL_execstate*estate,
259+
Oidsrctype,int32srctypmod,
260+
Oiddsttype,int32dsttypmod);
243261
staticvoidexec_init_tuple_store(PLpgSQL_execstate*estate);
244262
staticvoidexec_set_found(PLpgSQL_execstate*estate,boolstate);
245263
staticvoidplpgsql_create_econtext(PLpgSQL_execstate*estate);
@@ -6043,12 +6061,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
60436061
if (valtype!=reqtype||
60446062
(valtypmod!=reqtypmod&&reqtypmod!=-1))
60456063
{
6046-
ExprState*cast_expr;
6064+
plpgsql_CastHashEntry*cast_entry;
60476065

6048-
cast_expr=get_cast_expression(estate,
6066+
cast_entry=get_cast_hashentry(estate,
60496067
valtype,valtypmod,
60506068
reqtype,reqtypmod);
6051-
if (cast_expr)
6069+
if (cast_entry)
60526070
{
60536071
ExprContext*econtext=estate->eval_econtext;
60546072
MemoryContextoldcontext;
@@ -6058,7 +6076,12 @@ exec_cast_value(PLpgSQL_execstate *estate,
60586076
econtext->caseValue_datum=value;
60596077
econtext->caseValue_isNull=*isnull;
60606078

6061-
value=ExecEvalExpr(cast_expr,econtext,isnull,NULL);
6079+
cast_entry->cast_in_use= true;
6080+
6081+
value=ExecEvalExpr(cast_entry->cast_exprstate,econtext,
6082+
isnull,NULL);
6083+
6084+
cast_entry->cast_in_use= false;
60626085

60636086
MemoryContextSwitchTo(oldcontext);
60646087
}
@@ -6068,46 +6091,44 @@ exec_cast_value(PLpgSQL_execstate *estate,
60686091
}
60696092

60706093
/* ----------
6071-
* get_cast_expressionLook up how to perform a type cast
6072-
*
6073-
* Returns an expression evaluation tree based on a CaseTestExpr input,
6074-
* or NULL if the cast is a mere no-op relabeling.
6094+
* get_cast_hashentryLook up how to perform a type cast
60756095
*
6076-
* We cache the results of the lookup in a per-function hash table.
6077-
* It's tempting to consider using a session-wide hash table instead,
6078-
* but that introduces some corner-case questions that probably aren't
6079-
* worth dealing with; in particular that re-entrant use of an evaluation
6080-
* tree might occur. That would also set in stone the assumption that
6081-
* collation isn't important to a cast function.
6096+
* Returns a plpgsql_CastHashEntry if an expression has to be evaluated,
6097+
* or NULL if the cast is a mere no-op relabeling. If there's work to be
6098+
* done, the cast_exprstate field contains an expression evaluation tree
6099+
* based on a CaseTestExpr input, and the cast_in_use field should be set
6100+
* TRUE while executing it.
60826101
* ----------
60836102
*/
6084-
staticExprState*
6085-
get_cast_expression(PLpgSQL_execstate*estate,
6086-
Oidsrctype,int32srctypmod,Oiddsttype,int32dsttypmod)
6103+
staticplpgsql_CastHashEntry*
6104+
get_cast_hashentry(PLpgSQL_execstate*estate,
6105+
Oidsrctype,int32srctypmod,
6106+
Oiddsttype,int32dsttypmod)
60876107
{
6088-
HTAB*cast_hash=estate->func->cast_hash;
60896108
plpgsql_CastHashKeycast_key;
60906109
plpgsql_CastHashEntry*cast_entry;
60916110
boolfound;
6092-
CaseTestExpr*placeholder;
6093-
Node*cast_expr;
6094-
ExprState*cast_exprstate;
6111+
LocalTransactionIdcurlxid;
60956112
MemoryContextoldcontext;
60966113

6097-
/* Create the cast-info hash table if we didn't already */
6114+
/* Create thesession-widecast-info hash table if we didn't already */
60986115
if (cast_hash==NULL)
60996116
{
61006117
HASHCTLctl;
61016118

6119+
cast_hash_context=AllocSetContextCreate(TopMemoryContext,
6120+
"PLpgSQL cast info",
6121+
ALLOCSET_DEFAULT_MINSIZE,
6122+
ALLOCSET_DEFAULT_INITSIZE,
6123+
ALLOCSET_DEFAULT_MAXSIZE);
61026124
memset(&ctl,0,sizeof(ctl));
61036125
ctl.keysize=sizeof(plpgsql_CastHashKey);
61046126
ctl.entrysize=sizeof(plpgsql_CastHashEntry);
6105-
ctl.hcxt=estate->func->fn_cxt;
6127+
ctl.hcxt=cast_hash_context;
61066128
cast_hash=hash_create("PLpgSQL cast cache",
61076129
16,/* start small and extend */
61086130
&ctl,
61096131
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
6110-
estate->func->cast_hash=cast_hash;
61116132
}
61126133

61136134
/* Look for existing entry */
@@ -6118,102 +6139,131 @@ get_cast_expression(PLpgSQL_execstate *estate,
61186139
cast_entry= (plpgsql_CastHashEntry*)hash_search(cast_hash,
61196140
(void*)&cast_key,
61206141
HASH_FIND,NULL);
6121-
if (cast_entry)
6122-
returncast_entry->cast_exprstate;
61236142

6124-
/* Construct expression tree for coercion in function's context */
6125-
oldcontext=MemoryContextSwitchTo(estate->func->fn_cxt);
6143+
if (cast_entry==NULL)
6144+
{
6145+
/* We've not looked up this coercion before */
6146+
Node*cast_expr;
6147+
CaseTestExpr*placeholder;
61266148

6127-
/*
6128-
* We use a CaseTestExpr as the base of the coercion tree, since it's very
6129-
* cheap to insert the source value for that.
6130-
*/
6131-
placeholder=makeNode(CaseTestExpr);
6132-
placeholder->typeId=srctype;
6133-
placeholder->typeMod=srctypmod;
6134-
placeholder->collation=get_typcollation(srctype);
6135-
if (OidIsValid(estate->func->fn_input_collation)&&
6136-
OidIsValid(placeholder->collation))
6137-
placeholder->collation=estate->func->fn_input_collation;
6149+
/*
6150+
* Since we could easily fail (no such coercion), construct a
6151+
* temporary coercion expression tree in a short-lived context, then
6152+
* if successful copy it to cast_hash_context.
6153+
*/
6154+
oldcontext=MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
61386155

6139-
/*
6140-
* Apply coercion. We use ASSIGNMENT coercion because that's the closest
6141-
* match to plpgsql's historical behavior; in particular, EXPLICIT
6142-
* coercion would allow silent truncation to a destination
6143-
* varchar/bpchar's length, which we do not want.
6144-
*
6145-
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
6146-
* expects to see that for Const input nodes), so don't call it; we'll
6147-
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
6148-
* coercing RECORD to some other type, so skip for that too.
6149-
*/
6150-
if (srctype==UNKNOWNOID||srctype==RECORDOID)
6151-
cast_expr=NULL;
6152-
else
6153-
cast_expr=coerce_to_target_type(NULL,
6154-
(Node*)placeholder,srctype,
6155-
dsttype,dsttypmod,
6156-
COERCION_ASSIGNMENT,
6157-
COERCE_IMPLICIT_CAST,
6158-
-1);
6156+
/*
6157+
* We use a CaseTestExpr as the base of the coercion tree, since it's
6158+
* very cheap to insert the source value for that.
6159+
*/
6160+
placeholder=makeNode(CaseTestExpr);
6161+
placeholder->typeId=srctype;
6162+
placeholder->typeMod=srctypmod;
6163+
placeholder->collation=get_typcollation(srctype);
61596164

6160-
/*
6161-
* If there's no cast path according to the parser, fall back to using an
6162-
* I/O coercion; this is semantically dubious but matches plpgsql's
6163-
* historical behavior. We would need something of the sort for UNKNOWN
6164-
* literals in any case.
6165-
*/
6166-
if (cast_expr==NULL)
6167-
{
6168-
CoerceViaIO*iocoerce=makeNode(CoerceViaIO);
6169-
6170-
iocoerce->arg= (Expr*)placeholder;
6171-
iocoerce->resulttype=dsttype;
6172-
iocoerce->resultcollid=InvalidOid;
6173-
iocoerce->coerceformat=COERCE_IMPLICIT_CAST;
6174-
iocoerce->location=-1;
6175-
cast_expr= (Node*)iocoerce;
6176-
if (dsttypmod!=-1)
6165+
/*
6166+
* Apply coercion. We use ASSIGNMENT coercion because that's the
6167+
* closest match to plpgsql's historical behavior; in particular,
6168+
* EXPLICIT coercion would allow silent truncation to a destination
6169+
* varchar/bpchar's length, which we do not want.
6170+
*
6171+
* If source type is UNKNOWN, coerce_to_target_type will fail (it only
6172+
* expects to see that for Const input nodes), so don't call it; we'll
6173+
* apply CoerceViaIO instead. Likewise, it doesn't currently work for
6174+
* coercing RECORD to some other type, so skip for that too.
6175+
*/
6176+
if (srctype==UNKNOWNOID||srctype==RECORDOID)
6177+
cast_expr=NULL;
6178+
else
61776179
cast_expr=coerce_to_target_type(NULL,
6178-
cast_expr,dsttype,
6180+
(Node*)placeholder,srctype,
61796181
dsttype,dsttypmod,
61806182
COERCION_ASSIGNMENT,
61816183
COERCE_IMPLICIT_CAST,
61826184
-1);
6183-
}
61846185

6185-
/* Note: we don't bother labeling the expression tree with collation */
6186+
/*
6187+
* If there's no cast path according to the parser, fall back to using
6188+
* an I/O coercion; this is semantically dubious but matches plpgsql's
6189+
* historical behavior. We would need something of the sort for
6190+
* UNKNOWN literals in any case.
6191+
*/
6192+
if (cast_expr==NULL)
6193+
{
6194+
CoerceViaIO*iocoerce=makeNode(CoerceViaIO);
6195+
6196+
iocoerce->arg= (Expr*)placeholder;
6197+
iocoerce->resulttype=dsttype;
6198+
iocoerce->resultcollid=InvalidOid;
6199+
iocoerce->coerceformat=COERCE_IMPLICIT_CAST;
6200+
iocoerce->location=-1;
6201+
cast_expr= (Node*)iocoerce;
6202+
if (dsttypmod!=-1)
6203+
cast_expr=coerce_to_target_type(NULL,
6204+
cast_expr,dsttype,
6205+
dsttype,dsttypmod,
6206+
COERCION_ASSIGNMENT,
6207+
COERCE_IMPLICIT_CAST,
6208+
-1);
6209+
}
6210+
6211+
/* Note: we don't bother labeling the expression tree with collation */
61866212

6187-
/* Detect whether we have a no-op (RelabelType) coercion */
6188-
if (IsA(cast_expr,RelabelType)&&
6189-
((RelabelType*)cast_expr)->arg== (Expr*)placeholder)
6190-
cast_expr=NULL;
6213+
/* Detect whether we have a no-op (RelabelType) coercion */
6214+
if (IsA(cast_expr,RelabelType)&&
6215+
((RelabelType*)cast_expr)->arg== (Expr*)placeholder)
6216+
cast_expr=NULL;
61916217

6192-
if (cast_expr)
6193-
{
6194-
/* ExecInitExpr assumes we've planned the expression */
6195-
cast_expr= (Node*)expression_planner((Expr*)cast_expr);
6196-
/* Create an expression eval state tree for it */
6197-
cast_exprstate=ExecInitExpr((Expr*)cast_expr,NULL);
6218+
if (cast_expr)
6219+
{
6220+
/* ExecInitExpr assumes we've planned the expression */
6221+
cast_expr= (Node*)expression_planner((Expr*)cast_expr);
6222+
6223+
/* Now copy the tree into cast_hash_context */
6224+
MemoryContextSwitchTo(cast_hash_context);
6225+
6226+
cast_expr=copyObject(cast_expr);
6227+
}
6228+
6229+
MemoryContextSwitchTo(oldcontext);
6230+
6231+
/* Now we can fill in a hashtable entry. */
6232+
cast_entry= (plpgsql_CastHashEntry*)hash_search(cast_hash,
6233+
(void*)&cast_key,
6234+
HASH_ENTER,&found);
6235+
Assert(!found);/* wasn't there a moment ago */
6236+
cast_entry->cast_expr= (Expr*)cast_expr;
6237+
cast_entry->cast_exprstate=NULL;
6238+
cast_entry->cast_in_use= false;
6239+
cast_entry->cast_lxid=InvalidLocalTransactionId;
61986240
}
6199-
else
6200-
cast_exprstate=NULL;
62016241

6202-
MemoryContextSwitchTo(oldcontext);
6242+
/* Done if we have determined that this is a no-op cast. */
6243+
if (cast_entry->cast_expr==NULL)
6244+
returnNULL;
62036245

62046246
/*
6205-
* Now fill in a hashtable entry. If we fail anywhere up to/including
6206-
* this step, we've only leaked some memory in the function context, which
6207-
* isn't great but isn't disastrous either.
6208-
*/
6209-
cast_entry= (plpgsql_CastHashEntry*)hash_search(cast_hash,
6210-
(void*)&cast_key,
6211-
HASH_ENTER,&found);
6212-
Assert(!found);/* wasn't there a moment ago */
6213-
6214-
cast_entry->cast_exprstate=cast_exprstate;
6247+
* Prepare the expression for execution, if it's not been done already in
6248+
* the current transaction; also, if it's marked busy in the current
6249+
* transaction, abandon that expression tree and build a new one, so as to
6250+
* avoid potential problems with recursive cast expressions and failed
6251+
* executions. (We will leak some memory intra-transaction if that
6252+
* happens a lot, but we don't expect it to.) It's okay to update the
6253+
* hash table with the new tree because all plpgsql functions within a
6254+
* given transaction share the same simple_eval_estate.
6255+
*/
6256+
curlxid=MyProc->lxid;
6257+
if (cast_entry->cast_lxid!=curlxid||cast_entry->cast_in_use)
6258+
{
6259+
oldcontext=MemoryContextSwitchTo(estate->simple_eval_estate->es_query_cxt);
6260+
cast_entry->cast_exprstate=ExecInitExpr(cast_entry->cast_expr,NULL);
6261+
cast_entry->cast_in_use= false;
6262+
cast_entry->cast_lxid=curlxid;
6263+
MemoryContextSwitchTo(oldcontext);
6264+
}
62156265

6216-
returncast_exprstate;
6266+
returncast_entry;
62176267
}
62186268

62196269
/* ----------

‎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
@@ -760,9 +759,6 @@ typedef struct PLpgSQL_function
760759
/* function body parsetree */
761760
PLpgSQL_stmt_block*action;
762761

763-
/* table for performing casts needed in this function */
764-
HTAB*cast_hash;
765-
766762
/* these fields change when the function is used */
767763
structPLpgSQL_execstate*cur_estate;
768764
unsigned longuse_count;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp