Re: [HACKERS] Advisory locks seem rather broken

2012-05-30 Thread Robert Haas
On Fri, May 4, 2012 at 9:17 AM, Robert Haas wrote: > On Thu, May 3, 2012 at 5:18 PM, Tom Lane wrote: >> ... btw, it appears to me that the "fast path" patch has broken things >> rather badly in LockReleaseAll.  AFAICS it's not honoring either the >> lockmethodid restriction nor the allLocks restr

Re: [HACKERS] Advisory locks seem rather broken

2012-05-04 Thread Robert Haas
On Fri, May 4, 2012 at 10:21 AM, Tom Lane wrote: > Robert Haas writes: >> Originally, I thought that the patch should include some kind of >> accounting mechanism to prevent that from happening, where we'd keep >> track of the number of fast-path locks that were outstanding and make >> sure to ke

Re: [HACKERS] Advisory locks seem rather broken

2012-05-04 Thread Tom Lane
Robert Haas writes: > Originally, I thought that the patch should include some kind of > accounting mechanism to prevent that from happening, where we'd keep > track of the number of fast-path locks that were outstanding and make > sure to keep that many slots free in the main lock table, but Noah

Re: [HACKERS] Advisory locks seem rather broken

2012-05-04 Thread Robert Haas
On Fri, May 4, 2012 at 9:25 AM, Tom Lane wrote: > Robert Haas writes: >> In 9.1, we just did this: > >>                 if (locallock->proclock == NULL || locallock->lock == NULL) >>                 { >>                         /* >>                          * We must've run out of shared memory

Re: [HACKERS] Advisory locks seem rather broken

2012-05-04 Thread Tom Lane
Robert Haas writes: > In 9.1, we just did this: > if (locallock->proclock == NULL || locallock->lock == NULL) > { > /* > * We must've run out of shared memory while > trying to set up this >

Re: [HACKERS] Advisory locks seem rather broken

2012-05-04 Thread Robert Haas
On Thu, May 3, 2012 at 5:18 PM, Tom Lane wrote: > ... btw, it appears to me that the "fast path" patch has broken things > rather badly in LockReleaseAll.  AFAICS it's not honoring either the > lockmethodid restriction nor the allLocks restriction with respect to > fastpath locks.  Perhaps user lo

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
... btw, it appears to me that the "fast path" patch has broken things rather badly in LockReleaseAll. AFAICS it's not honoring either the lockmethodid restriction nor the allLocks restriction with respect to fastpath locks. Perhaps user locks and session locks are never taken fast path, but stil

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
Robert Haas writes: > On Thu, May 3, 2012 at 3:52 PM, Tom Lane wrote: >> I'm inclined to say that you can PREPARE if your session holds a given >> advisory lock at either session or transaction level, but not both. >> This is a bit annoying but doesn't seem likely to be a real problem in >> pract

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Robert Haas
On Thu, May 3, 2012 at 3:52 PM, Tom Lane wrote: > I'm inclined to say that you can PREPARE if your session holds a given > advisory lock at either session or transaction level, but not both. > This is a bit annoying but doesn't seem likely to be a real problem in > practice, so thinking of a hack

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
I wrote: > Robert Haas writes: >> In fact I'm a bit confused by the original complaint for the same >> reason - if LockRelationOid and LockRelationIdForSession can coexist, >> why doesn't the same thing work for advisory locks? > The problem (or problems) is bad implementation, not the specificat

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Andres Freund
On Thursday, May 03, 2012 06:12:04 PM Simon Riggs wrote: > AFAICS you'd either use transactional or session level, but to use > both seems bizarre. And if you really did need both, you can put a > wrapper around the function to check whether a session level exists > before you grant the transaction

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
Robert Haas writes: > On Thu, May 3, 2012 at 12:12 PM, Simon Riggs wrote: >> AFAICS you'd either use transactional or session level, but to use >> both seems bizarre. > I'm a bit confused by all this, because we use both transaction and > session level locks internally - on the same lock tags -

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Robert Haas
On Thu, May 3, 2012 at 12:12 PM, Simon Riggs wrote: > AFAICS you'd either use transactional or session level, but to use > both seems bizarre. I'm a bit confused by all this, because we use both transaction and session level locks internally - on the same lock tags - so I don't know why we think

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Josh Berkus
> AFAICS you'd either use transactional or session level, but to use > both seems bizarre. And if you really did need both, you can put a > wrapper around the function to check whether a session level exists > before you grant the transaction level lock, or vice versa. You wouldn't want to *inten

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
Merlin Moncure writes: > On Thu, May 3, 2012 at 11:04 AM, Tom Lane wrote: >> I'm inclined to think that a saner implementation would involve >> splitting the userlock lockmethod into two, one transactional and one >> not. > hm, would that be exposed through the pg_locks view? some users might >

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
Simon Riggs writes: > On Thu, May 3, 2012 at 5:04 PM, Tom Lane wrote: >> That gets rid of the when-to-release kluges, but instead we have >> to think of a way for two different lockmethods to share the same >> lock keyspace. If we don't split it then we definitely need to figure >> out someplace

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Simon Riggs
On Thu, May 3, 2012 at 5:04 PM, Tom Lane wrote: > I'm inclined to think that a saner implementation would involve > splitting the userlock lockmethod into two, one transactional and one > not. Agreed > That gets rid of the when-to-release kluges, but instead we have > to think of a way for two

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Merlin Moncure
On Thu, May 3, 2012 at 11:04 AM, Tom Lane wrote: > I'm inclined to think that a saner implementation would involve > splitting the userlock lockmethod into two, one transactional and one > not.  That gets rid of the when-to-release kluges, but instead we have > to think of a way for two different

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Tom Lane
Simon Riggs writes: > On Thu, May 3, 2012 at 1:19 AM, Tom Lane wrote: >> If this patch weren't already in a released branch I would be arguing >> for reverting it.  As is, I think we're going to have to clean it up. >> I don't have time to look at it in detail right now, though. > There was an a

Re: [HACKERS] Advisory locks seem rather broken

2012-05-03 Thread Simon Riggs
On Thu, May 3, 2012 at 1:19 AM, Tom Lane wrote: > According to > http://archives.postgresql.org/pgsql-general/2012-04/msg00374.php > advisory locks now cause problems for prepared transactions, which > ought to ignore them.  It appears to me that this got broken by > commit 62c7bd31c8878dd45c9b9b2

[HACKERS] Advisory locks seem rather broken

2012-05-02 Thread Tom Lane
According to http://archives.postgresql.org/pgsql-general/2012-04/msg00374.php advisory locks now cause problems for prepared transactions, which ought to ignore them. It appears to me that this got broken by commit 62c7bd31c8878dd45c9b9b2429ab7a12103f3590, which marked the userlock lock method as