Thread
-
SAVEPOINTs and COMMIT performance
Simon Riggs <simon@2ndquadrant.com> — 2010-07-20T18:11:47Z
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
-
Re: SAVEPOINTs and COMMIT performance
Bruce Momjian <bruce@momjian.us> — 2011-02-06T17:11:04Z
Did this ever get addressed? --------------------------------------------------------------------------- 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 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
-
Re: SAVEPOINTs and COMMIT performance
Simon Riggs <simon@2ndquadrant.com> — 2011-02-06T21:09:30Z
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
-
Re: SAVEPOINTs and COMMIT performance
Bruce Momjian <bruce@momjian.us> — 2011-03-11T12:18:27Z
What happened to this patch? --------------------------------------------------------------------------- Simon Riggs wrote: > 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 > [ Attachment, skipping... ] -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
-
Re: SAVEPOINTs and COMMIT performance
Robert Haas <robertmhaas@gmail.com> — 2011-03-11T12:53:34Z
On Fri, Mar 11, 2011 at 7:18 AM, Bruce Momjian <bruce@momjian.us> wrote: > What happened to this patch? I added it to the next CommitFest. It would be reasonably to apply it sooner, perhaps, but nobody's reviewed it. Want to volunteer? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: SAVEPOINTs and COMMIT performance
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-06T09:33:21Z
On 06.02.2011 23:09, Simon Riggs wrote: > 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. > @@ -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 */ We also get into this codepath at RELEASE SAVEPOINT, in which case it is wrong to not reassign the locks to the parent subtransaction. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -
Re: SAVEPOINTs and COMMIT performance
Simon Riggs <simon@2ndquadrant.com> — 2011-06-06T19:18:28Z
On Mon, Jun 6, 2011 at 10:33 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 06.02.2011 23:09, Simon Riggs wrote: >> >> 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. > >> @@ -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 */ > > We also get into this codepath at RELEASE SAVEPOINT, in which case it is > wrong to not reassign the locks to the parent subtransaction. Agreed. Thanks for the review. I'll change that. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: SAVEPOINTs and COMMIT performance
Simon Riggs <simon@2ndquadrant.com> — 2011-07-14T14:59:14Z
On Mon, Jun 6, 2011 at 10:33 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 06.02.2011 23:09, Simon Riggs wrote: >> >> 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. > >> @@ -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 */ > > We also get into this codepath at RELEASE SAVEPOINT, in which case it is > wrong to not reassign the locks to the parent subtransaction. Attached patch splits TBLOCK_SUBEND state into two new states: TBLOCK_SUBCOMMIT and TBLOCK_SUBRELEASE, so that we can do the right thing. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services