Thread: [HACKERS] 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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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.


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

I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing
thispatch (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
firstreview, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt
okaybut 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

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

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.


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

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


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