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 <and...@anarazel.de> 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers