Thread
Commits
-
Provide a test for variable existence in psql
- d57c7a7c5062 11.0 landed
-
psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-08-26T06:54:24Z
Hello, As a follow-up to the \if patch by Corey Huinker, here is a proposal to allow testing whether a client-side variable exists in psql. The syntax is as ugly as the current :'var' and :"var" things, but ISTM that this is the only simple option to have a working SQL-compatible syntax with both client-side substitution and server side execution. See the second example below. -- client side use psql> \set i 1 psql> \if :?i? psql> \echo 'i is defined' psql> \endif -- use server-side in an SQL expression psql> SELECT NOT :?VERSION_NUM? OR :'VERSION' <> VERSTION() AS bad_conf \gset psql> \if :bad_conf \echo 'too bad...' \quit \endif The other option would be to have some special keyword syntax, say "defined var", but then it would have to be parsed client side, and how to do that in an SQL expression is unclear, and moreover it would not look right in an SQL expression. If it would look like a function call, say "defined('var')", it would potentially interact with existing server-side user-defined functions, which is pretty undesirable. Hence the :?...? proposal above which is limited to variable subsitution syntax. -- Fabien. -
Re: psql - add ability to test whether a variable exists
Pavel Stehule <pavel.stehule@gmail.com> — 2017-08-26T07:44:30Z
2017-08-26 8:54 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>: > > Hello, > > As a follow-up to the \if patch by Corey Huinker, here is a proposal to > allow testing whether a client-side variable exists in psql. > > The syntax is as ugly as the current :'var' and :"var" things, but ISTM > that this is the only simple option to have a working SQL-compatible syntax > with both client-side substitution and server side execution. See the > second example below. > It is really ugly - the ? symbol is not used in pair usually - so it is much more visible - it is bad readable. Maybe some other syntax: :{fx xxx} .. where fx can be one from more possible operators ? ! ... > > -- client side use > psql> \set i 1 > psql> \if :?i? > psql> \echo 'i is defined' > psql> \endif > -- use server-side in an SQL expression > psql> SELECT NOT :?VERSION_NUM? OR :'VERSION' <> VERSTION() AS bad_conf > \gset > > psql> \if :bad_conf \echo 'too bad...' \quit \endif > > The other option would be to have some special keyword syntax, say > "defined var", but then it would have to be parsed client side, and how to > do that in an SQL expression is unclear, and moreover it would not look > right in an SQL expression. If it would look like a function call, say > "defined('var')", it would potentially interact with existing server-side > user-defined functions, which is pretty undesirable. Hence the :?...? > proposal above which is limited to variable subsitution syntax. should not be solved by introduction \ifdef ? > > -- > Fabien. > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-08-26T15:38:15Z
Hello Pavel, >> As a follow-up to the \if patch by Corey Huinker, here is a proposal to >> allow testing whether a client-side variable exists in psql. >> >> The syntax is as ugly as the current :'var' and :"var" things, but ISTM >> that this is the only simple option to have a working SQL-compatible syntax >> with both client-side substitution and server side execution. See the >> second example below. > > It is really ugly Yes, I agree:-) > - the ? symbol is not used in pair usually - so it is > much more visible - it is bad readable. Yep. > Maybe some other syntax: :{fx xxx} .. where fx can be one from more > possible operators ? ! ... Any colon prefixed syntax can be made to work because it is enough for the lexer to detect and handle... so :{defined varname} may be an option, although I do not like the space much because it adds some fuzzyness in the lexer which has to process it. It is probably doable, though. I like having a "?" because there is a question. Other suggestions somehow in line with your proposal could be :{?varname} :{varname?} what do you think? >> The other option would be to have some special keyword syntax, say >> "defined var", but then it would have to be parsed client side, and how to >> do that in an SQL expression is unclear, and moreover it would not look >> right in an SQL expression. If it would look like a function call, say >> "defined('var')", it would potentially interact with existing server-side >> user-defined functions, which is pretty undesirable. Hence the :?...? >> proposal above which is limited to variable subsitution syntax. > > should not be solved by introduction \ifdef ? This would be a client side only non extendable option: you can only test one variable at a time, you cannot say "NOT" or have to do \ifndef... CPP started like that and ended with #if bool-expr-with-defined in the end. The idea is to extend the newly added \if with client-side SQL-compatible expression syntax, and that the same syntax could be used server side with select, eg: -- client side \let i :j + 1 -- server side SELECT :j + 1 AS i \gset -- client-side conditional \if NOT :j > 1 OR ... The colon prefixed substitution syntax already works for server side, but there is no way to check whether a variable exists which is compatible with that, hence this patch. Pgbench has expressions with patches to improve it, especially adding boolean operators. I think that the simplest plan is, when stabalized, to move the parser & executir to fe_utils and to use it from both psql et pgbench. Also I plan to add \if to pgbench, possibly by factoring some of the code from psql in fe_utils as well because it would help with benchmarks. Now given the patch queue length and speed I'm not even thinking of starting doing all this. -- Fabien. -
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-08-26T17:55:34Z
> Any colon prefixed syntax can be made to work because it is enough for the > lexer to detect and handle... so > > :{defined varname} > > may be an option, although I do not like the space much because it adds some > fuzzyness in the lexer which has to process it. It is probably doable, > though. I like having a "?" because there is a question. Other > suggestions somehow in line with your proposal could be > :{?varname} > :{varname?} > what do you think? Here is a version with the :{?varname} syntax. -- Fabien. -
Re: psql - add ability to test whether a variable exists
Pavel Stehule <pavel.stehule@gmail.com> — 2017-08-26T18:09:27Z
2017-08-26 19:55 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>: > > Any colon prefixed syntax can be made to work because it is enough for the >> lexer to detect and handle... so >> >> :{defined varname} >> >> may be an option, although I do not like the space much because it adds >> some fuzzyness in the lexer which has to process it. It is probably doable, >> though. I like having a "?" because there is a question. Other >> suggestions somehow in line with your proposal could be >> :{?varname} >> :{varname?} >> what do you think? >> > > Here is a version with the :{?varname} syntax. It looks much better for me. Regards Pavel > > > -- > Fabien. -
Re: psql - add ability to test whether a variable exists
Corey Huinker <corey.huinker@gmail.com> — 2017-08-26T18:20:06Z
On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > 2017-08-26 19:55 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>: > >> >> Any colon prefixed syntax can be made to work because it is enough for >>> the lexer to detect and handle... so >>> >>> :{defined varname} >>> >>> may be an option, although I do not like the space much because it adds >>> some fuzzyness in the lexer which has to process it. It is probably doable, >>> though. I like having a "?" because there is a question. Other >>> suggestions somehow in line with your proposal could be >>> :{?varname} >>> :{varname?} >>> what do you think? >>> >> >> Here is a version with the :{?varname} syntax. > > > It looks much better for me. > > Regards > > Pavel > +1. Glad to have this feature. Any of the proposed syntaxes look good to me, with a slight preference for {?var}. -
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-08-26T18:40:24Z
>> Here is a version with the :{?varname} syntax. > > It looks much better for me. I'll admit that it looks better to me as well:-) -- Fabien. -
Re: psql - add ability to test whether a variable exists
Robins Tharakan <tharakan@gmail.com> — 2017-09-19T14:07:00Z
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation: tested, failed The patch applies cleanly and compiles + installs fine (although am unable to do installcheck-world on my Cygwin setup). This is how the patch works on my setup. $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost psql (11devel, server 9.6.1) Type "help" for help. postgres=# \set i 1 postgres=# \if :{?i} postgres=# \echo 'testing' testing postgres=# \endif postgres=# \if :{?id} postgres@# \echo 'testing' \echo command ignored; use \endif or Ctrl-C to exit current \if block postgres@# \endif postgres=# -
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-09-20T06:18:25Z
Hello Robins, Thanks for the review. > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: tested, failed Where ? > Spec compliant: not tested > Documentation: tested, failed Where ? I just regenerated the html doc on the patch without a glitch. > The patch applies cleanly and compiles + installs fine (although am > unable to do installcheck-world on my Cygwin setup). This is how the > patch works on my setup. > > $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost > psql (11devel, server 9.6.1) > Type "help" for help. > > postgres=# \set i 1 > postgres=# \if :{?i} > postgres=# \echo 'testing' > testing > postgres=# \endif > postgres=# \if :{?id} > postgres@# \echo 'testing' > \echo command ignored; use \endif or Ctrl-C to exit current \if block > postgres@# \endif > postgres=# ISTM that this is the expected behavior. In the first case, "i" is defined, so the test is true and the echo echoes. In the second case, "id" is undefined, the test is false and the echo is skipped. I do not understand why you conclude that the feature "failed". -- Fabien. -
Re: psql - add ability to test whether a variable exists
Robins Tharakan <tharakan@gmail.com> — 2017-09-20T06:37:46Z
Hi Fabien, I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. I've set this back to 'Needs Review' because clearly needs it. Apologies for the noise here. - robins On 20 September 2017 at 16:18, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Hello Robins, > > Thanks for the review. > > The following review has been posted through the commitfest application: >> make installcheck-world: not tested >> Implements feature: tested, failed >> > > Where ? > > Spec compliant: not tested >> Documentation: tested, failed >> > > Where ? I just regenerated the html doc on the patch without a glitch. > > The patch applies cleanly and compiles + installs fine (although am unable >> to do installcheck-world on my Cygwin setup). This is how the patch works >> on my setup. >> >> $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost >> psql (11devel, server 9.6.1) >> Type "help" for help. >> >> postgres=# \set i 1 >> postgres=# \if :{?i} >> postgres=# \echo 'testing' >> testing >> postgres=# \endif >> postgres=# \if :{?id} >> postgres@# \echo 'testing' >> \echo command ignored; use \endif or Ctrl-C to exit current \if block >> postgres@# \endif >> postgres=# >> > > ISTM that this is the expected behavior. > > In the first case, "i" is defined, so the test is true and the echo echoes. > > In the second case, "id" is undefined, the test is false and the echo is > skipped. > > I do not understand why you conclude that the feature "failed". > > -- > Fabien. > -
Re: psql - add ability to test whether a variable exists
Robins Tharakan <tharakan@gmail.com> — 2017-09-20T06:38:08Z
I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. I've set this back to 'Needs Review' because clearly needs it. Apologies for the noise here. The new status of this patch is: Needs review
-
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-09-20T07:13:05Z
Hello Robins, > I was able to test the functionality (which seemed to work fine) and fed in > my comment to assist anyone else reviewing this patch (and intentionally > let it's state as 'Needs Review'). > > While trying to provide my feedback, on hindsight I should have been more > detailed about what I didn't test. Being my first review, I didn't > understand that not checking a box meant 'failure'. For e.g. I read the > sgml changes, which felt okay but didn't click 'Passed' because my env > wasn't setup properly. Hmmm, ISTM that it was enough. The feature is psql specific, so the fact that it works against a 9.6 server is both expected and fine. So ISTM that your test "passed". Just running "make check" would run the non regression test, which is basically what you tested online, against the compiled version. Probably you should have a little look at the source code and doc as well. > I've set this back to 'Needs Review' because clearly needs it. Hmmm. If you do a review, which I think you have done, then you have done it:-) If you consider that your test was not a review and you do not intend to provide one, then thanks for the feedback anyway, and maybe you should consider removing yourself from the "Reviewer" column, otherwise nobody will provide a review. -- Fabien.
-
Re: psql - add ability to test whether a variable exists
Robins Tharakan <tharakan@gmail.com> — 2017-09-20T07:55:09Z
Correct Fabien. I have already removed myself as a reviewer. Thanks. - robins | mobile On 20 Sep. 2017 5:13 pm, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote: > > Hello Robins, > > I was able to test the functionality (which seemed to work fine) and fed in >> my comment to assist anyone else reviewing this patch (and intentionally >> let it's state as 'Needs Review'). >> >> While trying to provide my feedback, on hindsight I should have been more >> detailed about what I didn't test. Being my first review, I didn't >> understand that not checking a box meant 'failure'. For e.g. I read the >> sgml changes, which felt okay but didn't click 'Passed' because my env >> wasn't setup properly. >> > > Hmmm, ISTM that it was enough. The feature is psql specific, so the fact > that it works against a 9.6 server is both expected and fine. So ISTM that > your test "passed". > > Just running "make check" would run the non regression test, which is > basically what you tested online, against the compiled version. > > Probably you should have a little look at the source code and doc as well. > > I've set this back to 'Needs Review' because clearly needs it. >> > > Hmmm. > > If you do a review, which I think you have done, then you have done it:-) > > If you consider that your test was not a review and you do not intend to > provide one, then thanks for the feedback anyway, and maybe you should > consider removing yourself from the "Reviewer" column, otherwise nobody > will provide a review. > > -- > Fabien. >
-
Re: psql - add ability to test whether a variable exists
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-09-20T12:59:03Z
> Correct Fabien. I have already removed myself as a reviewer. Thanks. As you wish! Thanks for the feedback, which I understood as "works for me". -- Fabien.