Re: Application name patch - v4

Dave Page <dpage(at)pgadmin(dot)org> writes:
> On Tue, Dec 1, 2009 at 4:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I don't think that we need to bump the protocol version. The real
>> alternative here would be that libpq sends a startup packet that
>> includes application_name, and if it gets an error back from that,
>> it starts over without the app name.

> I looked (briefly) at doing that when we first ran into this
> suggestion. As you pointed out at the time, it seemed like that would
> require some fairly ugly hackery in fe-connect.c

I've committed a change for this. It turns out not to be quite as ugly
as I thought, and in fact quite a bit less code than the other method.
The reason it's less intertwined with the other retry logic than I was
expecting is that the server only looks at the startup options after
it's completed the authentication process. So the failure retry for
this amounts to an outer loop around the SSL and protocol-version
retries. Logically anyway --- as far as the actual code goes it's
another path in the state machine, and just requires a few more lines.

I tested it with some simple cases such as password authentication,
but it would be good to confirm that it does the right thing in more
complex cases like SSL prefer/allow/require and Kerberos auth. Anyone
set up to try CVS HEAD against an older server with configurations
like that?

BTW, it strikes me that it would only be a matter of a couple of lines
to persuade older servers to ignore application_name in the startup
packet, instead of throwing a tantrum. Obviously we must make libpq
work against unpatched older servers, but if we can save a connection
cycle (and some bleating in the postmaster log) when talking to an 8.5
application, it might be worth doing:

*** src/backend/tcop/postgres.c.orig Thu Jun 18 06:08:08 2009
--- src/backend/tcop/postgres.c Wed Dec 2 00:05:05 2009
***************
*** 3159,3164 ****
--- 3159,3168 ----
value = lfirst(gucopts);
gucopts = lnext(gucopts);

+ /* Ignore application_name for compatibility with 8.5 libpq */
+ if (strcmp(name, "application_name") == 0)
+ continue;
+
if (IsSuperuserConfigOption(name))
PendingConfigOption(name, value);
else

If we patch the back branches like that, anyone who's annoyed by the
extra connection cycle just has to update to latest minor release
of their server to make it work more smoothly. Comments?

regards, tom lane