Re: psql - add ability to test whether a variable exists

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.

2017-08-26 8:54 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

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.

> 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.

2017-08-26 19:55 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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.

On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
>
> 2017-08-26 19:55 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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}.

>> 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.

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(at)# \echo 'testing'
\echo command ignored; use \endif or Ctrl-C to exit current \if block
postgres(at)# \endif
postgres=#

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(at)# \echo 'testing'
> \echo command ignored; use \endif or Ctrl-C to exit current \if block
> postgres(at)# \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.

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(at)cri(dot)ensmp(dot)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(at)# \echo 'testing'
>> \echo command ignored; use \endif or Ctrl-C to exit current \if block
>> postgres(at)# \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.
>

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

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.

Correct Fabien. I have already removed myself as a reviewer. Thanks.

-
robins | mobile

On 20 Sep. 2017 5:13 pm, "Fabien COELHO" <coelho(at)cri(dot)ensmp(dot)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.
>

> 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.