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

Commit1f6a7eb

Browse files
committed
Repair unsafe use of shared typecast-lookup table in plpgsql DO blocks.
DO blocks use private simple_eval_estates to avoid intra-transaction memoryleakage, cf commitc7b849a. I had forgotten about that while writingcommit0fc94a5, but it means that expression execution trees createdwithin a DO block disappear immediately on exiting the DO block, and hencecan'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 privatesimple_eval_estate. This is less efficient than one could wish, sinceDO blocks can no longer share any cast lookup work with other plpgsqlexecution, but it shouldn't be too bad; in any case it's no worse thanwhat happened in DO blocks before commit0fc94a5.Per bug #13571 from Feike Steenbergen. Preliminary analysis byOleksandr Shulgin.
1 parent6942663 commit1f6a7eb

File tree

4 files changed

+71
-28
lines changed

4 files changed

+71
-28
lines changed

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

Lines changed: 51 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@ static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
9898
*
9999
* The evaluation state trees (cast_exprstate) are managed in the same way as
100100
* simple expressions (i.e., we assume cast expressions are always simple).
101+
*
102+
* As with simple expressions, DO blocks don't use the shared hash table but
103+
* must have their own. This isn't ideal, but we don't want to deal with
104+
* multiple simple_eval_estates within a DO block.
101105
*/
102106
typedefstruct/* lookup key for cast info */
103107
{
@@ -118,8 +122,8 @@ typedef struct/* cast_hash table entry */
118122
LocalTransactionIdcast_lxid;
119123
}plpgsql_CastHashEntry;
120124

121-
staticMemoryContextcast_hash_context=NULL;
122-
staticHTAB*cast_hash=NULL;
125+
staticMemoryContextshared_cast_context=NULL;
126+
staticHTAB*shared_cast_hash=NULL;
123127

