Re: Transaction-scope advisory locks

On Thu, Feb 3, 2011 at 00:24, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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