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

Commit03cd757

Browse files
committed
Fix the plpgsql memory leak exhibited in bug #4677. That leak was introduced
by my patch of 2007-01-28 to use per-subtransaction ExprContexts/EStates:since we re-prepared any expression tree when the current subtransaction IDchanged, we'd accumulate more and more leaked expression state trees in theoutermost subtransaction if the same function was executed at multiple levelsof subtransaction nesting. To fix, go back to the previous scheme wherethere was only one EState per transaction for simple plpgsql expressions.We really only need an ExprContext per subtransaction, not a whole EState,so it's possible to keep prepared expression state trees in the one EStatethroughout the transaction. This should be more efficient as well as notleaking memory for cases involving lots of subtransactions.The added regression test is the case that inspired the 2007-01-28 patch inthe first place, just to make sure we didn't go backwards. The currentmemory leak complaint is unfortunately hard to test for in the regressiontest framework, though manual testing shows it's fixed.Although this is a pre-existing bug, I'm not back-patching because I'd like tosee this method get some field testing first. Consider back-patching if itgets through 8.4beta unscathed.
1 parent4703250 commit03cd757

File tree

4 files changed

+176
-85
lines changed

4 files changed

+176
-85
lines changed

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

Lines changed: 99 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.239 2009/04/02 20:16:30 tgl Exp $
11+
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.240 2009/04/09 02:57:53 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -24,6 +24,7 @@
2424
#include"funcapi.h"
2525
#include"nodes/nodeFuncs.h"
2626
#include"parser/scansup.h"
27+
#include"storage/proc.h"
2728
#include"tcop/tcopprot.h"
2829
#include"utils/array.h"
2930
#include"utils/builtins.h"
@@ -51,27 +52,26 @@ typedef struct
5152
* creates its own "eval_econtext" ExprContext within this estate for
5253
* per-evaluation workspace. eval_econtext is freed at normal function exit,
5354
* and the EState is freed at transaction end (in case of error, we assume
54-
* that the abort mechanisms clean it all up).In order to be sure
55-
*ExprContext callbacks are handled properly, each subtransactionhas to have
56-
*its own such EState; hence weneed a stack.We use a simple counter to
57-
*distinguish different instantiations of the EState, so that we can tell
58-
*whether we have a current copy of a prepared expression.
55+
* that the abort mechanisms clean it all up). Furthermore, any exception
56+
*block within a functionhas to have its own eval_econtext separate from
57+
*the containing function's, so that wecan clean up ExprContext callbacks
58+
*properly at subtransaction exit. We maintain a stack that tracks the
59+
*individual econtexts so that we can clean up correctly at subxact exit.
5960
*
6061
* This arrangement is a bit tedious to maintain, but it's worth the trouble
6162
* so that we don't have to re-prepare simple expressions on each trip through
6263
* a function.(We assume the case to optimize is many repetitions of a
6364
* function within a transaction.)
6465
*/
65-
typedefstructSimpleEstateStackEntry
66+
typedefstructSimpleEcontextStackEntry
6667
{
67-
EState*xact_eval_estate;/* EState for current xact level */
68-
longintxact_estate_simple_id;/* ID for xact_eval_estate */
69-
SubTransactionIdxact_subxid;/* ID for current subxact */
70-
structSimpleEstateStackEntry*next;/* next stack entry up */
71-
}SimpleEstateStackEntry;
68+
ExprContext*stack_econtext;/* a stacked econtext */
69+
SubTransactionIdxact_subxid;/* ID for current subxact */
70+
structSimpleEcontextStackEntry*next;/* next stack entry up */
71+
}SimpleEcontextStackEntry;
7272

73-
staticSimpleEstateStackEntry*simple_estate_stack=NULL;
74-
staticlongintsimple_estate_id_counter=0;
73+
staticEState*simple_eval_estate=NULL;
74+
staticSimpleEcontextStackEntry*simple_econtext_stack=NULL;
7575

7676
/************************************************************
7777
* Local function forward declarations
@@ -193,6 +193,7 @@ static void validate_tupdesc_compat(TupleDesc expected, TupleDesc returned,
193193
constchar*msg);
194194
staticvoidexec_set_found(PLpgSQL_execstate*estate,boolstate);
195195
staticvoidplpgsql_create_econtext(PLpgSQL_execstate*estate);
196+
staticvoidplpgsql_destroy_econtext(PLpgSQL_execstate*estate);
196197
staticvoidfree_var(PLpgSQL_var*var);
197198
staticvoidassign_text_var(PLpgSQL_var*var,constchar*str);
198199
staticPreparedParamsData*exec_eval_using_params(PLpgSQL_execstate*estate,
@@ -452,8 +453,7 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo)
452453
((*plugin_ptr)->func_end) (&estate,func);
453454

454455
/* Clean up any leftover temporary memory */
455-
FreeExprContext(estate.eval_econtext);
456-
estate.eval_econtext=NULL;
456+
plpgsql_destroy_econtext(&estate);
457457
exec_eval_cleanup(&estate);
458458

