Re: proposal - assign result of query to psql variable

Hello

we cannot actually store result of query to psql variable

I propose a new slash statement "\eset for this purpose

Syntax:

\eset variable [, variable [..]] query -- it raise exception when
more than one row is returned or when no row is returned

Usage:

\eset var1, var2 select version(), current_date

Current workaround is not friendly and it is not usable for more than
one target variable

http://stackoverflow.com/questions/11654244/how-to-bind-a-sql-query-return-value-to-a-psql-variable/11654676#11654676

postgres=# \set myvar `psql -A -t -c "select version()" postgres `
postgres=# \echo :myvar
PostgreSQL 9.1.4 on x86_64-unknown-linux-gnu, compiled by gcc (GCC)
4.7.0 20120507 (Red Hat 4.7.0-5), 64-bit

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> \eset variable [, variable [..]] query -- it raise exception when
> more than one row is returned or when no row is returned

Better would be a variant on \g, that is you type in the query and
then tell it where to put the result. We have learned the hard way
that putting SQL commands into the arguments of backslash commands
is a horrid idea. Maybe

select x,y,... from ...
\gset var1 var2 ...

regards, tom lane

2012/7/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> \eset variable [, variable [..]] query -- it raise exception when
>> more than one row is returned or when no row is returned
>
> Better would be a variant on \g, that is you type in the query and
> then tell it where to put the result. We have learned the hard way
> that putting SQL commands into the arguments of backslash commands
> is a horrid idea. Maybe
>
> select x,y,... from ...
> \gset var1 var2 ...

it could be

Pavel

>
> regards, tom lane

On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > \eset variable [, variable [..]] query -- it raise exception when
> > more than one row is returned or when no row is returned
>
> Better would be a variant on \g, that is you type in the query and
> then tell it where to put the result. We have learned the hard way
> that putting SQL commands into the arguments of backslash commands
> is a horrid idea. Maybe
>
> select x,y,... from ...
> \gset var1 var2 ...

How about

\gset var1,,,var2,var3...

The above shows how one would skip assigning variables in the target
list, which one might want to do.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2012/7/26 David Fetter <david(at)fetter(dot)org>:
> On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote:
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> > \eset variable [, variable [..]] query -- it raise exception when
>> > more than one row is returned or when no row is returned
>>
>> Better would be a variant on \g, that is you type in the query and
>> then tell it where to put the result. We have learned the hard way
>> that putting SQL commands into the arguments of backslash commands
>> is a horrid idea. Maybe
>>
>> select x,y,... from ...
>> \gset var1 var2 ...
>
> How about
>
> \gset var1,,,var2,var3...
>

I don't like this - you can use fake variable - and ignoring some
variable has no big effect on client

Pavel

> The above shows how one would skip assigning variables in the target
> list, which one might want to do.
>
> Cheers,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
> Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote:
> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
> > On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote:
> >> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> >> > \eset variable [, variable [..]] query -- it raise exception when
> >> > more than one row is returned or when no row is returned
> >>
> >> Better would be a variant on \g, that is you type in the query and
> >> then tell it where to put the result. We have learned the hard way
> >> that putting SQL commands into the arguments of backslash commands
> >> is a horrid idea. Maybe
> >>
> >> select x,y,... from ...
> >> \gset var1 var2 ...
> >
> > How about
> >
> > \gset var1,,,var2,var3...
> >
>
> I don't like this - you can use fake variable - and ignoring some
> variable has no big effect on client

Why assign to a variable you'll never use?

Cheers,
David.

P.S. The bike shed should be puce with blaze orange pin-striping.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2012/7/26 David Fetter <david(at)fetter(dot)org>:
> On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote:
>> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
>> > On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote:
>> >> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> >> > \eset variable [, variable [..]] query -- it raise exception when
>> >> > more than one row is returned or when no row is returned
>> >>
>> >> Better would be a variant on \g, that is you type in the query and
>> >> then tell it where to put the result. We have learned the hard way
>> >> that putting SQL commands into the arguments of backslash commands
>> >> is a horrid idea. Maybe
>> >>
>> >> select x,y,... from ...
>> >> \gset var1 var2 ...
>> >
>> > How about
>> >
>> > \gset var1,,,var2,var3...
>> >
>>
>> I don't like this - you can use fake variable - and ignoring some
>> variable has no big effect on client
>
> Why assign to a variable you'll never use?

so why you get data from server, when you would not to use it ?

no offence, probably it is not hard to implement it - because we use
own parser, but I see this proposal little bit obscure

Tom - your proposal release of stored dataset just before next
statement, not like now on the end of statement?

Regards

Pavel

>
> Cheers,
> David.
>
> P.S. The bike shed should be puce with blaze orange pin-striping.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
> Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
>>>> How about
>>>> \gset var1,,,var2,var3...

>>> I don't like this - you can use fake variable - and ignoring some
>>> variable has no big effect on client

>> Why assign to a variable you'll never use?

> so why you get data from server, when you would not to use it ?

Yeah. I don't see why you'd be likely to write a select that computes
columns you don't actually want.

> Tom - your proposal release of stored dataset just before next
> statement, not like now on the end of statement?

Huh? I think you'd assign the values to the variables and then PQclear
the result right away.

regards, tom lane

Hello

2012/7/27 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
>>>>> How about
>>>>> \gset var1,,,var2,var3...
>
>>>> I don't like this - you can use fake variable - and ignoring some
>>>> variable has no big effect on client
>
>>> Why assign to a variable you'll never use?
>
>> so why you get data from server, when you would not to use it ?
>
> Yeah. I don't see why you'd be likely to write a select that computes
> columns you don't actually want.
>
>> Tom - your proposal release of stored dataset just before next
>> statement, not like now on the end of statement?
>
> Huh? I think you'd assign the values to the variables and then PQclear
> the result right away.

yes - I didn't understand \g mechanism well.

Here is patch - it is not nice at this moment and it is little bit
longer than I expected - but it works

It supports David's syntax

postgres=# select 'Hello', 'World' \gset a,b
postgres=# \echo :'a' :'b'
'Hello' 'World'
postgres=# select 'Hello', 'World';
?column? │ ?column?
──────────┼──────────
Hello │ World
(1 row)

postgres=# \gset a
to few target variables
postgres=# \gset a,
postgres=# \echo :'a'
'Hello'

Regards

Pavel

>
> regards, tom lane

On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote:
> Hello
>
> 2012/7/27 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> >> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
> >>>>> How about
> >>>>> \gset var1,,,var2,var3...
> >
> >>>> I don't like this - you can use fake variable - and ignoring some
> >>>> variable has no big effect on client
> >
> >>> Why assign to a variable you'll never use?
> >
> >> so why you get data from server, when you would not to use it ?
> >
> > Yeah. I don't see why you'd be likely to write a select that computes
> > columns you don't actually want.
> >
> >> Tom - your proposal release of stored dataset just before next
> >> statement, not like now on the end of statement?
> >
> > Huh? I think you'd assign the values to the variables and then PQclear
> > the result right away.
>
> yes - I didn't understand \g mechanism well.
>
> Here is patch - it is not nice at this moment and it is little bit
> longer than I expected - but it works
>
> It supports David's syntax
>
> postgres=# select 'Hello', 'World' \gset a,b
> postgres=# \echo :'a' :'b'
> 'Hello' 'World'
> postgres=# select 'Hello', 'World';
> ?column? │ ?column?
> ──────────┼──────────
> Hello │ World
> (1 row)
>
> postgres=# \gset a
> to few target variables
> postgres=# \gset a,
> postgres=# \echo :'a'
> 'Hello'
>
> Regards
>
> Pavel

Teensy code cleanup (trailing space) and SGML docs added.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hello

there is new version of this patch

* cleaned var list parser
* new regress tests
* support FETCH_COUNT > 0

Regards

Pavel Stehule

2012/8/1 David Fetter <david(at)fetter(dot)org>:
> On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote:
>> Hello
>>
>> 2012/7/27 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> > Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> >> 2012/7/26 David Fetter <david(at)fetter(dot)org>:
>> >>>>> How about
>> >>>>> \gset var1,,,var2,var3...
>> >
>> >>>> I don't like this - you can use fake variable - and ignoring some
>> >>>> variable has no big effect on client
>> >
>> >>> Why assign to a variable you'll never use?
>> >
>> >> so why you get data from server, when you would not to use it ?
>> >
>> > Yeah. I don't see why you'd be likely to write a select that computes
>> > columns you don't actually want.
>> >
>> >> Tom - your proposal release of stored dataset just before next
>> >> statement, not like now on the end of statement?
>> >
>> > Huh? I think you'd assign the values to the variables and then PQclear
>> > the result right away.
>>
>> yes - I didn't understand \g mechanism well.
>>
>> Here is patch - it is not nice at this moment and it is little bit
>> longer than I expected - but it works
>>
>> It supports David's syntax
>>
>> postgres=# select 'Hello', 'World' \gset a,b
>> postgres=# \echo :'a' :'b'
>> 'Hello' 'World'
>> postgres=# select 'Hello', 'World';
>> ?column? │ ?column?
>> ──────────┼──────────
>> Hello │ World
>> (1 row)
>>
>> postgres=# \gset a
>> to few target variables
>> postgres=# \gset a,
>> postgres=# \echo :'a'
>> 'Hello'
>>
>> Regards
>>
>> Pavel
>
> Teensy code cleanup (trailing space) and SGML docs added.
>
> Cheers,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
> Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:
> there is new version of this patch
>
> * cleaned var list parser
> * new regress tests
> * support FETCH_COUNT > 0

