Thread: Query grid

Query grid

From
Andreas Pflug
Date:
After several weeks I succeeded in reconnecting my win32 machine and
have a look at pgadmin stuff again. I fired up the query tool and my
first impression was "can we have this a little less ugly please?" when
I saw those grey spaces. Is the tool expected to display up to 10G of rows?

I remember the comment how fast this should have become, so I tried
"SELECT * from pg_attribute" on a small db. I had 3614 rows in
600+1400ms on a pre-grid version (ancient 700MHz Athlon), and 720 rows
in 600+47000ms on the grid version, with 2894 rows dropped!!!! I killed
another attempt after 5 minutes.

IIRC, Edward mentioned he used the original wxGridTable implementation
because it worked good for him, and consequently I didn't find any
SetTable() call. As I mentioned earlier and , an inevitable requirement
is the usage of virtual row/col management, to improve performance on
larger resultsets. The new ctlSqlGrid fails to implement it, and thus
fails even on result sets that can't really be called big.

This implementation is clearly a bad regression. Please revert ASAP and
come back with a non-grid version (virtual listbox or listview), I still
fail to see any advantage of wxGrid for this usage.

Regards,
Andreas

Re: Query grid

From
"Dave Page"
Date:

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 03 April 2006 14:22
> To: edigeronimo@xtracards.com; Dave Page
> Cc: pgadmin-hackers
> Subject: Query grid
>
> After several weeks I succeeded in reconnecting my win32
> machine and have a look at pgadmin stuff again. I fired up
> the query tool and my first impression was "can we have this
> a little less ugly please?" when I saw those grey spaces. Is
> the tool expected to display up to 10G of rows?

Well, those ugly grey spaces look identical to those in your View Data
code so any fix should be applied to both controls.

> I remember the comment how fast this should have become, so I
> tried "SELECT * from pg_attribute" on a small db. I had 3614 rows in
> 600+1400ms on a pre-grid version (ancient 700MHz Athlon), and 720 rows
> in 600+47000ms on the grid version, with 2894 rows
> dropped!!!! I killed another attempt after 5 minutes.

Yes, it was much faster. See the benchmarks I posted to the list at
http://www.pgadmin.org/archives/pgadmin-hackers/2006-02/msg00155.php

[For a 100,000 row query]
52.277 Secs + 9.123 Secs (v1.5, with your patch)
52.276 Secs + 39.688 Secs (v1.4.1)

However, I am seeing dismal performance today, so something has got
borked at some point.

> IIRC, Edward mentioned he used the original wxGridTable
> implementation because it worked good for him, and
> consequently I didn't find any
> SetTable() call. As I mentioned earlier and , an inevitable
> requirement is the usage of virtual row/col management, to
> improve performance on larger resultsets. The new ctlSqlGrid
> fails to implement it, and thus fails even on result sets
> that can't really be called big.

Originally it was a virtual table, however it was found to be faster yet
to use the built in table. I wonder now however, if when I tested the
later version of the patch I was actually testing the wrong version.
Maybe that needs to be reimplemented.

> This implementation is clearly a bad regression. Please
> revert ASAP and come back with a non-grid version (virtual
> listbox or listview), I still fail to see any advantage of
> wxGrid for this usage.

It has been proven to be significantly faster. I will not revert the
patch unless it proves impossible to fix the problem you are seeing,
which seems unlikely given the speedup which I saw on the earlier
versions. Reverting every patch because it is found not to be perfect
when first applied would be a very good way of ensuring that nothing
ever gets done.

Regards, Dave

Re: Query grid

