From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: Andres Freund <andres@anarazel.de>,
        Pavel Stehule <pavel.stehule@gmail.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: slow queries over information schema.tables
In-reply-to: 
 <CA+TgmoZTAPeh8jrMaaZNNAdj9X+q=cdT-dEvg-DmuJHkXVv9KA@mail.gmail.com>
References: 
 <CAFj8pRBUCX4LZ2rA2BbEkdD6NN59mgx+BLo1gO08Wod4RLtcTg@mail.gmail.com>
 <5978.1544030694@sss.pgh.pa.us>
 <20181205175657.s7mt6as356kwvrji@alap3.anarazel.de>
 <8210.1544034143@sss.pgh.pa.us>
 <20181205182555.x4wtnzffmhwsc3c7@alap3.anarazel.de>
 <8843.1544035292@sss.pgh.pa.us>
 <CA+TgmobB=Ky_GOa4-M3afJHup3LC9FoV-tLmU--XBdV+8Do-Tg@mail.gmail.com>
 <20577.1544113962@sss.pgh.pa.us>
 <CA+TgmoaNhWkCt2MBpvhHX2ckJd=JHTqDAS1oXRdQPuVtp2MD=g@mail.gmail.com>
 <21728.1544115813@sss.pgh.pa.us>
 <CA+TgmoYrs92UR6dEuzVp-ghuj0HAhZ0iyqb3memusje9Rr3uaw@mail.gmail.com>
 <17422.1544118632@sss.pgh.pa.us>
 <CA+TgmoZTAPeh8jrMaaZNNAdj9X+q=cdT-dEvg-DmuJHkXVv9KA@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Fri, 07 Dec 2018 15:24:32 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <24822.1544220272.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 07 Dec 2018 17:04:32 -0500
Message-ID: <24823.1544220272@sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Dec 6, 2018 at 12:50 PM Tom Lane <tgl@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 assumpti=
on
     * that such a call is not the first one in the current command, and s=
o 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 b=
een
     * done historically.)
     */
    if (lockmode !=3D 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