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

Dave Page <dpage(at)pgadmin(dot)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(at)pgadmin(dot)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(at)pgadmin(dot)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(at)gmail(dot)com> wrote:
> On Sat, Nov 28, 2009 at 06:47:49PM -0500, Tom Lane wrote:
>> Dave Page <dpage(at)pgadmin(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)pgadmin(dot)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(at)pgadmin(dot)org> writes:
> On Sat, Nov 28, 2009 at 11:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 connection value is not tied any more to something sensible as soon as you have pooling in there...

Regards,
--
dim

Dimitri Fontaine <dfontaine(at)hi-media(dot)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 connection value 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)pgadmin(dot)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(dot)fujii(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)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(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Nov 30, 2009 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)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 this or 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(at)hi-media(dot)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 this or 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 hard sell that reseting the session will change connection properties.

Regards,
--
dim

On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine
<dfontaine(at)hi-media(dot)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?

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(at)hi-media(dot)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?
>
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine
> <dfontaine(at)hi-media(dot)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?

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(at)gmail(dot)com> writes:
> > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine
> >
> > <dfontaine(at)hi-media(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Nov 30, 2009 at 4:54 PM, Dimitri Fontaine
> > <dfontaine(at)hi-media(dot)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?
>
>
> 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(at)anarazel(dot)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(at)anarazel(dot)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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(at)pgadmin(dot)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(at)pgadmin(dot)org> writes:
> On Tue, Dec 1, 2009 at 9:16 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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(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.  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(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

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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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
>
>
> 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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)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(at)gmail(dot)com> writes:
> On 12/1/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > On 12/1/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(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

2009/12/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> 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

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/

On Wed, Dec 2, 2009 at 8:14 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)net> writes:
> 2009/12/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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