124128
/************************************************************
125129
* Local function forward declarations
@@ -283,7 +287,9 @@ static char *format_preparedparamsdata(PLpgSQL_execstate *estate,
283287
* difference that this code is aware of is that for a DO block, we want
284288
* to use a private simple_eval_estate, which is created and passed in by
285289
* the caller. For regular functions, pass NULL, which implies using
286-
* shared_simple_eval_estate.
290+
* shared_simple_eval_estate. (When using a private simple_eval_estate,
291+
* we must also use a private cast hashtable, but that's taken care of
292+
* within plpgsql_estate_setup.)
287293
* ----------
288294
*/
289295
Datum
@@ -3286,6 +3292,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
32863292
ReturnSetInfo*rsi,
32873293
EState*simple_eval_estate)
32883294
{
3295+
HASHCTLctl;
3296+
32893297
/* this link will be restored at exit from plpgsql_call_handler */
32903298
func->cur_estate=estate;
32913299

@@ -3333,11 +3341,44 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
33333341
estate->paramLI->parserSetupArg=NULL;/* filled during use */
33343342
estate->paramLI->numParams=estate->ndatums;
33353343

3336-
/* set up for use of appropriate simple-expression EState */
3344+
/* set up for use of appropriate simple-expression EStateand cast hash*/
33373345
if (simple_eval_estate)
3346+
{
33383347
estate->simple_eval_estate=simple_eval_estate;
3348+
/* Private cast hash just lives in function's main context */
3349+
memset(&ctl,0,sizeof(ctl));
3350+
ctl.keysize=sizeof(plpgsql_CastHashKey);
3351+
ctl.entrysize=sizeof(plpgsql_CastHashEntry);
3352+
ctl.hcxt=CurrentMemoryContext;
3353+
estate->cast_hash=hash_create("PLpgSQL private cast cache",
3354+
16,/* start small and extend */
3355+
&ctl,
3356+
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
3357+
estate->cast_hash_context=CurrentMemoryContext;
3358+
}
33393359
else
3360+
{
33403361
estate->simple_eval_estate=shared_simple_eval_estate;
3362+
/* Create the session-wide cast-info hash table if we didn't already */
3363+
if (shared_cast_hash==NULL)
3364+
{
3365+
shared_cast_context=AllocSetContextCreate(TopMemoryContext,
3366+
"PLpgSQL cast info",
3367+
ALLOCSET_DEFAULT_MINSIZE,
3368+
ALLOCSET_DEFAULT_INITSIZE,
3369+
ALLOCSET_DEFAULT_MAXSIZE);
3370+
memset(&ctl,0,sizeof(ctl));
3371+
ctl.keysize=sizeof(plpgsql_CastHashKey);
3372+
ctl.entrysize=sizeof(plpgsql_CastHashEntry);
3373+
ctl.hcxt=shared_cast_context;
3374+
shared_cast_hash=hash_create("PLpgSQL cast cache",
3375+
16,/* start small and extend */
3376+
&ctl,
3377+
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
3378+
}
3379+
estate->cast_hash=shared_cast_hash;
3380+
estate->cast_hash_context=shared_cast_context;
3381+
}
33413382

33423383
estate->eval_tuptable=NULL;
33433384
estate->eval_processed=0;
@@ -6014,32 +6055,12 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
60146055
LocalTransactionIdcurlxid;
60156056
MemoryContextoldcontext;
60166057

6017-
/* Create the session-wide cast-info hash table if we didn't already */
6018-
if (cast_hash==NULL)
6019-
{
6020-
HASHCTLctl;
6021-
6022-
cast_hash_context=AllocSetContextCreate(TopMemoryContext,
6023-
"PLpgSQL cast info",
6024-
ALLOCSET_DEFAULT_MINSIZE,
6025-
ALLOCSET_DEFAULT_INITSIZE,
6026-
ALLOCSET_DEFAULT_MAXSIZE);
6027-
memset(&ctl,0,sizeof(ctl));
6028-
ctl.keysize=sizeof(plpgsql_CastHashKey);
6029-
ctl.entrysize=sizeof(plpgsql_CastHashEntry);
6030-
ctl.hcxt=cast_hash_context;
6031-
cast_hash=hash_create("PLpgSQL cast cache",
6032-
16,/* start small and extend */
6033-
&ctl,
6034-
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
6035-
}
6036-
60376058
/* Look for existing entry */
60386059
cast_key.srctype=srctype;
60396060
cast_key.dsttype=dsttype;
60406061
cast_key.srctypmod=srctypmod;
60416062
cast_key.dsttypmod=dsttypmod;
6042-
cast_entry= (plpgsql_CastHashEntry*)hash_search(cast_hash,
6063+
cast_entry= (plpgsql_CastHashEntry*)hash_search(estate->cast_hash,
60436064
(void*)&cast_key,
60446065
HASH_FIND,NULL);
60456066

@@ -6124,15 +6145,15 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
61246145
cast_expr= (Node*)expression_planner((Expr*)cast_expr);
61256146

61266147
/* Now copy the tree into cast_hash_context */
6127-
MemoryContextSwitchTo(cast_hash_context);
6148+
MemoryContextSwitchTo(estate->cast_hash_context);
61286149

61296150
cast_expr=copyObject(cast_expr);
61306151
}
61316152

61326153
MemoryContextSwitchTo(oldcontext);
61336154

61346155
/* Now we can fill in a hashtable entry. */
6135-
cast_entry= (plpgsql_CastHashEntry*)hash_search(cast_hash,
6156+
cast_entry= (plpgsql_CastHashEntry*)hash_search(estate->cast_hash,
61366157
(void*)&cast_key,
61376158
HASH_ENTER,&found);
61386159
Assert(!found);/* wasn't there a moment ago */
@@ -6154,7 +6175,9 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
61546175
* executions. (We will leak some memory intra-transaction if that
61556176
* happens a lot, but we don't expect it to.) It's okay to update the
61566177
* hash table with the new tree because all plpgsql functions within a
6157-
* given transaction share the same simple_eval_estate.
6178+
* given transaction share the same simple_eval_estate. (Well, regular
6179+
* functions do; DO blocks have private simple_eval_estates, and private
6180+
* cast hash tables to go with them.)
61586181
*/
61596182
curlxid=MyProc->lxid;
61606183
if (cast_entry->cast_lxid!=curlxid||cast_entry->cast_in_use)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,10 @@ typedef struct PLpgSQL_execstate
796796
/* EState to use for "simple" expression evaluation */
797797
EState*simple_eval_estate;
798798

799+
/* Lookup table to use for executing type casts */
800+
HTAB*cast_hash;
801+
MemoryContextcast_hash_context;
802+
799803
/* temporary state for results from evaluation of query or expr */
800804
SPITupleTable*eval_tuptable;
801805
uint32eval_processed;

‎src/test/regress/expected/plpgsql.out

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4764,6 +4764,13 @@ commit;
47644764
drop function cast_invoker(integer);
47654765
drop function sql_to_date(integer) cascade;
47664766
NOTICE: drop cascades to cast from integer to date
4767+
-- Test handling of cast cache inside DO blocks
4768+
-- (to check the original crash case, this must be a cast not previously
4769+
-- used in this session)
4770+
begin;
4771+
do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
4772+
do $$ declare x text[]; begin x := '{1.23, 4.56}'::numeric[]; end $$;
4773+
end;
47674774
-- Test for consistent reporting of error context
47684775
create function fail() returns int language plpgsql as $$
47694776
begin

‎src/test/regress/sql/plpgsql.sql

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3836,6 +3836,15 @@ commit;
38363836
dropfunction cast_invoker(integer);
38373837
dropfunction sql_to_date(integer) cascade;
38383838

3839+
-- Test handling of cast cache inside DO blocks
3840+
-- (to check the original crash case, this must be a cast not previously
3841+
-- used in this session)
3842+
3843+
begin;
3844+
do $$ declare xtext[];begin x :='{1.23, 4.56}'::numeric[]; end $$;
3845+
do $$ declare xtext[];begin x :='{1.23, 4.56}'::numeric[]; end $$;
3846+
end;
3847+
38393848
-- Test for consistent reporting of error context
38403849

38413850
createfunctionfail() returnsint language plpgsqlas $$

0 commit comments

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