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

Commit5b6289c

Browse files
committed
Handle elog(FATAL) during ROLLBACK more robustly.
Stress testing by Andreas Seltenreich disclosed longstanding problems thatoccur if a FATAL exit (e.g. due to receipt of SIGTERM) occurs while we aretrying to execute a ROLLBACK of an already-failed transaction. In such acase, xact.c is in TBLOCK_ABORT state, so that AbortOutOfAnyTransactionwould skip AbortTransaction and go straight to CleanupTransaction. Thisled to an assert failure in an assert-enabled build (due to the ROLLBACK'sportal still having a cleanup hook) or without assertions, to a FATAL exitcomplaining about "cannot drop active portal". The latter's notdisastrous, perhaps, but it's messy enough to want to improve it.We don't really want to run all of AbortTransaction in this code path.The minimum required to clean up the open portal safely is to doAtAbort_Memory and AtAbort_Portals. It seems like a good idea todo AtAbort_Memory unconditionally, to be entirely sure that we arestarting with a safe CurrentMemoryContext. That means that if themain loop in AbortOutOfAnyTransaction does nothing, we need an extrastep at the bottom to restore CurrentMemoryContext = TopMemoryContext,which I chose to do by invoking AtCleanup_Memory. This'll result incalling AtCleanup_Memory twice in many of the paths through this function,but that seems harmless and reasonably inexpensive.The original motivation for the assertion in AtCleanup_Portals was thatwe wanted to be sure that any user-defined code executed as a consequenceof the cleanup hook runs during AbortTransaction not CleanupTransaction.That still seems like a valid concern, and now that we've seen one caseof the assertion firing --- which means that exactly that would havehappened in a production build --- let's replace the Assert with a runtimecheck. If we see the cleanup hook still set, we'll emit a WARNING andjust drop the hook unexecuted.This has been like this a long time, so back-patch to all supportedbranches.Discussion:https://postgr.es/m/877ey7bmun.fsf@ansel.ydns.eu
1 parent7f1bb1d commit5b6289c

File tree

2 files changed

+44
-9
lines changed

2 files changed

+44
-9
lines changed

‎src/backend/access/transam/xact.c

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4226,6 +4226,9 @@ AbortOutOfAnyTransaction(void)
42264226
{
42274227
TransactionStates=CurrentTransactionState;
42284228

4229+
/* Ensure we're not running in a doomed memory context */
4230+
AtAbort_Memory();
4231+
42294232
/*
42304233
* Get out of any transaction or nested transaction
42314234
*/
@@ -4267,7 +4270,14 @@ AbortOutOfAnyTransaction(void)
42674270
break;
42684271
caseTBLOCK_ABORT:
42694272
caseTBLOCK_ABORT_END:
4270-
/* AbortTransaction already done, still need Cleanup */
4273+
4274+
/*
4275+
* AbortTransaction is already done, still need Cleanup.
4276+
* However, if we failed partway through running ROLLBACK,
4277+
* there will be an active portal running that command, which
4278+
* we need to shut down before doing CleanupTransaction.
4279+
*/
4280+
AtAbort_Portals();
42714281
CleanupTransaction();
42724282
s->blockState=TBLOCK_DEFAULT;
42734283
break;
@@ -4290,6 +4300,14 @@ AbortOutOfAnyTransaction(void)
42904300
caseTBLOCK_SUBABORT_END:
42914301
caseTBLOCK_SUBABORT_RESTART:
42924302
/* As above, but AbortSubTransaction already done */
4303+
if (s->curTransactionOwner)
4304+
{
4305+
/* As in TBLOCK_ABORT, might have a live portal to zap */
4306+
AtSubAbort_Portals(s->subTransactionId,
4307+
s->parent->subTransactionId,
4308+
s->curTransactionOwner,
4309+
s->parent->curTransactionOwner);
4310+
}
42934311
CleanupSubTransaction();
42944312
s=CurrentTransactionState;/* changed by pop */
42954313
break;
@@ -4298,6 +4316,9 @@ AbortOutOfAnyTransaction(void)
42984316

42994317
/* Should be out of all subxacts now */
43004318
Assert(s->parent==NULL);
4319+
4320+
/* If we didn't actually have anything to do, revert to TopMemoryContext */
4321+
AtCleanup_Memory();
43014322
}
43024323

43034324
/*

‎src/backend/utils/mmgr/portalmem.c

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -415,8 +415,8 @@ MarkPortalDone(Portal portal)
415415
* well do that now, since the portal can't be executed any more.
416416
*
417417
* In some cases involving execution of a ROLLBACK command in an already
418-
* aborted transaction, thisprevents an assertion failure caused by
419-
*reaching AtCleanup_Portalswith the cleanup hook still unexecuted.
418+
* aborted transaction, thisis necessary, or we'd reach AtCleanup_Portals
419+
* with the cleanup hook still unexecuted.
420420
*/
421421
if (PointerIsValid(portal->cleanup))
422422
{
@@ -443,8 +443,8 @@ MarkPortalFailed(Portal portal)
443443
* well do that now, since the portal can't be executed any more.
444444
*
445445
* In some cases involving cleanup of an already aborted transaction, this
446-
*prevents an assertion failure caused by reachingAtCleanup_Portals with
447-
*the cleanup hookstill unexecuted.
446+
*is necessary, or we'd reachAtCleanup_Portals with the cleanup hook
447+
* still unexecuted.
448448
*/
449449
if (PointerIsValid(portal->cleanup))
450450
{
@@ -842,8 +842,15 @@ AtCleanup_Portals(void)
842842
if (portal->portalPinned)
843843
portal->portalPinned= false;
844844

845-
/* We had better not be calling any user-defined code here */
846-
Assert(portal->cleanup==NULL);
845+
/*
846+
* We had better not call any user-defined code during cleanup, so if
847+
* the cleanup hook hasn't been run yet, too bad; we'll just skip it.
848+
*/
849+
if (PointerIsValid(portal->cleanup))
850+
{
851+
elog(WARNING,"skipping cleanup for portal \"%s\"",portal->name);
852+
portal->cleanup=NULL;
853+
}
847854

848855
/* Zap it. */
849856
PortalDrop(portal, false);
@@ -1026,8 +1033,15 @@ AtSubCleanup_Portals(SubTransactionId mySubid)
10261033
if (portal->portalPinned)
10271034
portal->portalPinned= false;
10281035

1029-
/* We had better not be calling any user-defined code here */
1030-
Assert(portal->cleanup==NULL);
1036+
/*
1037+
* We had better not call any user-defined code during cleanup, so if
1038+
* the cleanup hook hasn't been run yet, too bad; we'll just skip it.
1039+
*/
1040+
if (PointerIsValid(portal->cleanup))
1041+
{
1042+
elog(WARNING,"skipping cleanup for portal \"%s\"",portal->name);
1043+
portal->cleanup=NULL;
1044+
}
10311045

10321046
/* Zap it. */
10331047
PortalDrop(portal, false);

0 commit comments

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