Application name patch - v4

Started by Dave Pageover 16 years ago46 messageshackers
Jump to latest
#1Dave Page
dpage@pgadmin.org

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

Attachments:

appname-v4.diffapplication/octet-stream; name=appname-v4.diffDownload+400-103
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dave Page (#1)
Re: Application name patch - v4

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

#3Joshua Tolley
eggyknap@gmail.com
In reply to: Tom Lane (#2)
Re: Application name patch - v4

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

#4Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#2)
Re: Application name patch - v4

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

#5Robert Haas
robertmhaas@gmail.com
In reply to: Joshua Tolley (#3)
Re: Application name patch - v4

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

#6Dave Page
dpage@pgadmin.org
In reply to: Tom Lane (#2)
Re: Application name patch - v4

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

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dave Page (#6)
Re: Application name patch - v4

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

#8Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#7)
Re: Application name patch - v4

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

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#8)
Re: Application name patch - v4

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 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

#10Florian Pflug
fgp@phlo.org
In reply to: Tom Lane (#9)
Re: Application name patch - v4

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

#11Fujii Masao
masao.fujii@gmail.com
In reply to: Tom Lane (#2)
Re: Application name patch - v4

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

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Fujii Masao (#11)
Re: Application name patch - v4

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

#13Fujii Masao
masao.fujii@gmail.com
In reply to: Tom Lane (#12)
Re: Application name patch - v4

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

#14Fujii Masao
masao.fujii@gmail.com
In reply to: Fujii Masao (#13)
Re: Application name patch - v4

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

#15Andres Freund
andres@anarazel.de
In reply to: Florian Pflug (#10)
Re: Application name patch - v4

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

#16Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#9)
Re: Application name patch - v4

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

#17Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#16)
Re: Application name patch - v4

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 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

#18Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Robert Haas (#17)
Re: Application name patch - v4

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

#19Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#18)
Re: Application name patch - v4

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?

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

#20Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#19)
Re: Application name patch - v4

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'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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#19)
#22Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#21)
#23Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#21)
#24Dave Page
dpage@pgadmin.org
In reply to: Andres Freund (#22)
#25Andres Freund
andres@anarazel.de
In reply to: Dave Page (#24)
#26Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Dave Page (#24)
#27Dave Page
dpage@pgadmin.org
In reply to: Heikki Linnakangas (#26)
#28Andres Freund
andres@anarazel.de
In reply to: Heikki Linnakangas (#26)
#29Tatsuo Ishii
t-ishii@sra.co.jp
In reply to: Tom Lane (#21)
#30Marko Kreen
markokr@gmail.com
In reply to: Heikki Linnakangas (#26)
#31Brar Piening
brar@gmx.de
In reply to: Dave Page (#24)
#32Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dave Page (#27)
#33Dave Page
dpage@pgadmin.org
In reply to: Tom Lane (#32)
#34Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dave Page (#33)
#35Dave Page
dpage@pgadmin.org
In reply to: Tom Lane (#34)
#36Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#34)
#37Tom Lane
tgl@sss.pgh.pa.us
In reply to: Marko Kreen (#36)
#38Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#37)
#39Tom Lane
tgl@sss.pgh.pa.us
In reply to: Marko Kreen (#38)
#40Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#39)
#41Tom Lane
tgl@sss.pgh.pa.us
In reply to: Marko Kreen (#40)
#42Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#41)
#43Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dave Page (#33)
#44Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#43)
#45Dave Page
dpage@pgadmin.org
In reply to: Magnus Hagander (#44)
#46Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#44)