Re: Advisory locks seem rather broken

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Thu, May 3, 2012 at 1:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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 attempt to add a transactional advisory lock call type,
> but my understanding of the plan for that was not to change the
> existing advisory lock mechanism.

> It seems that was bungled, so some change is required, but maybe not
> total revoke.

> If the change was actually intended that way then I object to it and I
> also want it changed back.

After studying the patch a bit more I have the definite feeling that
it needs to be rewritten from scratch. It has turned the
LockMethodData.transactional flag into something completely useless for
telling whether a lock is session-level or transaction-local. And,
instead of removing that flag and forcing all the code that checks it to
be rewritten, it's dropped ad-hoc code into just some of those places.

And, as far as I can tell, the ad-hoc test that it's replaced the
transactionality tests with is "is this lock held by a ResourceOwner",
which is a flagrant abuse of the ResourceOwner mechanism.
ResourceOwners should only be used to make sure resources are released
at appropriate times, they should not cause fundamental changes in the
semantics of those resources.

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 lockmethods to share the same
lock keyspace. If we don't split it then we definitely need to figure
out someplace else to keep the transactionality flag.

Anyway, I'm going to go work on this now ...

regards, tom lane