Re: Cache invalidation after authentication (on-the-fly role creation)

Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> On Thu, Jul 5, 2018 at 9:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> That seems like a *really* ad-hoc place to put it. Why should it be
>> there, and not (say) somewhere inside InitializeSessionUserId, or maybe
>> (also?) inside PerformAuthentication? Why do the existing call sites for
>> AcceptInvalidationMessages --- in particular, the ones associated with
>> lock acquisition --- not solve the problem already?

> There is no lock acquisition involved here. The sequence is:
>
> 1. ClientAuthentication()->hba_getauthmethod()->check_hba()->get_role_oid()
> tries to look up user "fred" and doesn't find it (that lookup is used
> for group membership checks for hba line matching purposes, and not
> finding it here is not fatal, assuming you match with "all"); the
> cache now has a negative entry.
>
> 2. The configured authentication method runs, and via a side
> connection it creates the role "fred".
>
> 3. InitializeSessionUserId() looks up "fred", and finds the stale
> negative entry.

[ thinks about that for awhile... ] So really this is an instance of
a generic problem, which is that the result of a syscache lookup could
be stale: it's not guaranteed to reflect changes that committed since
the last AcceptInvalidationMessages call, which in the worst case is
the start of the current transaction. We have workarounds in place
that (mostly?) guarantee that relation-related lookups will get
sufficiently up-to-date data, but there's nothing much protecting other
catalogs such as pg_proc or pg_authid.

I can't help thinking we're going to need to fix that eventually. But
I can't think of any easy way to do so. Adding AcceptInvalidationMessages
to SearchSysCache itself is right out for performance reasons, I'm afraid,
unless we can somehow find a way to make the no-op path through it much
cheaper than it is now.

> I suppose the call to AcceptInvalidationMessages() could go at the end
> of ClientAuthentication(). That'd be closer to the code that creates
> the negative entry and immediately after the code that might modify
> the catalogs. Or in PeformAuthentication(), its caller. Or in
> IntializeSessionUserId() immediately before we try to look up the
> name, but that's also called by background workers that don't need it.

I think my preference would be to put it in InitializeSessionUserId
so that it's clearly associated with the syscache lookups we're trying
to protect. I don't entirely buy your argument that background workers
don't need up-to-date info for this.

regards, tom lane