Thread: LOCK_DEBUG is busted

LOCK_DEBUG is busted

From
Robert Haas
Date:
It's possible to compile the source tree with LOCK_DEBUG defined, but
the resulting postgres promptly dumps core, due to the fact that
user_lockmethod doesn't supply any value for trace_flag; thus, the
first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
This is the result of the following commit:

commit 0180bd6180511875db046bf8ddcaa633a2952dfd
Author: Bruce Momjian <bruce@momjian.us>
Date:   Thu Oct 13 19:59:13 2011 -0400
   Remove all "traces" of trace_userlocks, because userlocks were removed   in PG 8.2.

As far as I can see, that commit was just wrong and should be
reverted.  I believe that Bruce's motivation for this commit was the
following sentence from the documentation:

-        User locks were removed as of PostgreSQL version 8.2.  This option
-        currently has no effect.

The trouble with this is that it's just not true.  With that commit
reverted and LOCK_DEBUG defined in pg_config_manual.h:

rhaas=# set trace_userlocks=1;
LOG:  LockReleaseAll: lockmethod=2
STATEMENT:  set trace_userlocks=1;
LOG:  LockReleaseAll done
STATEMENT:  set trace_userlocks=1;
SET
rhaas=# select pg_advisory_lock(300001,300001);
LOG:  LockAcquire: lock [16384,300001] ExclusiveLock
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  LockAcquire: new: lock(0x103ad37c0)
id(16384,300001,300001,2,8,2) grantMask(0) req(0,0,0,0,0,0,0)=0
grant(0,0,0,0,0,0,0)=0 wait(0) type(ExclusiveLock)
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  LockAcquire: new: proclock(0x103b590b8) lock(0x103ad37c0)
method(2) proc(0x103d72b30) hold(0)
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  LockCheckConflicts: no conflict: proclock(0x103b590b8)
lock(0x103ad37c0) method(2) proc(0x103d72b30) hold(0)
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  GrantLock: lock(0x103ad37c0) id(16384,300001,300001,2,8,2)
grantMask(80) req(0,0,0,0,0,0,1)=1 grant(0,0,0,0,0,0,1)=1 wait(0)
type(ExclusiveLock)
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  LockReleaseAll: lockmethod=2
STATEMENT:  select pg_advisory_lock(300001,300001);
LOG:  LockReleaseAll done
STATEMENT:  select pg_advisory_lock(300001,300001);pg_advisory_lock
------------------

(1 row)

Now, whether or not this facility is well designed is a worthwhile
question.  Trace_lock_oidmin seems pretty sketchy to me, especially
because it's blindly applied to even to lock tags where the second
field isn't a relation - i.e. SET_LOCKTAG_TRANSACTION sets it to zero,
SET_LOCKTAG_VIRTUALTRANSACTION sets it to the localTransactionId,
SET_LOCKTAG_OBJECT sets it to the classId member of the objectaddress,
and advisory locks set it to 32 bits of the user's chosen locktag.  So
by default, with trace_userlocks turned on and no other changes,
pg_advisory_lock(16384,0) produces output like that shown above and
pg_advisory_lock(16383,0) is met with silence.  So maybe we should
just rip some or all of this stuff out instead of worrying too much
about it.  If we're not going to do that, then we should revert the
above commit, so that it works again, at least as much as it did
before.

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


Re: LOCK_DEBUG is busted

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> It's possible to compile the source tree with LOCK_DEBUG defined, but
> the resulting postgres promptly dumps core, due to the fact that
> user_lockmethod doesn't supply any value for trace_flag; thus, the
> first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
> This is the result of the following commit:

> commit 0180bd6180511875db046bf8ddcaa633a2952dfd

+1 for just reverting that commit.  I'm not sure how much use the
LOCK_DEBUG infrastructure has in exactly its current form, but I can
certainly imagine wanting to use it or some variant of it to debug
tough problems.  If it's gone entirely, people would have to reinvent
most of it for that type of debugging.  On the other side of the coin,
I don't have a clear enough use-case for it to want to spend time
right now on redesigning it, nor a clear idea of exactly what changes
might make it more useful.  So I think we should just revert and
not spend additional effort now.
        regards, tom lane


Re: LOCK_DEBUG is busted