459459
/*
@@ -718,8 +718,7 @@ plpgsql_exec_trigger(PLpgSQL_function *func,
718718
((*plugin_ptr)->func_end) (&estate,func);
719719

720720
/* Clean up any leftover temporary memory */
721-
FreeExprContext(estate.eval_econtext);
722-
estate.eval_econtext=NULL;
721+
plpgsql_destroy_econtext(&estate);
723722
exec_eval_cleanup(&estate);
724723

725724
/*
@@ -984,8 +983,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
984983
MemoryContextoldcontext=CurrentMemoryContext;
985984
ResourceOwneroldowner=CurrentResourceOwner;
986985
ExprContext*old_eval_econtext=estate->eval_econtext;
987-
EState*old_eval_estate=estate->eval_estate;
988-
longintold_eval_estate_simple_id=estate->eval_estate_simple_id;
989986

990987
estate->err_text=gettext_noop("during statement block entry");
991988

@@ -1034,10 +1031,11 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
10341031
MemoryContextSwitchTo(oldcontext);
10351032
CurrentResourceOwner=oldowner;
10361033

1037-
/* Revert to outer eval_econtext */
1034+
/*
1035+
* Revert to outer eval_econtext. (The inner one was automatically
1036+
* cleaned up during subxact exit.)
1037+
*/
10381038
estate->eval_econtext=old_eval_econtext;
1039-
estate->eval_estate=old_eval_estate;
1040-
estate->eval_estate_simple_id=old_eval_estate_simple_id;
10411039

10421040
/*
10431041
* AtEOSubXact_SPI() should not have popped any SPI context, but
@@ -1064,8 +1062,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
10641062

10651063
/* Revert to outer eval_econtext */
10661064
estate->eval_econtext=old_eval_econtext;
1067-
estate->eval_estate=old_eval_estate;
1068-
estate->eval_estate_simple_id=old_eval_estate_simple_id;
10691065

10701066
/*
10711067
* If AtEOSubXact_SPI() popped any SPI context of the subxact, it
@@ -4333,6 +4329,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
43334329
Oid*rettype)
43344330
{
43354331
ExprContext*econtext=estate->eval_econtext;
4332+
LocalTransactionIdcurlxid=MyProc->lxid;
43364333
CachedPlanSource*plansource;
43374334
CachedPlan*cplan;
43384335
ParamListInfoparamLI;
@@ -4373,14 +4370,14 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
43734370

43744371
/*
43754372
* Prepare the expression for execution, if it's not been done already in
4376-
* the currenteval_estate. (This will be forced to happen if we called
4373+
* the currenttransaction. (This will be forced to happen if we called
43774374
* exec_simple_check_plan above.)
43784375
*/
4379-
if (expr->expr_simple_id!=estate->eval_estate_simple_id)
4376+
if (expr->expr_simple_lxid!=curlxid)
43804377
{
43814378
expr->expr_simple_state=ExecPrepareExpr(expr->expr_simple_expr,
4382-
estate->eval_estate);
4383-
expr->expr_simple_id=estate->eval_estate_simple_id;
4379+
simple_eval_estate);
4380+
expr->expr_simple_lxid=curlxid;
43844381
}
43854382