Here are my review comments.

Submission
==========
The patch is formatted in context diff style, and it could be applied
cleanly against latest master. This patch include document and tests,
but IMO they need some enhancement.

Usability
=========
This patch provides new psql command \gset which sends content of query
buffer to server, and stores result of the query into psql variables.
The name "\gset" is mixture of \g, which sends result to file or pipe,
and \set, which sets variable to some value, so it would sound natural
to psql users.

Freature test
=============
Compile completed without warning. Regression tests for \gset passed,
but I have some comments on them.

- Other regression tests have comment "-- ERROR" just after queries
which should fail. It would be nice to follow this manner.
- Typo "to few" in expected file and source file.
- How about adding testing "\gset" (no variable list) to "should fail"?
- Is it intentional that \gset can set special variables such as
AUTOCOMMIT and HOST? I don't see any downside for this behavior,
because \set also can do that, but it is not documented nor tested at all.

Document
========
- Adding some description of \gset command, especially about limitation
of variable list, seems necessary.
- In addition to the meta-command section, "Advanced features" section
mentions how to set psql's variables, so we would need some mention
there too.
- The term "target list" might not be familiar to users, since it
appears in only sections mentioning PG internal relatively. I think
that the feature described in the section "Retrieving Query Results" in
ECPG document is similar to this feature.
http://www.postgresql.org/docs/devel/static/ecpg-variables.html

Coding
======
The code follows our coding conventions. Here are comments for coding.

- Some typo found in comments, please see attached patch.
- There is a code path which doesn't print error message even if libpq
reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
prints "bad response" message for those errors.

Although I'll look the code more closely later, but anyway I marked the
patch "Waiting on Author" for comments above.

Regards,
--
Shigeru HANADA

Hello

2012/9/19 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>> there is new version of this patch
>>
>> * cleaned var list parser
>> * new regress tests
>> * support FETCH_COUNT > 0
>
> Here are my review comments.
>
> Submission
> ==========
> The patch is formatted in context diff style, and it could be applied
> cleanly against latest master. This patch include document and tests,
> but IMO they need some enhancement.
>
> Usability
> =========
> This patch provides new psql command \gset which sends content of query
> buffer to server, and stores result of the query into psql variables.
> The name "\gset" is mixture of \g, which sends result to file or pipe,
> and \set, which sets variable to some value, so it would sound natural
> to psql users.
>
> Freature test
> =============
> Compile completed without warning. Regression tests for \gset passed,
> but I have some comments on them.
>
> - Other regression tests have comment "-- ERROR" just after queries
> which should fail. It would be nice to follow this manner.
> - Typo "to few" in expected file and source file.
> - How about adding testing "\gset" (no variable list) to "should fail"?
> - Is it intentional that \gset can set special variables such as
> AUTOCOMMIT and HOST? I don't see any downside for this behavior,
> because \set also can do that, but it is not documented nor tested at all.
>

I use a same "SetVariable" function, so a behave should be same

> Document
> ========
> - Adding some description of \gset command, especially about limitation
> of variable list, seems necessary.
> - In addition to the meta-command section, "Advanced features" section
> mentions how to set psql's variables, so we would need some mention
> there too.
> - The term "target list" might not be familiar to users, since it
> appears in only sections mentioning PG internal relatively. I think
> that the feature described in the section "Retrieving Query Results" in
> ECPG document is similar to this feature.
> http://www.postgresql.org/docs/devel/static/ecpg-variables.html

I invite any proposals about enhancing documentation. Personally I am
a PostgreSQL developer, so I don't known any different term other than
"target list" - but any user friendly description is welcome.

>
> Coding
> ======
> The code follows our coding conventions. Here are comments for coding.
>
> - Some typo found in comments, please see attached patch.
> - There is a code path which doesn't print error message even if libpq
> reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
> PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
> prints "bad response" message for those errors.

yes - it is question. I use same pattern like PrintQueryResult, but
"bad response" message should be used.

I am sending updated patch

>
> Although I'll look the code more closely later, but anyway I marked the
> patch "Waiting on Author" for comments above.
>
> Regards,
> --
> Shigeru HANADA

Hi Pavel,

(2012/09/21 2:01), Pavel Stehule wrote:
>> - Is it intentional that \gset can set special variables such as
>> AUTOCOMMIT and HOST? I don't see any downside for this behavior,
>> because \set also can do that, but it is not documented nor tested at all.
>>
>
> I use a same "SetVariable" function, so a behave should be same

It seems reasonable.

>> Document
>> ========
>> - Adding some description of \gset command, especially about limitation
>> of variable list, seems necessary.
>> - In addition to the meta-command section, "Advanced features" section
>> mentions how to set psql's variables, so we would need some mention
>> there too.
>> - The term "target list" might not be familiar to users, since it
>> appears in only sections mentioning PG internal relatively. I think
>> that the feature described in the section "Retrieving Query Results" in
>> ECPG document is similar to this feature.
>> http://www.postgresql.org/docs/devel/static/ecpg-variables.html
>
> I invite any proposals about enhancing documentation. Personally I am
> a PostgreSQL developer, so I don't known any different term other than
> "target list" - but any user friendly description is welcome.

How about to say "stores the query's result output into variable"?
Please see attached file for my proposal. I also mentioned about 1-row
limit and omit of variable.

>> Coding
>> ======
>> The code follows our coding conventions. Here are comments for coding.
>>
>> - Some typo found in comments, please see attached patch.
>> - There is a code path which doesn't print error message even if libpq
>> reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
>> PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
>> prints "bad response" message for those errors.
>
> yes - it is question. I use same pattern like PrintQueryResult, but
> "bad response" message should be used.
>
> I am sending updated patch

It seems ok.

BTW, as far as I see, no psql backslash command including \setenv (it
was added in 9.2) has regression test in core (I mean src/test/regress).
Is there any convention about this issue? If psql backslash commands
(or any psql feature else) don't need regression test, we can remove
psql.(sql|out).
# Of course we need to test new feature by hand.

Anyway, IMO the name psql impresses larger area than the patch
implements. How about to rename psql to psql_cmd or backslash_cmd than
psql as regression test name?

--
Shigeru HANADA

Hello

2012/9/21 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi Pavel,
>
> (2012/09/21 2:01), Pavel Stehule wrote:
>>> - Is it intentional that \gset can set special variables such as
>>> AUTOCOMMIT and HOST? I don't see any downside for this behavior,
>>> because \set also can do that, but it is not documented nor tested at all.
>>>
>>
>> I use a same "SetVariable" function, so a behave should be same
>
> It seems reasonable.
>
>>> Document
>>> ========
>>> - Adding some description of \gset command, especially about limitation
>>> of variable list, seems necessary.
>>> - In addition to the meta-command section, "Advanced features" section
>>> mentions how to set psql's variables, so we would need some mention
>>> there too.
>>> - The term "target list" might not be familiar to users, since it
>>> appears in only sections mentioning PG internal relatively. I think
>>> that the feature described in the section "Retrieving Query Results" in
>>> ECPG document is similar to this feature.
>>> http://www.postgresql.org/docs/devel/static/ecpg-variables.html
>>
>> I invite any proposals about enhancing documentation. Personally I am
>> a PostgreSQL developer, so I don't known any different term other than
>> "target list" - but any user friendly description is welcome.
>
> How about to say "stores the query's result output into variable"?
> Please see attached file for my proposal. I also mentioned about 1-row
> limit and omit of variable.

should be

>
>>> Coding
>>> ======
>>> The code follows our coding conventions. Here are comments for coding.
>>>
>>> - Some typo found in comments, please see attached patch.
>>> - There is a code path which doesn't print error message even if libpq
>>> reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
>>> PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
>>> prints "bad response" message for those errors.
>>
>> yes - it is question. I use same pattern like PrintQueryResult, but
>> "bad response" message should be used.
>>
>> I am sending updated patch
>
> It seems ok.
>
> BTW, as far as I see, no psql backslash command including \setenv (it
> was added in 9.2) has regression test in core (I mean src/test/regress).
> Is there any convention about this issue? If psql backslash commands
> (or any psql feature else) don't need regression test, we can remove
> psql.(sql|out).
> # Of course we need to test new feature by hand.

It is question for Tom or David - only server side functionalities has
regress tests. But result of some backslash command is verified in
other regress tests. I would to see some regression tests for this
functionality.

>
> Anyway, IMO the name psql impresses larger area than the patch
> implements. How about to rename psql to psql_cmd or backslash_cmd than
> psql as regression test name?
>

I have no idea - psql_cmd is good name

Regards

Pavel

> --
> Shigeru HANADA

Hello

here is updated version of gset patch.

* merge Shigeru's doc patch
* rename psql regression test from "psql" to "psql_cmd"

Regards

Pavel Stehule

