Re: Re: Server side cursor limitations for on demand loading of data in query tool [RM2137] [pgAdmin4]

Hi,

Here is rebased patch.

--
*Harshal Dhumal*
*Sr. Software Engineer*

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 12, 2017 at 1:28 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

> Hi,
>
> Pls find updated patch for on demand loading feature.
>
> Changes:
> 1. Added row number.
> 2. load renaming result set if user performs select all operation either
> on grid or column.
> 3. Fixed Row selection issue and row past issue.
> 4. Updated existing jasmine test cases.
>
>
> --
> *Harshal Dhumal*
> *Sr. Software Engineer*
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Tue, May 9, 2017 at 10:02 PM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> --
>> *Harshal Dhumal*
>> *Sr. Software Engineer*
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Tue, May 9, 2017 at 1:08 AM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>> wrote:
>>
>>> Hi Harshal!
>>>
>>> We applied your patch and ran the Javascript tests and realized there
>>> are 6 tests failing. After that we replicate the failures by hand and found
>>> the following issues:
>>>
>>> - Clicking in the checkboxes of the rows does not select the row anymore.
>>> - When copying and pasting (any) data, we realized that it copied an
>>> empty dataset.
>>>
>>> Thanks for pointing out these issues. I have fixed them at my end and
>> also remaining tests are failing because in earlier implementation of sql
>> editor we have used pain 2Darray to provide data to slick grid and In this
>> patch I have used Slick grid DataView
>> <https://github.com/mleibman/SlickGrid/wiki/DataView> to provide data to
>> slick gird. I'll need to update test cases to support this DataView change.
>> I'll send updated patch with all of the above changes along with suggestion
>> given by Dave.
>>
>>
>>
>>> We didn't review the patch further because there aren't any tests for
>>> the newly implemented functionality.
>>>
>>> Is there a place to find a dataset with 96k rows that you guys were
>>> testing with?
>>>
>>
>> Execute below SQL queries to create table dummy_data with dataset of
>> 100k records. (you can change number of records to generate by replacing
>> 100000 in below SQL query)
>>
>>
>>
>> *CREATE TABLE public.dummy_data*
>> *(*
>> * id serial NOT NULL, *
>> * name text*
>> *) *
>> *WITH (*
>> * OIDS = FALSE*
>> *)*
>> *;*
>> *ALTER TABLE public.dummy_data*
>> * OWNER TO postgres;*
>>
>>
>> *INSERT INTO public.dummy_data(*
>> * id, name)*
>> * SELECT generate_series(1,100000), 'dummy name';*
>>
>>
>>
>>
>>
>>
>>
>>>
>>> Note: To run the Javascript test you can use the following commands
>>> ```
>>> $ cd web
>>> $ yarn install
>>> $ yarn run karma start --single-run
>>> ```
>>>
>>> Thanks
>>> Joao & Sarah
>>>
>>> On Mon, May 8, 2017 at 9:16 AM, Harshal Dhumal <
>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> --
>>>> *Harshal Dhumal*
>>>> *Sr. Software Engineer*
>>>>
>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>> On Mon, May 8, 2017 at 4:49 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> This is fantastic! The speedup in the query tool is phenomenal with
>>>>> large result sets. For 96K rows, I'm now seeing 1 second until I can browse
>>>>> the data, vs. 13 in v1.4 (and 22 secs in pgAdmin 3)!! Awesome work Harshal
>>>>> :-)
>>>>>
>>>>> Questions/comments:
>>>>>
>>>>> - Can we put the row number in the left-hand column, to the right of
>>>>> the checkbox? This patch highlights just how much value that had in pgAdmin
>>>>> 3 (I hadn't realised we had missed it until now)
>>>>>
>>>>> - If the user clicks the checkbox to select all rows, we need to
>>>>> retrieve them all at that time, otherwise they may be inadvertently working
>>>>> with a truncated result set.
>>>>>
>>>>> Sure I'll do above both the changes.
>>>>
>>>>
>>>>> - Are any changes needed to ensure the Download button works? I'm
>>>>> seeing missing rows in my test here, but that could be because of the known
>>>>> issues there with Unicode.
>>>>>
>>>>> This is fixed and checked in.
>>>>
>>>>
>>>>
>>>>> Thanks!
>>>>>
>>>>>
>>>>> On Sun, May 7, 2017 at 6:10 PM, Harshal Dhumal <
>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Here is patch for initial implementation of on demand loading of
>>>>>> result set for query tool and datagrid.
>>>>>>
>>>>>> --
>>>>>> *Harshal Dhumal*
>>>>>> *Sr. Software Engineer*
>>>>>>
>>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>> On Tue, Apr 25, 2017 at 5:21 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Tue, Apr 25, 2017 at 8:41 AM, Harshal Dhumal <
>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Dave,
>>>>>>>>
>>>>>>>> To implement feature #2137
>>>>>>>> <https://redmine.postgresql.org/issues/2137> we'll need to use
>>>>>>>> server cursor. However server cursor has some
>>>>>>>> limitation.
>>>>>>>> For eg.
>>>>>>>> 1. It cannot execute BEGIN; query (basically user cannot start new
>>>>>>>> database transaction)
>>>>>>>> 2. In case if Auto commit is true then we try to execute user
>>>>>>>> queries inside BEGIN and END when ever it's possible even though user has
>>>>>>>> not put BEGIN and END in his query.
>>>>>>>>
>>>>>>>> Also not all queries executed using Query tool produces records as
>>>>>>>> result. So can we assume only
>>>>>>>> queries started with SELECT should be executed using server cursor
>>>>>>>> to support on demand loading.
>>>>>>>> Or should we give user an option to use on demand loading like we
>>>>>>>> have options for Auto commit? and Auto rollback?
>>>>>>>> In case of on demand loading option user will be responsible to
>>>>>>>> execute correct queries (queries which can be executed using server cursor)
>>>>>>>>
>>>>>>>> Let me know your opinion on this.
>>>>>>>>
>>>>>>>
>>>>>>> Hmm, those are good points.
>>>>>>>
>>>>>>> So, as a first step, there's no absolute requirement to use a server
>>>>>>> side cursor here. The results can be materialised in libpq/psycopg2
>>>>>>> (perhaps using an async query), then transferred to the client in batches
>>>>>>> as described in the ticket.
>>>>>>>
>>>>>>> I think this would be a significant improvemet - we can re-visit the
>>>>>>> possibility of using server side cursors in the future when we have more
>>>>>>> ability to parse the query string before executing it (something we will
>>>>>>> want to do when we merge query tool/edit grid functionality).
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>>>
>>>
>>
>