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

On 2013-11-05 16:25:53 -0500, Tom Lane wrote:
> 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.

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