Обсуждение: SVN Commit by guillaume: r8145 - in trunk/pgadmin3: . pgadmin/db pgadmin/frm

Author: guillaume

Date: 2009-12-29 17:39:36 +0000 (Tue, 29 Dec 2009)

New Revision: 8145

Revision summary: http://svn.pgadmin.org/cgi-bin/viewcvs.cgi/?rev=8145&view=rev

Log:
Use application_name for each window.
Add support of the new application_name column in activity report (8.5).
(fixes ticket #116)


Modified:
   trunk/pgadmin3/CHANGELOG
   trunk/pgadmin3/pgadmin/db/pgConn.cpp
   trunk/pgadmin3/pgadmin/frm/frmConfig.cpp
   trunk/pgadmin3/pgadmin/frm/frmEditGrid.cpp
   trunk/pgadmin3/pgadmin/frm/frmHbaConfig.cpp
   trunk/pgadmin3/pgadmin/frm/frmMainConfig.cpp
   trunk/pgadmin3/pgadmin/frm/frmQuery.cpp
   trunk/pgadmin3/pgadmin/frm/frmStatus.cpp

Sorry - I didn't get a chance to actually look at this patch until now.

The application_name should be set as a connection string parameter,
not an explicit SET command. This ensures that the application name is
included in the initial connection log message, and avoids a round
trip with the SET query. PQconninfoParse can be used to test if libpq
supports application_name at runtime.

I suspect the patch is also missing some changes to the debugger.

On Tue, Dec 29, 2009 at 5:39 PM,  <svn@pgadmin.org> wrote:
> Author: guillaume
>
> Date: 2009-12-29 17:39:36 +0000 (Tue, 29 Dec 2009)
>
> New Revision: 8145
>
> Revision summary: http://svn.pgadmin.org/cgi-bin/viewcvs.cgi/?rev=8145&view=rev
>
> Log:
> Use application_name for each window.
> Add support of the new application_name column in activity report (8.5).
> (fixes ticket #116)
>
>
> Modified:
>   trunk/pgadmin3/CHANGELOG
>   trunk/pgadmin3/pgadmin/db/pgConn.cpp
>   trunk/pgadmin3/pgadmin/frm/frmConfig.cpp
>   trunk/pgadmin3/pgadmin/frm/frmEditGrid.cpp
>   trunk/pgadmin3/pgadmin/frm/frmHbaConfig.cpp
>   trunk/pgadmin3/pgadmin/frm/frmMainConfig.cpp
>   trunk/pgadmin3/pgadmin/frm/frmQuery.cpp
>   trunk/pgadmin3/pgadmin/frm/frmStatus.cpp
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>



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

Le 30/12/2009 13:53, Dave Page a écrit :
> Sorry - I didn't get a chance to actually look at this patch until now.
>

Oops, sorry. I try not to be too quick, but as you already answered on
this patch...

> The application_name should be set as a connection string parameter,
> not an explicit SET command.

That was the first thing I wanted to do. But people won't be able to use
pgAdmin with older PostgreSQL releases if we use the connection string
parameter. Or we have to attempt a first connection with it, and if that
one fails, drop the application name in the connection string. I don't
think we should bother with this. It adds error messages on old
releases. I much prefer the explicit SET command approach.

> This ensures that the application name is
> included in the initial connection log message, and avoids a round
> trip with the SET query. PQconninfoParse can be used to test if libpq
> supports application_name at runtime.
>

I don't understand that. libpq and the server are two different things.
You can have a new libpq on your client and an old server. client's
PQconninfoParse will tell you application_name is available, but it
could be that the server doesn't understand it. Perhaps I need to get a
better look at PQconninfoParse, but don't have the time right now.

> I suspect the patch is also missing some changes to the debugger.
>

Probably, I'll look at this.


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

On Wed, Dec 30, 2009 at 1:14 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
>> The application_name should be set as a connection string parameter,
>> not an explicit SET command.
>
> That was the first thing I wanted to do. But people won't be able to use
> pgAdmin with older PostgreSQL releases if we use the connection string
> parameter.

Yes they will. We changed libpq to ensure that works properly.

>> This ensures that the application name is
>> included in the initial connection log message, and avoids a round
>> trip with the SET query. PQconninfoParse can be used to test if libpq
>> supports application_name at runtime.
>>
>
> I don't understand that. libpq and the server are two different things.
> You can have a new libpq on your client and an old server. client's
> PQconninfoParse will tell you application_name is available, but it
> could be that the server doesn't understand it. Perhaps I need to get a
> better look at PQconninfoParse, but don't have the time right now.

You need an 8.5 libpq to use application_name in the connection
string. It doesn't matter what server you have, as libpq will ignore
the parameter for older servers. To ensure you have a suitable libpq,
prepare the connection string, and feed it to PQconninfoParse. If you
get a null response, there's a problem (e.g. application_name is not
supported). If you get a non-null response, the connection string is
valid.

>> I suspect the patch is also missing some changes to the debugger.
>>
>
> Probably, I'll look at this.
>
>
> --
> Guillaume.
>  http://www.postgresqlfr.org
>  http://dalibo.com
>



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

Le 30/12/2009 18:09, Dave Page a écrit :
> On Wed, Dec 30, 2009 at 1:14 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>>> The application_name should be set as a connection string parameter,
>>> not an explicit SET command.
>>
>> That was the first thing I wanted to do. But people won't be able to use
>> pgAdmin with older PostgreSQL releases if we use the connection string
>> parameter.
>
> Yes they will. We changed libpq to ensure that works properly.
>

OK, I finally understood it. Sorry.

>>> This ensures that the application name is
>>> included in the initial connection log message, and avoids a round
>>> trip with the SET query. PQconninfoParse can be used to test if libpq
>>> supports application_name at runtime.
>>>
>>
>> I don't understand that. libpq and the server are two different things.
>> You can have a new libpq on your client and an old server. client's
>> PQconninfoParse will tell you application_name is available, but it
>> could be that the server doesn't understand it. Perhaps I need to get a
>> better look at PQconninfoParse, but don't have the time right now.
>
> You need an 8.5 libpq to use application_name in the connection
> string. It doesn't matter what server you have, as libpq will ignore
> the parameter for older servers. To ensure you have a suitable libpq,
> prepare the connection string, and feed it to PQconninfoParse. If you
> get a null response, there's a problem (e.g. application_name is not
> supported). If you get a non-null response, the connection string is
> valid.
>

