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

Commitdeeda01

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 parentba4cc05 commitdeeda01

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
@@ -85,6 +85,13 @@ typedef struct
8585
* has its own simple-expression EState, which is cleaned up at exit from
8686
* plpgsql_inline_handler(). DO blocks still use the simple_econtext_stack,
8787
* though, so that subxact abort cleanup does the right thing.
88+
*
89+
* (However, if a DO block executes COMMIT or ROLLBACK, then exec_stmt_commit
90+
* or exec_stmt_rollback will unlink it from the DO's simple-expression EState
91+
* and create a new shared EState that will be used thenceforth. The original
92+
* EState will be cleaned up when we get back to plpgsql_inline_handler. This
93+
* is a bit ugly, but it isn't worth doing better, since scenarios like this
94+
* can't result in indefinite accumulation of state trees.)
8895
*/
8996
typedefstructSimpleEcontextStackEntry
9097
{
@@ -2297,8 +2304,8 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
22972304
else
22982305
{
22992306
/*
2300-
* If we are in a new transaction after the call, we need toreset
2301-
*some internal state.
2307+
* If we are in a new transaction after the call, we need tobuild new
2308+
*simple-expression infrastructure.
23022309
*/
23032310
estate->simple_eval_estate=NULL;
23042311
plpgsql_create_econtext(estate);
@@ -4801,6 +4808,10 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
48014808
SPI_commit();
48024809
SPI_start_transaction();
48034810

4811+
/*
4812+
* We need to build new simple-expression infrastructure, since the old
4813+
* data structures are gone.
4814+
*/
48044815
estate->simple_eval_estate=NULL;
48054816
plpgsql_create_econtext(estate);
48064817

@@ -4818,6 +4829,10 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
48184829
SPI_rollback();
48194830
SPI_start_transaction();
48204831

4832+
/*
4833+
* We need to build new simple-expression infrastructure, since the old
4834+
* data structures are gone.
4835+
*/
48214836
estate->simple_eval_estate=NULL;
48224837
plpgsql_create_econtext(estate);
48234838

@@ -8082,8 +8097,13 @@ plpgsql_create_econtext(PLpgSQL_execstate *estate)
80828097
* one already in the current transaction. The EState is made a child of
80838098
* TopTransactionContext so it will have the right lifespan.
80848099
*
8085-
* Note that this path is never taken when executing a DO block; the
8086-
* required EState was already made by plpgsql_inline_handler.
8100+
* Note that this path is never taken when beginning a DO block; the
8101+
* required EState was already made by plpgsql_inline_handler. However,
8102+
* if the DO block executes COMMIT or ROLLBACK, then we'll come here and
8103+
* make a shared EState to use for the rest of the DO block. That's OK;
8104+
* see the comments for shared_simple_eval_estate. (Note also that a DO
8105+
* block will continue to use its private cast hash table for the rest of
8106+
* the block. That's okay for now, but it might cause problems someday.)
80878107
*/
80888108
if (estate->simple_eval_estate==NULL)
80898109
{
@@ -8155,15 +8175,18 @@ plpgsql_xact_cb(XactEvent event, void *arg)
81558175
* expect the regular abort recovery procedures to release everything of
81568176
* interest.
81578177
*/
8158-
if (event==XACT_EVENT_COMMIT||event==XACT_EVENT_PREPARE)
8178+
if (event==XACT_EVENT_COMMIT||
8179+
event==XACT_EVENT_PARALLEL_COMMIT||
8180+
event==XACT_EVENT_PREPARE)
81598181
{
81608182
simple_econtext_stack=NULL;
81618183

81628184
if (shared_simple_eval_estate)
81638185
FreeExecutorState(shared_simple_eval_estate);
81648186
shared_simple_eval_estate=NULL;
81658187
}
8166-
elseif (event==XACT_EVENT_ABORT)
8188+
elseif (event==XACT_EVENT_ABORT||
8189+
event==XACT_EVENT_PARALLEL_ABORT)
81678190
{
81688191
simple_econtext_stack=NULL;
81698192
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
@@ -326,7 +326,14 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
326326
flinfo.fn_oid=InvalidOid;
327327
flinfo.fn_mcxt=CurrentMemoryContext;
328328

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

332339
/* And run the function */

0 commit comments

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