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

Commit04a0231

Browse files
committed
Run a portal's cleanup hook immediately when pushing it to FAILED state.
This extends the changes of commit6252c4fso that we run the cleanup hook earlier for failure cases as well assuccess cases. As before, the point is to avoid an assertion failure froman Assert I added in commita874fe7, whichwas meant to check that no user-written code can be called during portalcleanup. This fixes a case reported by Pavan Deolasee in which the Assertcould be triggered during backend exit (see the new regression test case),and also prevents the possibility that the cleanup hook is run afterportions of the portal's state have already been recycled. That doesn'treally matter in current usage, but it foreseeably could matter in thefuture.Back-patch to 9.1 where the Assert in question was added.
1 parent421513b commit04a0231

File tree

6 files changed

+72
-42
lines changed

6 files changed

+72
-42
lines changed

‎src/backend/commands/portalcmds.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ PerformPortalClose(const char *name)
224224
}
225225

226226
/*
227-
* Note: PortalCleanup is called as a side-effect
227+
* Note: PortalCleanup is called as a side-effect, if not already done.
228228
*/
229229
PortalDrop(portal, false);
230230
}
@@ -234,6 +234,10 @@ PerformPortalClose(const char *name)
234234
*
235235
* Clean up a portal when it's dropped. This is the standard cleanup hook
236236
* for portals.
237+
*
238+
* Note: if portal->status is PORTAL_FAILED, we are probably being called
239+
* during error abort, and must be careful to avoid doing anything that
240+
* is likely to fail again.
237241
*/
238242
void
239243
PortalCleanup(Portalportal)
@@ -420,7 +424,7 @@ PersistHoldablePortal(Portal portal)
420424
PG_CATCH();
421425
{
422426
/* Uncaught error while executing portal: mark it dead */
423-
portal->status=PORTAL_FAILED;
427+
MarkPortalFailed(portal);
424428

425429
/* Restore global vars and propagate error */
426430
ActivePortal=saveActivePortal;

‎src/backend/tcop/pquery.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,7 @@ PortalStart(Portal portal, ParamListInfo params, Snapshot snapshot)
608608
PG_CATCH();
609609
{
610610
/* Uncaught error while executing portal: mark it dead */
611-
portal->status=PORTAL_FAILED;
611+
MarkPortalFailed(portal);
612612

613613
/* Restore global vars and propagate error */
614614
ActivePortal=saveActivePortal;
@@ -830,7 +830,7 @@ PortalRun(Portal portal, long count, bool isTopLevel,
830830
PG_CATCH();
831831
{
832832
/* Uncaught error while executing portal: mark it dead */
833-
portal->status=PORTAL_FAILED;
833+
MarkPortalFailed(portal);
834834

835835
/* Restore global vars and propagate error */
836836
if (saveMemoryContext==saveTopTransactionContext)
@@ -1447,7 +1447,7 @@ PortalRunFetch(Portal portal,
14471447
PG_CATCH();
14481448
{
14491449
/* Uncaught error while executing portal: mark it dead */
1450-
portal->status=PORTAL_FAILED;
1450+
MarkPortalFailed(portal);
14511451

14521452
/* Restore global vars and propagate error */
14531453
ActivePortal=saveActivePortal;

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

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,8 @@ UnpinPortal(Portal portal)
404404
/*
405405
* MarkPortalDone
406406
*Transition a portal from ACTIVE to DONE state.
407+
*
408+
* NOTE: never set portal->status = PORTAL_DONE directly; call this instead.
407409
*/
408410
void
409411
MarkPortalDone(Portalportal)
@@ -417,8 +419,36 @@ MarkPortalDone(Portal portal)
417419
* well do that now, since the portal can't be executed any more.
418420
*
419421
* In some cases involving execution of a ROLLBACK command in an already
420-
* aborted transaction, this prevents an assertion failure from reaching
421-
* AtCleanup_Portals with the cleanup hook still unexecuted.
422+
* aborted transaction, this prevents an assertion failure caused by
423+
* reaching AtCleanup_Portals with the cleanup hook still unexecuted.
424+
*/
425+
if (PointerIsValid(portal->cleanup))
426+
{
427+
(*portal->cleanup) (portal);
428+
portal->cleanup=NULL;
429+
}
430+
}
431+
432+
/*
433+
* MarkPortalFailed
434+
*Transition a portal into FAILED state.
435+
*
436+
* NOTE: never set portal->status = PORTAL_FAILED directly; call this instead.
437+
*/
438+
void
439+
MarkPortalFailed(Portalportal)
440+
{
441+
/* Perform the state transition */
442+
Assert(portal->status!=PORTAL_DONE);
443+
portal->status=PORTAL_FAILED;
444+
445+
/*
446+
* Allow portalcmds.c to clean up the state it knows about. We might as
447+
* well do that now, since the portal can't be executed any more.
448+
*
449+
* In some cases involving cleanup of an already aborted transaction, this
450+
* prevents an assertion failure caused by reaching AtCleanup_Portals with
451+
* the cleanup hook still unexecuted.
422452
*/
423453
if (PointerIsValid(portal->cleanup))
424454
{
@@ -454,6 +484,9 @@ PortalDrop(Portal portal, bool isTopCommit)
454484
* hook's responsibility to not try to do that more than once, in the case
455485
* that failure occurs and then we come back to drop the portal again
456486
* during transaction abort.
487+
*
488+
* Note: in most paths of control, this will have been done already in
489+
* MarkPortalDone or MarkPortalFailed. We're just making sure.
457490
*/
458491
if (PointerIsValid(portal->cleanup))
459492
{
@@ -712,7 +745,7 @@ AtAbort_Portals(void)
712745

713746
/* Any portal that was actually running has to be considered broken */
714747
if (portal->status==PORTAL_ACTIVE)
715-
portal->status=PORTAL_FAILED;
748+
MarkPortalFailed(portal);
716749

717750
/*
718751
* Do nothing else to cursors held over from a previous transaction.
@@ -727,9 +760,12 @@ AtAbort_Portals(void)
727760
* AtSubAbort_Portals.
728761
*/
729762
if (portal->status==PORTAL_READY)
730-
portal->status=PORTAL_FAILED;
763+
MarkPortalFailed(portal);
731764

732-
/* let portalcmds.c clean up the state it knows about */
765+
/*
766+
* Allow portalcmds.c to clean up the state it knows about, if we
767+
* haven't already.
768+
*/
733769
if (PointerIsValid(portal->cleanup))
734770
{
735771
(*portal->cleanup) (portal);
@@ -861,9 +897,12 @@ AtSubAbort_Portals(SubTransactionId mySubid,
861897
*/
862898
if (portal->status==PORTAL_READY||
863899
portal->status==PORTAL_ACTIVE)
864-
portal->status=PORTAL_FAILED;
900+
MarkPortalFailed(portal);
865901

866-
/* let portalcmds.c clean up the state it knows about */
902+
/*
903+
* Allow portalcmds.c to clean up the state it knows about, if we
904+
* haven't already.
905+
*/
867906
if (PointerIsValid(portal->cleanup))
868907
{
869908
(*portal->cleanup) (portal);

‎src/include/utils/portal.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,7 @@ extern Portal CreateNewPortal(void);
207207
externvoidPinPortal(Portalportal);
208208
externvoidUnpinPortal(Portalportal);
209209
externvoidMarkPortalDone(Portalportal);
210+
externvoidMarkPortalFailed(Portalportal);
210211
externvoidPortalDrop(Portalportal,boolisTopCommit);
211212
externPortalGetPortalByName(constchar*name);
212213
externvoidPortalDefineQuery(Portalportal,

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

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -601,28 +601,11 @@ fetch from foo;
601601
(1 row)
602602

603603
abort;
604-
-- tests for the "tid" type
605-
SELECT '(3, 3)'::tid = '(3, 4)'::tid;
606-
?column?
607-
----------
608-
f
609-
(1 row)
610-
611-
SELECT '(3, 3)'::tid = '(3, 3)'::tid;
612-
?column?
613-
----------
614-
t
615-
(1 row)
616-
617-
SELECT '(3, 3)'::tid <> '(3, 3)'::tid;
618-
?column?
619-
----------
620-
f
621-
(1 row)
622-
623-
SELECT '(3, 3)'::tid <> '(3, 4)'::tid;
624-
?column?
625-
----------
626-
t
627-
(1 row)
628-
604+
-- Test for successful cleanup of an aborted transaction at session exit.
605+
-- THIS MUST BE THE LAST TEST IN THIS FILE.
606+
begin;
607+
select 1/0;
608+
ERROR: division by zero
609+
rollback to X;
610+
ERROR: no such savepoint
611+
-- DO NOT ADD ANYTHING HERE.

‎src/test/regress/sql/transactions.sql

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -368,8 +368,11 @@ fetch from foo;
368368

369369
abort;
370370

371-
-- tests for the "tid" type
372-
SELECT'(3, 3)'::tid='(3, 4)'::tid;
373-
SELECT'(3, 3)'::tid='(3, 3)'::tid;
374-
SELECT'(3, 3)'::tid<>'(3, 3)'::tid;
375-
SELECT'(3, 3)'::tid<>'(3, 4)'::tid;
371+
-- Test for successful cleanup of an aborted transaction at session exit.
372+
-- THIS MUST BE THE LAST TEST IN THIS FILE.
373+
374+
begin;
375+
select1/0;
376+
rollback to X;
377+
378+
-- DO NOT ADD ANYTHING HERE.

0 commit comments

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