2012/9/27 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2012/9/21 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> Hi Pavel,
>>
>> (2012/09/21 2:01), Pavel Stehule wrote:
>>>> - Is it intentional that \gset can set special variables such as
>>>> AUTOCOMMIT and HOST? I don't see any downside for this behavior,
>>>> because \set also can do that, but it is not documented nor tested at all.
>>>>
>>>
>>> I use a same "SetVariable" function, so a behave should be same
>>
>> It seems reasonable.
>>
>>>> Document
>>>> ========
>>>> - Adding some description of \gset command, especially about limitation
>>>> of variable list, seems necessary.
>>>> - In addition to the meta-command section, "Advanced features" section
>>>> mentions how to set psql's variables, so we would need some mention
>>>> there too.
>>>> - The term "target list" might not be familiar to users, since it
>>>> appears in only sections mentioning PG internal relatively. I think
>>>> that the feature described in the section "Retrieving Query Results" in
>>>> ECPG document is similar to this feature.
>>>> http://www.postgresql.org/docs/devel/static/ecpg-variables.html
>>>
>>> I invite any proposals about enhancing documentation. Personally I am
>>> a PostgreSQL developer, so I don't known any different term other than
>>> "target list" - but any user friendly description is welcome.
>>
>> How about to say "stores the query's result output into variable"?
>> Please see attached file for my proposal. I also mentioned about 1-row
>> limit and omit of variable.
>
> should be
>
>>
>>>> Coding
>>>> ======
>>>> The code follows our coding conventions. Here are comments for coding.
>>>>
>>>> - Some typo found in comments, please see attached patch.
>>>> - There is a code path which doesn't print error message even if libpq
>>>> reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
>>>> PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
>>>> prints "bad response" message for those errors.
>>>
>>> yes - it is question. I use same pattern like PrintQueryResult, but
>>> "bad response" message should be used.
>>>
>>> I am sending updated patch
>>
>> It seems ok.
>>
>> BTW, as far as I see, no psql backslash command including \setenv (it
>> was added in 9.2) has regression test in core (I mean src/test/regress).
>> Is there any convention about this issue? If psql backslash commands
>> (or any psql feature else) don't need regression test, we can remove
>> psql.(sql|out).
>> # Of course we need to test new feature by hand.
>
> It is question for Tom or David - only server side functionalities has
> regress tests. But result of some backslash command is verified in
> other regress tests. I would to see some regression tests for this
> functionality.
>
>>
>> Anyway, IMO the name psql impresses larger area than the patch
>> implements. How about to rename psql to psql_cmd or backslash_cmd than
>> psql as regression test name?
>>
>
> I have no idea - psql_cmd is good name
>
> Regards
>
> Pavel
>
>> --
>> Shigeru HANADA

Hi Pavel,

On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule
<pavel(dot)stehule(at)gmail(dot)com> wrote:
> * merge Shigeru's doc patch
> * rename psql regression test from "psql" to "psql_cmd"

Those changes seem good.

Besides, I found an error message which doesn't end with '¥n' in
common.c. In general, messages passed to psql_error end with '¥n',
unless additional information follows. Please see attached patch for
additional change.

After you determine whether it's ok or unnecessary, I'll mark this patch
as "Ready for committer".

Regards,
--
Shigeru HANADA

2012/10/13 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi Pavel,
>
>
> On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>>
>> * merge Shigeru's doc patch
>> * rename psql regression test from "psql" to "psql_cmd"
>
>
> Those changes seem good.
>
> Besides, I found an error message which doesn't end with '¥n' in common.c.
> In general, messages passed to psql_error end with '¥n', unless additional
> information follows. Please see attached patch for additional change.
>
> After you determine whether it's ok or unnecessary, I'll mark this patch as
> "Ready for committer".
>

it is ok, thank you

Pavel

> Regards,
> --
> Shigeru HANADA

On Sat, October 13, 2012 19:26, Pavel Stehule wrote:
> 2012/10/13 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> After you determine whether it's ok or unnecessary, I'll mark this patch as
>> "Ready for committer".
>>
>

I found this behaviour which I think must count as a bug.
\gset doesn't allow more \\-separated lines behind it:

Only the last of these commands is problematic, and giving the syntax error

$ psql
psql (9.3devel-psql_var-20121012_2345-8b728e5c6e0ce6b6d6f54b92b390f14aa1aca6db)
Type "help" for help.

testdb=# select 1,2 \gset x,y
testdb=# \echo :x
1
testdb=# \echo :y
2
testdb=# \echo :x \\ \echo :y
1
2
testdb=# select 1,2 \gset x,y \\ \echo :x
\gset: syntax error
testdb=#

It'd be nice if it could be made to work

Thanks

Erik Rijkers

Hello

2012/10/14 Erik Rijkers <er(at)xs4all(dot)nl>:
> On Sat, October 13, 2012 19:26, Pavel Stehule wrote:
>> 2012/10/13 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>>> After you determine whether it's ok or unnecessary, I'll mark this patch as
>>> "Ready for committer".
>>>
>>
>
> I found this behaviour which I think must count as a bug.
> \gset doesn't allow more \\-separated lines behind it:
>
> Only the last of these commands is problematic, and giving the syntax error
>
> $ psql
> psql (9.3devel-psql_var-20121012_2345-8b728e5c6e0ce6b6d6f54b92b390f14aa1aca6db)
> Type "help" for help.
>
> testdb=# select 1,2 \gset x,y
> testdb=# \echo :x
> 1
> testdb=# \echo :y
> 2
> testdb=# \echo :x \\ \echo :y
> 1
> 2
> testdb=# select 1,2 \gset x,y \\ \echo :x
> \gset: syntax error
> testdb=#
>
> It'd be nice if it could be made to work
>

done

Regards

Pavel

> Thanks
>
> Erik Rijkers
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ gset_08.diff ]

In the course of adding a new backslash command, this patch manages to
touch:

