Hi,
There frequently have been bugs where (heap|relation|index)_open(NoLock)
was used without a previous locks which in some circumstances is an easy
mistake to make and which is hard to notice.
The attached patch adds --use-cassert only WARNINGs against doing so:
Add cassert-only checks against unlocked use of relations.
Specifically relation_open(), which also covers heap/index_open(), and
RelationIdGetRelation() WARN if the relation is opened without being
locked. index_open() now checks whether the heap relation the index is
covering is locked.
To make those checks possible add StrongestLocalRelationLock() which
returns the strongest locally held lock over a relation. It relies on
the also added StrongestLocalLock() which searches the local locktable
sequentially for matching locks.
After
1) 2a781d57dcd027df32d15ee2378b84d0c4d005d1
2)
http://archives.postgresql.org/message-id/1383681385.79520.YahooMailNeo%40web162902.mail.bf1.yahoo.com
3)
http://archives.postgresql.org/message-id/CAEZATCVCgboHKu_%2BK0nakrXW-AFEz_18icE0oWEQHsM-OepWhw%40mail.gmail.com
HEAD doesn't generate warnings anymore. I think Kevin will commit
something akin to 2), but 3) is an actual open bug, so that patch will
need to get applied beforehand.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From c02e2c00c0dc98ff7d5f63a1d30887b8bbfe0dc3 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Wed, 23 Oct 2013 02:34:51 +0200
Subject: [PATCH 1/4] Add cassert-only checks against unlocked use of
relations.
Specifically relation_open(), which also covers heap/index_open(), and
RelationIdGetRelation() WARN if the relation is opened without being
locked. index_open() now checks whether the heap relation the index is
covering is locked.
To make those checks possible add StrongestLocalRelationLock() which
returns the strongest locally held lock over a relation. It relies on
the also added StrongestLocalLock() which searches the local locktable
sequentially for matching locks.
---
src/backend/access/heap/heapam.c | 18 ++++++++++++++++++
src/backend/access/index/indexam.c | 23 +++++++++++++++++++++++
src/backend/storage/lmgr/lmgr.c | 23 ++++++++++++++++++++++-
src/backend/storage/lmgr/lock.c | 35 +++++++++++++++++++++++++++++++++++
src/backend/utils/cache/relcache.c | 18 ++++++++++++++++++
src/include/storage/lmgr.h | 2 ++
src/include/storage/lock.h | 2 ++
7 files changed, 120 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a21f31b..3fbadd4 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1038,6 +1038,24 @@ relation_open(Oid relationId, LOCKMODE lockmode)
pgstat_initstats(r);
+#if USE_ASSERT_CHECKING
+ /*
+ * Unless locked a relation's definition can change, thus relying
+ * on it is dangerous in that case.
+ * We cannot rely on correct locking during bootstrap, so don't
+ * check for it there.
+ */
+ if (assert_enabled && IsNormalProcessingMode() && lockmode == NoLock)
+ {
+ LOCKMODE curmode;
+
+ curmode = StrongestLocalRelationLock(relationId);
+ if (curmode == NoLock)
+ elog(WARNING, "relation_open(%u, NoLock) of relation \"%s\" without previously held lock",
+ relationId, RelationGetRelationName(r));
+ }
+#endif
+
return r;
}
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b878155..c75bee2 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -65,6 +65,8 @@
#include "postgres.h"
+#include "miscadmin.h"
+
#include "access/relscan.h"
#include "access/transam.h"
#include "catalog/index.h"
@@ -142,6 +144,8 @@ static IndexScanDesc index_beginscan_internal(Relation indexRelation,
* ----------------------------------------------------------------
*/
+#include "catalog/catalog.h"
+
/* ----------------
* index_open - open an index relation by relation OID
*
@@ -169,6 +173,25 @@ index_open(Oid relationId, LOCKMODE lockmode)
errmsg("\"%s\" is not an index",
RelationGetRelationName(r))));
+#if USE_ASSERT_CHECKING
+ /*
+ * Using an index's definition is only safe if the underlying
+ * relation is already locked.
+ * We cannot rely on correct locking during bootstrap, so don't
+ * check for it there.
+ */
+ if (assert_enabled && IsNormalProcessingMode())
+ {
+ LOCKMODE mode;
+
+ mode = StrongestLocalRelationLock(r->rd_index->indrelid);
+ if (mode == NoLock)
+ elog(WARNING, "index_open(%u) of index \"%s\" without lock on heap relation %u",
+ relationId, RelationGetRelationName(r),
+ r->rd_index->indrelid);
+ }
+#endif
+
return r;
}
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index a978172..a6cf74a 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -233,9 +233,30 @@ UnlockRelation(Relation relation, LOCKMODE lockmode)
}
/*
+ * StrongestLocalRelationLock
+ *
+ * Debugging function returning the strongest lock this backend is
+ * currently holding for the Relation with the Oid 'relid' or NoLock
+ * if it currently isn't locked.
+ *
+ * NB: This isn't particularly fast, so don't use in performance
+ * critical paths in a non-assert build.
+ */
+LOCKMODE
+StrongestLocalRelationLock(Oid relid)
+{
+ LOCKTAG tag;
+
+ SetLocktagRelationOid(&tag, relid);
+
+ return StrongestLocalLock(&tag);
+}
+
+
+/*
* LockHasWaitersRelation
*
- * This is a functiion to check if someone else is waiting on a
+ * This is a function to check if someone else is waiting on a
* lock, we are currently holding.
*/
bool
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index f4f32e9..b9d26e5 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2366,6 +2366,41 @@ LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent)
}
/*
+ * StrongestLocalLock
+ * Return the strongest lock held by this ackend for locks with
+ * the tag locktag, or NoLock if currently unlocked.
+ *
+ * NB: This searches the entire hash and thus isn't all that fast, so
+ * only in situations where that doesn't matter.
+ */
+LOCKMODE
+StrongestLocalLock(const LOCKTAG* locktag)
+{
+ HASH_SEQ_STATUS status;
+ LOCALLOCK *locallock;
+ LOCKMODE strongest = NoLock;
+
+ hash_seq_init(&status, LockMethodLocalHash);
+
+ /*
+ * We cannot trivially search the hash using hash_search() since
+ * the key includes the lockmode, so iterate over all local locks.
+ * XXX: We could instead search once for every lockmode, but that
+ * doesn't seem to be worth the complications for now.
+ */
+ while ((locallock = (LOCALLOCK *) hash_seq_search(&status)) != NULL)
+ {
+ /*
+ * compare just locktag, without mode, and remember the
+ * strongest LOCK
+ */
+ if (memcmp(&locallock->tag.lock, locktag, sizeof(LOCKTAG)) == 0)
+ strongest = Max(strongest, locallock->tag.mode);
+ }
+ return strongest;
+}
+
+/*
* FastPathGrantRelationLock
* Grant lock using per-backend fast-path array, if there is space.
*/
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index b4cc6ad..3d6e921 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1577,6 +1577,24 @@ RelationIdGetRelation(Oid relationId)
{
Relation rd;
+#ifdef USE_ASSERT_CHECKING
+ /*
+ * It is dangerous to use relcache entries without appropriate
+ * locks. This check is mostly redundant with the one in
+ * relation_open(), but code using RelationIdGetRelation()
+ * frequently gets added errorneously, so warn here as well.
+ */
+ if (assert_enabled && IsNormalProcessingMode())
+ {
+ LOCKMODE mode;
+
+ mode = StrongestLocalRelationLock(relationId);
+ if (mode == NoLock)
+ elog(WARNING, "RelationIdGetRelation(%u) without holding lock on the relation",
+ relationId);
+ }
+#endif
+
/*
* first try to find reldesc in the cache
*/
diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h
index 1a8c018..c2c6026 100644
--- a/src/include/storage/lmgr.h
+++ b/src/include/storage/lmgr.h
@@ -40,6 +40,8 @@ extern void UnlockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode);
extern void LockRelationForExtension(Relation relation, LOCKMODE lockmode);
extern void UnlockRelationForExtension(Relation relation, LOCKMODE lockmode);
+extern LOCKMODE StrongestLocalRelationLock(Oid relid);
+
/* Lock a page (currently only used within indexes) */
extern void LockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);
extern bool ConditionalLockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 7bcaf7c..ea915c8 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -495,6 +495,8 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks);
extern void LockReleaseSession(LOCKMETHODID lockmethodid);
extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks);
extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks);
+extern LOCKMODE StrongestLocalLock(const LOCKTAG* locktag);
+
extern bool LockHasWaiters(const LOCKTAG *locktag,
LOCKMODE lockmode, bool sessionLock);
extern VirtualTransactionId *GetLockConflicts(const LOCKTAG *locktag,
--
1.8.4.21.g992c386.dirty
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
