* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > The attached patch fixes a bug which was originally brought up in May
> > of 2002 in this thread:
>
> Now that I've looked at it, I find this patch seems fairly wrongheaded.
> AFAICS the entire point of the original coding is to allow the setup
> work needed to create the krb5_context etc to be amortized across
> multiple connections. The patch destroys that advantage, but yet keeps
> as much as it can of the notational cruft induced by the original
> design -- for instance, there seems little point in the
> pg_krb5_initialised flag if we aren't ever going to have any
> pre-initialized state.
I'm honestly not entirely sure I agree about that being the point of the
original coding but regardless the crux of the problem here is that
there's no way to get libpq to use a cache other than the one it's
initialized with for a given session. That part of the Kerberos API
which supports that isn't exposed in any way beyond the KRB5CCNAME
environment variable and the call to ask for the 'default' ccache is
called with the static variable the second time and it ignores the
request when there's an already valid (it thinks) ccache.
> I have little idea of how expensive the operations called by
> pg_krb5_init really are. If they are expensive then it'd probably
> make sense to keep the current static variables but treat 'em as a
> one-element cache, ie, recompute if a new user name is being demanded.
> If not, we ought to be able to simplify some things.
We'd have to recompute based on the KRB5CCNAME environment variable
changing, which is certainly an option. It's not necessairly the case
that the username is changing, possibly just the cache. Additionally,
the calls themselves are not very expensive when being called on an
existing cache, the most expensive thing is reaching out to the KDC to
get a new service ticket which will either need to be done, or won't,
depending on if a valid service ticket already exists in the cache or
not.
> Another point here is how all this interacts with thread safety.
> If we get rid of the static variables, do we still need the
> pglock_thread() operations?
Good question, I'm afraid probably not. I'd have to look through it
again but last I checked MIT Kerberos prior to 1.4 (and I'm not 100%
sure it's resolved in 1.4) wasn't threadsafe itself.
I'd certainly be happy to rework the patch based on these comments, of
course. Honestly, I'm pretty sure the original patch was intended to be
minimal (and is for the most part). These changes would introduce more
logic but if that's alright I'd be happy to do it.
Thanks,
Stephen
