Revise ResourceOwner code to avoid accumulating ResourceOwner objects

for every command executed within a transaction.  For long transactions
this was a significant memory leak.  Instead, we can delete a portal's
or subtransaction's ResourceOwner immediately, if we physically transfer
the information about its locks up to the parent owner.  This does not
fully solve the leak problem; we need to do something about counting
multiple acquisitions of the same lock in order to fix it.  But it's a
necessary step along the way.

src/backend/access/transam/xact.c

@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.178 2004/08/11 04:07:15 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.179 2004/08/25 18:43:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1716,11 +1716,16 @@ CommitTransactionCommand(void)
 			/*
 			 * We were issued a RELEASE command, so we end the current
 			 * subtransaction and return to the parent transaction.
+			 *
+			 * Since RELEASE can exit multiple levels of subtransaction,
+			 * we must loop here until we get out of all SUBEND'ed levels.
 			 */
 		case TBLOCK_SUBEND:
-			CommitSubTransaction();
-			PopTransaction();
-			s = CurrentTransactionState;		/* changed by pop */
+			do {
+				CommitSubTransaction();
+				PopTransaction();
+				s = CurrentTransactionState; /* changed by pop */
+			} while (s->blockState == TBLOCK_SUBEND);
 			break;
 
 			/*
@@ -2411,9 +2416,10 @@ void
 ReleaseSavepoint(List *options)
 {
 	TransactionState	s = CurrentTransactionState;
-	TransactionState	target = s;
-	char			   *name = NULL;
+	TransactionState target,
+					 xact;
 	ListCell		   *cell;
+	char			   *name = NULL;
 
 	/*
 	 * Check valid block state transaction status.
@@ -2462,11 +2468,10 @@ ReleaseSavepoint(List *options)
 
 	Assert(PointerIsValid(name));
 
-	while (target != NULL)
+	for (target = s; PointerIsValid(target); target = target->parent)
 	{
 		if (PointerIsValid(target->name) && strcmp(target->name, name) == 0)
 			break;
-		target = target->parent;
 	}
 
 	if (!PointerIsValid(target))
@@ -2474,7 +2479,27 @@ ReleaseSavepoint(List *options)
 				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
 				 errmsg("no such savepoint")));
 
-	CommitTransactionToLevel(target->nestingLevel);
+	/* disallow crossing savepoint level boundaries */
+	if (target->savepointLevel != s->savepointLevel)
+		ereport(ERROR,
+				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
+				 errmsg("no such savepoint")));
+
+	/*
+	 * Mark "commit pending" all subtransactions up to the target
+	 * subtransaction.  The actual commits will happen when control
+	 * gets to CommitTransactionCommand.
+	 */
+	xact = CurrentTransactionState;
+	for (;;)
+	{
+		Assert(xact->blockState == TBLOCK_SUBINPROGRESS);
+		xact->blockState = TBLOCK_SUBEND;
+		if (xact == target)
+			break;
+		xact = xact->parent;
+		Assert(PointerIsValid(xact));
+	}
 }
 
 /*
@@ -2969,18 +2994,13 @@ CommitSubTransaction(void)
 
 	CallXactCallbacks(XACT_EVENT_COMMIT_SUB, s->parent->transactionIdData);
 
-	/*
-	 * Note that we just release the resource owner's resources and don't
-	 * delete it.  This is because locks are not actually released here.
-	 * The owner object continues to exist as a child of its parent owner
-	 * (namely my parent transaction's resource owner), and the locks
-	 * effectively become that owner object's responsibility.
-	 */
 	ResourceOwnerRelease(s->curTransactionOwner,
 						 RESOURCE_RELEASE_BEFORE_LOCKS,
 						 true, false);
 	AtEOSubXact_Inval(true);
-	/* we can skip the LOCKS phase */
+	ResourceOwnerRelease(s->curTransactionOwner,
+						 RESOURCE_RELEASE_LOCKS,
+						 true, false);
 	ResourceOwnerRelease(s->curTransactionOwner,
 						 RESOURCE_RELEASE_AFTER_LOCKS,
 						 true, false);