* the main loop's concept of what types of backslash commands exist
(PSQL_CMD_NOSEND ... what's the point of that, rather than making
this work the same as \g?)
* SendQuery's concept of how to process command results (again, why
isn't this more like \g?)
* ExecQueryUsingCursor's concept of how to process command results
(why? surely we don't need \gset to use a cursor)
* the psql lexer (adding a whole bunch of stuff that probably doesn't
belong there)
* the core psql settings construct (to store something that is in
no way a persistent setting)

Surely there is a less ugly and invasive way to do this. The fact
that the reviewer keeps finding bizarre bugs like "another backslash
command on the same line doesn't work" seems to me to be a good
indication that this is touching things it shouldn't.

regards, tom lane

Hello

fast reply

2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> [ gset_08.diff ]
>
> In the course of adding a new backslash command, this patch manages to
> touch:
>
> * the main loop's concept of what types of backslash commands exist
> (PSQL_CMD_NOSEND ... what's the point of that, rather than making
> this work the same as \g?)
> * SendQuery's concept of how to process command results (again, why
> isn't this more like \g?)

If I remember, I had to do because I had a problem with shell, but I
have to diagnose it again

> * ExecQueryUsingCursor's concept of how to process command results
> (why? surely we don't need \gset to use a cursor)

There was two possibilities, but hardly non using cursor is better way

> * the psql lexer (adding a whole bunch of stuff that probably doesn't
> belong there)

??

> * the core psql settings construct (to store something that is in
> no way a persistent setting)
>

??

> Surely there is a less ugly and invasive way to do this. The fact
> that the reviewer keeps finding bizarre bugs like "another backslash
> command on the same line doesn't work" seems to me to be a good
> indication that this is touching things it shouldn't.

- all these bugs are based on lexer construct. A little modification
of lexer is possible

Regards

Pavel

>
> regards, tom lane

Hello

2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> [ gset_08.diff ]
>
> In the course of adding a new backslash command, this patch manages to
> touch:
>
> * the main loop's concept of what types of backslash commands exist
> (PSQL_CMD_NOSEND ... what's the point of that, rather than making
> this work the same as \g?)

This is necessary, because there is a new possible state - "query is
complete, but command is wrong" - so I cannot use \g implementation,
because there is no possible error in \g or ';'

> * SendQuery's concept of how to process command results (again, why
> isn't this more like \g?)

it is similar - difference is only in work with result - \gset uses
StoreQueryResult instead PrintQueryResults, but other is same

> * ExecQueryUsingCursor's concept of how to process command results
> (why? surely we don't need \gset to use a cursor)

It is my mistake - simply and correct way is not using cursor in this use case

> * the psql lexer (adding a whole bunch of stuff that probably doesn't
> belong there)

I had to modify lexer - current lexer supports symbols separated by
space, but not symbols separated by comma. We don't would to use
variable evaluation in target variable list.

> * the core psql settings construct (to store something that is in
> no way a persistent setting)

sorry, I don't understand to this issue

>
> Surely there is a less ugly and invasive way to do this. The fact
> that the reviewer keeps finding bizarre bugs like "another backslash
> command on the same line doesn't work" seems to me to be a good
> indication that this is touching things it shouldn't.
>

I had too strong in checking and raising errors. Is relative simple to
modify patch to enable more backslash statements on same line

I'll send updated patch early

Regards

Pavel

> regards, tom lane

2012/10/15 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>> [ gset_08.diff ]
>>
>> In the course of adding a new backslash command, this patch manages to
>> touch:
>>
>> * the main loop's concept of what types of backslash commands exist
>> (PSQL_CMD_NOSEND ... what's the point of that, rather than making
>> this work the same as \g?)
>
> This is necessary, because there is a new possible state - "query is
> complete, but command is wrong" - so I cannot use \g implementation,
> because there is no possible error in \g or ';'
>
>> * SendQuery's concept of how to process command results (again, why
>> isn't this more like \g?)
>
> it is similar - difference is only in work with result - \gset uses
> StoreQueryResult instead PrintQueryResults, but other is same
>
>
>> * ExecQueryUsingCursor's concept of how to process command results
>> (why? surely we don't need \gset to use a cursor)
>
> It is my mistake - simply and correct way is not using cursor in this use case

and it is question if cursor support is bad decision, because cursors
can help with large queries, that can returns thousands rows - and we
can raise error early, before we fall on "out of memory"

regards

Pavel

>
>> * the psql lexer (adding a whole bunch of stuff that probably doesn't
>> belong there)
>
> I had to modify lexer - current lexer supports symbols separated by
> space, but not symbols separated by comma. We don't would to use
> variable evaluation in target variable list.
>
>> * the core psql settings construct (to store something that is in
>> no way a persistent setting)
>
> sorry, I don't understand to this issue
>
>>
>> Surely there is a less ugly and invasive way to do this. The fact
>> that the reviewer keeps finding bizarre bugs like "another backslash
>> command on the same line doesn't work" seems to me to be a good
>> indication that this is touching things it shouldn't.
>>
>
> I had too strong in checking and raising errors. Is relative simple to
> modify patch to enable more backslash statements on same line
>
> I'll send updated patch early
>
> Regards
>
> Pavel
>
>> regards, tom lane

Hi Pavel,

First of all, I'm sorry that my previous review was rough. I looked
your patch and existing code closely again.

On 2012/10/15, at 12:57, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> * ExecQueryUsingCursor's concept of how to process command results
>> (why? surely we don't need \gset to use a cursor)
>
> There was two possibilities, but hardly non using cursor is better way

+1 for supporting the case when FETCH_COUNT > 0, because user might set
so mainly for other queries, and they would want to avoid changing
FETCH_COUNT setting during every query followed by \gset.

>> * the psql lexer (adding a whole bunch of stuff that probably doesn't
>> belong there)
>
> ??

I think that Tom is talking about psql_scan_slash_vars(). It seems too
specific to \gset command. How about to improve
psql_scan_slash_options() so that it can handle comma-separated variable
list? Although you might have tried it before.
# Unused OT_VARLIST macro gave me the idea.

>> * the core psql settings construct (to store something that is in
>> no way a persistent setting)
>>
>
> ??

I thought that having \gset arguments in pset is reasonable, since \g
uses pest.gfname to hold its one-shot setting. Or, we should refactor
\g to fit with \gset? I might be missing Tom's point...

>> Surely there is a less ugly and invasive way to do this. The fact
>> that the reviewer keeps finding bizarre bugs like "another backslash
>> command on the same line doesn't work" seems to me to be a good
>> indication that this is touching things it shouldn't.
>
> - all these bugs are based on lexer construct. A little modification
> of lexer is possible

IMHO those issues come from the design rather than the implementation of
lexer. AFAIK we don't have consensus about the case that both of \g and
\gset are used for a query like this:

postgres=# SELECT 1 \gset var \\ \g foo.txt

This seems regal. Should we store "1" into var and write the result
into foo.txt? Or, only either of them? It's just an idea and it
requires new special character, but how about use \g command for file,
pipe, and variable? In the case we choose '&' for variable prefix:

postgres=# SELECT 'hello', 'wonderful', 'world!' \g &var1,,var2

Anyway, we've had no psql's meta command which processes query result
other than \g. So, we might have more considerable issues about design.

BTW, what the word "comma_expected" means? It's in the comment above
psql_scan_slash_vars(). It might be a remaining of old implementation.

Regards,
--
Shigeru HANADA
shigeru(dot)hanada(at)gmail(dot)com

2012/10/15 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi Pavel,
>
> First of all, I'm sorry that my previous review was rough. I looked
> your patch and existing code closely again.
>
> On 2012/10/15, at 12:57, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> * ExecQueryUsingCursor's concept of how to process command results
>>> (why? surely we don't need \gset to use a cursor)
>>
>> There was two possibilities, but hardly non using cursor is better way
>
> +1 for supporting the case when FETCH_COUNT > 0, because user might set
> so mainly for other queries, and they would want to avoid changing
> FETCH_COUNT setting during every query followed by \gset.
>
>>> * the psql lexer (adding a whole bunch of stuff that probably doesn't
>>> belong there)
>>
>> ??
>
> I think that Tom is talking about psql_scan_slash_vars(). It seems too
> specific to \gset command. How about to improve
> psql_scan_slash_options() so that it can handle comma-separated variable
> list? Although you might have tried it before.
> # Unused OT_VARLIST macro gave me the idea.

yes, it is possible - I'll look on it at evening

>
>>> * the core psql settings construct (to store something that is in
>>> no way a persistent setting)
>>>
>>
>> ??
>
> I thought that having \gset arguments in pset is reasonable, since \g
> uses pest.gfname to hold its one-shot setting. Or, we should refactor
> \g to fit with \gset? I might be missing Tom's point...
>
>>> Surely there is a less ugly and invasive way to do this. The fact
>>> that the reviewer keeps finding bizarre bugs like "another backslash
>>> command on the same line doesn't work" seems to me to be a good
>>> indication that this is touching things it shouldn't.
>>
>> - all these bugs are based on lexer construct. A little modification
>> of lexer is possible
>
> IMHO those issues come from the design rather than the implementation of
> lexer. AFAIK we don't have consensus about the case that both of \g and
> \gset are used for a query like this:
>
> postgres=# SELECT 1 \gset var \\ \g foo.txt
>
> This seems regal. Should we store "1" into var and write the result
> into foo.txt? Or, only either of them? It's just an idea and it
> requires new special character, but how about use \g command for file,
> pipe, and variable? In the case we choose '&' for variable prefix:
>
> postgres=# SELECT 'hello', 'wonderful', 'world!' \g &var1,,var2
>
> Anyway, we've had no psql's meta command which processes query result
> other than \g. So, we might have more considerable issues about design.

a current design is rigid - a small implementation can stop parsing
target list, when other backslash statement is detected

>
> BTW, what the word "comma_expected" means? It's in the comment above
> psql_scan_slash_vars(). It might be a remaining of old implementation.

This identifier is mistaken - etc this comment is wrong and related to
old implementation - sorry. A first design was replaced by state
machine described by VarListParserState

>
> Regards,
> --
> Shigeru HANADA
> shigeru(dot)hanada(at)gmail(dot)com
>
>
>
>
>

2012/10/15 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2012/10/15 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> Hi Pavel,
>>
>> First of all, I'm sorry that my previous review was rough. I looked
>> your patch and existing code closely again.
>>
>> On 2012/10/15, at 12:57, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> 2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>> * ExecQueryUsingCursor's concept of how to process command results
>>>> (why? surely we don't need \gset to use a cursor)
>>>
>>> There was two possibilities, but hardly non using cursor is better way
>>
>> +1 for supporting the case when FETCH_COUNT > 0, because user might set
>> so mainly for other queries, and they would want to avoid changing
>> FETCH_COUNT setting during every query followed by \gset.
>>
>>>> * the psql lexer (adding a whole bunch of stuff that probably doesn't
>>>> belong there)
>>>
>>> ??
>>
>> I think that Tom is talking about psql_scan_slash_vars(). It seems too
>> specific to \gset command. How about to improve
>> psql_scan_slash_options() so that it can handle comma-separated variable
>> list? Although you might have tried it before.
>> # Unused OT_VARLIST macro gave me the idea.
>
> yes, it is possible - I'll look on it at evening

a reuse of psql_scan_slash_options is not good idea - a interface of
this function is out of my purposes - and I cannot to signalise error
from this procedure. But I can minimize psql_scan_slash_var and I can
move lot of code out of lexer file.

>
>>
>>>> * the core psql settings construct (to store something that is in
>>>> no way a persistent setting)
>>>>
>>>
>>> ??
>>
>> I thought that having \gset arguments in pset is reasonable, since \g
>> uses pest.gfname to hold its one-shot setting. Or, we should refactor
>> \g to fit with \gset? I might be missing Tom's point...
>>
>>>> Surely there is a less ugly and invasive way to do this. The fact
>>>> that the reviewer keeps finding bizarre bugs like "another backslash
>>>> command on the same line doesn't work" seems to me to be a good
>>>> indication that this is touching things it shouldn't.
>>>
>>> - all these bugs are based on lexer construct. A little modification
>>> of lexer is possible
>>
>> IMHO those issues come from the design rather than the implementation of
>> lexer. AFAIK we don't have consensus about the case that both of \g and
>> \gset are used for a query like this:
>>
>> postgres=# SELECT 1 \gset var \\ \g foo.txt
>>
>> This seems regal. Should we store "1" into var and write the result
>> into foo.txt? Or, only either of them? It's just an idea and it
>> requires new special character, but how about use \g command for file,
>> pipe, and variable? In the case we choose '&' for variable prefix:
>>
>> postgres=# SELECT 'hello', 'wonderful', 'world!' \g &var1,,var2
>>
>> Anyway, we've had no psql's meta command which processes query result
>> other than \g. So, we might have more considerable issues about design.
>
> a current design is rigid - a small implementation can stop parsing
> target list, when other backslash statement is detected
>
>>
>> BTW, what the word "comma_expected" means? It's in the comment above
>> psql_scan_slash_vars(). It might be a remaining of old implementation.
>
> This identifier is mistaken - etc this comment is wrong and related to
> old implementation - sorry. A first design was replaced by state
> machine described by VarListParserState
>
>
>
>>
>> Regards,
>> --
>> Shigeru HANADA
>> shigeru(dot)hanada(at)gmail(dot)com
>>
>>
>>
>>
>>

Hello

here is updated patch, I moved lot of code from lexer to command.com,
and now more \gset doesn't disable other backslash commands on same
line.

Regards

Pavel

2012/10/15 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2012/10/15 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> 2012/10/15 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>>> Hi Pavel,
>>>
>>> First of all, I'm sorry that my previous review was rough. I looked
>>> your patch and existing code closely again.
>>>
>>> On 2012/10/15, at 12:57, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> 2012/10/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>>> * ExecQueryUsingCursor's concept of how to process command results
>>>>> (why? surely we don't need \gset to use a cursor)
>>>>
>>>> There was two possibilities, but hardly non using cursor is better way
>>>
>>> +1 for supporting the case when FETCH_COUNT > 0, because user might set
>>> so mainly for other queries, and they would want to avoid changing
>>> FETCH_COUNT setting during every query followed by \gset.
>>>
>>>>> * the psql lexer (adding a whole bunch of stuff that probably doesn't
>>>>> belong there)
>>>>
>>>> ??
>>>
>>> I think that Tom is talking about psql_scan_slash_vars(). It seems too
>>> specific to \gset command. How about to improve
>>> psql_scan_slash_options() so that it can handle comma-separated variable
>>> list? Although you might have tried it before.
>>> # Unused OT_VARLIST macro gave me the idea.
>>
>> yes, it is possible - I'll look on it at evening
>
> a reuse of psql_scan_slash_options is not good idea - a interface of
> this function is out of my purposes - and I cannot to signalise error
> from this procedure. But I can minimize psql_scan_slash_var and I can
> move lot of code out of lexer file.
>
>>
>>>
>>>>> * the core psql settings construct (to store something that is in
>>>>> no way a persistent setting)
>>>>>
>>>>
>>>> ??
>>>
>>> I thought that having \gset arguments in pset is reasonable, since \g
>>> uses pest.gfname to hold its one-shot setting. Or, we should refactor
>>> \g to fit with \gset? I might be missing Tom's point...
>>>
>>>>> Surely there is a less ugly and invasive way to do this. The fact
>>>>> that the reviewer keeps finding bizarre bugs like "another backslash
>>>>> command on the same line doesn't work" seems to me to be a good
>>>>> indication that this is touching things it shouldn't.
>>>>
>>>> - all these bugs are based on lexer construct. A little modification
>>>> of lexer is possible
>>>
>>> IMHO those issues come from the design rather than the implementation of
>>> lexer. AFAIK we don't have consensus about the case that both of \g and
>>> \gset are used for a query like this:
>>>
>>> postgres=# SELECT 1 \gset var \\ \g foo.txt
>>>
>>> This seems regal. Should we store "1" into var and write the result
>>> into foo.txt? Or, only either of them? It's just an idea and it
>>> requires new special character, but how about use \g command for file,
>>> pipe, and variable? In the case we choose '&' for variable prefix:
>>>
>>> postgres=# SELECT 'hello', 'wonderful', 'world!' \g &var1,,var2
>>>
>>> Anyway, we've had no psql's meta command which processes query result
>>> other than \g. So, we might have more considerable issues about design.
>>
>> a current design is rigid - a small implementation can stop parsing
>> target list, when other backslash statement is detected
>>
>>>
>>> BTW, what the word "comma_expected" means? It's in the comment above
>>> psql_scan_slash_vars(). It might be a remaining of old implementation.
>>
>> This identifier is mistaken - etc this comment is wrong and related to
>> old implementation - sorry. A first design was replaced by state
>> machine described by VarListParserState
>>
>>
>>
>>>
>>> Regards,
>>> --
>>> Shigeru HANADA
>>> shigeru(dot)hanada(at)gmail(dot)com
>>>
>>>
>>>
>>>
>>>

Hi Pavel,

On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:
> here is updated patch, I moved lot of code from lexer to command.com,
> and now more \gset doesn't disable other backslash commands on same
> line.

* lexer changes
IIUC, new function psql_scan_varlist_varname scans input and returns a
variable name or a comma at each call, and command.c handles the error
such as invalid # of variables. This new design seems better than old one.

However, IMHO the name psql_scan_varlist_varname sounds redundant and
unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
that that function is expected to be used for arguments of backslash
commands, like psql_scan_slash_command and psql_scan_slash_option.
Thoughts?

* multiple meta command
Now both of the command sequences

$ SELECT 1, 2 \gset var1, var2 \g foo.txt
$ SELECT 1, 2 \g foo.txt \gset var1, var2

set var1 and v2 to "1" and "2" respectively, and also write the result
into foo.txt. This would be what users expected.

* Duplication of variables
I found an issue we have not discussed. Currently \gset accepts same
variable names in the list, and stores last SELECT item in duplicated
variables. For instance,

$ SELECT 1, 2 \gset var, var

stores "2" into var. I think this behavior is acceptable, but it might
be worth mentioning in document.

* extra fixes
I fixed some minor issues below. Please see attached v10 patch for details.

* remove unused macro OT_VARLIST
* remove unnecessary #include directive for common.h
* fill comment within 80 columns
* indent short variable name with tab
* add regression test case for combination of \g and \gset

* bug on FETCH_COUNT = 1
When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
\gset shows extra "(1 row)". This would be a bug in
ExecQueryUsingCursor. Please see the last test case in regression test
psql_cmd.

I'll mark this patch as "waiting author".

Regards,
--
Shigeru HANADA

2012/10/16 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi Pavel,
>
> On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>> here is updated patch, I moved lot of code from lexer to command.com,
>> and now more \gset doesn't disable other backslash commands on same
>> line.
>
> * lexer changes
> IIUC, new function psql_scan_varlist_varname scans input and returns a
> variable name or a comma at each call, and command.c handles the error
> such as invalid # of variables. This new design seems better than old one.
>
> However, IMHO the name psql_scan_varlist_varname sounds redundant and
> unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
> that that function is expected to be used for arguments of backslash
> commands, like psql_scan_slash_command and psql_scan_slash_option.
> Thoughts?
>
> * multiple meta command
> Now both of the command sequences
>
> $ SELECT 1, 2 \gset var1, var2 \g foo.txt
> $ SELECT 1, 2 \g foo.txt \gset var1, var2
>
> set var1 and v2 to "1" and "2" respectively, and also write the result
> into foo.txt. This would be what users expected.
>
> * Duplication of variables
> I found an issue we have not discussed. Currently \gset accepts same
> variable names in the list, and stores last SELECT item in duplicated
> variables. For instance,
>
> $ SELECT 1, 2 \gset var, var
>
> stores "2" into var. I think this behavior is acceptable, but it might
> be worth mentioning in document.
>
> * extra fixes
> I fixed some minor issues below. Please see attached v10 patch for details.
>
> * remove unused macro OT_VARLIST
> * remove unnecessary #include directive for common.h
> * fill comment within 80 columns
> * indent short variable name with tab
> * add regression test case for combination of \g and \gset
>
> * bug on FETCH_COUNT = 1
> When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
> \gset shows extra "(1 row)". This would be a bug in
> ExecQueryUsingCursor. Please see the last test case in regression test
> psql_cmd.

I fixed this bug

Regards

Pavel

>
> I'll mark this patch as "waiting author".
>
> Regards,
> --
> Shigeru HANADA

On Tue, Oct 16, 2012 at 12:24 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2012/10/16 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> Hi Pavel,
>>
>> On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>>> here is updated patch, I moved lot of code from lexer to command.com,
>>> and now more \gset doesn't disable other backslash commands on same
>>> line.
>>
>> * lexer changes
>> IIUC, new function psql_scan_varlist_varname scans input and returns a
>> variable name or a comma at each call, and command.c handles the error
>> such as invalid # of variables. This new design seems better than old one.
>>
>> However, IMHO the name psql_scan_varlist_varname sounds redundant and
>> unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
>> that that function is expected to be used for arguments of backslash
>> commands, like psql_scan_slash_command and psql_scan_slash_option.
>> Thoughts?
>>
>> * multiple meta command
>> Now both of the command sequences
>>
>> $ SELECT 1, 2 \gset var1, var2 \g foo.txt
>> $ SELECT 1, 2 \g foo.txt \gset var1, var2
>>
>> set var1 and v2 to "1" and "2" respectively, and also write the result
>> into foo.txt. This would be what users expected.
>>
>> * Duplication of variables
>> I found an issue we have not discussed. Currently \gset accepts same
>> variable names in the list, and stores last SELECT item in duplicated
>> variables. For instance,
>>
>> $ SELECT 1, 2 \gset var, var
>>
>> stores "2" into var. I think this behavior is acceptable, but it might
>> be worth mentioning in document.
>>
>> * extra fixes
>> I fixed some minor issues below. Please see attached v10 patch for details.
>>
>> * remove unused macro OT_VARLIST
>> * remove unnecessary #include directive for common.h
>> * fill comment within 80 columns
>> * indent short variable name with tab
>> * add regression test case for combination of \g and \gset
>>
>> * bug on FETCH_COUNT = 1
>> When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
>> \gset shows extra "(1 row)". This would be a bug in
>> ExecQueryUsingCursor. Please see the last test case in regression test
>> psql_cmd.
>
> I fixed this bug
>
> Regards
>
> Pavel
>
>>
>> I'll mark this patch as "waiting author".
>>
>> Regards,
>> --
>> Shigeru HANADA
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

My first review...

Patch applied cleanly to master and make check was fine.

I tested it out and it operates as advertised. There were a couple
things that stood out to me though.

1) NULL values are not displayed properly after \pset null is run.

postgres=# \pset null '(null)'
Null display is "(null)".
postgres=# select NULL \gset var1
postgres=# \echo :var1

postgres=# select NULL;
?column?
----------
(null)
(1 row)

I know this doesn't come back from the server like this, but you
should be able to pull this from the options and display
appropriately. Not sure if it should be in variable display code, or
when you store it into the variable.

2) The error messages seemed kind of terse. Other error messages are
capitalized and have punctuation.

3) We don't find out about incorrect number of columns until after
query returns. I know this is hard/impossible to fix, but it might be
nice to print out the result normally if you can't store it in the
variables.

3b) You throw an error on too many variables, but still store the data
since you have fewer columns than variables. This makes sense, but you
don't inform the user at all.

On to the code:

1) Introduction of random newlines:

*************** cleanup:
*** 1254,1259 ****
--- 1383,1389 ----
PQclear(results);
}

+
if (pset.timing)
{
INSTR_TIME_SET_CURRENT(after);

I saw that in a couple places, but that was the most obvious.

2) TargetList - Why not use the built in linked list operations rather
than creating your own? Are they not accessible to client binaries
like this?

Overall I think this is a useful feature and I think you integrated it
well within the existing infrastructure, ie combining concepts of \set
and \g.

I gave this a look. I think it needs to be revised by somebody with a
better understanding of scanner (flex) than me, but I didn't like the
changes in psqlscan.l at all; the new <xvl> pattern is too unlike the
rest of the surrounding patterns, and furthermore it has been placed
within the block that says it mirrors the backend scanner, when it
obviously has no equivalent there.

I assume there's a better way to do this. Hints would be appreciated.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I gave this a look. I think it needs to be revised by somebody with a
> better understanding of scanner (flex) than me, but I didn't like the
> changes in psqlscan.l at all; the new <xvl> pattern is too unlike the
> rest of the surrounding patterns, and furthermore it has been placed
> within the block that says it mirrors the backend scanner, when it
> obviously has no equivalent there.

> I assume there's a better way to do this. Hints would be appreciated.

Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?

Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.

regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > I gave this a look. I think it needs to be revised by somebody with a
> > better understanding of scanner (flex) than me, but I didn't like the
> > changes in psqlscan.l at all; the new <xvl> pattern is too unlike the
> > rest of the surrounding patterns, and furthermore it has been placed
> > within the block that says it mirrors the backend scanner, when it
> > obviously has no equivalent there.
>
> > I assume there's a better way to do this. Hints would be appreciated.
>
> Personally I saw no reason for this patch to touch psqlscan.l in the
> first place. Commands such as \set just scan variable names with
> psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
>
> Moreover, the proposed lexer rules are flat out *wrong*, in that they
> insist on the target variable names being {identifier}s, a restriction
> not imposed by \set.

Great, thanks for the feedback. Marking as returned in CF. I hope to
see a new version after pgconf.eu.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2012/10/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> I gave this a look. I think it needs to be revised by somebody with a
>> better understanding of scanner (flex) than me, but I didn't like the
>> changes in psqlscan.l at all; the new <xvl> pattern is too unlike the
>> rest of the surrounding patterns, and furthermore it has been placed
>> within the block that says it mirrors the backend scanner, when it
>> obviously has no equivalent there.
>
>> I assume there's a better way to do this. Hints would be appreciated.
>
> Personally I saw no reason for this patch to touch psqlscan.l in the
> first place. Commands such as \set just scan variable names with
> psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
>

it cannot be same, because current scan doesn't know comma as
separator. So if you don't like changes in scanner, than we can't to
use "var1, var2," syntax and we can't to use leaky list syntax ",x,"

> Moreover, the proposed lexer rules are flat out *wrong*, in that they
> insist on the target variable names being {identifier}s, a restriction
> not imposed by \set.
>

do you like to support referenced varnames??

postgres=# \varname xxx
Invalid command \varname. Try \? for help.
postgres=# \set varname xxx
postgres=# \set :varname Hello
postgres=# \set
varname = 'xxx'
xxx = 'Hello'

yes, \set support it, but this can be source of "strange behave" for
some people, because people use :varname like $varname in classic
scripting languages, and it is significantly different - so I didn't
support it as little bit dangerous feature. It is easy support it,
although I am thinking, so it is not good idea, because behave is
really different than users expect and I don't know any use case for
this indirect referencing. But I would to talk about it, and I invite
opinion of others.

Regards

Pavel

> regards, tom lane

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2012/10/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Personally I saw no reason for this patch to touch psqlscan.l in the
>> first place. Commands such as \set just scan variable names with
>> psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?

> it cannot be same, because current scan doesn't know comma as
> separator. So if you don't like changes in scanner, than we can't to
> use "var1, var2," syntax and we can't to use leaky list syntax ",x,"

Uh, no, that doesn't follow. It wouldn't be any more code to have
command.c process the commas (or even more likely, just save the \gset
argument(s) as a string, and split on commas after we've done the
command). Even if we wanted to do that in psqlscan.l, this was a pretty
bad/ugly implementation of it.

>> Moreover, the proposed lexer rules are flat out *wrong*, in that they
>> insist on the target variable names being {identifier}s, a restriction
>> not imposed by \set.

> yes, \set support it, but this can be source of "strange behave" for
> some people, because people use :varname like $varname in classic
> scripting languages, and it is significantly different - so I didn't
> support it as little bit dangerous feature.

[ shrug... ] If you want to argue for imposing a restriction on
psql variable names across-the-board, we could have that discussion;
but personally I've not seen even one user complaint that could be
traced to \set's laxity in the matter, so I don't see a need for
a restriction. In any case, having \gset enforce a restriction
that \set doesn't is useless and inconsistent.

regards, tom lane

2012/10/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2012/10/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> Personally I saw no reason for this patch to touch psqlscan.l in the
>>> first place. Commands such as \set just scan variable names with
>>> psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
>
>> it cannot be same, because current scan doesn't know comma as
>> separator. So if you don't like changes in scanner, than we can't to
>> use "var1, var2," syntax and we can't to use leaky list syntax ",x,"
>
> Uh, no, that doesn't follow. It wouldn't be any more code to have
> command.c process the commas (or even more likely, just save the \gset
> argument(s) as a string, and split on commas after we've done the
> command). Even if we wanted to do that in psqlscan.l, this was a pretty
> bad/ugly implementation of it.

I don't understand, why we have to move lexer work from scanner to
command processing?

then I afraid of another issue - when we do late separation in command

somebody can do

\set targetvars a,b,c

select xxxx
\gset x1,x2,:targetvars,x3

We would to do this? Then we moving to TeX liked languages. I am asking.

>
>>> Moreover, the proposed lexer rules are flat out *wrong*, in that they
>>> insist on the target variable names being {identifier}s, a restriction
>>> not imposed by \set.
>
>> yes, \set support it, but this can be source of "strange behave" for
>> some people, because people use :varname like $varname in classic
>> scripting languages, and it is significantly different - so I didn't
>> support it as little bit dangerous feature.
>
> [ shrug... ] If you want to argue for imposing a restriction on
> psql variable names across-the-board, we could have that discussion;
> but personally I've not seen even one user complaint that could be
> traced to \set's laxity in the matter, so I don't see a need for
> a restriction. In any case, having \gset enforce a restriction
> that \set doesn't is useless and inconsistent.

ok, it has a sense

>
> regards, tom lane

Hello

>
> My first review...
>
> Patch applied cleanly to master and make check was fine.
>
> I tested it out and it operates as advertised. There were a couple
> things that stood out to me though.
>
> 1) NULL values are not displayed properly after \pset null is run.
>
> postgres=# \pset null '(null)'
> Null display is "(null)".
> postgres=# select NULL \gset var1
> postgres=# \echo :var1
>
> postgres=# select NULL;
> ?column?
> ----------
> (null)
> (1 row)
>
> I know this doesn't come back from the server like this, but you
> should be able to pull this from the options and display
> appropriately. Not sure if it should be in variable display code, or
> when you store it into the variable.

ok

>
> 2) The error messages seemed kind of terse. Other error messages are
> capitalized and have punctuation.