43864383
/*
@@ -5103,7 +5100,7 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
51035100
*/
51045101
expr->expr_simple_expr=tle->expr;
51055102
expr->expr_simple_state=NULL;
5106-
expr->expr_simple_id=-1;
5103+
expr->expr_simple_lxid=InvalidLocalTransactionId;
51075104
/* Also stash away the expression result type */
51085105
expr->expr_simple_type=exprType((Node*)tle->expr);
51095106
}
@@ -5165,46 +5162,69 @@ exec_set_found(PLpgSQL_execstate *estate, bool state)
51655162
/*
51665163
* plpgsql_create_econtext --- create an eval_econtext for the current function
51675164
*
5168-
* We may need to create a neweval_estate too, if there's not one already
5169-
* for the current (sub) transaction. The EState will be cleaned up at
5170-
*(sub)transaction end.
5165+
* We may need to create a newsimple_eval_estate too, if there's not one
5166+
*alreadyfor the current transaction. The EState will be cleaned up at
5167+
* transaction end.
51715168
*/
51725169
staticvoid
51735170
plpgsql_create_econtext(PLpgSQL_execstate*estate)
51745171
{
5175-
SubTransactionIdmy_subxid=GetCurrentSubTransactionId();
5176-
SimpleEstateStackEntry*entry=simple_estate_stack;
5172+
SimpleEcontextStackEntry*entry;
51775173

5178-
/* Create new EState if not one for current subxact */
5179-
if (entry==NULL||
5180-
entry->xact_subxid!=my_subxid)
5174+
/*
5175+
* Create an EState for evaluation of simple expressions, if there's not
5176+
* one already in the current transaction.The EState is made a child of
5177+
* TopTransactionContext so it will have the right lifespan.
5178+
*/
5179+
if (simple_eval_estate==NULL)
51815180
{
51825181
MemoryContextoldcontext;
51835182

5184-
/* Stack entries are kept in TopTransactionContext for simplicity */
5185-
entry= (SimpleEstateStackEntry*)
5186-
MemoryContextAlloc(TopTransactionContext,
5187-
sizeof(SimpleEstateStackEntry));
5188-
5189-
/* But each EState should be a child of its CurTransactionContext */
5190-
oldcontext=MemoryContextSwitchTo(CurTransactionContext);
5191-
entry->xact_eval_estate=CreateExecutorState();
5183+
oldcontext=MemoryContextSwitchTo(TopTransactionContext);
5184+
simple_eval_estate=CreateExecutorState();
51925185
MemoryContextSwitchTo(oldcontext);
5186+
}
51935187

5194-
/* Assign a reasonably-unique ID to this EState */
5195-
entry->xact_estate_simple_id=simple_estate_id_counter++;
5196-
entry->xact_subxid=my_subxid;
5188+
/*
5189+
* Create a child econtext for the current function.
5190+
*/
5191+
estate->eval_econtext=CreateExprContext(simple_eval_estate);
51975192

5198-
entry->next=simple_estate_stack;
5199-
simple_estate_stack=entry;
5200-
}
5193+
/*
5194+
* Make a stack entry so we can clean up the econtext at subxact end.
5195+
* Stack entries are kept in TopTransactionContext for simplicity.
5196+
*/
5197+
entry= (SimpleEcontextStackEntry*)
5198+
MemoryContextAlloc(TopTransactionContext,
5199+
sizeof(SimpleEcontextStackEntry));
52015200

5202-
/* Link plpgsql estate to it */
5203-
estate->eval_estate=entry->xact_eval_estate;
5204-
estate->eval_estate_simple_id=entry->xact_estate_simple_id;
5201+
entry->stack_econtext=estate->eval_econtext;
5202+
entry->xact_subxid=GetCurrentSubTransactionId();
52055203

5206-
/* And create a child econtext for the current function */
5207-
estate->eval_econtext=CreateExprContext(estate->eval_estate);
5204+
entry->next=simple_econtext_stack;
5205+
simple_econtext_stack=entry;
5206+
}
5207+
5208+
/*
5209+
* plpgsql_destroy_econtext --- destroy function's econtext
5210+
*
5211+
* We check that it matches the top stack entry, and destroy the stack
5212+
* entry along with the context.
5213+
*/
5214+
staticvoid
5215+
plpgsql_destroy_econtext(PLpgSQL_execstate*estate)
5216+
{
5217+
SimpleEcontextStackEntry*next;
5218+
5219+
Assert(simple_econtext_stack!=NULL);
5220+
Assert(simple_econtext_stack->stack_econtext==estate->eval_econtext);
5221+
5222+
next=simple_econtext_stack->next;
5223+
pfree(simple_econtext_stack);
5224+
simple_econtext_stack=next;
5225+
5226+
FreeExprContext(estate->eval_econtext);
5227+
estate->eval_econtext=NULL;
52085228
}
52095229

52105230
/*
@@ -5220,29 +5240,31 @@ plpgsql_xact_cb(XactEvent event, void *arg)
52205240
* If we are doing a clean transaction shutdown, free the EState (so that
52215241
* any remaining resources will be released correctly). In an abort, we
52225242
* expect the regular abort recovery procedures to release everything of
5223-
* interest. We don't need to free the individual stack entries since
5224-
* TopTransactionContext is about to go away anyway.
5225-
*
5226-
* Note: if plpgsql_subxact_cb is doing its job, there should be at most
5227-
* one stack entry, but we may as well code this as a loop.
5243+
* interest.
52285244
*/
52295245
if (event!=XACT_EVENT_ABORT)
52305246
{
5231-
while (simple_estate_stack!=NULL)
5232-
{
5233-
FreeExecutorState(simple_estate_stack->xact_eval_estate);
5234-
simple_estate_stack=simple_estate_stack->next;
5235-
}
5247+
/* Shouldn't be any econtext stack entries left at commit */
5248+
Assert(simple_econtext_stack==NULL);
5249+
5250+
if (simple_eval_estate)
5251+
FreeExecutorState(simple_eval_estate);
5252+
simple_eval_estate=NULL;
52365253
}
52375254
else
5238-
simple_estate_stack=NULL;
5255+
{
5256+
simple_econtext_stack=NULL;
5257+
simple_eval_estate=NULL;
5258+
}
52395259
}
52405260

52415261
/*
52425262
* plpgsql_subxact_cb --- post-subtransaction-commit-or-abort cleanup
52435263
*
5244-
* If a simple-expression EState was created in the current subtransaction,
5245-
* it has to be cleaned up.
5264+
* Make sure any simple-expression econtexts created in the current
5265+
* subtransaction get cleaned up. We have to do this explicitly because
5266+
* no other code knows which child econtexts of simple_eval_estate belong
5267+
* to which level of subxact.
52465268
*/
52475269
void
52485270
plpgsql_subxact_cb(SubXactEventevent,SubTransactionIdmySubid,
@@ -5251,16 +5273,15 @@ plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid,
52515273
if (event==SUBXACT_EVENT_START_SUB)
52525274
return;
52535275

5254-
if (simple_estate_stack!=NULL&&
5255-
simple_estate_stack->xact_subxid==mySubid)
5276+
while (simple_econtext_stack!=NULL&&
5277+
simple_econtext_stack->xact_subxid==mySubid)
52565278
{
5257-
SimpleEstateStackEntry*next;
5279+
SimpleEcontextStackEntry*next;
52585280

5259-
if (event==SUBXACT_EVENT_COMMIT_SUB)
5260-
FreeExecutorState(simple_estate_stack->xact_eval_estate);
5261-
next=simple_estate_stack->next;
5262-
pfree(simple_estate_stack);
5263-
simple_estate_stack=next;
5281+
FreeExprContext(simple_econtext_stack->stack_econtext);
5282+
next=simple_econtext_stack->next;
5283+
pfree(simple_econtext_stack);
5284+
simple_econtext_stack=next;
52645285
}
52655286
}
52665287

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.109 2009/02/17 11:34:34 petere Exp $
11+
* $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.110 2009/04/09 02:57:53 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -205,12 +205,12 @@ typedef struct PLpgSQL_expr
205205
Oidexpr_simple_type;/* result type Oid, if simple */
206206

207207
/*
208-
* if expr is simple AND prepared in currenteval_estate,
209-
* expr_simple_state is valid.Test validity by seeing ifexpr_simple_id
210-
* matcheseval_estate_simple_id.
208+
* if expr is simple AND prepared in currenttransaction,
209+
* expr_simple_state is valid.Test validity by seeing ifexpr_simple_lxid
210+
* matchescurrent LXID.
211211
*/
212212
ExprState*expr_simple_state;
213-
longintexpr_simple_id;
213+
LocalTransactionIdexpr_simple_lxid;
214214

215215
/* params to pass to expr */
216216
intnparams;
@@ -719,8 +719,6 @@ typedef struct
719719
uint32eval_processed;
720720
Oideval_lastoid;
721721
ExprContext*eval_econtext;/* for executing simple expressions */
722-
EState*eval_estate;/* EState containing eval_econtext */
723-
longinteval_estate_simple_id;/* ID for eval_estate */
724722

725723
/* status information for error context reporting */
726724
PLpgSQL_function*err_func;/* current func */

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3698,3 +3698,42 @@ NOTICE: f 0
36983698
(4 rows)
36993699

37003700
drop function rttest();
3701+
-- Test for proper cleanup at subtransaction exit. This example
3702+
-- exposed a bug in PG 8.2.
3703+
CREATE FUNCTION leaker_1(fail BOOL) RETURNS INTEGER AS $$
3704+
DECLARE
3705+
v_var INTEGER;
3706+
BEGIN
3707+
BEGIN
3708+
v_var := (leaker_2(fail)).error_code;
3709+
EXCEPTION
3710+
WHEN others THEN RETURN 0;
3711+
END;
3712+
RETURN 1;
3713+
END;
3714+
$$ LANGUAGE plpgsql;
3715+
CREATE FUNCTION leaker_2(fail BOOL, OUT error_code INTEGER, OUT new_id INTEGER)
3716+
RETURNS RECORD AS $$
3717+
BEGIN
3718+
IF fail THEN
3719+
RAISE EXCEPTION 'fail ...';
3720+
END IF;
3721+
error_code := 1;
3722+
new_id := 1;
3723+
RETURN;
3724+
END;
3725+
$$ LANGUAGE plpgsql;
3726+
SELECT * FROM leaker_1(false);
3727+
leaker_1
3728+
----------
3729+
1
3730+
(1 row)
3731+
3732+
SELECT * FROM leaker_1(true);
3733+
leaker_1
3734+
----------
3735+
0
3736+
(1 row)
3737+
3738+
DROP FUNCTION leaker_1(bool);
3739+
DROP FUNCTION leaker_2(bool);

0 commit comments

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