Обсуждение: SVN Commit by dpage: r7895 - trunk/pgadmin3/pgadmin/schema

Author: dpage

Date: 2009-06-04 12:56:00 +0100 (Thu, 04 Jun 2009)

New Revision: 7895

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

Log:
Query optimisation, per Kieran McCusker [Ashesh Vashi]



Modified:
   trunk/pgadmin3/pgadmin/schema/pgColumn.cpp

This patch fixes many problems in the handling of GPDB CSV format logs for the frmStatus display.

It's also a useful start for supporting PostgreSQL 8.4 CSV format logs.

To extend this to support PostgreSQL 8.4 CSV format log files, I think we need to do a couple of things:

First, the pg_logdir_ls function from the admin pack would need to be updated to look for .log or .csv files, not just
.log.

Then we would need to recognize that we are reading a file with extension .csv, and handle it the way I do in the patch
forGPDB (rather than being based on GetIsGreenplum()).
 

Finally, we'd just need to figure out what each field being logged by PostgreSQL 8.4 is, and update the code so that it
understandsthat as well as the GPDB fields.
 

> -----Original Message-----
> From: pgadmin-hackers-owner@postgresql.org [mailto:pgadmin-hackers-
> owner@postgresql.org] On Behalf Of Chuck McDevitt
> Sent: Thursday, June 04, 2009 7:40 PM
> To: pgadmin-hackers@postgresql.org
> Subject: [pgadmin-hackers] New patch for bugs in GPDB csv format log
> handling
> 
> This patch fixes many problems in the handling of GPDB CSV format logs
> for the frmStatus display.
> 
> It's also a useful start for supporting PostgreSQL 8.4 CSV format logs.

On Fri, Jun 5, 2009 at 8:03 PM, Chuck McDevitt<cmcdevitt@greenplum.com> wrote:
> To extend this to support PostgreSQL 8.4 CSV format log files, I think we need to do a couple of things:
>
> First, the pg_logdir_ls function from the admin pack would need to be updated to look for .log or .csv files, not
just.log. 

The deadline for submitting changes like that in PostgreSQL 8.4 was November!

> Then we would need to recognize that we are reading a file with extension .csv, and handle it the way I do in the
patchfor GPDB (rather than being based on GetIsGreenplum()). 
>
> Finally, we'd just need to figure out what each field being logged by PostgreSQL 8.4 is, and update the code so that
itunderstands that as well as the GPDB fields. 

That'll have to wait for the next cycle.

FYI, PostgreSQL and pgAdmin will go to RC1 on Thursday. The intention
is to build the GA release on 25/26 June, for release on the 29th.


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

> -----Original Message-----
> From: Dave Page [mailto:dpage@pgadmin.org]
> Sent: Saturday, June 06, 2009 2:29 PM
> To: Chuck McDevitt
> Cc: pgadmin-hackers@postgresql.org
> Subject: Re: [pgadmin-hackers] Supporting CSV format log files
>
> On Fri, Jun 5, 2009 at 8:03 PM, Chuck McDevitt<cmcdevitt@greenplum.com>
> wrote:
> > To extend this to support PostgreSQL 8.4 CSV format log files, I
> think we need to do a couple of things:
> >
> > First, the pg_logdir_ls function from the admin pack would need to be
> updated to look for .log or .csv files, not just .log.
>
> The deadline for submitting changes like that in PostgreSQL 8.4 was
> November!
>
> > Then we would need to recognize that we are reading a file with
> extension .csv, and handle it the way I do in the patch for GPDB
> (rather than being based on GetIsGreenplum()).
> >
> > Finally, we'd just need to figure out what each field being logged by
> PostgreSQL 8.4 is, and update the code so that it understands that as
> well as the GPDB fields.
>
> That'll have to wait for the next cycle.
>
> FYI, PostgreSQL and pgAdmin will go to RC1 on Thursday. The intention
> is to build the GA release on 25/26 June, for release on the 29th.

Yes, supporting PG CSV logs can certainly wait till next release cycle.

But I do need the patch I submitted to be committed, as otherwise the frmStatus display is broken for GPDB, and I'd
certainlynot want pgAdmin to come out in a form that is broken when using with GPDB. 

Yup, i intend to look at your patch on Monday.

