From fe455ee1d4cc5d400c345f361d4bfbfc1178231c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 25 Aug 2004 18:43:43 +0000 Subject: [PATCH] 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 | 55 ++++++++++++++++++--------- src/backend/utils/mmgr/portalmem.c | 5 +-- src/backend/utils/resowner/README | 13 +++++-- src/backend/utils/resowner/resowner.c | 29 ++++++++++---- 4 files changed, 72 insertions(+), 30 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 594a2fcca1..25d5048ed8 100644 --- 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 11abf52b87..e05eb5c795 100644 --- 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 229a80a5a1..474dc76c77 100644 --- 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 e3835971d9..05bd9b5ab5 100644 --- 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 */ + } } } }