From
Bruce Momjian
Date:
Robert Haas wrote:
> Now, whether or not this facility is well designed is a worthwhile
> question.  Trace_lock_oidmin seems pretty sketchy to me, especially
> because it's blindly applied to even to lock tags where the second
> field isn't a relation - i.e. SET_LOCKTAG_TRANSACTION sets it to zero,
> SET_LOCKTAG_VIRTUALTRANSACTION sets it to the localTransactionId,
> SET_LOCKTAG_OBJECT sets it to the classId member of the objectaddress,
> and advisory locks set it to 32 bits of the user's chosen locktag.  So
> by default, with trace_userlocks turned on and no other changes,
> pg_advisory_lock(16384,0) produces output like that shown above and
> pg_advisory_lock(16383,0) is met with silence.  So maybe we should
> just rip some or all of this stuff out instead of worrying too much
> about it.

Please rip out whatever I missed.  Thanks.  The user locks were the old
lock type before we had advisor locks, as far as I remember.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: LOCK_DEBUG is busted

From
Bruce Momjian
Date:
Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > It's possible to compile the source tree with LOCK_DEBUG defined, but
> > the resulting postgres promptly dumps core, due to the fact that
> > user_lockmethod doesn't supply any value for trace_flag; thus, the
> > first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
> > This is the result of the following commit:
> 
> > commit 0180bd6180511875db046bf8ddcaa633a2952dfd
> 
> +1 for just reverting that commit.  I'm not sure how much use the
> LOCK_DEBUG infrastructure has in exactly its current form, but I can
> certainly imagine wanting to use it or some variant of it to debug
> tough problems.  If it's gone entirely, people would have to reinvent
> most of it for that type of debugging.  On the other side of the coin,
> I don't have a clear enough use-case for it to want to spend time
> right now on redesigning it, nor a clear idea of exactly what changes
> might make it more useful.  So I think we should just revert and
> not spend additional effort now.

I am confused.   I thought it was lock_debug referencing user locks that
was broken.  Does lock_debug need user locks?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: LOCK_DEBUG is busted

From
Robert Haas
Date:
On Thu, Nov 10, 2011 at 5:07 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>> > It's possible to compile the source tree with LOCK_DEBUG defined, but
>> > the resulting postgres promptly dumps core, due to the fact that
>> > user_lockmethod doesn't supply any value for trace_flag; thus, the
>> > first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
>> > This is the result of the following commit:
>>
>> > commit 0180bd6180511875db046bf8ddcaa633a2952dfd
>>
>> +1 for just reverting that commit.  I'm not sure how much use the
>> LOCK_DEBUG infrastructure has in exactly its current form, but I can
>> certainly imagine wanting to use it or some variant of it to debug
>> tough problems.  If it's gone entirely, people would have to reinvent
>> most of it for that type of debugging.  On the other side of the coin,
>> I don't have a clear enough use-case for it to want to spend time
>> right now on redesigning it, nor a clear idea of exactly what changes
>> might make it more useful.  So I think we should just revert and
>> not spend additional effort now.
>
> I am confused.   I thought it was lock_debug referencing user locks that
> was broken.  Does lock_debug need user locks?

It supports tracing them.

The point is, they're not gone.

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


Re: LOCK_DEBUG is busted

From
Robert Haas
Date:
On Thu, Nov 10, 2011 at 5:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> It's possible to compile the source tree with LOCK_DEBUG defined, but
>> the resulting postgres promptly dumps core, due to the fact that
>> user_lockmethod doesn't supply any value for trace_flag; thus, the
>> first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
>> This is the result of the following commit:
>
>> commit 0180bd6180511875db046bf8ddcaa633a2952dfd
>
> +1 for just reverting that commit.  I'm not sure how much use the
> LOCK_DEBUG infrastructure has in exactly its current form, but I can
> certainly imagine wanting to use it or some variant of it to debug
> tough problems.  If it's gone entirely, people would have to reinvent
> most of it for that type of debugging.  On the other side of the coin,
> I don't have a clear enough use-case for it to want to spend time
> right now on redesigning it, nor a clear idea of exactly what changes
> might make it more useful.  So I think we should just revert and
> not spend additional effort now.