On 6/6/09, Chuck McDevitt <cmcdevitt@greenplum.com> wrote:
>
>> -----Original Message-----
>> From: Dave Page [mailto:dpage@pgadmin.org]
>> Sent: Saturday, June 06, 2009 2:29 PM
>> To: Chuck McDevitt
>> Cc: pgadmin-hackers@postgresql.org
>> Subject: Re: [pgadmin-hackers] Supporting CSV format log files
>>
>> On Fri, Jun 5, 2009 at 8:03 PM, Chuck McDevitt<cmcdevitt@greenplum.com>
>> wrote:
>> > To extend this to support PostgreSQL 8.4 CSV format log files, I
>> think we need to do a couple of things:
>> >
>> > First, the pg_logdir_ls function from the admin pack would need to be
>> updated to look for .log or .csv files, not just .log.
>>
>> The deadline for submitting changes like that in PostgreSQL 8.4 was
>> November!
>>
>> > Then we would need to recognize that we are reading a file with
>> extension .csv, and handle it the way I do in the patch for GPDB
>> (rather than being based on GetIsGreenplum()).
>> >
>> > Finally, we'd just need to figure out what each field being logged by
>> PostgreSQL 8.4 is, and update the code so that it understands that as
>> well as the GPDB fields.
>>
>> That'll have to wait for the next cycle.
>>
>> FYI, PostgreSQL and pgAdmin will go to RC1 on Thursday. The intention
>> is to build the GA release on 25/26 June, for release on the 29th.
>
> Yes, supporting PG CSV logs can certainly wait till next release cycle.
>
> But I do need the patch I submitted to be committed, as otherwise the
> frmStatus display is broken for GPDB, and I'd certainly not want pgAdmin to
> come out in a form that is broken when using with GPDB.
>
>


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

On Fri, Jun 5, 2009 at 3:40 AM, Chuck McDevitt<cmcdevitt@greenplum.com> wrote:
> This patch fixes many problems in the handling of GPDB CSV format logs for the frmStatus display.
>
> It's also a useful start for supporting PostgreSQL 8.4 CSV format logs.

Hi,

Some issues with this, 2 minor, one less so:

- Please rearrange the code in frmStatus.h to remove the class
definition. We try to keep the headers purely for declarations and
only include one-line functions in them. I would suggest a new file in
utils/ given that it's a generally useful class.

- Status bar messages such as _("Reading log from server.") should
generally end with ... to indicate the task is ongoing.

- Messages passed to wxLogError should be translatable, and explain
the problem to the user without being phrased as a question (wxT("Log
line does not start with timestamp?: %s \n")). Messages passed to
wxLogNotice needn't be translated however.

Regards, Dave.

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

Thanks Dave.  Some questions:

> Hi,
>
> Some issues with this, 2 minor, one less so:
>
> - Please rearrange the code in frmStatus.h to remove the class
> definition. We try to keep the headers purely for declarations and
> only include one-line functions in them. I would suggest a new file in
> utils/ given that it's a generally useful class.

I would have done this already, but was worried you wouldn't accept new files this late in the cycle.
I will put the class definition in its own file.   Should I also move the class declaration to its own .h file?
I think that would be cleaner.

>
> - Status bar messages such as _("Reading log from server.") should
> generally end with ... to indicate the task is ongoing.

Will do.

>
> - Messages passed to wxLogError should be translatable, and explain
> the problem to the user without being phrased as a question (wxT("Log
> line does not start with timestamp?: %s \n")). Messages passed to
> wxLogNotice needn't be translated however.

Will do.  Hopefully, this can only be hit if a log file is corrupted.
Or should this just be a wxLogNotice, since the code is supposed to just recover any continue without any user
interaction.
I'd made it a LogError mostly to help with my debugging when I was working on the code.



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

On Mon, Jun 8, 2009 at 10:02 PM, Chuck McDevitt<cmcdevitt@greenplum.com> wrote:
> Thanks Dave.  Some questions:
>
>> Hi,
>>
>> Some issues with this, 2 minor, one less so:
>>
>> - Please rearrange the code in frmStatus.h to remove the class
>> definition. We try to keep the headers purely for declarations and
>> only include one-line functions in them. I would suggest a new file in
>> utils/ given that it's a generally useful class.
>
> I would have done this already, but was worried you wouldn't accept new files this late in the cycle.
> I will put the class definition in its own file.   Should I also move the class declaration to its own .h file?
> I think that would be cleaner.

New files are fine. I'd rather that, than cram new general purpose
classes into the header for a window class.

>> - Status bar messages such as _("Reading log from server.") should
>> generally end with ... to indicate the task is ongoing.
>
> Will do.
>
>>
>> - Messages passed to wxLogError should be translatable, and explain
>> the problem to the user without being phrased as a question (wxT("Log
>> line does not start with timestamp?: %s \n")). Messages passed to
>> wxLogNotice needn't be translated however.
>
> Will do.  Hopefully, this can only be hit if a log file is corrupted.
> Or should this just be a wxLogNotice, since the code is supposed to just recover any continue without any user
interaction.
> I'd made it a LogError mostly to help with my debugging when I was working on the code.

I'm fine with wxLogNotice.


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

Improved patch.

This also has a fix for non-superusers running the frmStatus display.

Forgot to change "Reading log from server." To "Reading log from server..."
Here is new diff

Thanks, applied.

On Tue, Jun 9, 2009 at 1:51 AM, Chuck McDevitt<cmcdevitt@greenplum.com> wrote:
> Forgot to change "Reading log from server." To "Reading log from server..."
> Here is new diff
>



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