ok

>
> 3) We don't find out about incorrect number of columns until after
> query returns. I know this is hard/impossible to fix, but it might be
> nice to print out the result normally if you can't store it in the
> variables.

a changing behave when error is occurred is not good, but I can show a
number of returned columns

>
> 3b) You throw an error on too many variables, but still store the data
> since you have fewer columns than variables. This makes sense, but you
> don't inform the user at all.

there is not a some like stack, so I cannot to return values to
previous state. It should be documented - after error, a related
variables has undefined values.

>
> On to the code:
>
> 1) Introduction of random newlines:
>
> *************** cleanup:
> *** 1254,1259 ****
> --- 1383,1389 ----
> PQclear(results);
> }
>
> +
> if (pset.timing)
> {
> INSTR_TIME_SET_CURRENT(after);
>
> I saw that in a couple places, but that was the most obvious.
>
> 2) TargetList - Why not use the built in linked list operations rather
> than creating your own? Are they not accessible to client binaries
> like this?

actually, there are no support for lists on client side now. So it is
reason. But I'll remove it, because I'll move parsing to command
implementation, and then I don't need a list support

>
> Overall I think this is a useful feature and I think you integrated it
> well within the existing infrastructure, ie combining concepts of \set
> and \g.

Thank you

Regards

Pavel