From
Andreas Pflug
Date:
Dave Page wrote:
>
>
>
>>-----Original Message-----
>>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
>>Sent: 03 April 2006 14:22
>>To: edigeronimo@xtracards.com; Dave Page
>>Cc: pgadmin-hackers
>>Subject: Query grid
>>
>>After several weeks I succeeded in reconnecting my win32
>>machine and have a look at pgadmin stuff again. I fired up
>>the query tool and my first impression was "can we have this
>>a little less ugly please?" when I saw those grey spaces. Is
>>the tool expected to display up to 10G of rows?
>
>
> Well, those ugly grey spaces look identical to those in your View Data
> code so any fix should be applied to both controls.
>
>
>>I remember the comment how fast this should have become, so I
>>tried "SELECT * from pg_attribute" on a small db. I had 3614 rows in
>>600+1400ms on a pre-grid version (ancient 700MHz Athlon), and 720 rows
>>in 600+47000ms on the grid version, with 2894 rows
>>dropped!!!! I killed another attempt after 5 minutes.
>
>
> Yes, it was much faster. See the benchmarks I posted to the list at
> http://www.pgadmin.org/archives/pgadmin-hackers/2006-02/msg00155.php
>
> [For a 100,000 row query]
> 52.277 Secs + 9.123 Secs (v1.5, with your patch)
> 52.276 Secs + 39.688 Secs (v1.4.1)
>
> However, I am seeing dismal performance today, so something has got
> borked at some point.
>
>
>>IIRC, Edward mentioned he used the original wxGridTable
>>implementation because it worked good for him, and
>>consequently I didn't find any
>>SetTable() call. As I mentioned earlier and , an inevitable
>>requirement is the usage of virtual row/col management, to
>>improve performance on larger resultsets. The new ctlSqlGrid
>>fails to implement it, and thus fails even on result sets
>>that can't really be called big.
>
>
> Originally it was a virtual table, however it was found to be faster yet
> to use the built in table. I wonder now however, if when I tested the
> later version of the patch I was actually testing the wrong version.
> Maybe that needs to be reimplemented.

The builtin table can *never* be faster than a clean virtual implementation.

>
>
>>This implementation is clearly a bad regression. Please
>>revert ASAP and come back with a non-grid version (virtual
>>listbox or listview), I still fail to see any advantage of
>>wxGrid for this usage.
>
>
> It has been proven to be significantly faster. I will not revert the
> patch unless it proves impossible to fix the problem you are seeing,
> which seems unlikely given the speedup which I saw on the earlier
> versions. Reverting every patch because it is found not to be perfect
> when first applied would be a very good way of ensuring that nothing
> ever gets done.

IMNSHO this patch is rotten from the ground. wxGrid is known to be
flakey, extending its use is a bad idea.  The speed issue is not a
question of grid or non-grid, it's a question of virtual data/display
management.

Regards,
Andreas

Re: Query grid

From
"Dave Page"
Date:

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 03 April 2006 14:50
> To: Dave Page
> Cc: edigeronimo@xtracards.com; pgadmin-hackers
> Subject: Re: [pgadmin-hackers] Query grid
>
> IMNSHO this patch is rotten from the ground. wxGrid is known to be
> flakey, extending its use is a bad idea.  The speed issue is not a
> question of grid or non-grid, it's a question of virtual data/display
> management.

As you know, there is no alternative that can provide the other
functionality that people want (copy/paste etc), and we've already seen
that the grid can be significantly faster than the list.

I know you will say that the tool has only one purpose from your point
of view, but regardless of that, pgAdmin *is* a generic tool intended to
meet the needs of the majority of it's hundreds of thousands of users,
and that includes the query tool which many will be comparing with SQL's
Query Analyzer and similar programs. Having seen the grid work at more
than acceptable speeds, I see no reason that it cannot fulfill both use
cases once the bugs are ironed out. And that is precisely why it's in
SVN trunk and all the resulting snapshots etc.

Regards, Dave

Re: Query grid

