Обсуждение: 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? 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
> -----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
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
> -----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
> -----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
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
> -----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
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
> -----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
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
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
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