Application name patch - v3

Hi Dave,

On Thursday 22 October 2009 15:07:13 Dave Page wrote:

Updated patch attached. Per discussion, this:
- Changes the envvar name to PGAPPNAME
- Removes support for setting application_name in the startup packet,
and instead sends an explicit SET command as part of the connection
setup in PQconnectPoll. In order to avoid adding to the
application-visible connection states, this is overloaded on the
CONNECTION_SETENV state which is only used in the v2 protocol at
present and seems like an ideal fit for such a similar use.

I had some free time so I started to take a look at that patch:

+ PostgresPollingStatusType
+ pqAppnamePoll(PGconn *conn)
...
+ 			case APPNAME_STATE_OPTION_WAIT:
...
+ 				else
+ 				{
+ 					/* Query finished, so we're done */
+ 					conn->setenv_state = APPNAME_STATE_IDLE;
+ 					return PGRES_POLLING_OK;
+ 				}
+ 				break;
+ 			}
Shouldnt that set appname_state?

The attached patch fixes this and also a couple occurances of trailing
whitespace.

What about pg_dump/psql setting fallback_application_name?

Andres

Hi Andres,

On Thu, Nov 12, 2009 at 11:32 PM, Andres Freund <andres@anarazel.de> wrote:

I had some free time so I started to take a look at that patch:

+ PostgresPollingStatusType
+ pqAppnamePoll(PGconn *conn)
...
+                       case APPNAME_STATE_OPTION_WAIT:
...
+                               else
+                               {
+                                       /* Query finished, so we're done */
+                                       conn->setenv_state = APPNAME_STATE_IDLE;
+                                       return PGRES_POLLING_OK;
+                               }
+                               break;
+                       }
Shouldnt that set appname_state?

Yup, well spotted.

The attached patch fixes this and also a couple occurances of trailing
whitespace.

Thanks.

What about pg_dump/psql setting fallback_application_name?

Per Tom, I'm waiting on the possible new array-based libpq connect API
which will make a conversion of those utilities from PQsetdbLogin a
lot cleaner than moving to PQconnectdb (and all the ugly connection
string building that would require).

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Hi,

On Thursday 22 October 2009 15:07:13 Dave Page wrote:

Updated patch attached. Per discussion, this:

- Changes the envvar name to PGAPPNAME
- Removes support for setting application_name in the startup packet,
and instead sends an explicit SET command as part of the connection
setup in PQconnectPoll. In order to avoid adding to the
application-visible connection states, this is overloaded on the
CONNECTION_SETENV state which is only used in the v2 protocol at
present and seems like an ideal fit for such a similar use.

Other features are as per the last version.

One more question: Per my reading of the discussion (which very well might be
flawed), wasnt the plan to limit the availale characters in the application
name to ascii?

Greetings,

Andres

On Wed, Nov 25, 2009 at 1:22 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On Thursday 22 October 2009 15:07:13 Dave Page wrote:

Updated patch attached. Per discussion, this:

- Changes the envvar name to PGAPPNAME
- Removes support for setting application_name in the startup packet,
and instead sends an explicit SET command as part of the connection
setup in PQconnectPoll. In order to avoid adding to the
application-visible connection states, this is overloaded on the
CONNECTION_SETENV state which is only used in the v2 protocol at
present and seems like an ideal fit for such a similar use.

Other features are as per the last version.

One more question: Per my reading of the discussion (which very well might be
flawed), wasnt the plan to limit the availale characters in the application
name to ascii?

That was suggested, but I thought the eventual outcome was to not bother.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

On Wednesday 25 November 2009 14:28:14 Dave Page wrote:

On Wed, Nov 25, 2009 at 1:22 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On Thursday 22 October 2009 15:07:13 Dave Page wrote:

Updated patch attached. Per discussion, this:

- Changes the envvar name to PGAPPNAME
- Removes support for setting application_name in the startup packet,
and instead sends an explicit SET command as part of the connection
setup in PQconnectPoll. In order to avoid adding to the
application-visible connection states, this is overloaded on the
CONNECTION_SETENV state which is only used in the v2 protocol at
present and seems like an ideal fit for such a similar use.

Other features are as per the last version.

One more question: Per my reading of the discussion (which very well
might be flawed), wasnt the plan to limit the availale characters in the
application name to ascii?

That was suggested, but I thought the eventual outcome was to not bother.

Then I dont see any reason to delay any further (sorry!). I personally would
prefer making it a bit more strict but it surely is not imporant.

Markes as ready for comitter.

Andres

Dave Page <dpage@pgadmin.org> writes:

On Wed, Nov 25, 2009 at 1:22 PM, Andres Freund <andres@anarazel.de> wrote:

One more question: Per my reading of the discussion (which very well might be
flawed), wasnt the plan to limit the availale characters in the application
name to ascii?

That was suggested, but I thought the eventual outcome was to not bother.

I think that's really essential, not optional. The proposed patch will
transfer the application name from one backend to another without any
encoding conversion. If it contains non-ASCII characters that will
result in injection of badly-encoded data inside the backend, which is
something we have been trying hard to avoid in recent versions.

The only other thing you could do about this would be to try to convert
the data from the source backend's encoding to the target's. Which
would lead to assorted failure scenarios when no conversion is possible.

ISTM restricting the name to ASCII-only is the most reasonable tradeoff.
Of course, as a speaker of English I may be a bit biased here --- but
doing nothing about the issue doesn't seem acceptable.

regards, tom lane

On Wednesday 25 November 2009 23:01:35 Tom Lane wrote:

Dave Page <dpage@pgadmin.org> writes:

On Wed, Nov 25, 2009 at 1:22 PM, Andres Freund <andres@anarazel.de> wrote:

One more question: Per my reading of the discussion (which very well
might be flawed), wasnt the plan to limit the availale characters in the
application name to ascii?

That was suggested, but I thought the eventual outcome was to not bother.

I think that's really essential, not optional. The proposed patch will
transfer the application name from one backend to another without any
encoding conversion. If it contains non-ASCII characters that will
result in injection of badly-encoded data inside the backend, which is
something we have been trying hard to avoid in recent versions.

Isn't that similarly the case with pg_stat_activity?

ISTM restricting the name to ASCII-only is the most reasonable tradeoff.
Of course, as a speaker of English I may be a bit biased here --- but
doing nothing about the issue doesn't seem acceptable.

I actually having a hard time imaging a use case where this would be a real
problem...

I have to admit though that while I am not from a English speaking country but
from Germany the amount of non ASCII chars used there routinely is not that
big, so ...

Andres

Andres Freund <andres@anarazel.de> writes:

On Wednesday 25 November 2009 23:01:35 Tom Lane wrote:

I think that's really essential, not optional. The proposed patch will
transfer the application name from one backend to another without any
encoding conversion. If it contains non-ASCII characters that will
result in injection of badly-encoded data inside the backend, which is
something we have been trying hard to avoid in recent versions.

Isn't that similarly the case with pg_stat_activity?

Well, we do still have some un-plugged holes there, but that's not an
excuse for adding more.

regards, tom lane

On Wed, Nov 25, 2009 at 10:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

ISTM restricting the name to ASCII-only is the most reasonable tradeoff.
Of course, as a speaker of English I may be a bit biased here --- but
doing nothing about the issue doesn't seem acceptable.

OK - something like this? Should keep non-printable/control characters
out of logs too...

static const char *
assign_application_name(const char *newval, bool doit, GucSource source)
{
/* Only allow clean ASCII chars in the application name */
int x;

char *repval = guc_malloc(ERROR, strlen(newval) + 1);
repval[0] = 0;

for (x=0; x<strlen(newval); x++)
{
if (newval[x] < 32 || newval[x] > 126)
repval[x] = '?';
else
repval[x] = newval[x];
}

repval[x+1] = 0;
return repval;
}

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Dave Page <dpage@pgadmin.org> writes:

OK - something like this? Should keep non-printable/control characters
out of logs too...

Personally I'd use guc_strdup and then modify the string in-place,
but that's just a matter of taste I guess. Otherwise it seems
reasonable.

regards, tom lane

Le 13/11/2009 12:11, Dave Page a �crit :

[...]

What about pg_dump/psql setting fallback_application_name?

Per Tom, I'm waiting on the possible new array-based libpq connect API
which will make a conversion of those utilities from PQsetdbLogin a
lot cleaner than moving to PQconnectdb (and all the ugly connection
string building that would require).

Is it still to be done? I don't see psql pr pg_dump set an application
name on alpha 3. There are also pg_restore, vacuumdb, reindexdb, etc.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On Sun, Dec 27, 2009 at 11:15 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:

Le 13/11/2009 12:11, Dave Page a écrit :

[...]

What about pg_dump/psql setting fallback_application_name?

Per Tom, I'm waiting on the possible new array-based libpq connect API
which will make a conversion of those utilities from PQsetdbLogin a
lot cleaner than moving to PQconnectdb (and all the ugly connection
string building that would require).

Is it still to be done? I don't see psql pr pg_dump set an application
name on alpha 3. There are also pg_restore, vacuumdb, reindexdb, etc.

Yes, still waiting on the new API.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Le 28/12/2009 10:07, Dave Page a �crit :

On Sun, Dec 27, 2009 at 11:15 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:

Le 13/11/2009 12:11, Dave Page a �crit :

[...]

What about pg_dump/psql setting fallback_application_name?

Per Tom, I'm waiting on the possible new array-based libpq connect API
which will make a conversion of those utilities from PQsetdbLogin a
lot cleaner than moving to PQconnectdb (and all the ugly connection
string building that would require).