Hello

here is updated patch

main change - it doesn't touch psql lexer - like Tom proposed
other points respect Phil's notices

>>
>
> My first review...
>
> Patch applied cleanly to master and make check was fine.
>
> I tested it out and it operates as advertised. There were a couple
> things that stood out to me though.
>
> 1) NULL values are not displayed properly after \pset null is run.
>
> postgres=# \pset null '(null)'
> Null display is "(null)".
> postgres=# select NULL \gset var1
> postgres=# \echo :var1
>
> postgres=# select NULL;
> ?column?
> ----------
> (null)
> (1 row)
>
> I know this doesn't come back from the server like this, but you
> should be able to pull this from the options and display
> appropriately. Not sure if it should be in variable display code, or
> when you store it into the variable.

fixed and add to regress test

>
> 2) The error messages seemed kind of terse. Other error messages are
> capitalized and have punctuation.

After some thinking I didn't change it - it is consistent with other
messages in psql - short messages that are not complete sentence are
no capitalized and have not punctuation like other short messages in
psql

>
> 3) We don't find out about incorrect number of columns until after
> query returns. I know this is hard/impossible to fix, but it might be
> nice to print out the result normally if you can't store it in the
> variables.

I didn't change it because a) I don't think so change behave after
error is good idea, b) \gset doesn't remove SQL from query buffer, so
you can repeat it

