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

Commit4bfe68d

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 parentedec8c8 commit4bfe68d

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
@@ -607,7 +607,7 @@ PortalStart(Portal portal, ParamListInfo params, bool use_active_snapshot)
607607
PG_CATCH();
608608
{
609609
/* Uncaught error while executing portal: mark it dead */
610-
portal->status=PORTAL_FAILED;
610+
MarkPortalFailed(portal);
611611

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

834834
/* Restore global vars and propagate error */
835835
if (saveMemoryContext==saveTopTransactionContext)
@@ -1446,7 +1446,7 @@ PortalRunFetch(Portal portal,
14461446
PG_CATCH();
14471447
{
14481448
/* Uncaught error while executing portal: mark it dead */
1449-
portal->status=PORTAL_FAILED;
1449+
MarkPortalFailed(portal);
14501450

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

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

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

714747
/* Any portal that was actually running has to be considered broken */
715748
if (portal->status==PORTAL_ACTIVE)
716-
portal->status=PORTAL_FAILED;
749+
MarkPortalFailed(portal);
717750

718751
/*
719752
* Do nothing else to cursors held over from a previous transaction.
@@ -728,9 +761,12 @@ AtAbort_Portals(void)
728761
* AtSubAbort_Portals.
729762
*/
730763
if (portal->status==PORTAL_READY)
731-
portal->status=PORTAL_FAILED;
764+
MarkPortalFailed(portal);
732765

733-
/* let portalcmds.c clean up the state it knows about */
766+
/*
767+
* Allow portalcmds.c to clean up the state it knows about, if we
768+
* haven't already.
769+
*/
734770
if (PointerIsValid(portal->cleanup))
735771
{
736772
(*portal->cleanup) (portal);
@@ -862,9 +898,12 @@ AtSubAbort_Portals(SubTransactionId mySubid,
862898
*/
863899
if (portal->status==PORTAL_READY||
864900
portal->status==PORTAL_ACTIVE)
865-
portal->status=PORTAL_FAILED;
901+
MarkPortalFailed(portal);
866902

867-
/* let portalcmds.c clean up the state it knows about */
903+
/*
904+
* Allow portalcmds.c to clean up the state it knows about, if we
905+
* haven't already.
906+
*/
868907
if (PointerIsValid(portal->cleanup))
869908
{
870909
(*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