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 */
+                               }
                        }
                }
        }