Re: Transaction-scope advisory locks

On 2/9/2011 2:12 PM, Itagaki Takahiro wrote:
> 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.

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