Re: Add cassert-only checks against unlocked use of relations

Andres Freund <andres(at)2ndquadrant(dot)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