Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-20 Thread Dave Page 
Thanks, applied.

On Tue, Sep 20, 2016 at 11:39 AM, Surinder Kumar
 wrote:
> Please find updated patch with suggested changes and inline comments.
>
> Also, Incase if user tries to download csv on queries other than select, we
> are catching that exception and write into 'csv' with name 'error.csv'.
>
> On Tue, Sep 20, 2016 at 2:33 PM, Dave Page  wrote:
>>
>> On Tue, Sep 20, 2016 at 6:43 AM, Surinder Kumar
>>  wrote:
>> >
>> > On Mon, Sep 19, 2016 at 8:59 PM, Surinder Kumar
>> >  wrote:
>> >>
>> >> On Mon, Sep 19, 2016 at 8:48 PM, Dave Page  wrote:
>> >>>
>> >>> Hi
>> >>>
>> >>> On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
>> >>>  wrote:
>> >>> > Hi
>> >>> >
>> >>> > Please find attached patch with fix.
>> >>> >>
>> >>> >> Download as CSV
>> >>> >>  button should be enabled only for SELECT queries.
>> >>>
>> >>> I'm not sure there's a way to do this without parsing the query.
>> >>> Simply matching on ^select certainly won't work reliably - for
>> >>> example, there could be multiple statements in the script, or it could
>> >>> be PERFORM ...  or UPDATE ... RETURNING ...
>> >>
>> >> I wasn't aware of such cases.
>> >>>
>> >>>
>> >>> I think we need to leave the button enabled, but give the user a
>> >>> message if no data is returned, e.g.
>> >>
>> >> ok. sure.
>> >
>> > I discussed the issue with Harshal and this is how download csv works:
>> > We just hit the url to download csv, and let the browser handle the
>> > response
>> > from the server side. we don't have access to response data to check if
>> > no
>> > data is returned.
>>
>> Hmm, yeah that's true.
>>
>> > The other approach is to hit the same query twice. First time an ajax
>> > query
>> > to check if data is returned or not. if returned, fire another query to
>> > download csv otherwise set message in message panel.
>> > But the limitation with this approach is that we are just increasing
>> > load
>> > over server by hitting same query twice, incase returned data rows in
>> > billions.
>> > Thoughts ?
>>
>> Yeah, we can't do that.
>>
>> The only thing I can think of right now is to simply write the error
>> message to the CSV file, e.g.
>>
>> "The query executed did not return any data."
>>
>> (with the quotes, so it's valid CSV). That way at least the user will
>> always get a file downloaded, and will see the error when they open
>> it.
>
> Thats' done.
>>
>>
>> --
>> 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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-20 Thread Surinder Kumar 
Please find updated patch with suggested changes and inline comments.

Also, Incase if user tries to download csv on queries other than select, we
are catching that exception and write into 'csv' with name 'error.csv'.

On Tue, Sep 20, 2016 at 2:33 PM, Dave Page  wrote:

> On Tue, Sep 20, 2016 at 6:43 AM, Surinder Kumar
>  wrote:
> >
> > On Mon, Sep 19, 2016 at 8:59 PM, Surinder Kumar
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 8:48 PM, Dave Page  wrote:
> >>>
> >>> Hi
> >>>
> >>> On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
> >>>  wrote:
> >>> > Hi
> >>> >
> >>> > Please find attached patch with fix.
> >>> >>
> >>> >> Download as CSV
> >>> >>  button should be enabled only for SELECT queries.
> >>>
> >>> I'm not sure there's a way to do this without parsing the query.
> >>> Simply matching on ^select certainly won't work reliably - for
> >>> example, there could be multiple statements in the script, or it could
> >>> be PERFORM ...  or UPDATE ... RETURNING ...
> >>
> >> I wasn't aware of such cases.
> >>>
> >>>
> >>> I think we need to leave the button enabled, but give the user a
> >>> message if no data is returned, e.g.
> >>
> >> ok. sure.
> >
> > I discussed the issue with Harshal and this is how download csv works:
> > We just hit the url to download csv, and let the browser handle the
> response
> > from the server side. we don't have access to response data to check if
> no
> > data is returned.
>
> Hmm, yeah that's true.
>
> > The other approach is to hit the same query twice. First time an ajax
> query
> > to check if data is returned or not. if returned, fire another query to
> > download csv otherwise set message in message panel.
> > But the limitation with this approach is that we are just increasing load
> > over server by hitting same query twice, incase returned data rows in
> > billions.
> > Thoughts ?
>
> Yeah, we can't do that.
>
> The only thing I can think of right now is to simply write the error
> message to the CSV file, e.g.
>
> "The query executed did not return any data."
>
> (with the quotes, so it's valid CSV). That way at least the user will
> always get a file downloaded, and will see the error when they open
> it.
>
​Thats' done.​

>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


RM1592_v1.patch
Description: Binary data

-- 
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-20 Thread Dave Page 
On Tue, Sep 20, 2016 at 6:43 AM, Surinder Kumar
 wrote:
>
> On Mon, Sep 19, 2016 at 8:59 PM, Surinder Kumar
>  wrote:
>>
>> On Mon, Sep 19, 2016 at 8:48 PM, Dave Page  wrote:
>>>
>>> Hi
>>>
>>> On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
>>>  wrote:
>>> > Hi
>>> >
>>> > Please find attached patch with fix.
>>> >>
>>> >> Download as CSV
>>> >>  button should be enabled only for SELECT queries.
>>>
>>> I'm not sure there's a way to do this without parsing the query.
>>> Simply matching on ^select certainly won't work reliably - for
>>> example, there could be multiple statements in the script, or it could
>>> be PERFORM ...  or UPDATE ... RETURNING ...
>>
>> I wasn't aware of such cases.
>>>
>>>
>>> I think we need to leave the button enabled, but give the user a
>>> message if no data is returned, e.g.
>>
>> ok. sure.
>
> I discussed the issue with Harshal and this is how download csv works:
> We just hit the url to download csv, and let the browser handle the response
> from the server side. we don't have access to response data to check if no
> data is returned.

Hmm, yeah that's true.

> The other approach is to hit the same query twice. First time an ajax query
> to check if data is returned or not. if returned, fire another query to
> download csv otherwise set message in message panel.
> But the limitation with this approach is that we are just increasing load
> over server by hitting same query twice, incase returned data rows in
> billions.
> Thoughts ?

Yeah, we can't do that.

The only thing I can think of right now is to simply write the error
message to the CSV file, e.g.

"The query executed did not return any data."

(with the quotes, so it's valid CSV). That way at least the user will
always get a file downloaded, and will see the error when they open
it.

-- 
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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-19 Thread Surinder Kumar 
On Mon, Sep 19, 2016 at 8:59 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> On Mon, Sep 19, 2016 at 8:48 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
>>  wrote:
>> > Hi
>> >
>> > Please find attached patch with fix.
>> >>
>> >> Download as CSV
>> >>  button should be enabled only for SELECT queries.
>>
>> I'm not sure there's a way to do this without parsing the query.
>> Simply matching on ^select certainly won't work reliably - for
>> example, there could be multiple statements in the script, or it could
>> be PERFORM ...  or UPDATE ... RETURNING ...
>>
> I wasn't aware of such cases.
>
>>
>> I think we need to leave the button enabled, but give the user a
>> message if no data is returned, e.g.
>>
> ​ok.​ sure.
>
​I discussed the issue with Harshal and this is how download csv works:
​We just hit the url to download csv, and let the browser handle the
response from the server side. we don't have access to response data to
check if no data is returned.

The other approach is to hit the same query twice. First time an ajax query
to check if data is returned or not. if returned, fire another query to
download csv otherwise set message in message panel.
But the limitation with this approach is that we are just increasing load
over server by hitting same query twice, incase returned data rows in
billions.
Thoughts ?

>
>> ===
>> Download as CSV
>> -
>>
>> The query executed did not return any data.
>>
>> [OK]
>> 
>
>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-19 Thread Surinder Kumar 
On Mon, Sep 19, 2016 at 8:48 PM, Dave Page  wrote:

> Hi
>
> On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
>  wrote:
> > Hi
> >
> > Please find attached patch with fix.
> >>
> >> Download as CSV
> >>  button should be enabled only for SELECT queries.
>
> I'm not sure there's a way to do this without parsing the query.
> Simply matching on ^select certainly won't work reliably - for
> example, there could be multiple statements in the script, or it could
> be PERFORM ...  or UPDATE ... RETURNING ...
>
I wasn't aware of such cases.

>
> I think we need to leave the button enabled, but give the user a
> message if no data is returned, e.g.
>
​ok.​ sure.

>
> ===
> Download as CSV
> -
>
> The query executed did not return any data.
>
> [OK]
> 


> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [pgadmin-hackers] [pgAdmin4][Patch]: RM1592 - Download as CSV should be supported for DDL

2016-09-19 Thread Dave Page 
Hi

On Mon, Sep 19, 2016 at 3:03 PM, Surinder Kumar
 wrote:
> Hi
>
> Please find attached patch with fix.
>>
>> Download as CSV
>>  button should be enabled only for SELECT queries.

I'm not sure there's a way to do this without parsing the query.
Simply matching on ^select certainly won't work reliably - for
example, there could be multiple statements in the script, or it could
be PERFORM ...  or UPDATE ... RETURNING ...

I think we need to leave the button enabled, but give the user a
message if no data is returned, e.g.

===
Download as CSV
-

The query executed did not return any data.

[OK]


-- 
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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers