Application name patch - v4
in [PgSql]
|
Prev: Backup history file should be replicated in Streaming Replication?
Next: [PATCH] hstore documentation update
From: Marko Kreen on 1 Dec 2009 16:30 On 12/1/09, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Marko Kreen <markokr(a)gmail.com> writes: > > On 12/1/09, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > > >> If you're happy with handling the existing connection parameters in a given > >> way, why would you not want application_name behaving that same way? > > > Well, in pgbouncer case, the parameters tracked via ParamStatus are > > handled transparently. (client_encoding, datestyle, timezone, > > standard_conforming_strings) > > > Hmm, I had not thought about that. Is it sensible to mark > application_name as GUC_REPORT so that pgbouncer can be smart about it? > The actual overhead of such a thing would be probably be unmeasurable in > the normal case where it's only set via the startup packet, but it seems > a bit odd. IMHO it is sensible, if we really want the option to follow client. -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 2 Dec 2009 00:09 Dave Page <dpage(a)pgadmin.org> writes: > On Tue, Dec 1, 2009 at 4:19 PM, Tom Lane <tgl(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Magnus Hagander on 2 Dec 2009 03:14 2009/12/2 Tom Lane <tgl(a)sss.pgh.pa.us>: > Dave Page <dpage(a)pgadmin.org> writes: >> On Tue, Dec 1, 2009 at 4:19 PM, Tom Lane <tgl(a)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 Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Dave Page on 2 Dec 2009 03:34 On Wed, Dec 2, 2009 at 8:14 AM, Magnus Hagander <magnus(a)hagander.net> wrote: >> 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... Agreed. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 2 Dec 2009 10:19
Magnus Hagander <magnus(a)hagander.net> writes: > 2009/12/2 Tom Lane <tgl(a)sss.pgh.pa.us>: >> 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: > 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. Yeah. I wouldn't even propose this, except that given the new code an unpatched older server will log FATAL: unrecognized configuration parameter "application_name" anytime it gets a connection from newer libpq. I'm sure we'll get some complaints/bugreports about it if we allow that to be the norm. However, if we backpatch now, there will be relatively few situations in the field where anyone tries to use 8.5 libpq against an unpatched older server. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |