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