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.

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 594a2fcca1ac7db9c8a136f26c5563383245b146..25d5048ed82eadc55ad188426ff0ec3d46ce35bc 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/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();

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 11abf52b87078cb2b09d3dfb7241e0e0cacbefc2..e05eb5c7957e68c8d6f2d2bae57755dbcdd127af 100644 (file)
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/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;
 

diff --git a/src/backend/utils/resowner/README b/src/backend/utils/resowner/README
index 229a80a5a1e37afe3845c94e836e912c4970d146..474dc76c77584f7cf171683759b9398ff83173a8 100644 (file)
--- a/src/backend/utils/resowner/README
+++ b/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

diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index e3835971d91cd50057861956760561770dc36949..05bd9b5ab5cf818085d30ec91637b8d5b6b0e848 100644 (file)
--- a/src/backend/utils/resowner/resowner.c
+++ b/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 */
+               }
            }
        }
    }