Run a portal's cleanup hook immediately when pushing it to FAILED state.

tglsfdc · tglsfdc · commit 4bfe68dfab00 · 2012-02-15T16:19:01.000-05:00
This extends the changes of commit 6252c4f so that we run the cleanup hook earlier for failure cases as well as success cases. As before, the point is to avoid an assertion failure from an Assert I added in commit a874fe7, which was meant to check that no user-written code can be called during portal cleanup. This fixes a case reported by Pavan Deolasee in which the Assert could be triggered during backend exit (see the new regression test case), and also prevents the possibility that the cleanup hook is run after portions of the portal's state have already been recycled. That doesn't really matter in current usage, but it foreseeably could matter in the future. Back-patch to 9.1 where the Assert in question was added.
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
@@ -224,7 +224,7 @@ PerformPortalClose(const char *name)
 	}
 
 	/*
-	 * Note: PortalCleanup is called as a side-effect
+	 * Note: PortalCleanup is called as a side-effect, if not already done.
 	 */
 	PortalDrop(portal, false);
 }
@@ -234,6 +234,10 @@ PerformPortalClose(const char *name)
  *
  * Clean up a portal when it's dropped.  This is the standard cleanup hook
  * for portals.
+ *
+ * Note: if portal->status is PORTAL_FAILED, we are probably being called
+ * during error abort, and must be careful to avoid doing anything that
+ * is likely to fail again.
  */
 void
 PortalCleanup(Portal portal)
