Thread

Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-13T22:52:28Z

Hi,

I often find myself wanting advisory locks that are automatically 
released when the transaction ends, so here's a small patch trying to do 
just that.  I don't know much about the lock system so the patch is in 
the state "it looks like this would work".  Any comments on the 
technical details are welcome.  There's obviously a lot of documentation 
and READMEs to change too, but I thought I'd see what people think about 
the idea before going there.

So, thoughts?


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Szymon Lipiński <mabewlun@gmail.com> — 2010-12-13T23:08:31Z

On 13 December 2010 23:52, Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi>wrote:

> Hi,
>
> I often find myself wanting advisory locks that are automatically released
> when the transaction ends, so here's a small patch trying to do just that.
>  I don't know much about the lock system so the patch is in the state "it
> looks like this would work".  Any comments on the technical details are
> welcome.  There's obviously a lot of documentation and READMEs to change
> too, but I thought I'd see what people think about the idea before going
> there.
>
> So, thoughts?
>
>
>
In my opinion changing current behavior is not a good idea. I know some
software that relies on current behavior and this would break it. Maybe add
that as an option, or add another type of advisory lock?

regards
Szymon

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-13T23:14:22Z

On 2010-12-14 1:08 AM +0200, Szymon Guz wrote:
> On 13 December 2010 23:52, Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>wrote:
>> So, thoughts?
>>
> In my opinion changing current behavior is not a good idea. I know some
> software that relies on current behavior and this would break it. Maybe add
> that as an option, or add another type of advisory lock?

Oh, I forgot to mention.  The patch doesn't change any existing 
behaviour; the new behaviour can be invoked only by adding a new boolean 
argument:

SELECT pg_advisory_lock(1, false);

The lock space is the same though, but I don't feel strongly about it.

Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Simon Riggs <simon@2ndquadrant.com> — 2010-12-14T00:35:26Z

On Tue, 2010-12-14 at 01:14 +0200, Marko Tiikkaja wrote:
> On 2010-12-14 1:08 AM +0200, Szymon Guz wrote:
> > On 13 December 2010 23:52, Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>wrote:
> >> So, thoughts?
> >>
> > In my opinion changing current behavior is not a good idea. I know some
> > software that relies on current behavior and this would break it. Maybe add
> > that as an option, or add another type of advisory lock?
> 
> Oh, I forgot to mention.  The patch doesn't change any existing 
> behaviour; the new behaviour can be invoked only by adding a new boolean 
> argument:
> 
> SELECT pg_advisory_lock(1, false);

Don't like adding a boolean. Nobody remembers what it is for and we have
bugs. How about pg_advisory_xact_lock()

> The lock space is the same though, but I don't feel strongly about it.

Same lock space is good. Easy to separate if required.

Explicitly nameable lock spaces would be even better, since if multiple
applications use them you get strange and unmanageable contention.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Transaction-scope advisory locks

Andrew Dunstan <andrew@dunslane.net> — 2010-12-14T00:50:21Z

On 12/13/2010 07:35 PM, Simon Riggs wrote:
> Same lock space is good. Easy to separate if required.
>
> Explicitly nameable lock spaces would be even better, since if multiple
> applications use them you get strange and unmanageable contention.

Yeah. I have a table of lock names for different locks, and do stuff like:

    perform pg_advisory_lock(l.lockid, some_value)
    from my_advisory_locks l
    where l.lockname = 'my_lock_name';

I don't know that we need a separately nameable lockspace for 
transaction-scoped locks, though, do we?

cheers

andrew

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-14T00:51:05Z

On 2010-12-14 2:35 AM +0200, Simon Riggs wrote:
> On Tue, 2010-12-14 at 01:14 +0200, Marko Tiikkaja wrote:
>> Oh, I forgot to mention.  The patch doesn't change any existing
>> behaviour; the new behaviour can be invoked only by adding a new boolean
>> argument:
>>
>> SELECT pg_advisory_lock(1, false);
>
> Don't like adding a boolean. Nobody remembers what it is for and we have
> bugs. How about pg_advisory_xact_lock()

That's the other option I was thinking of, but didn't like that too 
much.  But you're right about the boolean, it is a bit hard to remember 
which behaviour is which.

>> The lock space is the same though, but I don't feel strongly about it.
>
> Same lock space is good. Easy to separate if required.
>
> Explicitly nameable lock spaces would be even better, since if multiple
> applications use them you get strange and unmanageable contention.

