Robert Haas <robertmh...@gmail.com> writes:
> On Thu, Dec 6, 2018 at 12:50 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>> No. You need to do AIM *after* obtaining the lock, else you still
>> have the race condition that you can execute a query on a table
>> without being aware of recent DDL on it.
> Huh? The call in relation_openrv_extended() happens before acquiring the lock.
Oh, I was looking at the wrong AIM call :-(. You're talking about this
one:
/*
* Check for shared-cache-inval messages before trying to open the
* relation. This is needed even if we already hold a lock on the
* relation, because GRANT/REVOKE are executed without taking any lock on
* the target relation, and we want to be sure we see current ACL
* information. We can skip this if asked for NoLock, on the assumption
* that such a call is not the first one in the current command, and so we
* should be reasonably up-to-date already. (XXX this all could stand to
* be redesigned, but for the moment we'll keep doing this like it's been
* done historically.)
*/
if (lockmode != NoLock)
AcceptInvalidationMessages();
which is indeed about as bletcherous as it could possibly be.
The ideal thing would be for GRANT/REVOKE to act more like other DDL,
but I'm not sure how we get to that point: REVOKE SELECT, at the very
least, would have to take AccessExclusiveLock so that it'd block SELECT.
People doing things like GRANT/REVOKE ON ALL TABLES would doubtless
complain about the greatly increased risk of deadlock from taking a
pile of AELs. And that still would only fix the problem for table
privileges, not privileges on other object types that we have no
consistent locking scheme for.
I can see the attraction of replacing these AIM calls (and, probably,
the one in AtStart_Cache) with a single call that occurs somewhere
during statement startup. Not sure exactly where that should be;
but there is certainly not anything principled about doing it in
relation_open_xxx().
regards, tom lane
