Thread: LOCK_DEBUG is busted
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
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
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. +
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. +
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
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
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
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. +
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