I think something like this has been suggested in the past, and was 
rejected at that time.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Josh Berkus <josh@agliodbs.com> — 2010-12-14T02:21:41Z

> Oh, I forgot to mention.  The patch doesn't change any existing
> behaviour; the new behaviour can be invoked only by adding a new boolean
> argument:
> 
> SELECT pg_advisory_lock(1, false);
> 
> The lock space is the same though, but I don't feel strongly about it.

I could use this, and I think a lot more people would use advisory locks
with it.  Put it in the next CF and remind me to test it.

-- 
                                  -- Josh Berkus
                                     PostgreSQL Experts Inc.
                                     http://www.pgexperts.com

Re: Transaction-scope advisory locks

Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-14T02:23:05Z

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2010-12-14 1:08 AM +0200, Szymon Guz wrote:
>> In my opinion changing current behavior is not a good idea. I know some
>> software that relies on current behavior and this would break it. Maybe add
>> that as an option, or add another type of advisory lock?

> Oh, I forgot to mention.  The patch doesn't change any existing 
> behaviour; the new behaviour can be invoked only by adding a new boolean 
> argument:

Uh, I don't think so.  It sure looks like you have changed the user
lockmethod to be transactional, ie, auto-release on commit/abort.  As
Szymon stated, that is an utter non-starter, because all current uses of
advisory locks consider the current behavior to be a feature not a bug.

			regards, tom lane

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-14T08:30:27Z

On 2010-12-14 4:23 AM +0200, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> On 2010-12-14 1:08 AM +0200, Szymon Guz wrote:
>>> In my opinion changing current behavior is not a good idea. I know some
>>> software that relies on current behavior and this would break it. Maybe add
>>> that as an option, or add another type of advisory lock?
>
>> Oh, I forgot to mention.  The patch doesn't change any existing
>> behaviour; the new behaviour can be invoked only by adding a new boolean
>> argument:
>
> Uh, I don't think so.  It sure looks like you have changed the user
> lockmethod to be transactional, ie, auto-release on commit/abort.

I was under the impression that passing sessionLock=true to 
LockAcquire(), combined with allLocks=false to LockReleaseAll() would be 
enough to prevent that from happening.  My tests seem to agree with this.

Am I missing something?


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2010-12-14T10:52:25Z

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> I often find myself wanting advisory locks that are automatically released
> when the transaction ends, so here's a small patch trying to do just that.

Excellent idea, I sure need that (been doing some pl stuff to track
locks granted then unlock them, transaction scope would mean pure SQL
function work). Thanks! :)

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Re: Transaction-scope advisory locks

Andres Freund <andres@anarazel.de> — 2010-12-14T12:07:09Z

On Tuesday 14 December 2010 00:14:22 Marko Tiikkaja wrote:
> The lock space is the same though, but I don't feel strongly about it.
I feel strongly that it needs the same locking space. I pretty frequently have 
the need for multiple clients trying to acquiring a lock in transaction scope 
(i.e. for accessing the cache) and one/few acquiring it in session scope (for 
building the cache).

Andres

Re: Transaction-scope advisory locks

Merlin Moncure <mmoncure@gmail.com> — 2010-12-14T14:19:32Z

On Tue, Dec 14, 2010 at 7:07 AM, Andres Freund <andres@anarazel.de> wrote:
> On Tuesday 14 December 2010 00:14:22 Marko Tiikkaja wrote:
>> The lock space is the same though, but I don't feel strongly about it.
> I feel strongly that it needs the same locking space. I pretty frequently have
> the need for multiple clients trying to acquiring a lock in transaction scope
> (i.e. for accessing the cache) and one/few acquiring it in session scope (for
> building the cache).

Not that I'm necessarily against the proposal, but what does this do
that can't already be done by locking a table or a table's row?

merlin

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-14T14:23:51Z

On 2010-12-14 4:19 PM +0200, Merlin Moncure wrote:
> On Tue, Dec 14, 2010 at 7:07 AM, Andres Freund<andres@anarazel.de>  wrote:
>> On Tuesday 14 December 2010 00:14:22 Marko Tiikkaja wrote:
>>> The lock space is the same though, but I don't feel strongly about it.
>> I feel strongly that it needs the same locking space. I pretty frequently have
>> the need for multiple clients trying to acquiring a lock in transaction scope
>> (i.e. for accessing the cache) and one/few acquiring it in session scope (for
>> building the cache).
>
> Not that I'm necessarily against the proposal, but what does this do
> that can't already be done by locking a table or a table's row?

