On Sun, 2011-02-06 at 12:11 -0500, Bruce Momjian wrote:
> Did this ever get addressed?
Patch attached.
Seems like the easiest fix I can come up with.
> Simon Riggs wrote:
> >
> > As part of a performance investigation for a customer I've noticed an
> > O(N^2) performance issue on COMMITs of transactions that contain many
> > SAVEPOINTs. I've consistently measured COMMIT times of around 9 seconds,
> > with 49% CPU, mostly in LockReassignCurrentOwner().
> >
> > BEGIN;
> > INSERT...
> > SAVEPOINT ...
> > INSERT...
> > SAVEPOINT ...
> > ... (repeat 10,000 times)
> > COMMIT;
> >
> > The way SAVEPOINTs work is that each is nested within the previous one,
> > so that at COMMIT time we must recursively commit all the
> > subtransactions before we issue final commit.
> >
> > That's a shame because ResourceOwnerReleaseInternal() contains an
> > optimisation to speed up final commit, by calling ProcReleaseLocks().
> >
> > What we actually do is recursively call LockReassignCurrentOwner() which
> > sequentially scans LockMethodLocalHash at each level of transaction. The
> > comments refer to this as "retail" rather than the wholesale method,
> > which never gets to execute anything worthwhile in this case.
> >
> > This issue does NOT occur in PLpgSQL functions that contain many
> > EXCEPTION clauses in a loop, since in that case the subtransactions are
> > started and committed from the top level so that the subxact nesting
> > never goes too deep.
> >
> > Fix looks like we need special handling for the depth-first case, rather
> > than just a recursion loop in CommitTransactionCommand().
> >
> > Issues looks like it goes all the way back, no fix for 9.0.
> >
> > I notice also that the nesting model of SAVEPOINTs also means that
> > read-only subtransactions will still generate an xid when followed by a
> > DML statement. That's unnecessary, but required given current design.
> >
> > --
> > Simon Riggs www.2ndQuadrant.com
> > PostgreSQL Development, 24x7 Support, Training and Services
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 18e9ce1..07c84b4 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -269,7 +269,7 @@ static TransactionId RecordTransactionAbort(bool isSubXact);
static void StartTransaction(void);
static void StartSubTransaction(void);
-static void CommitSubTransaction(void);
+static void CommitSubTransaction(bool isTopLevel);
static void AbortSubTransaction(void);
static void CleanupSubTransaction(void);
static void PushTransaction(void);
@@ -2518,7 +2518,7 @@ CommitTransactionCommand(void)
case TBLOCK_SUBEND:
do
{
- CommitSubTransaction();
+ CommitSubTransaction(true);
s = CurrentTransactionState; /* changed by pop */
} while (s->blockState == TBLOCK_SUBEND);
/* If we had a COMMIT command, finish off the main xact too */
@@ -3664,7 +3664,7 @@ ReleaseCurrentSubTransaction(void)
BlockStateAsString(s->blockState));
Assert(s->state == TRANS_INPROGRESS);
MemoryContextSwitchTo(CurTransactionContext);
- CommitSubTransaction();
+ CommitSubTransaction(false);
s = CurrentTransactionState; /* changed by pop */
Assert(s->state == TRANS_INPROGRESS);
}
@@ -3925,9 +3925,13 @@ StartSubTransaction(void)
*
* The caller has to make sure to always reassign CurrentTransactionState
* if it has a local pointer to it after calling this function.
+ *
+ * isTopLevel means that this CommitSubTransaction() is being issued as a
+ * sequence of actions leading directly to a main transaction commit
+ * allowing some actions to be optimised.
*/
static void
-CommitSubTransaction(void)
+CommitSubTransaction(bool isTopLevel)
{
TransactionState s = CurrentTransactionState;
@@ -3974,15 +3978,21 @@ CommitSubTransaction(void)
/*
* The only lock we actually release here is the subtransaction XID lock.
- * The rest just get transferred to the parent resource owner.
*/
CurrentResourceOwner = s->curTransactionOwner;
if (TransactionIdIsValid(s->transactionId))
XactLockTableDelete(s->transactionId);
+ /*
+ * Other locks should get transferred to their parent resource owner.
+ * Doing that is an O(N^2) operation, so if isTopLevel then we can just
+ * leave the lock records as they are, knowing they will all get released
+ * by the top level commit using ProcReleaseLocks(). We only optimize
+ * this for commit; aborts may need to do other cleanup.
+ */
ResourceOwnerRelease(s->curTransactionOwner,
RESOURCE_RELEASE_LOCKS,
- true, false);
+ true, isTopLevel);
ResourceOwnerRelease(s->curTransactionOwner,
RESOURCE_RELEASE_AFTER_LOCKS,
true, false);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
