Thread: BUG #2246: Only call pg_fe_getauthname if none given

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Right offhand I like the idea of pushing it into connectOptions2 --- can
> you experiment with that?  Seems like there is no reason to call
> Kerberos if the user supplies the name to connect as.

Patch attached.  After looking through the code around this I discovered
that conninfo_parse is also called by PQconndefaults.  This patch
changes the 'default' returned for user to NULL (instead of whatever
pg_fe_getauthname returns).  This is probably better than not calling
Kerberos in pg_fe_getauthname and potentially returning the wrong thing
(if the username doesn't match the princ, a common situation), but it
might break existing applications.  Are those applications wrong to be
asking libpq to provide what it thinks the username is when asking for
the defaults?  I'd say probably yes, but then we don't provide another
way for them to get it either.

Patch tested w/ 'trust', 'ident', 'md5' and 'krb5' methods from psql.

    Enjoy,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Right offhand I like the idea of pushing it into connectOptions2 --- can
>> you experiment with that?  Seems like there is no reason to call
>> Kerberos if the user supplies the name to connect as.

> Patch attached.  After looking through the code around this I discovered
> that conninfo_parse is also called by PQconndefaults.  This patch
> changes the 'default' returned for user to NULL (instead of whatever
> pg_fe_getauthname returns).

This is probably not a good idea --- changing the API behavior in
pursuit of saving a few cycles is just going to get people mad at us.

After looking more closely, I see that there is inefficiency only in the
PQsetdbLogin case --- in the PQconnectdb case, we only bother to compute
"fallback" values for parameters where they're actually needed.  I think
that at the time this code was last restructured, the assumption was
that PQsetdbLogin would die soon and there wasn't any need to worry
about whether it wastes a few cycles.  If you don't subscribe to that
argument, probably the best answer is to keep the fallback-computation
loop (fe-connect.c lines 2681-2735 in CVS tip) more or less as-is, but
split it out from conninfo_parse, and somehow arrange for PQsetdbLogin
to be able to insert the values that it wants before the fallbacks are
computed.  The trick here is that PQsetdbLogin wants to work on a PGconn
not a PQconninfoOption array, and that isn't going to work nicely.

I think we'd have to refactor the code so that PQsetdbLogin gets a
PQconninfoOption array, overrides values *in that array*, then calls the
fallback-substitution code etc.  Not sure if it's worth the trouble.
The extra complexity of searching the array for values to override could
eat up the cycles we're hoping to save, too :-(

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> This is probably not a good idea --- changing the API behavior in
> pursuit of saving a few cycles is just going to get people mad at us.

Fair enough.

> I think we'd have to refactor the code so that PQsetdbLogin gets a
> PQconninfoOption array, overrides values *in that array*, then calls the
> fallback-substitution code etc.  Not sure if it's worth the trouble.
> The extra complexity of searching the array for values to override could
> eat up the cycles we're hoping to save, too :-(

Perhaps I'm missing something obvious (and if so, I'm sorry) but
couldn't we just build up the character array in PQsetdbLogin to be
passed in to connectOptions1?  If we do that we could probably also
merge the two connectOptions...  That would simplify things a great deal
I think and would also avoid the extra processing to pick up the
'defaults'...

    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Perhaps I'm missing something obvious (and if so, I'm sorry) but
> couldn't we just build up the character array in PQsetdbLogin to be
> passed in to connectOptions1?

That's a possibility too, though by the time you've finished building
that string (with appropriate quoting) and then tearing it apart again
in conninfo_parse, it's likely that you've eaten the cycles you hoped to
save ... especially since that processing would happen for all uses of
PQsetdbLogin, whether or not they could avoid calling pg_fe_getauthname.

I'm starting to feel that it's not worth messing with, which is
obviously the same conclusion we came to last time round ...

            regards, tom lane