Is it still to be done? I don't see psql pr pg_dump set an application
name on alpha 3. There are also pg_restore, vacuumdb, reindexdb, etc.

Yes, still waiting on the new API.

Is there something I can do to make this move forward?

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 10:07, Dave Page a �crit :

Yes, still waiting on the new API.

Is there something I can do to make this move forward?

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

regards, tom lane

Le 28/12/2009 17:06, Tom Lane a �crit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 10:07, Dave Page a �crit :

Yes, still waiting on the new API.

Is there something I can do to make this move forward?

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

I'm interested in working on this. But I don't find the thread that talk
about this. I feel pretty dumb, but I re-read every mail on "Application
name patch - v2", "Application name patch - v3", and "Application name
patch - v4" threads. I also re-read the "Client application name"
thread. The only mail I see that relates to the new API is the one from
Dave (the one I answered today).

So, can someone point me to the thread that deals with this "new
array-based libpq connect API"? or can someone explain it to me?

Thanks.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 17:06, Tom Lane a �crit :

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

I'm interested in working on this. But I don't find the thread that talk
about this.

Try here
http://archives.postgresql.org/message-id/4AAE8CCF.9070808@esilo.com

regards, tom lane

Le 28/12/2009 22:59, Tom Lane a �crit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 17:06, Tom Lane a �crit :

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

I'm interested in working on this. But I don't find the thread that talk
about this.

Try here
http://archives.postgresql.org/message-id/4AAE8CCF.9070808@esilo.com

Thanks. I've read all the "new version of PQconnectdb" and "Determining
client_encoding from client locale" threads. I think I understand the
goal. Still need to re-read this one
(http://archives.postgresql.org/message-id/6222.1253734019@sss.pgh.pa.us) and
completely understand it (will probably need to look at the code, at
least the PQconnectdb one). But I'm definitely working on this.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Le 29/12/2009 00:03, Guillaume Lelarge a �crit :

Le 28/12/2009 22:59, Tom Lane a �crit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 17:06, Tom Lane a �crit :

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

I'm interested in working on this. But I don't find the thread that talk
about this.

Try here
http://archives.postgresql.org/message-id/4AAE8CCF.9070808@esilo.com

Thanks. I've read all the "new version of PQconnectdb" and "Determining
client_encoding from client locale" threads. I think I understand the
goal. Still need to re-read this one
(http://archives.postgresql.org/message-id/6222.1253734019@sss.pgh.pa.us) and
completely understand it (will probably need to look at the code, at
least the PQconnectdb one). But I'm definitely working on this.

If I try to sum up my readings so far, this is what we still have to do:

1. try the one-array approach
PGconn *PQconnectParams(const char **params)

2. try the two-arrays approach
PGconn *PQconnectParams(const char **keywords, const char **values)

Instead of doing a wrapper around PQconnectdb, we need to refactor the
whole function, so that we can get rid of the parsing of the conninfo
string (which is quite complicated).

Using psql as an example would be a good idea, AFAICT.

Am I right? did I misunderstand or forget something?

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Le 29/12/2009 14:12, Guillaume Lelarge a �crit :

Le 29/12/2009 00:03, Guillaume Lelarge a �crit :

Le 28/12/2009 22:59, Tom Lane a �crit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le 28/12/2009 17:06, Tom Lane a �crit :

I think we were stalled on the question of whether to use one array
or two parallel arrays. Do you want to try coding up a sample usage
of each possibility so we can see which one seems more useful?

I'm interested in working on this. But I don't find the thread that talk
about this.

Try here
http://archives.postgresql.org/message-id/4AAE8CCF.9070808@esilo.com

Thanks. I've read all the "new version of PQconnectdb" and "Determining
client_encoding from client locale" threads. I think I understand the
goal. Still need to re-read this one
(http://archives.postgresql.org/message-id/6222.1253734019@sss.pgh.pa.us) and
completely understand it (will probably need to look at the code, at
least the PQconnectdb one). But I'm definitely working on this.

If I try to sum up my readings so far, this is what we still have to do:

1. try the one-array approach
PGconn *PQconnectParams(const char **params)

2. try the two-arrays approach
PGconn *PQconnectParams(const char **keywords, const char **values)

Instead of doing a wrapper around PQconnectdb, we need to refactor the
whole function, so that we can get rid of the parsing of the conninfo
string (which is quite complicated).

Using psql as an example would be a good idea, AFAICT.

Am I right? did I misunderstand or forget something?

I supposed I was right since noone yell at me :)

I worked on this tonight. You'll find two patches attached, one for the
one-array approach, one for the two-arrays approach. I know some more
factoring can be done (at least, the "get the fallback resources..."
part). I'm OK to do them. I just need to know if I'm on the right track.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com