OK. Here is a new patch which calls PQconninfoParse and acts according
to its return code. It also removes part of the old patch.

Is it better?

>>> I suspect the patch is also missing some changes to the debugger.
>>>
>>
>> Probably, I'll look at this.
>>

Done too.


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

On Sat, Jan 2, 2010 at 9:57 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:

> OK. Here is a new patch which calls PQconninfoParse and acts according
> to its return code. It also removes part of the old patch.
>
> Is it better?

Certainly looks more as I imagined. There is a little inconsistency though:

+            wxString applicationname = wxT("pgAdmin (Query Tool)");

...

+    wxString applicationname = wxT("pgAdmin - Query Tool");

Also, doesn't the debugger falback to the libc encoding as the main
connection class does?

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

Le 03/01/2010 10:55, Dave Page a écrit :
> On Sat, Jan 2, 2010 at 9:57 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>
>> OK. Here is a new patch which calls PQconninfoParse and acts according
>> to its return code. It also removes part of the old patch.
>>
>> Is it better?
>
> Certainly looks more as I imagined. There is a little inconsistency though:
>
> +            wxString applicationname = wxT("pgAdmin (Query Tool)");
>
> ...
>
> +    wxString applicationname = wxT("pgAdmin - Query Tool");
>

Will fix this.

> Also, doesn't the debugger falback to the libc encoding as the main
> connection class does?
>

It didn't have the fallback libc encoding code before I began to work on
this, so I didn't add it. But I can add that pretty easily.


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

On Sun, Jan 3, 2010 at 10:11 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
>> Also, doesn't the debugger falback to the libc encoding as the main
>> connection class does?
>>
>
> It didn't have the fallback libc encoding code before I began to work on
> this, so I didn't add it. But I can add that pretty easily.

It seems like an oversight, so please do if you have the time.

Thanks!


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

Le 03/01/2010 11:15, Dave Page a écrit :
> On Sun, Jan 3, 2010 at 10:11 AM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>>> Also, doesn't the debugger falback to the libc encoding as the main
>>> connection class does?
>>>
>>
>> It didn't have the fallback libc encoding code before I began to work on
>> this, so I didn't add it. But I can add that pretty easily.
>
> It seems like an oversight, so please do if you have the time.
>

Done. You'll find the new patch attached.

Other comments?


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

On Sun, Jan 3, 2010 at 1:33 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> Le 03/01/2010 11:15, Dave Page a écrit :
>> On Sun, Jan 3, 2010 at 10:11 AM, Guillaume Lelarge
>> <guillaume@lelarge.info> wrote:
>>>> Also, doesn't the debugger falback to the libc encoding as the main
>>>> connection class does?
>>>>
>>>
>>> It didn't have the fallback libc encoding code before I began to work on
>>> this, so I didn't add it. But I can add that pretty easily.
>>
>> It seems like an oversight, so please do if you have the time.
>>
>
> Done. You'll find the new patch attached.
>
> Other comments?

Nope.


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

Le 04/01/2010 10:39, Dave Page a écrit :
> On Sun, Jan 3, 2010 at 1:33 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>> Le 03/01/2010 11:15, Dave Page a écrit :
>>> On Sun, Jan 3, 2010 at 10:11 AM, Guillaume Lelarge
>>> <guillaume@lelarge.info> wrote:
>>>>> Also, doesn't the debugger falback to the libc encoding as the main
>>>>> connection class does?
>>>>>
>>>>
>>>> It didn't have the fallback libc encoding code before I began to work on
>>>> this, so I didn't add it. But I can add that pretty easily.
>>>
>>> It seems like an oversight, so please do if you have the time.
>>>
>>
>> Done. You'll find the new patch attached.
>>
>> Other comments?
>
> Nope.
>

I commited it. Thanks for your review.


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