[HACKERS] Add cassert-only checks against unlocked use of relations
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
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
Andres Freund and...@2ndquadrant.com writes: 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: While I agree that there seems to be a problem here, I'm not convinced that this is the solution. The implication of a heap_open(NoLock) is that the programmer believes that some previous action must have taken a lock on the relation; if he's wrong, then the causal link that he thought existed doesn't really. But this patch is not checking for a causal link; it'll be fooled just as easily as the programmer is by a happenstance (that is, unrelated) previous lock on the relation. What's more, it entirely fails to check whether the previous lock is really strong enough for what we're going to do. I also find it unduly expensive to search the whole lock hashtable on every relation open. That's going to be a O(N^2) cost for a transaction touching N relations, and that doesn't sound acceptable, not even for assert-only code. If we're sufficiently worried by this type of bug, ISTM we'd be better off just disallowing heap_open(NoLock). At the time we invented that, every lock request went to shared memory; but now that we have the local lock table, re-locking just requires a local hash lookup followed by incrementing a local counter. That's probably pretty cheap --- certainly a lot cheaper than what you've got here. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
On 2013-11-05 16:25:53 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: 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: While I agree that there seems to be a problem here, I'm not convinced that this is the solution. The implication of a heap_open(NoLock) is that the programmer believes that some previous action must have taken a lock on the relation; if he's wrong, then the causal link that he thought existed doesn't really. But this patch is not checking for a causal link; it'll be fooled just as easily as the programmer is by a happenstance (that is, unrelated) previous lock on the relation. What's more, it entirely fails to check whether the previous lock is really strong enough for what we're going to do. Sure. But it already found several bugs as evidenced by the referenced thread, so it seems to be helpful enough. I also find it unduly expensive to search the whole lock hashtable on every relation open. That's going to be a O(N^2) cost for a transaction touching N relations, and that doesn't sound acceptable, not even for assert-only code. We could relatively easily optimize that to a constant factor by just iterating over the possible lockmodes. Should only take a couple more lines. I'd be really, really surprised if it even comes close to the overhead of AtEOXact_Buffers() though. Which is not to say it's a bad idea to make this check more effective though ;) If we're sufficiently worried by this type of bug, ISTM we'd be better off just disallowing heap_open(NoLock). At the time we invented that, every lock request went to shared memory; but now that we have the local lock table, re-locking just requires a local hash lookup followed by incrementing a local counter. That's probably pretty cheap --- certainly a lot cheaper than what you've got here. Hm. That only works though if we're using the same lockmode as before - often enough the *_open(NoLock) checks would use a weaker locklevel than the previous lock. So I think the cost of doing so would probably be noticeable. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
Andres Freund and...@2ndquadrant.com writes: On 2013-11-05 16:25:53 -0500, Tom Lane wrote: If we're sufficiently worried by this type of bug, ISTM we'd be better off just disallowing heap_open(NoLock). At the time we invented that, every lock request went to shared memory; but now that we have the local lock table, re-locking just requires a local hash lookup followed by incrementing a local counter. That's probably pretty cheap --- certainly a lot cheaper than what you've got here. Hm. That only works though if we're using the same lockmode as before - often enough the *_open(NoLock) checks would use a weaker locklevel than the previous lock. So I think the cost of doing so would probably be noticeable. If you're not using the same lockmode as before, it's probably a bug anyway. As I said already, the entire NoLock coding technique is dependent on having a very clear idea of which previous lock-taking you're riding on the coattails of. Why wouldn't you duplicate that lock level, if we say you can't use NoLock anymore? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
On 2013-11-05 16:45:49 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-11-05 16:25:53 -0500, Tom Lane wrote: If we're sufficiently worried by this type of bug, ISTM we'd be better off just disallowing heap_open(NoLock). At the time we invented that, every lock request went to shared memory; but now that we have the local lock table, re-locking just requires a local hash lookup followed by incrementing a local counter. That's probably pretty cheap --- certainly a lot cheaper than what you've got here. Hm. That only works though if we're using the same lockmode as before - often enough the *_open(NoLock) checks would use a weaker locklevel than the previous lock. So I think the cost of doing so would probably be noticeable. If you're not using the same lockmode as before, it's probably a bug anyway. As I said already, the entire NoLock coding technique is dependent on having a very clear idea of which previous lock-taking you're riding on the coattails of. Why wouldn't you duplicate that lock level, if we say you can't use NoLock anymore? Hm. That doesn't really seem to match my reading of the code. There's quite some code around that relies the fact that the parser has taken an appropriately strong lock already. E.g. the parser chooses the lockmode in addRangeTableEntry() - I don't think we want to add duplicate isLockedRefname() calls everywhere. Other users of that relation will likely just use AccessShareLock since that's sufficient for their own use. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
On 2013-11-05 22:35:41 +0100, Andres Freund wrote: We could relatively easily optimize that to a constant factor by just iterating over the possible lockmodes. Should only take a couple more lines. On that note, any chance you remember why you increased MAX_LOCKMODE by 2 to 10 back in 2001 although AccessExclusiveLock is 8? The relevant commit is E4fe42dfbc3bafa0ea615239d716a6b37d67da253 . Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
Andres Freund and...@2ndquadrant.com writes: On that note, any chance you remember why you increased MAX_LOCKMODE by 2 to 10 back in 2001 although AccessExclusiveLock is 8? The relevant commit is 4fe42dfbc3bafa0ea615239d716a6b37d67da253 . Probably because it seemed like a round number, which 9 wasn't ... keep in mind that this data structure is nominally intended to support other lock semantics besides what LockConflicts[] defines. (BTW, it's a conceptual error to imagine that the numerical values of the lock mode codes define a strict strength ordering, which is another reason I don't care for your patch.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add cassert-only checks against unlocked use of relations
On 2013-11-05 17:19:21 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On that note, any chance you remember why you increased MAX_LOCKMODE by 2 to 10 back in 2001 although AccessExclusiveLock is 8? The relevant commit is 4fe42dfbc3bafa0ea615239d716a6b37d67da253 . Probably because it seemed like a round number, which 9 wasn't ... keep in mind that this data structure is nominally intended to support other lock semantics besides what LockConflicts[] defines. Hm. Given that there are Assert()s for MAX_LOCKMODE around, that adding a new method isn't possible without editing lock.c and that we use it to in shared memory structures I am not sure I see the point of that slop. Anyway, it was just a point of curiosity. (BTW, it's a conceptual error to imagine that the numerical values of the lock mode codes define a strict strength ordering, which is another reason I don't care for your patch.) Well, while I don't thing it has too much practical bearing, we could just check for *any* lock already held instead, that's all we need for the added checks. I thought it might be more useful to get the strongest lock rather than any lock for other potential checks, but if that's a contentious point... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
