Thread: Application name patch - v4
Updated application name patch, including a GUC assign hook to clean the application name of any unsafe characters, per discussion. Regards, Dave -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Attachment
Dave Page <dpage@pgadmin.org> writes: > Updated application name patch, including a GUC assign hook to clean > the application name of any unsafe characters, per discussion. Applied with assorted editorialization. There were a couple of definitional issues that I don't recall if we had consensus on: 1. The patch prevents non-superusers from seeing other users' application names in pg_stat_activity. This seems at best pretty debatable to me. Yes, it supports usages in which you want to put security-sensitive information into the appname, but at the cost of disabling (perfectly reasonable) usages where you don't. If we made the app name universally visible, people simply wouldn't put security sensitive info in it, the same as they don't put it on the command line. Should we change this? (While I'm looking at it, I wonder why client_addr and client_port are similarly hidden.) 2. I am wondering if we should mark application_name as GUC_NO_RESET_ALL. As-is, the value sent at libpq initialization will be lost during RESET ALL, which would probably surprise people. On the other hand, not resetting it might surprise other people. If we were able to send it in the startup packet then this wouldn't be a problem, but we are far from being able to do that. Comments? regards, tom lane
On Sat, Nov 28, 2009 at 06:47:49PM -0500, Tom Lane wrote: > Dave Page <dpage@pgadmin.org> writes: > > Updated application name patch, including a GUC assign hook to clean > > the application name of any unsafe characters, per discussion. > > Applied with assorted editorialization. There were a couple of > definitional issues that I don't recall if we had consensus on: > > 1. The patch prevents non-superusers from seeing other users' > application names in pg_stat_activity. This seems at best pretty > debatable to me. Yes, it supports usages in which you want to put > security-sensitive information into the appname, but at the cost of > disabling (perfectly reasonable) usages where you don't. If we made > the app name universally visible, people simply wouldn't put security > sensitive info in it, the same as they don't put it on the command line. > Should we change this? > > (While I'm looking at it, I wonder why client_addr and client_port > are similarly hidden.) I vote for showing it to everyone, superuser or otherwise, though I can't really say why I feel that way. > 2. I am wondering if we should mark application_name as > GUC_NO_RESET_ALL. As-is, the value sent at libpq initialization > will be lost during RESET ALL, which would probably surprise people. > On the other hand, not resetting it might surprise other people. > If we were able to send it in the startup packet then this wouldn't > be a problem, but we are far from being able to do that. Nothing I've written uses RESET ALL, but if it did, I expect it would be because whatever the connection was being used for in the past differs substantially from whatever I plan to use it for in the future, which seems a suitable time also to change application_name. I vote against GUC_NO_RESET_ALL. -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
On Sunday 29 November 2009 00:47:49 Tom Lane wrote: > Dave Page <dpage@pgadmin.org> writes: > > Updated application name patch, including a GUC assign hook to clean > > the application name of any unsafe characters, per discussion. > > Applied with assorted editorialization. There were a couple of > definitional issues that I don't recall if we had consensus on: > > 1. The patch prevents non-superusers from seeing other users' > application names in pg_stat_activity. This seems at best pretty > debatable to me. Yes, it supports usages in which you want to put > security-sensitive information into the appname, but at the cost of > disabling (perfectly reasonable) usages where you don't. If we made > the app name universally visible, people simply wouldn't put security > sensitive info in it, the same as they don't put it on the command line. > Should we change this? I personally would prefer if it were not protected and explicitly documented as such - I cant really see a use case where one would want to store something really private in there. > (While I'm looking at it, I wonder why client_addr and client_port > are similarly hidden.) In a shared hosting environment this is somewhat sensible - afair some data protection laws even require that nobody except the designated receiver of information is able to get that information. Whether shared hosting is sensible is another matter. > 2. I am wondering if we should mark application_name as > GUC_NO_RESET_ALL. As-is, the value sent at libpq initialization > will be lost during RESET ALL, which would probably surprise people. > On the other hand, not resetting it might surprise other people. > If we were able to send it in the startup packet then this wouldn't > be a problem, but we are far from being able to do that. One possibility would be to make it possible to issue SETs that behave as if set in a startup packet - imho its an implementation detail that SET currently is used. Andres
On Sat, Nov 28, 2009 at 7:27 PM, Joshua Tolley <eggyknap@gmail.com> wrote: > On Sat, Nov 28, 2009 at 06:47:49PM -0500, Tom Lane wrote: >> Dave Page <dpage@pgadmin.org> writes: >> > Updated application name patch, including a GUC assign hook to clean >> > the application name of any unsafe characters, per discussion. >> >> Applied with assorted editorialization. There were a couple of >> definitional issues that I don't recall if we had consensus on: >> >> 1. The patch prevents non-superusers from seeing other users' >> application names in pg_stat_activity. This seems at best pretty >> debatable to me. Yes, it supports usages in which you want to put >> security-sensitive information into the appname, but at the cost of >> disabling (perfectly reasonable) usages where you don't. If we made >> the app name universally visible, people simply wouldn't put security >> sensitive info in it, the same as they don't put it on the command line. >> Should we change this? >> >> (While I'm looking at it, I wonder why client_addr and client_port >> are similarly hidden.) > > I vote for showing it to everyone, superuser or otherwise, though I can't > really say why I feel that way. +1. >> 2. I am wondering if we should mark application_name as >> GUC_NO_RESET_ALL. As-is, the value sent at libpq initialization >> will be lost during RESET ALL, which would probably surprise people. >> On the other hand, not resetting it might surprise other people. >> If we were able to send it in the startup packet then this wouldn't >> be a problem, but we are far from being able to do that. > > Nothing I've written uses RESET ALL, but if it did, I expect it would be > because whatever the connection was being used for in the past differs > substantially from whatever I plan to use it for in the future, which seems a > suitable time also to change application_name. I vote against > GUC_NO_RESET_ALL. +1 to this, too. ...Robert
On Sat, Nov 28, 2009 at 11:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dave Page <dpage@pgadmin.org> writes: >> Updated application name patch, including a GUC assign hook to clean >> the application name of any unsafe characters, per discussion. > > Applied with assorted editorialization. There were a couple of > definitional issues that I don't recall if we had consensus on: > > 1. The patch prevents non-superusers from seeing other users' > application names in pg_stat_activity. This seems at best pretty > debatable to me. Yes, it supports usages in which you want to put > security-sensitive information into the appname, but at the cost of > disabling (perfectly reasonable) usages where you don't. If we made > the app name universally visible, people simply wouldn't put security > sensitive info in it, the same as they don't put it on the command line. > Should we change this? Uh, yeah, I guess. That wasn't a concious decision, more a copy n paste inherited 'feature'. > (While I'm looking at it, I wonder why client_addr and client_port > are similarly hidden.) > > 2. I am wondering if we should mark application_name as > GUC_NO_RESET_ALL. As-is, the value sent at libpq initialization > will be lost during RESET ALL, which would probably surprise people. > On the other hand, not resetting it might surprise other people. > If we were able to send it in the startup packet then this wouldn't > be a problem, but we are far from being able to do that. In the use cases I envisage for this, the appname is more a property of the connection than the session, thus I wouldn't expect it to change following a RESET ALL. That said, one could then argue that it should RESET to the connection-time value... I think we should use GUC_NO_RESET_ALL. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page <dpage@pgadmin.org> writes: > On Sat, Nov 28, 2009 at 11:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 1. The patch prevents non-superusers from seeing other users' >> application names in pg_stat_activity. �This seems at best pretty >> debatable to me. �Yes, it supports usages in which you want to put >> security-sensitive information into the appname, but at the cost of >> disabling (perfectly reasonable) usages where you don't. �If we made >> the app name universally visible, people simply wouldn't put security >> sensitive info in it, the same as they don't put it on the command line. >> Should we change this? > Uh, yeah, I guess. That wasn't a concious decision, more a copy n > paste inherited 'feature'. OK. Everybody seems to agree it should not be hidden, so I'll go change that. >> 2. I am wondering if we should mark application_name as >> GUC_NO_RESET_ALL. > I think we should use GUC_NO_RESET_ALL. I agree with you, but it seems we have at least as many votes to not do that. Any other votes out there? regards, tom lane
Hi, Le 29 nov. 2009 à 18:22, Tom Lane a écrit : >> I think we should use GUC_NO_RESET_ALL. > > I agree with you, but it seems we have at least as many votes to not do > that. Any other votes out there? Driven by the pooler use case (pgbouncer, even), I'd say RESET ALL should reset also the application name. And the connectionvalue is not tied any more to something sensible as soon as you have pooling in there... Regards, -- dim
Dimitri Fontaine <dfontaine@hi-media.com> writes: > Le 29 nov. 2009 � 18:22, Tom Lane a �crit : >>> I think we should use GUC_NO_RESET_ALL. >> >> I agree with you, but it seems we have at least as many votes to not do >> that. Any other votes out there? > Driven by the pooler use case (pgbouncer, even), I'd say RESET ALL should reset also the application name. And the connectionvalue is not tied any more to something sensible as soon as you have pooling in there... The thing is that the libpq API treats application_name as a *property of the connection*. You shouldn't expect it to go away on RESET ALL, any more than you'd expect RESET ALL to cause you to be reconnected to some other database. If a pooler wants application_name to be cleared when it issues RESET ALL, I think it ought to be setting the name via SET, not via the libpq connection option. But it's certainly true that using GUC_NO_RESET_ALL would be a quick kluge rather than a proper solution. Andres Freund suggested upthread that we should fix this by extending SET: : One possibility would be to make it possible to issue SETs that behave : as if set in a startup packet - imho its an implementation detail that : SET currently is used. I think there's a good deal of merit in this, and it would't be hard at all to implement, seeing that we already have SET LOCAL and SET SESSION. We could add a third keyword, say SET DEFAULT, that would have the behavior of setting the value in a fashion that would persist across resets. I'm not sure that DEFAULT is exactly le mot juste here, but agreeing on a keyword would probably be the hardest part of making it happen. regards, tom lane
Tom Lane wrote: > : One possibility would be to make it possible to issue SETs that > behave : as if set in a startup packet - imho its an implementation > detail that : SET currently is used. > > I think there's a good deal of merit in this, and it would't be hard > at all to implement, seeing that we already have SET LOCAL and SET > SESSION. We could add a third keyword, say SET DEFAULT, that would > have the behavior of setting the value in a fashion that would > persist across resets. I'm not sure that DEFAULT is exactly le mot > juste here, but agreeing on a keyword would probably be the hardest > part of making it happen. Hm, but without a way to prevent the users of a connection pool from issuing "SET DEFAULT", that leaves a connection pool with no way to revert a connection to a known state. How about "SET CONNECTION", with an additional GUC called connection_setup which can only be set to true, never back to false. Once connection_setup is set to true, further SET CONNECTION attempts would fail. In a way, this mimics startup-packet SETs without actually doing things in the startup packet. best regards, Florian Pflug
On Sun, Nov 29, 2009 at 8:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dave Page <dpage@pgadmin.org> writes: >> Updated application name patch, including a GUC assign hook to clean >> the application name of any unsafe characters, per discussion. > > Applied with assorted editorialization. There were a couple of > definitional issues that I don't recall if we had consensus on: Why doesn't application_name appear in postgresql.conf.sample? That is expected to be set from only libpq? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Fujii Masao <masao.fujii@gmail.com> writes: > Why doesn't application_name appear in postgresql.conf.sample? > That is expected to be set from only libpq? It would seem pretty silly to set it in the conf file. You *can*, if you want, but I see no reason to list it there. regards, tom lane
On Mon, Nov 30, 2009 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Fujii Masao <masao.fujii@gmail.com> writes: >> Why doesn't application_name appear in postgresql.conf.sample? >> That is expected to be set from only libpq? > > It would seem pretty silly to set it in the conf file. You *can*, > if you want, but I see no reason to list it there. Yeah, I see your point. But, is it a policy not to put such parameter (other than that for debug) on postgresql.conf.sample? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Mon, Nov 30, 2009 at 11:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Mon, Nov 30, 2009 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Fujii Masao <masao.fujii@gmail.com> writes: >>> Why doesn't application_name appear in postgresql.conf.sample? >>> That is expected to be set from only libpq? >> >> It would seem pretty silly to set it in the conf file. You *can*, >> if you want, but I see no reason to list it there. > > Yeah, I see your point. But, is it a policy not to put such parameter > (other than that for debug) on postgresql.conf.sample? Ooops! I missed GUC_NOT_IN_SAMPLE parameters. Sorry for noise. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Hi, On Monday 30 November 2009 01:16:43 Florian G. Pflug wrote: > Tom Lane wrote: > > : One possibility would be to make it possible to issue SETs that > > > > behave : as if set in a startup packet - imho its an implementation > > detail that : SET currently is used. > > > > I think there's a good deal of merit in this, and it would't be hard > > at all to implement, seeing that we already have SET LOCAL and SET > > SESSION. We could add a third keyword, say SET DEFAULT, that would > > have the behavior of setting the value in a fashion that would > > persist across resets. I'm not sure that DEFAULT is exactly le mot > > juste here, but agreeing on a keyword would probably be the hardest > > part of making it happen. > Hm, but without a way to prevent the users of a connection pool from > issuing "SET DEFAULT", that leaves a connection pool with no way to > revert a connection to a known state. Perhaps we should only allow a few parameters to be SET as a connection default - then the pooler would have to issue those just as it has to do for actual connection defaults. > How about "SET CONNECTION", with an additional GUC called > connection_setup which can only be set to true, never back to false. > Once connection_setup is set to true, further SET CONNECTION attempts > would fail. How would that help the pooler case? The next connection to it might be from a different application. Andres
Le 30 nov. 2009 à 00:25, Tom Lane a écrit : > The thing is that the libpq API treats application_name as a *property > of the connection*. Oh. Yeah. > We could add a third keyword, say SET DEFAULT, that would have the > behavior of setting the value in a fashion that would persist across > resets. I'm not sure that DEFAULT is exactly le mot juste here, but > agreeing on a keyword would probably be the hardest part of making it > happen. I vaguely remember you explaining how hard it would be to be able to predict the value we RESET to as soon as we add thisor that possibility. That's very vague, sorry, but only leaves a bad impression on the keyword choice (bikeshedding,I should open a club). So what about SET CONNECTION application_name TO 'whatever'? Regards, -- dim
On Mon, Nov 30, 2009 at 4:11 PM, Dimitri Fontaine <dfontaine@hi-media.com> wrote: > Le 30 nov. 2009 à 00:25, Tom Lane a écrit : >> The thing is that the libpq API treats application_name as a *property >> of the connection*. > > Oh. Yeah. > >> We could add a third keyword, say SET DEFAULT, that would have the >> behavior of setting the value in a fashion that would persist across >> resets. I'm not sure that DEFAULT is exactly le mot juste here, but >> agreeing on a keyword would probably be the hardest part of making it >> happen. > > I vaguely remember you explaining how hard it would be to be able to predict the value we RESET to as soon as we add thisor that possibility. That's very vague, sorry, but only leaves a bad impression on the keyword choice (bikeshedding,I should open a club). > > So what about SET CONNECTION application_name TO 'whatever'? I still don't really understand why we wouldn't want RESET ALL to reset the application name. In what circumstances would you want the application name to stay the same across a RESET ALL? ...Robert
Le 30 nov. 2009 à 22:38, Robert Haas a écrit : > I still don't really understand why we wouldn't want RESET ALL to > reset the application name. In what circumstances would you want the > application name to stay the same across a RESET ALL? I can't see any use case, but SET/RESET is tied to SESSION whereas application_name is a CONNECTION property. So it's a hardsell that reseting the session will change connection properties. Regards, -- dim
On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine <dfontaine@hi-media.com> wrote: > Le 30 nov. 2009 à 22:38, Robert Haas a écrit : >> I still don't really understand why we wouldn't want RESET ALL to >> reset the application name. In what circumstances would you want the >> application name to stay the same across a RESET ALL? > > I can't see any use case, but SET/RESET is tied to SESSION whereas application_name is a CONNECTION property. So it's ahard sell that reseting the session will change connection properties. Is there any technical difference between a connection property and a session property? If so, what is it? ISTM that the only time you're likely going to use RESET ALL is in a connection pooling environment, and that if you're in a connection pooling environment you probably want to reset the application name along with everything else. I might be wrong, but that's how it seems to me at first blush. ...Robert
Robert Haas wrote: > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine > <dfontaine@hi-media.com> wrote: > > Le 30 nov. 2009 ? 22:38, Robert Haas a ?crit : > >> I still don't really understand why we wouldn't want RESET ALL to > >> reset the application name. ?In what circumstances would you want the > >> application name to stay the same across a RESET ALL? > > > > I can't see any use case, but SET/RESET is tied to SESSION whereas application_name is a CONNECTION property. So it'sa hard sell that reseting the session will change connection properties. > > Is there any technical difference between a connection property and a > session property? If so, what is it? > > ISTM that the only time you're likely going to use RESET ALL is in a > connection pooling environment, and that if you're in a connection > pooling environment you probably want to reset the application name > along with everything else. I might be wrong, but that's how it seems > to me at first blush. Uh, what does it mean to reset the application name? Are you resetting it to what it was before the session started, or to a blank string? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine > <dfontaine@hi-media.com> wrote: >> Le 30 nov. 2009 � 22:38, Robert Haas a �crit : >>> I still don't really understand why we wouldn't want RESET ALL to >>> reset the application name. �In what circumstances would you want the >>> application name to stay the same across a RESET ALL? >> >> I can't see any use case, but SET/RESET is tied to SESSION whereas application_name is a CONNECTION property. So it'sa hard sell that reseting the session will change connection properties. > Is there any technical difference between a connection property and a > session property? If so, what is it? The point is that every other thing you can set in a libpq connection string is persistent throughout the connection. For the ones that you can change at all, such as client_encoding, *RESET ALL actually resets it to what was specified in the connection string*. It does not satisfy the POLA for application_name to behave differently. I think the argument about poolers expecting something different is hogwash. A pooler would want RESET ALL to revert the connection state to what it was at establishment. That would include whatever application name the pooler would have specified when it started the connection, I should think. The only reason we're even having this discussion is that libpq isn't able to make application_name work exactly like its other connection parameters because of the backwards-compatibility issue. Maybe we should think a bit harder about that. Or else give up having libpq manage it like a connection parameter. regards, tom lane
On Tuesday 01 December 2009 01:11:13 Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine > > > > <dfontaine@hi-media.com> wrote: > >> Le 30 nov. 2009 à 22:38, Robert Haas a écrit : > >>> I still don't really understand why we wouldn't want RESET ALL to > >>> reset the application name. In what circumstances would you want the > >>> application name to stay the same across a RESET ALL? > >> > >> I can't see any use case, but SET/RESET is tied to SESSION whereas > >> application_name is a CONNECTION property. So it's a hard sell that > >> reseting the session will change connection properties. > > > > Is there any technical difference between a connection property and a > > session property? If so, what is it? > I think the argument about poolers expecting something different is > hogwash. A pooler would want RESET ALL to revert the connection state > to what it was at establishment. That would include whatever > application name the pooler would have specified when it started the > connection, I should think. Actually I think the poolers make a good case for a SET variant which emulates connection set variables... RESET ALL in a connection pooler does different things than RESET ALL outside of one. Andres
On 12/1/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine > > <dfontaine@hi-media.com> wrote: > >> Le 30 nov. 2009 à 22:38, Robert Haas a écrit : > >>> I still don't really understand why we wouldn't want RESET ALL to > >>> reset the application name. In what circumstances would you want the > >>> application name to stay the same across a RESET ALL? > >> > >> I can't see any use case, but SET/RESET is tied to SESSION whereas application_name is a CONNECTION property. So it'sa hard sell that reseting the session will change connection properties. > > > Is there any technical difference between a connection property and a > > session property? If so, what is it? > > > The point is that every other thing you can set in a libpq connection > string is persistent throughout the connection. For the ones that you > can change at all, such as client_encoding, *RESET ALL actually resets > it to what was specified in the connection string*. It does not satisfy > the POLA for application_name to behave differently. +1 This SESSION/CONNECITION terminology is confusing, better would be talk about client connection/session (client->pooler) and server connection/session (pooler->server) if you are talking about pooling. > I think the argument about poolers expecting something different is > hogwash. A pooler would want RESET ALL to revert the connection state > to what it was at establishment. That would include whatever > application name the pooler would have specified when it started the > connection, I should think. > > The only reason we're even having this discussion is that libpq > isn't able to make application_name work exactly like its other > connection parameters because of the backwards-compatibility issue. > Maybe we should think a bit harder about that. Or else give up > having libpq manage it like a connection parameter. Making it work in session pooling mode (pgpool) is easy - RESET ALL and SET needs to work. The question is whether it should work also in transaction pooling mode (pgbouncer / JDBC). I see 2 variants: 1. Clients are allowed to specify it only in startup packet. But, uh, poolers can set it also in the middle of session. 2. Make it into protocol-tracked variable. The 1) seems inconsistent and backwards-incompatible - client does not know server version yet and old servers dont accept it. I don't see problems with 2). Or we could decide it is not meant for transaction pooling environments. -- marko
On Tue, Dec 1, 2009 at 12:26 AM, Andres Freund <andres@anarazel.de> wrote: > Actually I think the poolers make a good case for a SET variant which emulates > connection set variables... > > RESET ALL in a connection pooler does different things than RESET ALL outside > of one. Eh? Not sure I follow that, but then I haven't had a coffee yet. I do see the argument that RESET ALL should revert user changes to application_name though, but I maintain they should reset to the value set at connection time, not to null. As has been pointed out already, other values set at connection time cannot be reset, so allowing that for application name does seem like a POLA violation. Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which could be used to set the default GUC value that RESET would revert to. This seems to me to be the ideal solution, and I'd somewhat hesitantly volunteer to work on it (hesitantly as it means touching the parser and other areas of the code I currently have no experience of). -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Tuesday 01 December 2009 09:59:17 Dave Page wrote: > On Tue, Dec 1, 2009 at 12:26 AM, Andres Freund <andres@anarazel.de> wrote: > > Actually I think the poolers make a good case for a SET variant which > > emulates connection set variables... > > > > RESET ALL in a connection pooler does different things than RESET ALL > > outside of one. > > Eh? Not sure I follow that, but then I haven't had a coffee yet. Well. RESET ALL in a pooler sets values to the initial connection values the pooler had, not the ones of pooled connection. On the same time there are multiple people complaining about such default values being contraproductive to pooling environments because they reset to the wrong values. I dont really get that argument - the pooler should just issue a SET CONNECTION DEFAULT for all connection values. That would make it far more transparent than before... > Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which > could be used to set the default GUC value that RESET would revert to. > This seems to me to be the ideal solution, and I'd somewhat hesitantly > volunteer to work on it (hesitantly as it means touching the parser > and other areas of the code I currently have no experience of). As I had initially suggested something like that I agree here. Andres
Dave Page wrote: > Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which > could be used to set the default GUC value that RESET would revert to. > This seems to me to be the ideal solution, and I'd somewhat hesitantly > volunteer to work on it (hesitantly as it means touching the parser > and other areas of the code I currently have no experience of). If an application can do SET DEFAULT, how does the connection pooler *really* reset the value back to what it was? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, Dec 1, 2009 at 9:16 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Dave Page wrote: >> Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which >> could be used to set the default GUC value that RESET would revert to. >> This seems to me to be the ideal solution, and I'd somewhat hesitantly >> volunteer to work on it (hesitantly as it means touching the parser >> and other areas of the code I currently have no experience of). > > If an application can do SET DEFAULT, how does the connection pooler > *really* reset the value back to what it was? There has to be some level of trust here :-). As the alternative would involve bumping the fe-be protocol version, it seems like a reasonable approach to me. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Tuesday 01 December 2009 10:16:45 Heikki Linnakangas wrote: > Dave Page wrote: > > Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which > > could be used to set the default GUC value that RESET would revert to. > > This seems to me to be the ideal solution, and I'd somewhat hesitantly > > volunteer to work on it (hesitantly as it means touching the parser > > and other areas of the code I currently have no experience of). > > If an application can do SET DEFAULT, how does the connection pooler > *really* reset the value back to what it was? Why does it need to? SET DEFAULT should imho only be allowed for values whcih can be set during connection initiation. For those it can simply issue the sets anyway. Andres
> The point is that every other thing you can set in a libpq connection > string is persistent throughout the connection. For the ones that you > can change at all, such as client_encoding, *RESET ALL actually resets > it to what was specified in the connection string*. It does not satisfy > the POLA for application_name to behave differently. > > I think the argument about poolers expecting something different is > hogwash. A pooler would want RESET ALL to revert the connection state > to what it was at establishment. That would include whatever > application name the pooler would have specified when it started the > connection, I should think. +1. Connection poolers shoud be transparent to the clients. If some connection poolers want to behavior differently, then probably they would be better to be called "TP monitor" or some such. TP monitor has its own API and it is at liberty behave what it wants. Don't get me wrong. I would not say TP monitor is useless, rather it has very usefull use cases I think. However, pushing its semantics about sessions to PostgreSQL side, would be counterproductive for both TP monitor and PostgreSQL. -- Tatsuo Ishii SRA OSS, Inc. Japan
On 12/1/09, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Dave Page wrote: > > Upthread, Tom suggested a new 'SET DEFAULT ...' variant of SET which > > could be used to set the default GUC value that RESET would revert to. > > This seems to me to be the ideal solution, and I'd somewhat hesitantly > > volunteer to work on it (hesitantly as it means touching the parser > > and other areas of the code I currently have no experience of). > > > If an application can do SET DEFAULT, how does the connection pooler > *really* reset the value back to what it was? By doing SET DEFAULT... There actually *is* a problem that SET DEFAULT would solve: 1) Pooler creates a connection with one default value. 2) Client creates a connection with another default value (giving param in startup pkt) 3) Pooler does SET to apply client's default values. 4) Client does SET to some random value 5) Client does RESET foo/ALL; expecting get default value from 2), instead it gets poolers default value from 1). The inconsistency would be fixed if pooler could do SET DEFAULT in 3). Note - client doing SET DEFAULT itself would not break anything. As long we are talking about protocol-tracked parameters... OTOH, the only app that exhibits the such RESET problem is src/test/regress so I'm not sure it's worth spending effort to fix this. Especially as this open door on app doing SET DEFAULT on non-tracked GUC vars, which seems to be a much bigger problem. I don't see how this SET DEFAULT would fix the appname vs. poolers problem in any way. -- marko
On Tue, 1 Dec 2009 09:59:17 +0100, Dave Page <dpage@pgadmin.org> wrote: > I do see the argument that RESET ALL should revert user changes to > application_name though, but I maintain they should reset to the value > set at connection time, not to null. As has been pointed out already, > other values set at connection time cannot be reset, so allowing that > for application name does seem like a POLA violation. > I'd like to support this Argument. As I understand this patch from http://archives.postgresql.org/pgsql-hackers/2009-10/msg00711.php it is intended to support some kind of feature like the SQL Server "...;Application Name=MyApp;..." connection string value, making the name of the user level (or whatever) application name available at the Database/SQL level. I don't know about pgpool but as far as I know, some client side connection pooling implementations use one pool per connection string/url (.Net Data Providers, JDBC). They would probably want set the application_name in the startup message and will expect it to fall back to this value when calling RESET ALL (or what ever you like to be the command to go back to the values that were requested on connection startup) on recycling a connection from the pool. Any other solution would greatly complicate recycling of connections for per connection string pooling szenarios. Regards, Brar
Dave Page <dpage@pgadmin.org> writes: > On Tue, Dec 1, 2009 at 9:16 AM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> If an application can do SET DEFAULT, how does the connection pooler >> *really* reset the value back to what it was? > There has to be some level of trust here :-). As the alternative would > involve bumping the fe-be protocol version, it seems like a reasonable > approach to me. 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. The main disadvantage would be that you'd get a double connection attempt == more overhead anytime you use an 8.5+ libpq to connect to 8.4- server. People never complained that hard about the similar double connection attempt when 7.4+ libpq connected to 7.3- servers, so maybe we should just go that way. regards, tom lane
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. The main disadvantage would > be that you'd get a double connection attempt == more overhead > anytime you use an 8.5+ libpq to connect to 8.4- server. People > never complained that hard about the similar double connection attempt > when 7.4+ libpq connected to 7.3- servers, so maybe we should just > go that way. 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 -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
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 Perhaps, but at the time it wasn't apparent that issuing a separate SET would create user-visible behavioral inconsistencies. Now that we've realized that, I think we should reconsider. If people are agreed that double connect is a better alternative I'm willing to go look at how to make it happen. regards, tom lane
On Tue, Dec 1, 2009 at 4:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If people are agreed that double connect is a better alternative I still kinda like 'SET DEFAULT', but I'm far from wed to it. A double connect certainly seems like it would be better than the inconsistency. > I'm willing to go look at how to make it happen. That's good, 'cos I'm sure it'll end up being a whole lot less ugly than if I did it :-) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On 12/1/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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 > > > Perhaps, but at the time it wasn't apparent that issuing a separate SET > would create user-visible behavioral inconsistencies. Now that we've > realized that, I think we should reconsider. > > If people are agreed that double connect is a better alternative > I'm willing to go look at how to make it happen. Is it supposed to work with pooling or not? If the pooler gets new connection with same username:database as some existing connection, but with different appname, what it is supposed to do? -- marko
Marko Kreen <markokr@gmail.com> writes: > If the pooler gets new connection with same username:database > as some existing connection, but with different appname, > what it is supposed to do? Whatever it wants to. People seem to be imagining that the appname isn't under the control of the pooler. It's a connection property, remember? It won't be set at all unless the pooler explicitly sets it or allows it to be set. I would imagine that typically a pooler would consider the whole connection string as defining connection properties and so appname would work the same as username or anything else, ie, you get shunted into a different connection pool if you ask for a different appname. regards, tom lane
On 12/1/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > If the pooler gets new connection with same username:database > > as some existing connection, but with different appname, > > what it is supposed to do? > > > Whatever it wants to. People seem to be imagining that the appname > isn't under the control of the pooler. It's a connection property, > remember? It won't be set at all unless the pooler explicitly sets it > or allows it to be set. > > I would imagine that typically a pooler would consider the whole > connection string as defining connection properties and so appname would > work the same as username or anything else, ie, you get shunted into > a different connection pool if you ask for a different appname. No, at least both pgbouncer and pgpool consider only (username, database) pair as pool identifier. Rest of the startup params are tuned on the fly. And I think that should stay that way. Instead, could we make it equal to rest of startup params and track it's changes via ParamStatus? That makes it possible for poolers to handle it transparently. (IOW, you can put several poolers between client and server and nothing breaks) -- marko
Marko Kreen <markokr@gmail.com> writes: > No, at least both pgbouncer and pgpool consider only (username, database) > pair as pool identifier. Rest of the startup params are tuned on the fly. > And I think that should stay that way. 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? regards, tom lane
On 12/1/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > > No, at least both pgbouncer and pgpool consider only (username, database) > > pair as pool identifier. Rest of the startup params are tuned on the fly. > > And I think that should stay that way. > > > 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) Any other parameter is handled via "ignore_startup_parameters" option: if client supplies random option not appearing there, it is kicked out. The point being that as pgbouncer cannot handle it transparently, the admin needs to set the param in postgresql.conf if it is important, fix the client or let pgbouncer ignore it if client is unfixable. I have no problem handling appname with latter method, I just wanted to clarify the target audience for the feature. -- marko
Marko Kreen <markokr@gmail.com> writes: > On 12/1/09, Tom Lane <tgl@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. regards, tom lane
On 12/1/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > On 12/1/09, Tom Lane <tgl@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
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
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/
On Wed, Dec 2, 2009 at 8:14 AM, Magnus Hagander <magnus@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
Magnus Hagander <magnus@hagander.net> writes: > 2009/12/2 Tom Lane <tgl@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