Try without throwing an error.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Andres Freund <andres@anarazel.de> — 2010-12-14T14:30:37Z

On Tuesday 14 December 2010 15:19:32 Merlin Moncure wrote:
> On Tue, Dec 14, 2010 at 7:07 AM, Andres Freund <andres@anarazel.de> wrote:
> > On Tuesday 14 December 2010 00:14:22 Marko Tiikkaja wrote:
> >> The lock space is the same though, but I don't feel strongly about it.
> > 
> > I feel strongly that it needs the same locking space. I pretty frequently
> > have the need for multiple clients trying to acquiring a lock in
> > transaction scope (i.e. for accessing the cache) and one/few acquiring
> > it in session scope (for building the cache).
> 
> Not that I'm necessarily against the proposal, but what does this do
> that can't already be done by locking a table or a table's row?
1. trylock without raising errors (the other possibility is nowait, but that 
doesnt work very well as it ERRORs).

2. mixing session and transaction scope (I would like to have that e.g. for 
materialized views. The writers uses session scope and the readers use 
transaction scope. Its not that easy to make code ERROR/exception safe when 
you only control some view or such. In contrast the computationally expensive 
part of computing the materialized view should be way much more easy to do 
sensibly in session scope).

3. nonlocking dequeuing of a table-based queue can e.g. be done with advisory 
locks but not with row level locks.

Andres

Re: Transaction-scope advisory locks

Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-14T14:51:48Z

Merlin Moncure <mmoncure@gmail.com> writes:
> Not that I'm necessarily against the proposal, but what does this do
> that can't already be done by locking a table or a table's row?

I agree with Andres' point about this: sometimes it'd be more convenient
for an advisory lock to be released automatically at transaction end.
If you have a mix of clients that want that behavior with others that
want a persistent hold on the same locks, you can't do it with regular
locks.

			regards, tom lane

Re: Transaction-scope advisory locks

Andrew Dunstan <andrew@dunslane.net> — 2010-12-14T14:59:03Z


On 12/14/2010 09:51 AM, Tom Lane wrote:
> Merlin Moncure<mmoncure@gmail.com>  writes:
>> Not that I'm necessarily against the proposal, but what does this do
>> that can't already be done by locking a table or a table's row?
> I agree with Andres' point about this: sometimes it'd be more convenient
> for an advisory lock to be released automatically at transaction end.
> If you have a mix of clients that want that behavior with others that
> want a persistent hold on the same locks, you can't do it with regular
> locks.
>

Right. And that's why they need to be in the same lockspace.

cheers

andrew

Re: Transaction-scope advisory locks

Merlin Moncure <mmoncure@gmail.com> — 2010-12-14T15:06:31Z

On Tue, Dec 14, 2010 at 9:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> Not that I'm necessarily against the proposal, but what does this do
>> that can't already be done by locking a table or a table's row?
>
> I agree with Andres' point about this: sometimes it'd be more convenient
> for an advisory lock to be released automatically at transaction end.
> If you have a mix of clients that want that behavior with others that
> want a persistent hold on the same locks, you can't do it with regular
> locks.

right, plus 4:

automatic lock release on error.  right now if I'm grabbing
in-transaction lock inside a function, I have to put in sub
transaction handler to guarantee release if anything non trivial
happens mid lock.

merlin

Re: Transaction-scope advisory locks

Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-14T17:05:28Z

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2010-12-14 4:23 AM +0200, Tom Lane wrote:
>> Uh, I don't think so.  It sure looks like you have changed the user
>> lockmethod to be transactional, ie, auto-release on commit/abort.

> I was under the impression that passing sessionLock=true to 
> LockAcquire(), combined with allLocks=false to LockReleaseAll() would be 
> enough to prevent that from happening.  My tests seem to agree with this.

> Am I missing something?

All the places that look at LockMethodData->transactional ?

			regards, tom lane

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-12-14T21:05:53Z