postgres=> select 10,20,30
postgres-> \gset a,b
too few target variables
postgres=> \g
?column? │ ?column? │ ?column?
──────────┼──────────┼──────────
10 │ 20 │ 30
(1 row)

postgres=> \gset a,b,c

>
> 3b) You throw an error on too many variables, but still store the data
> since you have fewer columns than variables. This makes sense, but you
> don't inform the user at all.

it is commented in doc

+ <para>
+ When this command fails, then related <replaceable
+ class="parameter">variables</replaceable> has undefined content.
+ </para>

>
> On to the code:
>
> 1) Introduction of random newlines:
>
> *************** cleanup:
> *** 1254,1259 ****
> --- 1383,1389 ----
> PQclear(results);
> }
>
> +
> if (pset.timing)
> {
> INSTR_TIME_SET_CURRENT(after);
>
> I saw that in a couple places, but that was the most obvious.
>

I hope so I moved to /dev/null all

> 2) TargetList - Why not use the built in linked list operations rather
> than creating your own? Are they not accessible to client binaries
> like this?

There was not support for lists on client part to today. So I had to
created own simple implementation.

>
> Overall I think this is a useful feature and I think you integrated it
> well within the existing infrastructure, ie combining concepts of \set
> and \g.

Thank you very much again

Regards

Pavel

On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> here is updated patch
>
> main change - it doesn't touch psql lexer - like Tom proposed
> other points respect Phil's notices

I reviewed v12 patch. It provides gset command which works
consistently with other psql commands, such as \g and \set, and
implementation seems reasonable, and follows other reviewer's comments
properly. I think we can mark it as "ready for committer", once you
have fixed some minor issues below.

* Skipping leading blank in inner while loop of command.c seems
unnecessary, because (IIUC) psql's scanner skips blanks. Is there any
case that scanner returns token with leading/trailing blank?
* ISTM that VARLIST_INITIAL can be removed. AFAIS it's same state as
VARLIST_EXPECTED_COMMA_OR_IDENT.
* I found some cosmetic flaw and typo. Please see attached patch for details.
* How about pulling up codes for PGRES_TUPLES_OK case in
StoreQueryResult to new static function, say StoreQueryTuple? It
would make StoreQueryResult more similar to PrintQueryResult's style,
and IMO it makes the code more readable.

Regards,
--
Shigeru HANADA

2012/12/17 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Hello
>>
>> here is updated patch
>>
>> main change - it doesn't touch psql lexer - like Tom proposed
>> other points respect Phil's notices
>
> I reviewed v12 patch. It provides gset command which works
> consistently with other psql commands, such as \g and \set, and
> implementation seems reasonable, and follows other reviewer's comments
> properly. I think we can mark it as "ready for committer", once you
> have fixed some minor issues below.
>
> * Skipping leading blank in inner while loop of command.c seems
> unnecessary, because (IIUC) psql's scanner skips blanks. Is there any
> case that scanner returns token with leading/trailing blank?

removed

> * ISTM that VARLIST_INITIAL can be removed. AFAIS it's same state as
> VARLIST_EXPECTED_COMMA_OR_IDENT.

removed

> * I found some cosmetic flaw and typo. Please see attached patch for details.

it is ok, merged

> * How about pulling up codes for PGRES_TUPLES_OK case in
> StoreQueryResult to new static function, say StoreQueryTuple? It
> would make StoreQueryResult more similar to PrintQueryResult's style,
> and IMO it makes the code more readable.

good idea
done

Attached updated patch

Regards

Pavel

>
> Regards,
> --
> Shigeru HANADA

On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Attached updated patch

Seems fine. I'll mark this as "ready for committer".

Nice work!

--
Shigeru HANADA

2012/12/19 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Attached updated patch
>
> Seems fine. I'll mark this as "ready for committer".
>
> Nice work!

thank you very much

Regards

Pavel

>
> --
> Shigeru HANADA

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ gset_13.diff ]

I looked at this a bit. I think you need to reconsider when and how the
\gset state gets cleaned up. Doing it inside StoreQueryResult is not
very good because that only gets reached upon successful execution.
Consider for example

select 1/0 \gset x

You'll get an ERROR from this, and a reasonable user would suppose that
that was that and the \gset is no longer in effect. But guess what,
it's still lurking under the surface, waiting to capture his next command.

This is also causing you unnecessary complication in
ExecQueryUsingCursor, which has to work around the fact that
StoreQueryResult destroys state.

I think it would be better to remove that responsibility from
StoreQueryResult and instead put the gset-list cleanup somewhere at the
end of query processing. Didn't really look into where would be the
best place, but it should be someplace that control passes through no
matter what came back from the server.

BTW, is StoreQueryResult in itself (that is, the switch on
PQresultStatus) actually doing anything useful? It appears to me that
the error cases are handled elsewhere, such that control never gets to
it unless the PQresultStatus is an expected value. If that were not the
case, printouts as laconic as "bad response\n" would certainly not be
acceptable --- people would want to see the underlying error message.

Also, I'm not sure I like the PSQL_CMD_NOSEND business, ie, trashing
the query buffer if anything can be found wrong with the \gset itself.
If I've done

big long multiline query here
\gset x y

I'd expect that the error only discards the \gset and not the query.
An error in some other sort of backslash command in that situation
wouldn't discard the query buffer. For instance try this:

regression=# select 3+2
regression-# \goofus
Invalid command \goofus. Try \? for help.
regression-# ;
?column?
----------
5
(1 row)

regression=#

So AFAICS, PSQL_CMD_NOSEND just represents extra code that's making
things worse not better.

One more gripe is that the parsing logic for \gset is pretty
unintelligible. You've got a "state" variable with only two values,
whose names seem to boil down to whether a comma is expected or not.
And then you've got a separate "process_comma" flag, which means
... well, I'm not sure, but possibly the very same thing. For sure it's
not clear whether all four possible combinations of those two variables
are valid and what they'd mean. This could use another round of
thinking and rewriting. Or at least better comments.

regards, tom lane

I wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> [ gset_13.diff ]

> One more gripe is that the parsing logic for \gset is pretty
> unintelligible.

After further thought, it seems to me that the problem here is an
overcomplicated definition of the \gset command; it could be made
both more usable and simpler to implement, if we looked at it
differently.

First off, why is there a provision to omit variable names for some
columns, ie why bother with saying that you can write \gset x,,y to
store only the first and third columns? If you didn't want the second
value, why didn't you leave it out of the SELECT to start with?
It seems like the only possible reason for that is if you were lazy
and typed "SELECT *" instead of listing the columns ... but then you
still need to list the columns in \gset, and it's pretty error-prone
to make sure that the \gset variable list lines up with what "*" will
emit.

In fact, it's pretty error-prone to have to make the \gset variable list
line up with the SELECT columns in any case. So here's my proposal:
let's forget the variable list entirely, and use the column names
returned by the server as the variable names to assign to. So instead
of
select 1, 2 \gset x,y
you would write
select 1 as x, 2 as y \gset
or just
select 1 x, 2 y \gset
which is exactly as much typing as the existing definition, but much
harder to screw up by misaligning the SELECT's values with the target
names. It also makes it really trivial to do the "SELECT *" case ---
you just do it, and ignore any variables for columns you don't care
about.

A probably-useful extension to this basic concept is to allow \gset
to specify an optional prefix, that is
select 1 as x, 2 as y \gset p_
would set p_x and p_y. This would make it easier to manage results from
multiple \gset operations, and to be sure that you didn't accidentally
overwrite some built-in variable.

So this seems to me to be easier and less error-prone to use than the
existing definition. It would also take a lot less code to implement,
since the parsing logic for \gset would reduce to a couple of lines,
and you'd not need the variable-name-list data structure at all.

regards, tom lane

On 01/26/2013 11:42 AM, Tom Lane wrote:

> A probably-useful extension to this basic concept is to allow \gset
> to specify an optional prefix, that is
> select 1 as x, 2 as y \gset p_
> would set p_x and p_y. This would make it easier to manage results from
> multiple \gset operations, and to be sure that you didn't accidentally
> overwrite some built-in variable.

+1. This looks quite nifty. Maybe useful too to have a default prefix
via some setting.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> +1. This looks quite nifty. Maybe useful too to have a default prefix
> via some setting.

Meh. I would expect that "\gset :foo" would work to specify a computed
prefix if you wanted it --- isn't that sufficient indirection? I'm not
thrilled with further expanding the set of magic variables in psql.

regards, tom lane

Hello

2013/1/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> I wrote:
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>> [ gset_13.diff ]
>
>> One more gripe is that the parsing logic for \gset is pretty
>> unintelligible.
>
> After further thought, it seems to me that the problem here is an
> overcomplicated definition of the \gset command; it could be made
> both more usable and simpler to implement, if we looked at it
> differently.
>
> First off, why is there a provision to omit variable names for some
> columns, ie why bother with saying that you can write \gset x,,y to
> store only the first and third columns? If you didn't want the second
> value, why didn't you leave it out of the SELECT to start with?
> It seems like the only possible reason for that is if you were lazy
> and typed "SELECT *" instead of listing the columns ... but then you
> still need to list the columns in \gset, and it's pretty error-prone
> to make sure that the \gset variable list lines up with what "*" will
> emit.

