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

Hi,

On Sat, May 13, 2017 at 12:35 AM, Joao Pedro De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> We were only able to apply the patch on 1f903ba2 (were seeing patch does
> not apply due to sqleditor.js conflicts)
> The javascript tests passed, but we were unable to copy rows or columns or
> cells when running the application. Could you run feature tests?
>
There are three modes sqleditor can be launched
1. Query tool (Tools menus -> Query Tool)
2. Datagrid. (Right click on any table/view -> View Data -> View
All/First 100/Last 100/Filtered rows)
3. Scripts (Right click on any table/view ->
INSERT/CREATE/UPDATE/DELETE/SELECT)

Paste functionality is only enabled in Datagrid and table has Primary key
otherwise it's disabled. In your case row might have been copied but you
were unable paste because you might be trying to paste the rows in Query
tool. Please try again in Datagrid with table having Primary key.

>
> Now that more functionality is being added to sqleditor.js, this may be a
> good time to extract the functionality to separate files. This will
> increase readability, and encourage separation of concerns. It will also
> make changes easier to test in isolation.
>
Ok. Let me check if I can separate out ant functionalities.

> It's probably a good idea to test the changes made to the python as well
> as javascript code. In this case, the new behavior of poll() in sqleditor
> __init__ should be tested.
>

At this point we don't have any python unit tests that can test sqleditor
backend (python code).
@Dave should I include python unit test cases in this patch?

>
> --Joao and George
>
>
> On Fri, May 12, 2017 at 8:32 AM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>> --
>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>