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

Commit86e5bad

Browse files
committed
Ensure that plpgsql cleans up cleanly during parallel-worker exit.
plpgsql_xact_cb ought to treat events XACT_EVENT_PARALLEL_COMMIT andXACT_EVENT_PARALLEL_ABORT like XACT_EVENT_COMMIT and XACT_EVENT_ABORTrespectively, since its goal is to do process-local cleanup. Thisoversight caused plpgsql's end-of-transaction cleanup to not get donein parallel workers. Since a parallel worker will exit just after thetransaction cleanup, the effects of this are limited. I couldn't findany case in the core code with user-visible effects, but perhaps thereare some in extensions. In any case it's wrong, so let's fix it beforeit bites us not after.In passing, add some comments around the handling of expressionevaluation resources in DO blocks. There's no live bug there, but it'squite unobvious what's happening; at least I thought so. This isn'trelated to the other issue, except that I found both things while pokingat expression-evaluation performance.Back-patch the plpgsql_xact_cb fix to 9.5 where those event typeswere introduced, and the DO-block commentary to v11 where DO blocksgained the ability to issue COMMIT/ROLLBACK.Discussion:https://postgr.es/m/10353.1585247879@sss.pgh.pa.us
1 parenteff5b24 commit86e5bad

File tree

2 files changed

+37
-7
lines changed

2 files changed

+37
-7
lines changed

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

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,13 @@ typedef struct
8484
* has its own simple-expression EState, which is cleaned up at exit from
8585
* plpgsql_inline_handler(). DO blocks still use the simple_econtext_stack,
8686
* though, so that subxact abort cleanup does the right thing.
87+
*
88+
* (However, if a DO block executes COMMIT or ROLLBACK, then exec_stmt_commit
89+
* or exec_stmt_rollback will unlink it from the DO's simple-expression EState
90+
* and create a new shared EState that will be used thenceforth. The original
91+
* EState will be cleaned up when we get back to plpgsql_inline_handler. This
92+
* is a bit ugly, but it isn't worth doing better, since scenarios like this
93+
* can't result in indefinite accumulation of state trees.)
8794
*/
8895
typedefstructSimpleEcontextStackEntry
8996
{
@@ -2315,8 +2322,8 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
23152322
else
23162323
{
23172324
/*
2318-
* If we are in a new transaction after the call, we need toreset
2319-
*some internal state.
2325+
* If we are in a new transaction after the call, we need tobuild new
2326+
*simple-expression infrastructure.
23202327
*/
23212328
estate->simple_eval_estate=NULL;
23222329
plpgsql_create_econtext(estate);
@@ -4824,6 +4831,10 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
48244831
SPI_start_transaction();
48254832
}
48264833

4834+
/*
4835+
* We need to build new simple-expression infrastructure, since the old
4836+
* data structures are gone.
4837+
*/
48274838
estate->simple_eval_estate=NULL;
48284839
plpgsql_create_econtext(estate);
48294840

@@ -4846,6 +4857,10 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
48464857
SPI_start_transaction();
48474858
}
48484859

4860+
/*
4861+
* We need to build new simple-expression infrastructure, since the old
4862+
* data structures are gone.
4863+
*/
48494864
estate->simple_eval_estate=NULL;
48504865
plpgsql_create_econtext(estate);
48514866

@@ -8208,8 +8223,13 @@ plpgsql_create_econtext(PLpgSQL_execstate *estate)
82088223
* one already in the current transaction. The EState is made a child of
82098224
* TopTransactionContext so it will have the right lifespan.
82108225
*
8211-
* Note that this path is never taken when executing a DO block; the
8212-
* required EState was already made by plpgsql_inline_handler.
8226+
* Note that this path is never taken when beginning a DO block; the
8227+
* required EState was already made by plpgsql_inline_handler. However,
8228+
* if the DO block executes COMMIT or ROLLBACK, then we'll come here and
8229+
* make a shared EState to use for the rest of the DO block. That's OK;
8230+
* see the comments for shared_simple_eval_estate. (Note also that a DO
8231+
* block will continue to use its private cast hash table for the rest of
8232+
* the block. That's okay for now, but it might cause problems someday.)
82138233
*/
82148234
if (estate->simple_eval_estate==NULL)
82158235
{
@@ -8281,15 +8301,18 @@ plpgsql_xact_cb(XactEvent event, void *arg)
82818301
* expect the regular abort recovery procedures to release everything of
82828302
* interest.
82838303
*/
8284-
if (event==XACT_EVENT_COMMIT||event==XACT_EVENT_PREPARE)
8304+
if (event==XACT_EVENT_COMMIT||
8305+
event==XACT_EVENT_PARALLEL_COMMIT||
8306+
event==XACT_EVENT_PREPARE)
82858307
{
82868308
simple_econtext_stack=NULL;
82878309

82888310
if (shared_simple_eval_estate)
82898311
FreeExecutorState(shared_simple_eval_estate);
82908312
shared_simple_eval_estate=NULL;
82918313
}
8292-
elseif (event==XACT_EVENT_ABORT)
8314+
elseif (event==XACT_EVENT_ABORT||
8315+
event==XACT_EVENT_PARALLEL_ABORT)
82938316
{
82948317
simple_econtext_stack=NULL;
82958318
shared_simple_eval_estate=NULL;

‎src/pl/plpgsql/src/pl_handler.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,14 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
323323
flinfo.fn_oid=InvalidOid;
324324
flinfo.fn_mcxt=CurrentMemoryContext;
325325

326-
/* Create a private EState for simple-expression execution */
326+
/*
327+
* Create a private EState for simple-expression execution. Notice that
328+
* this is NOT tied to transaction-level resources; it must survive any
329+
* COMMIT/ROLLBACK the DO block executes, since we will unconditionally
330+
* try to clean it up below. (Hence, be wary of adding anything that
331+
* could fail between here and the PG_TRY block.) See the comments for
332+
* shared_simple_eval_estate.
333+
*/
327334
simple_eval_estate=CreateExecutorState();
328335

329336
/* And run the function */

0 commit comments

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