Re: Application name patch - v4

2009/12/2 Tom Lane <tgl@sss.pgh.pa.us>:
> Dave Page <dpage@pgadmin.org> writes:
>> On Tue, Dec 1, 2009 at 4:19 PM, Tom Lane <tgl@sss.pgh.pa.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

Given that this can probably be considered an *extremely* safe patch
:-), I say go for it. It'll certainly make for less error reports
around something that's not an error.

If the patch was in any way complex I'd object against it, but this
clearly isn't...

-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/