On 2010-12-14 7:05 PM +0200, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> On 2010-12-14 4:23 AM +0200, Tom Lane wrote:
>>> Uh, I don't think so.  It sure looks like you have changed the user
>>> lockmethod to be transactional, ie, auto-release on commit/abort.
>
>> I was under the impression that passing sessionLock=true to
>> LockAcquire(), combined with allLocks=false to LockReleaseAll() would be
>> enough to prevent that from happening.  My tests seem to agree with this.
>
>> Am I missing something?
>
> All the places that look at LockMethodData->transactional ?

As far as I can tell, every code path that looks at 
LockMethodData->transactional either has an explicit sessionLock boolean 
or looks whether owner == NULL to actually check whether the lock in 
question is a session lock or not instead of blindly trusting 
->transactional.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-01-15T21:20:27Z

On 2010-12-14 12:52 AM +0200, Marko Tiikkaja wrote:
> <patch>

Here's the latest version of the patch.  It now uses the API proposed by 
Simon, but still lacks documentation changes, which I'm going to send 
tomorrow.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-17T07:28:10Z

On Sun, Jan 16, 2011 at 06:20, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> Here's the latest version of the patch.  It now uses the API proposed by
> Simon, but still lacks documentation changes, which I'm going to send
> tomorrow.

Here is a short review for Transaction scoped advisory locks:
https://commitfest.postgresql.org/action/patch_view?id=518

== Features ==
The patch adds pg_[try_]advisory_xact_lock[_shared] functions.
The function names follows the past discussion -- it's better than
"bool isXact" argument or changing the existing behavior.

== Coding ==
The patch itself is well-formed and be applied cleanly.
I expect documentation will come soon.
There is no regression test, but we have no regression test for
advisory locks even now. Tests for lock conflict might be difficult,
but we could have single-threaded test for lock/unlock and pg_locks view.

== Questions ==
I have a question about unlocking transaction-scope advisory locks.
We cannot unlock them with pg_advisory_unlock(), but can unlock with
pg_advisory_unlock_all(). It's inconsistent behavior.
Furthermore, I wonder we can allow unlocking transaction-scope locks
-- we have LOCK TABLE but don't have UNLOCK TABLE.

postgres=# BEGIN;
BEGIN
postgres=# SELECT pg_advisory_xact_lock(1);
 pg_advisory_xact_lock
-----------------------

(1 row)

postgres=# SELECT pg_advisory_unlock(1);
WARNING:  you don't own a lock of type ExclusiveLock
 pg_advisory_unlock
--------------------
 f
(1 row)

postgres=# SELECT pg_advisory_unlock_all();
 pg_advisory_unlock_all
------------------------

(1 row)

postgres=# ROLLBACK;
ROLLBACK

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-01-20T10:22:37Z

On 2011-01-17 9:28 AM +0200, Itagaki Takahiro wrote:
> Here is a short review for Transaction scoped advisory locks:
> https://commitfest.postgresql.org/action/patch_view?id=518

Thanks for reviewing!

> == Features ==
> The patch adds pg_[try_]advisory_xact_lock[_shared] functions.
> The function names follows the past discussion -- it's better than
> "bool isXact" argument or changing the existing behavior.
>
> == Coding ==
> I expect documentation will come soon.

I'm sorry about this, I have been occupied with other stuff.  I'm going 
to work on this tonight.

> There is no regression test, but we have no regression test for
> advisory locks even now. Tests for lock conflict might be difficult,
> but we could have single-threaded test for lock/unlock and pg_locks view.

Seems useful.

> == Questions ==
> I have a question about unlocking transaction-scope advisory locks.
> We cannot unlock them with pg_advisory_unlock(), but can unlock with
> pg_advisory_unlock_all(). It's inconsistent behavior.
> Furthermore, I wonder we can allow unlocking transaction-scope locks
> -- we have LOCK TABLE but don't have UNLOCK TABLE.

I guess we could add new pg_advisory_txn_unlock() functions to unlock 
transaction-scope locks, but I do share your doubt on whether or not we 
want to allow this at all.  On the other hand, the reasons why we don't 
allow non-advisory locks to be unreleased is a lot more clear than the 
issue at hand.  I have no strong opinion on this.

Another thing I now see is this:

BEGIN;
SELECT pg_advisory_xact_lock(1);

-- do something here

-- upgrade to session lock
SELECT pg_advisory_lock(1);
COMMIT;