@@ -420,7 +424,7 @@ PersistHoldablePortal(Portal portal)
 	PG_CATCH();
 	{
 		/* Uncaught error while executing portal: mark it dead */
-		portal->status = PORTAL_FAILED;
+		MarkPortalFailed(portal);
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
@@ -607,7 +607,7 @@ PortalStart(Portal portal, ParamListInfo params, bool use_active_snapshot)
 	PG_CATCH();
 	{
 		/* Uncaught error while executing portal: mark it dead */
-		portal->status = PORTAL_FAILED;
+		MarkPortalFailed(portal);
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
@@ -829,7 +829,7 @@ PortalRun(Portal portal, long count, bool isTopLevel,
 	PG_CATCH();
 	{
 		/* Uncaught error while executing portal: mark it dead */
-		portal->status = PORTAL_FAILED;
+		MarkPortalFailed(portal);
 
 		/* Restore global vars and propagate error */
 		if (saveMemoryContext == saveTopTransactionContext)
@@ -1446,7 +1446,7 @@ PortalRunFetch(Portal portal,
 	PG_CATCH();
 	{
 		/* Uncaught error while executing portal: mark it dead */
-		portal->status = PORTAL_FAILED;
+		MarkPortalFailed(portal);
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
@@ -405,6 +405,8 @@ UnpinPortal(Portal portal)
 /*
  * MarkPortalDone
  *		Transition a portal from ACTIVE to DONE state.
+ *
+ * NOTE: never set portal->status = PORTAL_DONE directly; call this instead.
  */
 void
 MarkPortalDone(Portal portal)
@@ -418,8 +420,36 @@ MarkPortalDone(Portal portal)
 	 * well do that now, since the portal can't be executed any more.
 	 *
 	 * In some cases involving execution of a ROLLBACK command in an already
-	 * aborted transaction, this prevents an assertion failure from reaching
-	 * AtCleanup_Portals with the cleanup hook still unexecuted.
+	 * aborted transaction, this prevents an assertion failure caused by
+	 * reaching AtCleanup_Portals with the cleanup hook still unexecuted.
+	 */
+	if (PointerIsValid(portal->cleanup))
+	{
+		(*portal->cleanup) (portal);
+		portal->cleanup = NULL;
+	}
+}
+
+/*
+ * MarkPortalFailed
+ *		Transition a portal into FAILED state.
+ *
+ * NOTE: never set portal->status = PORTAL_FAILED directly; call this instead.
+ */
+void
+MarkPortalFailed(Portal portal)
+{
+	/* Perform the state transition */
+	Assert(portal->status != PORTAL_DONE);
+	portal->status = PORTAL_FAILED;
+
+	/*
+	 * Allow portalcmds.c to clean up the state it knows about.  We might as
+	 * well do that now, since the portal can't be executed any more.
+	 *
+	 * In some cases involving cleanup of an already aborted transaction, this
+	 * prevents an assertion failure caused by reaching AtCleanup_Portals with
+	 * the cleanup hook still unexecuted.
 	 */
 	if (PointerIsValid(portal->cleanup))
 	{
@@ -455,6 +485,9 @@ PortalDrop(Portal portal, bool isTopCommit)
 	 * hook's responsibility to not try to do that more than once, in the case
 	 * that failure occurs and then we come back to drop the portal again
 	 * during transaction abort.
+	 *
+	 * Note: in most paths of control, this will have been done already in
+	 * MarkPortalDone or MarkPortalFailed.  We're just making sure.
 	 */
 	if (PointerIsValid(portal->cleanup))
 	{
@@ -713,7 +746,7 @@ AtAbort_Portals(void)
 
 		/* Any portal that was actually running has to be considered broken */
 		if (portal->status == PORTAL_ACTIVE)
-			portal->status = PORTAL_FAILED;
+			MarkPortalFailed(portal);
 
 		/*
 		 * Do nothing else to cursors held over from a previous transaction.
@@ -728,9 +761,12 @@ AtAbort_Portals(void)
 		 * AtSubAbort_Portals.
 		 */
 		if (portal->status == PORTAL_READY)
-			portal->status = PORTAL_FAILED;
+			MarkPortalFailed(portal);
 
-		/* let portalcmds.c clean up the state it knows about */
+		/*
+		 * Allow portalcmds.c to clean up the state it knows about, if we
+		 * haven't already.
+		 */
 		if (PointerIsValid(portal->cleanup))
 		{
 			(*portal->cleanup) (portal);
@@ -862,9 +898,12 @@ AtSubAbort_Portals(SubTransactionId mySubid,
 		 */
 		if (portal->status == PORTAL_READY ||
 			portal->status == PORTAL_ACTIVE)
-			portal->status = PORTAL_FAILED;
+			MarkPortalFailed(portal);
 
-		/* let portalcmds.c clean up the state it knows about */
+		/*
+		 * Allow portalcmds.c to clean up the state it knows about, if we
+		 * haven't already.
+		 */
 		if (PointerIsValid(portal->cleanup))
 		{
 			(*portal->cleanup) (portal);
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
@@ -207,6 +207,7 @@ extern Portal CreateNewPortal(void);
 extern void PinPortal(Portal portal);
 extern void UnpinPortal(Portal portal);
 extern void MarkPortalDone(Portal portal);
+extern void MarkPortalFailed(Portal portal);
 extern void PortalDrop(Portal portal, bool isTopCommit);
 extern Portal GetPortalByName(const char *name);
 extern void PortalDefineQuery(Portal portal,
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
@@ -601,28 +601,11 @@ fetch from foo;
 (1 row)
 
 abort;
--- tests for the "tid" type
-SELECT '(3, 3)'::tid = '(3, 4)'::tid;
- ?column? 
-----------
- f
-(1 row)
-
-SELECT '(3, 3)'::tid = '(3, 3)'::tid;
- ?column? 
-----------
- t
-(1 row)
-
-SELECT '(3, 3)'::tid <> '(3, 3)'::tid;
- ?column? 
-----------
- f
-(1 row)
-
-SELECT '(3, 3)'::tid <> '(3, 4)'::tid;
- ?column? 
-----------
- t
-(1 row)
-
+-- Test for successful cleanup of an aborted transaction at session exit.
+-- THIS MUST BE THE LAST TEST IN THIS FILE.
+begin;
+select 1/0;
+ERROR:  division by zero
+rollback to X;
+ERROR:  no such savepoint
+-- DO NOT ADD ANYTHING HERE.
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
@@ -368,8 +368,11 @@ fetch from foo;
 
 abort;
 
--- tests for the "tid" type
-SELECT '(3, 3)'::tid = '(3, 4)'::tid;
-SELECT '(3, 3)'::tid = '(3, 3)'::tid;
-SELECT '(3, 3)'::tid <> '(3, 3)'::tid;
-SELECT '(3, 3)'::tid <> '(3, 4)'::tid;
+-- Test for successful cleanup of an aborted transaction at session exit.
+-- THIS MUST BE THE LAST TEST IN THIS FILE.
+
+begin;
+select 1/0;
+rollback to X;
+
+-- DO NOT ADD ANYTHING HERE.