From
"Dave Page"
Date:

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 03 April 2006 15:29
> To: Dave Page
> Subject: Re: [pgadmin-hackers] Query grid
>
> Dave Page wrote:
> >
> >
> >
> >>-----Original Message-----
> >>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> >>Sent: 03 April 2006 14:50
> >>To: Dave Page
> >>Cc: edigeronimo@xtracards.com; pgadmin-hackers
> >>Subject: Re: [pgadmin-hackers] Query grid
> >>
> >>IMNSHO this patch is rotten from the ground. wxGrid is known to be
> >>flakey, extending its use is a bad idea.  The speed issue is not a
> >>question of grid or non-grid, it's a question of virtual
> data/display
> >>management.
> >
> >
> > As you know, there is no alternative that can provide the other
> > functionality that people want (copy/paste etc),
>
> The old listview already _does_ support copying.

Only of complete rows. You cannot copy individual cells, columns or
arbitrary regions.

> and we've already seen
> > that the grid can be significantly faster than the list.
>
> This is plain wrong. I never said the current listview
> implementation is good; it isn't. But to rewrite it with
> wxVIRTUAL or however the style is called is for sure easier
> than doing it with wxGrid.

Actually you've said on a number of occasions that the current
implementation is limited by the OS's underlying controls; for example:

http://archives.postgresql.org/pgsql-performance/2005-10/msg00248.php

Which implies that we need a better control, not a fix to our code.


> > I know you will say that the tool has only one purpose from
> your point
> > of view, but regardless of that, pgAdmin *is* a generic
> tool intended
> > to meet the needs of the majority of it's hundreds of thousands of
> > users, and that includes the query tool which many will be
> comparing
> > with SQL's Query Analyzer and similar programs.
>
> Which can do better in exactly which aspect?

Copy/paste?

/D

Re: Query grid

From
Andreas Pflug
Date:
Dave Page wrote:

>
>
>>and we've already seen
>>
>>>that the grid can be significantly faster than the list.
>>
>>This is plain wrong. I never said the current listview
>>implementation is good; it isn't. But to rewrite it with
>>wxVIRTUAL or however the style is called is for sure easier
>>than doing it with wxGrid.
>
>
> Actually you've said on a number of occasions that the current
> implementation is limited by the OS's underlying controls; for example:

Yes, when using the non-virtual versions. They're managing their data
most ineffectively. That's why the TODO item is there for ages now.

If the current implementation isn't rewritten, I'll commit a virtual
rewrite of wxListView based ctlSqlResult (as soon as I find the time,
which will certainly not be this week). It will probably require more
work on frmQuery (remove the ctl filling loop, no need for the second
timing display) than on the ctl itself, making it incompatible to
ctlSqlResult implementations that are not managing their data virtually.

Regards,
Andreas

Re: Query grid

From
"Dave Page"
Date:

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 03 April 2006 16:06
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] Query grid
>
> If the current implementation isn't rewritten, I'll commit a virtual
> rewrite of wxListView based ctlSqlResult (as soon as I find the time,
> which will certainly not be this week). It will probably require more
> work on frmQuery (remove the ctl filling loop, no need for the second
> timing display) than on the ctl itself, making it incompatible to
> ctlSqlResult implementations that are not managing their data
> virtually.

Yeah, well hold fire on that - I just found what broke it, and now get
(second of 2 runs in each version, from the same server):

1.5.0:

-- Executing query:
select * from pbx_log limit 10000

Total query runtime: 8187 ms.
Data retrieval runtime: 1062 ms.
10000 rows retrieved.


1.4.1:

-- Executing query:
select * from pbx_log limit 10000

Total query runtime: 8187 ms.
Data retrieval runtime: 9125 ms.
10000 rows retrieved.

I was killing that query after a few minutes in 1.5.0 until I found the
bug (introduced here:
http://svn.pgadmin.org/cgi-bin/viewcvs.cgi?rev=5068&view=rev).

I'll find a netter way of solving that issue and commit the appropriate
fixes.

Now I don't think you can deny that with or without a virtual table,
that definitely works better than the listview does - and that was
tested on Mac and *nix. If you can improve it further then feel free,
but please do not break the new copy 'n' paste capabilities.

Regards, Dave

Re: Query grid

From
Andreas Pflug
Date:
Dave Page wrote:
>
>
>
>>-----Original Message-----
>>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
>>Sent: 03 April 2006 16:06
>>To: Dave Page
>>Cc: pgadmin-hackers
>>Subject: Re: [pgadmin-hackers] Query grid
>>
>>If the current implementation isn't rewritten, I'll commit a virtual
>>rewrite of wxListView based ctlSqlResult (as soon as I find the time,
>>which will certainly not be this week). It will probably require more
>>work on frmQuery (remove the ctl filling loop, no need for the second
>>timing display) than on the ctl itself, making it incompatible to
>>ctlSqlResult implementations that are not managing their data
>>virtually.
>
>
> Yeah, well hold fire on that - I just found what broke it, and now get
> (second of 2 runs in each version, from the same server):
>
> 1.5.0:
>
> -- Executing query:
> select * from pbx_log limit 10000
>
> Total query runtime: 8187 ms.
> Data retrieval runtime: 1062 ms.
> 10000 rows retrieved.

A correct implementation has *no* retrieval time, just some microseconds
of setting up the virtual control.

Regards,
Andreas


Re: Query grid

From
"Dave Page"
Date:

> -----Original Message-----
> From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
> Sent: 03 April 2006 16:40
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: Re: [pgadmin-hackers] Query grid
>
> A correct implementation has *no* retrieval time, just some
> microseconds of setting up the virtual control.

Unless I'm misunderstanding, what you would like to see is a class
derived from wxGridTableBase which is attached to the grid using
SetTable()? If so, that is exactly what Ed implemented originally, and
it *was* slower than what is there now. If not, then please explain
further.

Regardless of that, I have now committed a fix to the issue you were
complaining about, and the SVN code should be ~8 times quicker than the
old wxListView version.

Regards, Dave

Re: Query grid

From
"Edward Di Geronimo Jr."
Date:
Andreas Pflug wrote:

> A correct implementation has *no* retrieval time, just some
> microseconds of setting up the virtual control.

Do you have any advice on how exactly to implement that behavior?

It looked like the View Data grid did it by only retrieving the actual
results as they were needed. When I started on this, you were rather
firm that the way the results were retrieved should not be altered in
any way.

If you want, I'll look into merging the virtual table from the View Data
grid with the results grid.

Ed

Re: Query grid

From
Andreas Pflug
Date:
Dave Page wrote:
>
>
>
>>-----Original Message-----
>>From: Andreas Pflug [mailto:pgadmin@pse-consulting.de]
>>Sent: 03 April 2006 16:40
>>To: Dave Page
>>Cc: pgadmin-hackers
>>Subject: Re: [pgadmin-hackers] Query grid
>>
>>A correct implementation has *no* retrieval time, just some
>>microseconds of setting up the virtual control.
>
>
> Unless I'm misunderstanding, what you would like to see is a class
> derived from wxGridTableBase which is attached to the grid using
> SetTable()? If so, that is exactly what Ed implemented originally, and
> it *was* slower than what is there now.

Then the implementation was wrong. ATM I'm not sure if wxGridTableBase
has virtual methods, and if it had I wouldn't trust them.

Regards,
Andreas

Re: Query grid

From
Andreas Pflug
Date:
Edward Di Geronimo Jr. wrote:
> Andreas Pflug wrote:
>
>> A correct implementation has *no* retrieval time, just some
>> microseconds of setting up the virtual control.
>
>
> Do you have any advice on how exactly to implement that behavior?
>
> It looked like the View Data grid did it by only retrieving the actual
> results as they were needed. When I started on this, you were rather
> firm that the way the results were retrieved should not be altered in
> any way.
>
> If you want, I'll look into merging the virtual table from the View Data
> grid with the results grid.

As I said in an earlier mail, I'd expect more work on frmQuery than on
ctlSqlResult(wxListView) to make it virtual. Implementing it will be
probable faster for me than explaining what to do. I'm short on time, so
if you insist on using wxGrid you're on your own.

Regards,
Andreas