This seems useful, since the xact lock would be automatically released 
if an error happens during "-- do something here" so you wouldn't need 
to worry about releasing the lock elsewhere.  But I'm not sure this is 
safe.  Can anyone see a problem with it?

Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-20T15:35:01Z

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> Another thing I now see is this:

> BEGIN;
> SELECT pg_advisory_xact_lock(1);

> -- do something here

> -- upgrade to session lock
> SELECT pg_advisory_lock(1);
> COMMIT;


> This seems useful, since the xact lock would be automatically released 
> if an error happens during "-- do something here" so you wouldn't need 
> to worry about releasing the lock elsewhere.  But I'm not sure this is 
> safe.  Can anyone see a problem with it?

I think the POLA dictates that the behavior of that should be that you
now have both a transactional and a nontransactional hold on the lock;
and only the transactional hold goes away at commit.

			regards, tom lane

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-01-21T17:18:33Z

On 1/20/2011 7:35 AM, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> This seems useful, since the xact lock would be automatically released
>> if an error happens during "-- do something here" so you wouldn't need
>> to worry about releasing the lock elsewhere.  But I'm not sure this is
>> safe.  Can anyone see a problem with it?
>
> I think the POLA dictates that the behavior of that should be that you
> now have both a transactional and a nontransactional hold on the lock;
> and only the transactional hold goes away at commit.

Yes, I believe that's what happens now.  But I guess you answered my 
question too by not pointing out a huge flaw in that thinking.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Robert Haas <robertmhaas@gmail.com> — 2011-01-23T02:24:18Z

On Thu, Jan 20, 2011 at 5:22 AM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> On 2011-01-17 9:28 AM +0200, Itagaki Takahiro wrote:
>>
>> Here is a short review for Transaction scoped advisory locks:
>> https://commitfest.postgresql.org/action/patch_view?id=518
>
> Thanks for reviewing!
>
>> == Features ==
>> The patch adds pg_[try_]advisory_xact_lock[_shared] functions.
>> The function names follows the past discussion -- it's better than
>> "bool isXact" argument or changing the existing behavior.
>>
>> == Coding ==
>> I expect documentation will come soon.
>
> I'm sorry about this, I have been occupied with other stuff.  I'm going to
> work on this tonight.

Any update on this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-01-28T08:12:43Z

On 1/23/2011 4:24 AM, Robert Haas wrote:
> On Thu, Jan 20, 2011 at 5:22 AM, Marko Tiikkaja
> <marko.tiikkaja@cs.helsinki.fi>  wrote:
>> On 2011-01-17 9:28 AM +0200, Itagaki Takahiro wrote:
>>> == Coding ==
>>> I expect documentation will come soon.
>>
>> I'm sorry about this, I have been occupied with other stuff.  I'm going to
>> work on this tonight.
>
> Any update on this?

