Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix oversight in new code for printing rangetable aliases.
- 8b728e5c6e0c 9.3.0 cited
-
proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-07-26T05:13:01Z
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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-26T05:36:17Z
Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-07-26T05:37:40Z
2012/7/26 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
David Fetter <david@fetter.org> — 2012-07-26T06:19:55Z
On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: > Pavel Stehule <pavel.stehule@gmail.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-07-26T06:31:13Z
2012/7/26 David Fetter <david@fetter.org>: > On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: >> Pavel Stehule <pavel.stehule@gmail.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@fetter.org> http://fetter.org/ > Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter > Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
David Fetter <david@fetter.org> — 2012-07-26T06:36:55Z
On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote: > 2012/7/26 David Fetter <david@fetter.org>: > > On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: > >> Pavel Stehule <pavel.stehule@gmail.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-07-26T06:44:23Z
2012/7/26 David Fetter <david@fetter.org>: > On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote: >> 2012/7/26 David Fetter <david@fetter.org>: >> > On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: >> >> Pavel Stehule <pavel.stehule@gmail.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@fetter.org> http://fetter.org/ > Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter > Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-27T05:49:28Z
Pavel Stehule <pavel.stehule@gmail.com> writes: > 2012/7/26 David Fetter <david@fetter.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-07-28T16:11:21Z
Hello 2012/7/27 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> 2012/7/26 David Fetter <david@fetter.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
-
Re: proposal - assign result of query to psql variable
David Fetter <david@fetter.org> — 2012-08-01T04:05:26Z
On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote: > Hello > > 2012/7/27 Tom Lane <tgl@sss.pgh.pa.us>: > > Pavel Stehule <pavel.stehule@gmail.com> writes: > >> 2012/7/26 David Fetter <david@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-08-09T18:21:25Z
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@fetter.org>: > On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote: >> Hello >> >> 2012/7/27 Tom Lane <tgl@sss.pgh.pa.us>: >> > Pavel Stehule <pavel.stehule@gmail.com> writes: >> >> 2012/7/26 David Fetter <david@fetter.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@fetter.org> http://fetter.org/ > Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter > Skype: davidfetter XMPP: david.fetter@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-09-19T09:51:13Z
On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-09-20T17:01:35Z
Hello 2012/9/19 Shigeru HANADA <shigeru.hanada@gmail.com>: > On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-09-21T08:24:59Z
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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-09-27T19:09:19Z
Hello 2012/9/21 Shigeru HANADA <shigeru.hanada@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-12T15:58:05Z
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.stehule@gmail.com>: > Hello > > 2012/9/21 Shigeru HANADA <shigeru.hanada@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-10-13T05:08:31Z
Hi Pavel, On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-13T17:26:18Z
2012/10/13 Shigeru HANADA <shigeru.hanada@gmail.com>: > Hi Pavel, > > > On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Erik Rijkers <er@xs4all.nl> — 2012-10-13T22:11:14Z
On Sat, October 13, 2012 19:26, Pavel Stehule wrote: > 2012/10/13 Shigeru HANADA <shigeru.hanada@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-14T18:23:20Z
Hello 2012/10/14 Erik Rijkers <er@xs4all.nl>: > On Sat, October 13, 2012 19:26, Pavel Stehule wrote: >> 2012/10/13 Shigeru HANADA <shigeru.hanada@gmail.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 >
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-14T21:13:40Z
Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T03:57:42Z
Hello fast reply 2012/10/14 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T09:34:09Z
Hello 2012/10/14 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T09:56:39Z
2012/10/15 Pavel Stehule <pavel.stehule@gmail.com>: > Hello > > 2012/10/14 Tom Lane <tgl@sss.pgh.pa.us>: >> Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-10-15T10:17:42Z
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.stehule@gmail.com> wrote: > 2012/10/14 Tom Lane <tgl@sss.pgh.pa.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.hanada@gmail.com
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T10:48:53Z
2012/10/15 Shigeru HANADA <shigeru.hanada@gmail.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.stehule@gmail.com> wrote: >> 2012/10/14 Tom Lane <tgl@sss.pgh.pa.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.hanada@gmail.com > > > > >
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T12:16:25Z
2012/10/15 Pavel Stehule <pavel.stehule@gmail.com>: > 2012/10/15 Shigeru HANADA <shigeru.hanada@gmail.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.stehule@gmail.com> wrote: >>> 2012/10/14 Tom Lane <tgl@sss.pgh.pa.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.hanada@gmail.com >> >> >> >> >>
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-15T21:59:10Z
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.stehule@gmail.com>: > 2012/10/15 Pavel Stehule <pavel.stehule@gmail.com>: >> 2012/10/15 Shigeru HANADA <shigeru.hanada@gmail.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.stehule@gmail.com> wrote: >>>> 2012/10/14 Tom Lane <tgl@sss.pgh.pa.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.hanada@gmail.com >>> >>> >>> >>> >>>
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-10-16T08:14:36Z
Hi Pavel, On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-16T16:24:12Z
2012/10/16 Shigeru HANADA <shigeru.hanada@gmail.com>: > Hi Pavel, > > On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Phil Sorber <phil@omniti.com> — 2012-10-24T18:48:07Z
On Tue, Oct 16, 2012 at 12:24 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2012/10/16 Shigeru HANADA <shigeru.hanada@gmail.com>: >> Hi Pavel, >> >> On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel.stehule@gmail.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@postgresql.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. -
Re: proposal - assign result of query to psql variable
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-25T14:24:14Z
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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-25T14:38:15Z
Alvaro Herrera <alvherre@2ndquadrant.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 -
Re: proposal - assign result of query to psql variable
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-25T15:03:56Z
Tom Lane escribió: > Alvaro Herrera <alvherre@2ndquadrant.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 -
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-25T15:37:07Z
2012/10/25 Tom Lane <tgl@sss.pgh.pa.us>: > Alvaro Herrera <alvherre@2ndquadrant.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 -
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-25T15:49:00Z
Pavel Stehule <pavel.stehule@gmail.com> writes: > 2012/10/25 Tom Lane <tgl@sss.pgh.pa.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 -
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-25T15:58:58Z
2012/10/25 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> 2012/10/25 Tom Lane <tgl@sss.pgh.pa.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 -
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-27T17:23:38Z
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 -
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-27T22:16:39Z
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 -
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-12-17T09:44:55Z
On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-12-17T17:52:19Z
2012/12/17 Shigeru Hanada <shigeru.hanada@gmail.com>: > On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-12-19T06:39:00Z
On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Attached updated patch Seems fine. I'll mark this as "ready for committer". Nice work! -- Shigeru HANADA
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-12-19T08:03:13Z
2012/12/19 Shigeru Hanada <shigeru.hanada@gmail.com>: > On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-26T03:50:00Z
Pavel Stehule <pavel.stehule@gmail.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 -
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-26T16:42:37Z
I wrote: > Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Andrew Dunstan <andrew@dunslane.net> — 2013-01-26T17:04:35Z
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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-26T17:25:28Z
Andrew Dunstan <andrew@dunslane.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-01-26T17:26:37Z
Hello 2013/1/26 Tom Lane <tgl@sss.pgh.pa.us>: > I wrote: >> Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-01-28T20:00:37Z
Hello 2013/1/26 Tom Lane <tgl@sss.pgh.pa.us>: > Andrew Dunstan <andrew@dunslane.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-01T05:22:59Z
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.stehule@gmail.com>: > Hello > > 2013/1/26 Tom Lane <tgl@sss.pgh.pa.us>: >> Andrew Dunstan <andrew@dunslane.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-02-01T22:56:05Z
Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-02T10:30:12Z
Hello 2013/2/1 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Shigeru Hanada <shigeru.hanada@gmail.com> — 2013-02-02T11:25:45Z
On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-02T14:43:46Z
2013/2/2 Shigeru Hanada <shigeru.hanada@gmail.com>: > On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-02-02T17:38:29Z
Shigeru Hanada <shigeru.hanada@gmail.com> writes: > On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-02T20:39:02Z
Hello 2013/2/2 Tom Lane <tgl@sss.pgh.pa.us>: > Shigeru Hanada <shigeru.hanada@gmail.com> writes: >> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-02-02T22:12:25Z
Pavel Stehule <pavel.stehule@gmail.com> writes: > 2013/2/2 Tom Lane <tgl@sss.pgh.pa.us>: >> Shigeru Hanada <shigeru.hanada@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-03T18:22:17Z
2013/2/2 Tom Lane <tgl@sss.pgh.pa.us>: > Shigeru Hanada <shigeru.hanada@gmail.com> writes: >> On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Tom Lane <tgl@sss.pgh.pa.us> — 2013-02-03T19:02:27Z
Pavel Stehule <pavel.stehule@gmail.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
-
Re: proposal - assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2013-02-03T19:34:24Z
2013/2/3 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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