Re: [HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
On Thu, Jun 14, 2012 at 10:45 AM, Tom Lane wrote: > Robert Haas writes: >> Is RESOURCE_RELEASE_AFTER_LOCKS actually used for anything? Is it >> just for extensions? > > I'm too lazy to go look, but it certainly ought to be in use. > The idea is that that's the phase for post-lock-release cleanup, > and anything that can possibly be postponed till after releasing > locks certainly should be ... Oh, you're right. I missed the logic in ResourceOwnerReleaseInternal. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
Robert Haas writes: > Is RESOURCE_RELEASE_AFTER_LOCKS actually used for anything? Is it > just for extensions? I'm too lazy to go look, but it certainly ought to be in use. The idea is that that's the phase for post-lock-release cleanup, and anything that can possibly be postponed till after releasing locks certainly should be ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
On Sun, Jun 10, 2012 at 5:37 PM, Tom Lane wrote: > Robert Haas writes: >> On Sun, Jun 10, 2012 at 4:19 PM, Noah Misch wrote: >>> Agreed. We now have $OLD_SUBJECT, but this is a win independently. I have >>> reviewed the code that runs between the old and new call sites, and I did >>> not >>> identify a hazard of moving it as you describe. > >> I looked at this when we last discussed it and didn't see a problem >> either, so I tend to agree that we ought to go ahead and do this, > > +1, as long as you mean in 9.3 not 9.2. I don't see any risk either, > but the time for taking new risks in 9.2 is past. > > Noah, please add this patch to the upcoming CF, if you didn't already. I re-reviewed this and committed it. Is RESOURCE_RELEASE_AFTER_LOCKS actually used for anything? Is it just for extensions? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
Robert Haas writes: > On Sun, Jun 10, 2012 at 4:19 PM, Noah Misch wrote: >> Agreed. We now have $OLD_SUBJECT, but this is a win independently. I have >> reviewed the code that runs between the old and new call sites, and I did not >> identify a hazard of moving it as you describe. > I looked at this when we last discussed it and didn't see a problem > either, so I tend to agree that we ought to go ahead and do this, +1, as long as you mean in 9.3 not 9.2. I don't see any risk either, but the time for taking new risks in 9.2 is past. Noah, please add this patch to the upcoming CF, if you didn't already. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
On Sun, Jun 10, 2012 at 4:19 PM, Noah Misch wrote: > On Wed, Aug 24, 2011 at 03:38:09PM -0400, Tom Lane wrote: >> Merlin Moncure writes: >> > On Wed, Aug 24, 2011 at 1:24 PM, Daniel Farina wrote: >> >> At Heroku we use CREATE INDEX CONCURRENTLY with great success, but >> >> recently when frobbing around some indexes I realized that there is no >> >> equivalent for DROP INDEX, and this is a similar but lesser problem >> >> (as CREATE INDEX takes much longer), as DROP INDEX takes an ACCESS >> >> EXCLUSIVE lock on the parent table while doing the work to unlink >> >> files, which nominally one would think to be trivial, but I assure you >> >> it is not at times for even indexes that are a handful of gigabytes >> >> (let's say ~=< a dozen). >> >> > Are you sure that you are really waiting on the time to unlink the >> > file? there's other stuff going on in there like waiting for lock, >> > plan invalidation, etc. Point being, maybe the time consuming stuff >> > can't really be deferred which would make the proposal moot. >> >> Assuming the issue really is the physical unlinks (which I agree I'd >> like to see some evidence for), I wonder whether the problem could be > > I'd believe it. With a cold cache (sudo sysctl -w vm.drop_caches=3), my > desktop takes 2.6s to "rm" a 985 MiB file. > >> addressed by moving smgrDoPendingDeletes() to after locks are released, >> instead of before, in CommitTransaction/AbortTransaction. There does >> not seem to be any strong reason why we have to do that before lock >> release, since incoming potential users of a table should not be trying >> to access the old physical storage after that anyway. > > Agreed. We now have $OLD_SUBJECT, but this is a win independently. I have > reviewed the code that runs between the old and new call sites, and I did not > identify a hazard of moving it as you describe. I looked at this when we last discussed it and didn't see a problem either, so I tend to agree that we ought to go ahead and do this, unless someone else sees a problem. Holding locks for even slightly less time is a good idea, and if it turns out to be substantially less time in some cases, then we win more. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] unlink for DROPs after releasing locks (was Re: Should I implement DROP INDEX CONCURRENTLY?)
On Wed, Aug 24, 2011 at 03:38:09PM -0400, Tom Lane wrote: > Merlin Moncure writes: > > On Wed, Aug 24, 2011 at 1:24 PM, Daniel Farina wrote: > >> At Heroku we use CREATE INDEX CONCURRENTLY with great success, but > >> recently when frobbing around some indexes I realized that there is no > >> equivalent for DROP INDEX, and this is a similar but lesser problem > >> (as CREATE INDEX takes much longer), as DROP INDEX takes an ACCESS > >> EXCLUSIVE lock on the parent table while doing the work to unlink > >> files, which nominally one would think to be trivial, but I assure you > >> it is not at times for even indexes that are a handful of gigabytes > >> (let's say ~=< a dozen). > > > Are you sure that you are really waiting on the time to unlink the > > file? there's other stuff going on in there like waiting for lock, > > plan invalidation, etc. Point being, maybe the time consuming stuff > > can't really be deferred which would make the proposal moot. > > Assuming the issue really is the physical unlinks (which I agree I'd > like to see some evidence for), I wonder whether the problem could be I'd believe it. With a cold cache (sudo sysctl -w vm.drop_caches=3), my desktop takes 2.6s to "rm" a 985 MiB file. > addressed by moving smgrDoPendingDeletes() to after locks are released, > instead of before, in CommitTransaction/AbortTransaction. There does > not seem to be any strong reason why we have to do that before lock > release, since incoming potential users of a table should not be trying > to access the old physical storage after that anyway. Agreed. We now have $OLD_SUBJECT, but this is a win independently. I have reviewed the code that runs between the old and new call sites, and I did not identify a hazard of moving it as you describe. nm diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 8f00186..8e4a455 100644 *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *** *** 1944,1957 CommitTransaction(void) */ AtEOXact_Inval(true); - /* -* Likewise, dropping of files deleted during the transaction is best done -* after releasing relcache and buffer pins. (This is not strictly -* necessary during commit, since such pins should have been released -* already, but this ordering is definitely critical during abort.) -*/ - smgrDoPendingDeletes(true); - AtEOXact_MultiXact(); ResourceOwnerRelease(TopTransactionResourceOwner, --- 1944,1949 *** *** 1961,1966 CommitTransaction(void) --- 1953,1969 RESOURCE_RELEASE_AFTER_LOCKS, true, true); + /* +* Likewise, dropping of files deleted during the transaction is best done +* after releasing relcache and buffer pins. (This is not strictly +* necessary during commit, since such pins should have been released +* already, but this ordering is definitely critical during abort.) Since +* this may take many seconds, also delay until after releasing locks. +* Other backends will observe the attendant catalog changes and not +* attempt to access affected files. +*/ + smgrDoPendingDeletes(true); + /* Check we've released all catcache entries */ AtEOXact_CatCache(true); *** *** 2354,2360 AbortTransaction(void) AtEOXact_Buffers(false); AtEOXact_RelationCache(false); AtEOXact_Inval(false); - smgrDoPendingDeletes(false); AtEOXact_MultiXact(); ResourceOwnerRelease(TopTransactionResourceOwner, RESOURCE_RELEASE_LOCKS, --- 2357,2362 *** *** 2362,2367 AbortTransaction(void) --- 2364,2370 ResourceOwnerRelease(TopTransactionResourceOwner, RESOURCE_RELEASE_AFTER_LOCKS, false, true); + smgrDoPendingDeletes(false); AtEOXact_CatCache(false); AtEOXact_GUC(false, 1); *** *** 4238,4250 AbortSubTransaction(void) AtEOSubXact_RelationCache(false, s->subTransactionId, s->parent->subTransactionId); AtEOSubXact_Inval(false); - AtSubAbort_smgr(); ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_LOCKS, false, false); ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_AFTER_LO
