Re: Add SKIP LOCKED to VACUUM and ANALYZE

On Tue, Jul 17, 2018 at 2:21 AM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> For this part, it seems to me that we can do better than what is in
> HEAD:
> - Call RangeVarGetRelidExtended without lock.

I haven't been following this work closely, but I just want to point
out that the reason why RangeVarGetRelidExtended exists is because:

(1) we cannot lock a relation without looking up its OID, and should
not lock it without doing permissions checks first, and at least as
currently designed, those functions expect an OID, but

(2) we cannot look up a relation first and only lock it afterwards,
because DDL might happen in the middle and leave us locking the wrong
relation

When RangeVarGetRelidExtended is called with an argument other than
NoLock, it effectively makes locking, permissions checking, and the
name lookup happen simultaneously, so that it is neither possible to
lock a relation for which you don't have permissions, nor to change
the identity of the relation after the name lookup has been done and
before the lock is acquired. On the other hand, when you call it with
NoLock, you don't get those nice behaviors.

So I'm inclined to think that any place in the source code that calls
RangeVarGetRelidExtended with NoLock is a bug, and we should be trying
to get rid of the cases we still have, not adding more.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company