@@ -3003,6 +3023,7 @@ CommitSubTransaction(void)
 
 	CurrentResourceOwner = s->parent->curTransactionOwner;
 	CurTransactionResourceOwner = s->parent->curTransactionOwner;
+	ResourceOwnerDelete(s->curTransactionOwner);
 	s->curTransactionOwner = NULL;
 
 	AtSubCommit_Memory();

src/backend/utils/mmgr/portalmem.c

@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.68 2004/08/02 21:42:18 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.69 2004/08/25 18:43:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -354,8 +354,7 @@ PortalDrop(Portal portal, bool isTopCommit)
 		ResourceOwnerRelease(portal->resowner,
 							 RESOURCE_RELEASE_AFTER_LOCKS,
 							 isCommit, false);
-		if (!isCommit)
-			ResourceOwnerDelete(portal->resowner);
+		ResourceOwnerDelete(portal->resowner);
 	}
 	portal->resowner = NULL;
 

src/backend/utils/resowner/README

@@ -1,4 +1,4 @@
-$PostgreSQL: pgsql/src/backend/utils/resowner/README,v 1.2 2004/07/31 00:45:40 tgl Exp $
+$PostgreSQL: pgsql/src/backend/utils/resowner/README,v 1.3 2004/08/25 18:43:43 tgl Exp $
 
 Notes about resource owners
 ---------------------------
@@ -30,8 +30,9 @@ ownership of the acquired resources in that ResourceOwner object.
 When a Portal is closed, any remaining resources (typically only locks)
 become the responsibility of the current transaction.  This is represented
 by making the Portal's ResourceOwner a child of the current transaction's
-ResourceOwner.  Similarly, subtransaction ResourceOwners are children of
-their immediate parent.
+ResourceOwner.  resowner.c automatically transfers the resources to the
+parent object when releasing the child.  Similarly, subtransaction
+ResourceOwners are children of their immediate parent.
 
 We need transaction-related ResourceOwners as well as Portal-related ones
 because transactions may initiate operations that require resources (such
@@ -53,6 +54,12 @@ The basic operations on a ResourceOwner are:
 * delete a ResourceOwner (including child owner objects); all resources
   must have been released beforehand
 
+Locks are handled specially because in non-error situations a lock should
+be held until end of transaction, even if it was originally taken by a
+subtransaction or portal.  Therefore, the "release" operation on a child
+ResourceOwner transfers lock ownership to the parent instead of actually
+releasing the lock, if isCommit is true.
+
 Currently, ResourceOwners contain direct support for recording ownership
 of buffer pins, lmgr locks, and catcache and relcache references.  Other
 objects can be associated with a ResourceOwner by recording the address of

src/backend/utils/resowner/resowner.c

@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.2 2004/07/31 00:45:40 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.3 2004/08/25 18:43:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -277,25 +277,40 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
 			/* Mark object as holding no locks, just for sanity */
 			owner->nlocks = 0;
 		}
-		else if (!isCommit)
+		else
 		{
 			/*
 			 * Release locks retail.  Note that LockRelease will remove
 			 * the lock entry from my list, so I just have to iterate till
 			 * there are none.  Also note that if we are committing a
-			 * subtransaction, we do NOT release its locks yet.
+			 * subtransaction, we do NOT release its locks yet, but transfer
+			 * them to the parent.
 			 *
 			 * XXX as above, this is a bit inefficient but probably not worth
 			 * the trouble to optimize more.
 			 */
+			Assert(owner->parent != NULL);
 			while (owner->nlocks > 0)
 			{
 				LockIdData *lockid = &owner->locks[owner->nlocks - 1];
 
-				LockRelease(lockid->locktag.lockmethodid,
-							&lockid->locktag,
-							lockid->xid,
-							lockid->lockmode);
+				if (isCommit)
+				{
+					ResourceOwnerEnlargeLocks(owner->parent);
+					ResourceOwnerRememberLock(owner->parent,
+											  &lockid->locktag,
+											  lockid->xid,
+											  lockid->lockmode);
+					owner->nlocks--;
+				}
+				else
+				{
+					LockRelease(lockid->locktag.lockmethodid,
+								&lockid->locktag,
+								lockid->xid,
+								lockid->lockmode);
+					/* LockRelease will have removed the entry from list */
+				}
 			}
 		}
 	}