Transaction-scope advisory locks

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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