possibility to skip some variables is David Fetter's idea. I see only
one possible use case - it enable use some query from history without
necessity to modify query or creating some auxiliary variables.
Personally, I can live without this feature, but it question for David
mainly.

>
> In fact, it's pretty error-prone to have to make the \gset variable list
> line up with the SELECT columns in any case. So here's my proposal:
> let's forget the variable list entirely, and use the column names
> returned by the server as the variable names to assign to. So instead
> of
> select 1, 2 \gset x,y
> you would write
> select 1 as x, 2 as y \gset
> or just
> select 1 x, 2 y \gset
> which is exactly as much typing as the existing definition, but much
> harder to screw up by misaligning the SELECT's values with the target
> names. It also makes it really trivial to do the "SELECT *" case ---
> you just do it, and ignore any variables for columns you don't care
> about.

hard to say

your proposal has advantages - and implementation is simple, but it is
looking little bit strange - but like other psql features.

I have no strong opinion - I prefer original design, as more explicit
with clean separation line between query and console part, but I am
able to see advantages of your proposal - so depends what will speak
others. I have no problem with your design, although I am thinking so
original design is little bit more safer (but not with significant
difference).

>
> A probably-useful extension to this basic concept is to allow \gset
> to specify an optional prefix, that is
> select 1 as x, 2 as y \gset p_
> would set p_x and p_y. This would make it easier to manage results from
> multiple \gset operations, and to be sure that you didn't accidentally
> overwrite some built-in variable.

I understand to motivation - but I am not enthused. Now - a work with
variables is strange - and with it will be more stranger.

>
> So this seems to me to be easier and less error-prone to use than the
> existing definition. It would also take a lot less code to implement,
> since the parsing logic for \gset would reduce to a couple of lines,
> and you'd not need the variable-name-list data structure at all.

I will waiting for others - I can live with this proposal.

Regards

Pavel

>
> regards, tom lane

Hello

2013/1/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> +1. This looks quite nifty. Maybe useful too to have a default prefix
>> via some setting.
>
> Meh. I would expect that "\gset :foo" would work to specify a computed
> prefix if you wanted it --- isn't that sufficient indirection? I'm not
> thrilled with further expanding the set of magic variables in psql.
>

here is patch related to your proposal

Regards

Pavel

> regards, tom lane

Hello

can you look, please, on updated version - it respects Tom's proposal
and it is significantly reduced?

Thank you

Pavel Stehule

2013/1/28 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2013/1/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> +1. This looks quite nifty. Maybe useful too to have a default prefix
>>> via some setting.
>>
>> Meh. I would expect that "\gset :foo" would work to specify a computed
>> prefix if you wanted it --- isn't that sufficient indirection? I'm not
>> thrilled with further expanding the set of magic variables in psql.
>>
>
> here is patch related to your proposal
>
> Regards
>
> Pavel
>
>> regards, tom lane

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> here is patch related to your proposal

I looked at this a bit. It's getting there, though I still don't trust
the places where you've chosen to clear the prefix setting. (Looking at
it, I'm now not sure that I trust the implementation of \g either.)

However, what I wanted to ask about was this:

> + if (PQgetisnull(result, 0, i))
> + value = pset.popt.nullPrint ? pset.popt.nullPrint : "";
> + else
> + value = PQgetvalue(result, 0, i);

What's the argument for using nullPrint here? ISTM that's effectively a
form of escaping, and I'd not expect that to get applied to values going
into variables, any more than any other formatting we do when printing
results.

Admittedly, if we just take the PQgetvalue result blindly, there'll
be no way to tell the difference between empty-string and NULL results.
But I'm not convinced that this approach is better. It would certainly
need more than no documentation.

regards, tom lane

Hello

2013/2/1 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> here is patch related to your proposal
>
> I looked at this a bit. It's getting there, though I still don't trust
> the places where you've chosen to clear the prefix setting. (Looking at
> it, I'm now not sure that I trust the implementation of \g either.)
>
> However, what I wanted to ask about was this:
>
>> + if (PQgetisnull(result, 0, i))
>> + value = pset.popt.nullPrint ? pset.popt.nullPrint : "";
>> + else
>> + value = PQgetvalue(result, 0, i);
>
> What's the argument for using nullPrint here? ISTM that's effectively a
> form of escaping, and I'd not expect that to get applied to values going
> into variables, any more than any other formatting we do when printing
> results.
>
> Admittedly, if we just take the PQgetvalue result blindly, there'll
> be no way to tell the difference between empty-string and NULL results.
> But I'm not convinced that this approach is better. It would certainly
> need more than no documentation.
>

I have not strong opinion about storing NULL value - and nullPrint is
a best from simple variants -

possible variants

a) don't store NULL values - and remove existing variable when NULL
be assigned - it is probably best, but should be confusing for users
b) implement flag IS NULL - for variables
c) use nullPrint
d) use empty String

@d is subset of @c, and I think so it put some better possibilities
with only two lines more

@a is probably best - but significant change - not hard to implement it

SELECT NULL AS x \g pref_
SELECT :'pref_' IS NULL;

is can be nice

but it should be premature optimization too - nullPrint is enough for
typical use cases

Regards

Pavel

Regards

Pavel

> regards, tom lane

On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> possible variants
>
> a) don't store NULL values - and remove existing variable when NULL
> be assigned - it is probably best, but should be confusing for users
> b) implement flag IS NULL - for variables
> c) use nullPrint
> d) use empty String

+1 for a). If users want to determine whether the result was NULL, or
want to use substitute string for NULL result, they can use coalesce
in SELECT clause. Anyway the feature should be documented clearly.

--
Shigeru HANADA

2013/2/2 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> possible variants
>>
>> a) don't store NULL values - and remove existing variable when NULL
>> be assigned - it is probably best, but should be confusing for users
>> b) implement flag IS NULL - for variables
>> c) use nullPrint
>> d) use empty String
>
> +1 for a). If users want to determine whether the result was NULL, or
> want to use substitute string for NULL result, they can use coalesce
> in SELECT clause. Anyway the feature should be documented clearly.
>

yes, this has one other advantage - it doesn't block possible
enhancing variables about NULL support in future. And other - it
doesn't depends on psql settings

Regards

Pavel

> --
> Shigeru HANADA

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> possible variants
>>
>> a) don't store NULL values - and remove existing variable when NULL
>> be assigned - it is probably best, but should be confusing for users
>> b) implement flag IS NULL - for variables
>> c) use nullPrint
>> d) use empty String

> +1 for a). If users want to determine whether the result was NULL, or
> want to use substitute string for NULL result, they can use coalesce
> in SELECT clause. Anyway the feature should be documented clearly.

Yeah, I was considering that one too. Let's do it that way.

regards, tom lane

Hello

2013/2/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> possible variants
>>>
>>> a) don't store NULL values - and remove existing variable when NULL
>>> be assigned - it is probably best, but should be confusing for users
>>> b) implement flag IS NULL - for variables
>>> c) use nullPrint
>>> d) use empty String
>
>> +1 for a). If users want to determine whether the result was NULL, or
>> want to use substitute string for NULL result, they can use coalesce
>> in SELECT clause. Anyway the feature should be documented clearly.
>
> Yeah, I was considering that one too. Let's do it that way.

updated version

Regards

Pavel

>
> regards, tom lane

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2013/2/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>>> +1 for a). If users want to determine whether the result was NULL, or
>>> want to use substitute string for NULL result, they can use coalesce
>>> in SELECT clause. Anyway the feature should be documented clearly.

>> Yeah, I was considering that one too. Let's do it that way.

> updated version

Applied with corrections.

regards, tom lane

2013/2/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> possible variants
>>>
>>> a) don't store NULL values - and remove existing variable when NULL
>>> be assigned - it is probably best, but should be confusing for users
>>> b) implement flag IS NULL - for variables
>>> c) use nullPrint
>>> d) use empty String
>
>> +1 for a). If users want to determine whether the result was NULL, or
>> want to use substitute string for NULL result, they can use coalesce
>> in SELECT clause. Anyway the feature should be documented clearly.
>
> Yeah, I was considering that one too. Let's do it that way.
>
> regards, tom lane

possible simple enhancing of this behave (for 9.4).

now missing variables is replaced by variable's name. We can implement
some pset option - some like define what do with missing variable

\pset missing_variable (use_name | use_null | error )

when this option will be active, then missing variable will be
replaced by NULL. With this feature sequences of SQL statements joined
by some variables can work.

SELECT NULL as myvar \gset
\pset missing_variable use_null
SELECT :'myvar' IS NULL;

ideas, opinions ?

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> now missing variables is replaced by variable's name. We can implement
> some pset option - some like define what do with missing variable

> \pset missing_variable (use_name | use_null | error )

No, it isn't "replaced by variable's name". What actually happens is we
don't attempt a replacement unless the string after the colon matches an
existing variable. Tampering with that seems dangerous and foolish.

regards, tom lane

2013/2/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> now missing variables is replaced by variable's name. We can implement
>> some pset option - some like define what do with missing variable
>
>> \pset missing_variable (use_name | use_null | error )
>
> No, it isn't "replaced by variable's name". What actually happens is we
> don't attempt a replacement unless the string after the colon matches an
> existing variable. Tampering with that seems dangerous and foolish.

some other ideas?

do you think so full NULL support has sense?

Regards

Pavel

>
> regards, tom lane