I don't feel like it accomplishes much of anything that can't be
trivially accomplished by throwing in a couple of ad-hoc elog() calls
wherever you happen to want them.  I experimented with this when
developing the fastlock patches and found it didn't tell me what I
wanted to know, so just stuck in debugging code in the places that
were relevant to my patch's then-current problems.  Once those bugs
were fixed, I took the debugging code back out.  I think the author of
this code did pretty much the same thing, but then developed the
pretension that the particular places he stuck the elog() calls in
would be generally relevant, which I don't believe to be the case.  Or
maybe they were relevant at one time, but this code has been with us
for an awfully long time, and I think it's considerably outlived its
usefulness.  What I think it's mostly doing at this point is making it
more difficult to make further changes - you do whatever you want to
do, and then you have to go figure out what to do about the crazy
LOCK_DEBUG stuff that no one uses.

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


Re: LOCK_DEBUG is busted

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> ...  What I think it's mostly doing at this point is making it
> more difficult to make further changes - you do whatever you want to
> do, and then you have to go figure out what to do about the crazy
> LOCK_DEBUG stuff that no one uses.

[ shrug... ]  If you're sufficiently exercised about it to take it
out entirely, I won't stand in the way.  I have not found it to be
an impediment myself, though.
        regards, tom lane


Rename trace_userlocks? WAS Re: LOCK_DEBUG is busted

From
Bruce Momjian
Date:
Robert Haas wrote:
> On Thu, Nov 10, 2011 at 5:07 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > Tom Lane wrote:
> >> Robert Haas <robertmhaas@gmail.com> writes:
> >> > It's possible to compile the source tree with LOCK_DEBUG defined, but
> >> > the resulting postgres promptly dumps core, due to the fact that
> >> > user_lockmethod doesn't supply any value for trace_flag; thus, the
> >> > first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
> >> > This is the result of the following commit:
> >>
> >> > commit 0180bd6180511875db046bf8ddcaa633a2952dfd
> >>
> >> +1 for just reverting that commit. ?I'm not sure how much use the
> >> LOCK_DEBUG infrastructure has in exactly its current form, but I can
> >> certainly imagine wanting to use it or some variant of it to debug
> >> tough problems. ?If it's gone entirely, people would have to reinvent
> >> most of it for that type of debugging. ?On the other side of the coin,
> >> I don't have a clear enough use-case for it to want to spend time
> >> right now on redesigning it, nor a clear idea of exactly what changes
> >> might make it more useful. ?So I think we should just revert and
> >> not spend additional effort now.
> >
> > I am confused. ? I thought it was lock_debug referencing user locks that
> > was broken. ?Does lock_debug need user locks?
> 
> It supports tracing them.
> 
> The point is, they're not gone.

Now that we know that the GUC trace_userlocks is used for advisory
locks, should we rename it to trace_advisory_locks?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Rename trace_userlocks? WAS Re: LOCK_DEBUG is busted

From
Robert Haas
Date:
On Fri, Nov 11, 2011 at 9:40 AM, Bruce Momjian <bruce@momjian.us> wrote:
> Robert Haas wrote:
>> On Thu, Nov 10, 2011 at 5:07 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> > Tom Lane wrote:
>> >> Robert Haas <robertmhaas@gmail.com> writes:
>> >> > It's possible to compile the source tree with LOCK_DEBUG defined, but
>> >> > the resulting postgres promptly dumps core, due to the fact that
>> >> > user_lockmethod doesn't supply any value for trace_flag; thus, the
>> >> > first LockReleaseAll(USER_LOCKMETHOD) dereferences a NULL pointer.
>> >> > This is the result of the following commit:
>> >>
>> >> > commit 0180bd6180511875db046bf8ddcaa633a2952dfd
>> >>
>> >> +1 for just reverting that commit. ?I'm not sure how much use the
>> >> LOCK_DEBUG infrastructure has in exactly its current form, but I can
>> >> certainly imagine wanting to use it or some variant of it to debug
>> >> tough problems. ?If it's gone entirely, people would have to reinvent
>> >> most of it for that type of debugging. ?On the other side of the coin,
>> >> I don't have a clear enough use-case for it to want to spend time
>> >> right now on redesigning it, nor a clear idea of exactly what changes
>> >> might make it more useful. ?So I think we should just revert and
>> >> not spend additional effort now.
>> >
>> > I am confused. ? I thought it was lock_debug referencing user locks that
>> > was broken. ?Does lock_debug need user locks?
>>
>> It supports tracing them.
>>
>> The point is, they're not gone.
>
> Now that we know that the GUC trace_userlocks is used for advisory
> locks, should we rename it to trace_advisory_locks?

They referred to as USER_LOCKMETHOD in the code, and anyone who is
using this facility is probably also reading the code, so I think it's
clear enough as-is.

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