8000 Handle elog(FATAL) during ROLLBACK more robustly. · dirbacke/postgres@daafb11 · GitHub
[go: up one dir, main page]
Skip to content

Commit daafb11

Browse files
tglsfdctglsfdc
committed
Handle elog(FATAL) during ROLLBACK more robustly.
Stress testing by Andreas Seltenreich disclosed longstanding problems that occur if a FATAL exit (e.g. due to receipt of SIGTERM) occurs while we are trying to execute a ROLLBACK of an already-failed transaction. In such a case, xact.c is in TBLOCK_ABORT state, so that AbortOutOfAnyTransaction would skip AbortTransaction and go straight to CleanupTransaction. This led to an assert failure in an assert-enabled build (due to the ROLLBACK's portal still having a cleanup hook) or without assertions, to a FATAL exit complaining about "cannot drop active portal". The latter's not disastrous, 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 do AtAbort_Memory and AtAbort_Portals. It seems like a good idea to do AtAbort_Memory unconditionally, to be entirely sure that we are starting with a safe CurrentMemoryContext. That means that if the main loop in AbortOutOfAnyTransaction does nothing, we need an extra step at the bottom to restore CurrentMemoryContext = TopMemoryContext, which I chose to do by invoking AtCleanup_Memory. This'll result in calling 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 that we wanted to be sure that any user-defined code executed as a consequence of the cleanup hook runs during AbortTransaction not CleanupTransaction. That still seems like a valid concern, and now that we've seen one case of the assertion firing --- which means that exactly that would have happened in a production build --- let's replace the Assert with a runtime check. If we see the cleanup hook still set, we'll emit a WARNING and just drop the hook unexecuted. This has been like this a long time, so back-patch to all supported branches. Discussion: https://postgr.es/m/877ey7bmun.fsf@ansel.ydns.eu
1 parent 01de7ea commit daafb11

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
@@ -3961,6 +3961,9 @@ AbortOutOfAnyTransaction(void)
39613961
{
39623962
TransactionState s = CurrentTransactionState;
39633963

3964+
/* Ensure we're not running in a doomed memory context */
3965+
AtAbort_Memory();
3966+
39643967
/*
39653968
* Get out of any transaction or nested transaction
39663969
*/
@@ -4001,7 +4004,14 @@ AbortOutOfAnyTransaction(void)
40014004
break;
40024005
case TBLOCK_ABORT:
40034006
case TBLOCK_ABORT_END:
4004-
/* AbortTransaction already done, still need Cleanup */
4007+
4008+
/*
4009+
* AbortTransaction is already done, still need Cleanup.
4010+
* However, if we failed partway through running ROLLBACK,
4011+
* there will be an active portal running that command, which
4012+
* we need to shut down before doing CleanupTransaction.
4013+
*/
4014+
AtAbort_Portals();
40054015
CleanupTransaction();
40064016
s->blockState = TBLOCK_DEFAULT;
40074017
break;
@@ -4024,6 +4034,14 @@ AbortOutOfAnyTransaction(void)
40244034
case TBLOCK_SUBABORT_END:
40254035
case TBLOCK_SUBABORT_RESTART:
40264036
/* As above, but AbortSubTransaction already done */
4037+
if (s->curTransactionOwner)
4038+
{
4039+
/* As in TBLOCK_ABORT, might have a live portal to zap */
4040+
AtSubAbort_Portals(s->subTransactionId,
4041+
s->parent->subTransactionId,
4042+
s->curTransactionOwner,
4043+
s->parent->curTransactionOwner);
4044+
}
40274045
CleanupSubTransaction();
40284046
s = CurrentTransactionState; /* changed by pop */
40294047
break;
@@ -4032,6 +4050,9 @@ AbortOutOfAnyTransaction(void)
40324050

40334051
/* Should be out of all subxacts now */
40344052
Assert(s->parent == NULL);
4053+
4054+
/* If we didn't actually have anything to do, revert to TopMemoryContext */
4055+
AtCleanup_Memory();
40354056
}
40364057

40374058
/*

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

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -440,8 +440,8 @@ MarkPortalDone(Portal portal)
440440
* well do that now, since the portal can't be executed any more.
441441
*
442442
* In some cases involving execution of a ROLLBACK command in an already
443-
* aborted transaction, this prevents an assertion failure caused by
444-
* reaching AtCleanup_Portals with the cleanup hook still unexecuted.
443+
* aborted transaction, this is necessary, or we'd reach AtCleanup_Portals
444+
* with the cleanup hook still unexecuted.
445445
*/
446446
if (PointerIsValid(portal->cleanup))
447447
{
@@ -468,8 +468,8 @@ MarkPortalFailed(Portal portal)
468468
* well do that now, since the portal can't be executed any more.
469469
*
470470
* In some cases involving cleanup of an already aborted transaction, this
471-
* prevents an assertion failure caused by reaching AtCleanup_Portals with
472-
* the cleanup hook still unexecuted.
471+
* is necessary, or we'd reach AtCleanup_Portals with the cleanup hook
472+
* still unexecuted.
473473
*/
474474
if (PointerIsValid(portal->cleanup))
475475
{
@@ -846,8 +846,15 @@ AtCleanup_Portals(void)
846846
if (portal->portalPinned)
847847
portal->portalPinned = false;
848848

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

852859
/* Zap it. */
853860
PortalDrop(portal, false);
@@ -1025,8 +1032,15 @@ AtSubCleanup_Portals(SubTransactionId mySubid)
10251032
if (portal->portalPinned)
10261033
portal->portalPinned = false;
10271034

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

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

0 commit comments

Comments
 (0)
0