Again, my apologies for the delay :-(  Things haven't been going as 
planned during the last few weeks.

Here's an updated patch with proposed doc changes.  I still didn't 
address the issue with pg_advisory_unlock_all() releasing transaction 
scoped locks, but I'm going to.  Another issue I found while testing the 
behaviour here:
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01939.php
is that if a session holds both a transaction level and a session level 
lock on the same resource, only one of them will appear in pg_locks.  Is 
that going to be a problem from the user's perspective?  Could it be an 
indication of a well-hidden bug?  Based on my tests it seems to work, 
but I'm not at all confident with the code.

Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-01T12:28:59Z

On Fri, Jan 28, 2011 at 17:12, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> I still didn't address
> the issue with pg_advisory_unlock_all() releasing transaction scoped locks,

I guess you don't want independent locks, right? If an user object
is locked by session locks, it also blocks backends trying to lock it
with transaction locks.

If so, I think an ideal behavior is below:
- The transaction-or-session property is overwritten by the last lock
  function call. We can promote session locks from/to transaction locks.
- Shared and exclusive locks are managed independently.
  We could have shared session lock and exclusive transaction
  lock on the same resource in a transaction.
- Unlock functions releases both transaction and session locks.
- unlock_all() releases all both locks.

Those might be odd in DBMS-perspective, but would be natural as
programming languages. I guess advisory locks are often used in
standard programming like flows.

> Another issue I found while testing the behaviour here:
> http://archives.postgresql.org/pgsql-hackers/2011-01/msg01939.php
> is that if a session holds both a transaction level and a session level lock
> on the same resource, only one of them will appear in pg_locks.  Is that
> going to be a problem from the user's perspective?  Could it be an
> indication of a well-hidden bug?  Based on my tests it seems to work, but
> I'm not at all confident with the code.

In the above proposal, we won't have both session and transaction lock
on the same resource at the same time, though we still need to show
exclusive and shared locks in different lines.

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Robert Haas <robertmhaas@gmail.com> — 2011-02-01T14:00:03Z

On Tue, Feb 1, 2011 at 7:28 AM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> On Fri, Jan 28, 2011 at 17:12, Marko Tiikkaja
> <marko.tiikkaja@cs.helsinki.fi> wrote:
>> I still didn't address
>> the issue with pg_advisory_unlock_all() releasing transaction scoped locks,
>
> I guess you don't want independent locks, right? If an user object
> is locked by session locks, it also blocks backends trying to lock it
> with transaction locks.
>
> If so, I think an ideal behavior is below:
> - The transaction-or-session property is overwritten by the last lock
>  function call. We can promote session locks from/to transaction locks.

No.  The lock manager already supports session-locks.  This patch
should be worried about making sure that LockAcquire() gets called
with the flags the user wants, NOT with redefining the interaction
between transaction locks and session locks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-02T15:24:40Z

On 2011-01-28 10:12 AM +0200, I wrote:
> I still didn't
> address the issue with pg_advisory_unlock_all() releasing transaction
> scoped locks, but I'm going to.

.. and here's the patch.  I'm not too confident with the code I added to 
storage/lmgr/lock.c, but it seems to be working.

Earlier there was some discussion about adding regression tests for 
advisory locks.  However, I don't see where they would fit in our 
current .sql files and adding a new one just for a few tests didn't seem 
right.  Anyone have an idea where they should go or should I just add a 
new one?

Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-09T12:12:56Z

On Thu, Feb 3, 2011 at 00:24, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> .. and here's the patch.  I'm not too confident with the code I added to
> storage/lmgr/lock.c, but it seems to be working.

Sorry for the delayed review.

The patch needs adjustment of OIDs for recently commits, but it still works
well. See the attached small fix.  The patch looks almost ready to commit
unless we want to fix the pg_locks issue below.

=== Features ===
Now unlock functions only release session-level locks and the behavior
is documented, so no confusion here. We don't have "upgrade" method
for advisory locks actually -- session and xact locks block each other,
but they are acquired and released independently.

One issue might be in pg_locks, as you pointed out in the previous mail:
> if a session holds both a transaction level and a session level lock
> on the same resource, only one of them will appear in pg_locks.
Also, we cannot distinguish transaction-level locks from session-level
locks from pg_locks.

It was not an issue before because session locks are only used in
internal implementation. It looks as a transaction from users.
However, this feature reveals the status in public. We might need
to add some bits to shared lock state to show which lock is session-level.

=== Implementation ===
* pg_advisory_unlock_all() calls LockReleaseSession(), ant it releases
not only advisory locks but also all session-level locks.
We use session-level locks in some places, but there is no chance
for user to send SQL commands during the lock. The behavior is safe
as of now, but it might break something in the future.
So I'd recommend to keep locktype checks in it.

* user_lockmethod.transactional was changed to 'true', so we don't have
any differences between it and default_lockmethod except trace_flag.
LockMethodData is now almost useless, but we could keep it for compatibility.

> Earlier there was some discussion about adding regression tests for advisory
> locks.  However, I don't see where they would fit in our current .sql files
> and adding a new one just for a few tests didn't seem right.  Anyone have an
> idea where they should go or should I just add a new one?

I think you can add advisory_lock.sql for the test.

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Robert Haas <robertmhaas@gmail.com> — 2011-02-09T17:28:45Z

On Wed, Feb 9, 2011 at 7:12 AM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> One issue might be in pg_locks, as you pointed out in the previous mail:
>> if a session holds both a transaction level and a session level lock
>> on the same resource, only one of them will appear in pg_locks.
> Also, we cannot distinguish transaction-level locks from session-level
> locks from pg_locks.
>
> It was not an issue before because session locks are only used in
> internal implementation. It looks as a transaction from users.
> However, this feature reveals the status in public. We might need
> to add some bits to shared lock state to show which lock is session-level.

Presumably that would carry a small performance penalty, since
changing the status of the lock would require modifications to the
shared hash table, not just the backend-private one.

It may still be worth doing, but I'm inclined to think that it's a
separate patch that someone could submit for 9.2.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-09T23:36:52Z

On 2/9/2011 2:12 PM, Itagaki Takahiro wrote:
> On Thu, Feb 3, 2011 at 00:24, Marko Tiikkaja
> <marko.tiikkaja@cs.helsinki.fi>  wrote:
>> .. and here's the patch.  I'm not too confident with the code I added to
>> storage/lmgr/lock.c, but it seems to be working.
>
> Sorry for the delayed review.

It's okay.  I really appreciate you looking at this.

> The patch needs adjustment of OIDs for recently commits, but it still works
> well. See the attached small fix.  The patch looks almost ready to commit
> unless we want to fix the pg_locks issue below.

Thanks, applied.

> === Features ===
> Now unlock functions only release session-level locks and the behavior
> is documented, so no confusion here. We don't have "upgrade" method
> for advisory locks actually -- session and xact locks block each other,
> but they are acquired and released independently.
>
> One issue might be in pg_locks, as you pointed out in the previous mail:
>> if a session holds both a transaction level and a session level lock
>> on the same resource, only one of them will appear in pg_locks.
> Also, we cannot distinguish transaction-level locks from session-level
> locks from pg_locks.
>
> It was not an issue before because session locks are only used in
> internal implementation. It looks as a transaction from users.
> However, this feature reveals the status in public. We might need
> to add some bits to shared lock state to show which lock is session-level.

Robert suggested not doing this for 9.1, and I don't have anything 
against that.

> === Implementation ===
> * pg_advisory_unlock_all() calls LockReleaseSession(), ant it releases
> not only advisory locks but also all session-level locks.
> We use session-level locks in some places, but there is no chance
> for user to send SQL commands during the lock. The behavior is safe
> as of now, but it might break something in the future.
> So I'd recommend to keep locktype checks in it.

Whoops.  Good catch, that was unintentional.  Fixed.

> * user_lockmethod.transactional was changed to 'true', so we don't have
> any differences between it and default_lockmethod except trace_flag.
> LockMethodData is now almost useless, but we could keep it for compatibility.

Agreed.

>> Earlier there was some discussion about adding regression tests for advisory
>> locks.  However, I don't see where they would fit in our current .sql files
>> and adding a new one just for a few tests didn't seem right.  Anyone have an
>> idea where they should go or should I just add a new one?
>
> I think you add advisory_lock.sql for the test.

Ok.


Updated patch attached.


Regards,
Marko Tiikkaja

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-10T08:15:28Z

On Thu, Feb 10, 2011 at 08:36, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
>> One issue might be in pg_locks
> Robert suggested not doing this for 9.1, and I don't have anything against
> that.

Agreed.

> Updated patch attached.

Looks good to commit. I note a few minor issues for committer:

* Functions listed in "Table 9-62. Advisory Lock Functions" might need
  sorted in alphabetical order.

* We could extend LockReleaseAll() to have the 3rd mode
  instead of LockReleaseSession().  Existing behavior is:
| LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
|   allLocks == true: release all locks including session locks.
|   allLocks == false: release all non-session locks.

* Or, we might have one subroutine for LockReleaseSession() and
  LockReleaseCurrentOwner(). They have similar codes.

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-17T08:05:13Z

On Thu, Feb 10, 2011 at 17:15, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
>> Updated patch attached.
> Looks good to commit.

I did a few cosmetic fixes, mainly lmgr/README and make a subroutine
ReleaseLockForOwner() for LockReleaseSession and LockReleaseCurrentOwner.

If no more comments nor objections, I'll apply the version to git.

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-18T05:16:21Z

On Thu, Feb 17, 2011 at 17:05, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> I did a few cosmetic fixes, mainly lmgr/README and make a subroutine
> ReleaseLockForOwner() for LockReleaseSession and LockReleaseCurrentOwner.

Committed with a few typo fixes. Thanks, Marko!

-- 
Itagaki Takahiro

Re: Transaction-scope advisory locks

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-18T08:39:18Z

On 2011-02-18 7:16 AM +0200, Itagaki Takahiro wrote:
> Committed with a few typo fixes. Thanks, Marko!

Thanks a lot!


Regards,
Marko Tiikkaja