Assert(LWLockHeldByMeInMode(lock, LW_EXCLUSIVE))
Hi hackers,
Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.
What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
bufmgr-assert-lwlock-mode.patchapplication/octet-stream; name=bufmgr-assert-lwlock-mode.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a67e518..d066092 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1460,8 +1460,8 @@ MarkBufferDirty(Buffer buffer)
bufHdr = GetBufferDescriptor(buffer - 1);
Assert(BufferIsPinned(buffer));
- /* unfortunately we can't check if the lock is held exclusively */
- Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
+ Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
+ LW_EXCLUSIVE));
old_buf_state = pg_atomic_read_u32(&bufHdr->state);
for (;;)
test-lwlock-mode.patchapplication/octet-stream; name=test-lwlock-mode.patchDownload
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 7ffa87d..7e0dd02 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1883,10 +1883,9 @@ LWLockReleaseAll(void)
/*
- * LWLockHeldByMe - test whether my process currently holds a lock
+ * LWLockHeldByMe - test whether my process holds a lock in any mode
*
- * This is meant as debug support only. We currently do not distinguish
- * whether the lock is held shared or exclusive.
+ * This is meant as debug support only.
*/
bool
LWLockHeldByMe(LWLock *l)
@@ -1900,3 +1899,21 @@ LWLockHeldByMe(LWLock *l)
}
return false;
}
+
+/*
+ * LWLockHeldByMeInMode - test whether my process holds a lock in mode X
+ *
+ * This is meant as debug support only.
+ */
+bool
+LWLockHeldByMeInMode(LWLock *l, LWLockMode mode)
+{
+ int i;
+
+ for (i = 0; i < num_held_lwlocks; i++)
+ {
+ if (held_lwlocks[i].lock == l && held_lwlocks[i].mode == mode)
+ return true;
+ }
+ return false;
+}
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 3db11e4..fc46719 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -175,6 +175,7 @@ extern void LWLockRelease(LWLock *lock);
extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val);
extern void LWLockReleaseAll(void);
extern bool LWLockHeldByMe(LWLock *lock);
+extern bool LWLockHeldByMeInMode(LWLock *lock, LWLockMode mode);
extern bool LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval);
extern void LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 value);
On 18 June 2016 at 11:28, Thomas Munro <thomas.munro@enterprisedb.com>
wrote:
Hi hackers,
Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.
I've wanted this before too, and was surprised it wasn't present. TBH I
assumed there was a technical reason it wasn't and didn't investigate
further because I just assumed it'd have been added with the original
LWLockHeldByMe if it were simple.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Jun 20, 2016 at 3:43 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 18 June 2016 at 11:28, Thomas Munro <thomas.munro@enterprisedb.com>
wrote:Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.I've wanted this before too, and was surprised it wasn't present. TBH I
assumed there was a technical reason it wasn't and didn't investigate
further because I just assumed it'd have been added with the original
LWLockHeldByMe if it were simple.
Before ab5194e6f (25 December 2014) held_lwlocks didn't record the mode.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 18 June 2016 at 04:28, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
Hi hackers,
Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.
Committed, thanks.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 20/06/2016 06:28, Thomas Munro wrote:
On Mon, Jun 20, 2016 at 3:43 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 18 June 2016 at 11:28, Thomas Munro <thomas.munro@enterprisedb.com>
wrote:Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.I've wanted this before too [...]
same here.
Before ab5194e6f (25 December 2014) held_lwlocks didn't record the mode.
I just reviewed both patches. They applies cleanly on current HEAD,
work as intended and make check run smoothly. Patches are pretty
straightforward, so I don't have much to say.
My only remark is on following comment:
+ * LWLockHeldByMeInMode - test whether my process holds a lock in mode X
Maybe something like "test whether my process holds a lock in given
mode" would be better?
Otherwise, I think they're ready for committer.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/09/2016 11:55, Julien Rouhaud wrote:
On 20/06/2016 06:28, Thomas Munro wrote:
On Mon, Jun 20, 2016 at 3:43 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 18 June 2016 at 11:28, Thomas Munro <thomas.munro@enterprisedb.com>
wrote:Several times now when reading, debugging and writing code I've wished
that LWLockHeldByMe assertions specified the expected mode, especially
where exclusive locking is required.What do you think about something like the attached? See also an
example of use. I will add this to the next commitfest.I've wanted this before too [...]
same here.
Before ab5194e6f (25 December 2014) held_lwlocks didn't record the mode.
I just reviewed both patches. They applies cleanly on current HEAD,
work as intended and make check run smoothly. Patches are pretty
straightforward, so I don't have much to say.My only remark is on following comment:
+ * LWLockHeldByMeInMode - test whether my process holds a lock in mode X
Maybe something like "test whether my process holds a lock in given
mode" would be better?Otherwise, I think they're ready for committer.
Didn't saw that Simon just committed it, sorry about it.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers