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

[Fabien COELHO]

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

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

[Robins Tharakan]

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

-
robins | mobile

On 20 Sep. 2017 5:13 pm, "Fabien COELHO"  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: [HACKERS] psql - add ability to test whether a variable exists

[Fabien COELHO]

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

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

[Robins Tharakan]

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

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

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

[Robins Tharakan]

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  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: [HACKERS] psql - add ability to test whether a variable exists

[Fabien COELHO]

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

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

[Robins Tharakan]

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

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

[Fabien COELHO]

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.


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

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

[Corey Huinker]

On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule 
wrote:

>
>
> 2017-08-26 19:55 GMT+02:00 Fabien COELHO :
>
>>
>> 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: [HACKERS] psql - add ability to test whether a variable exists

[Pavel Stehule]

2017-08-26 19:55 GMT+02:00 Fabien COELHO :

>
> 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: [HACKERS] psql - add ability to test whether a variable exists

[Fabien COELHO]

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.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..58f8e9a 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -781,6 +781,10 @@ testdb=
 The forms :'variable_name' and
 :"variable_name" described there
 work as well.
+The :{?variable_name} syntax allows
+to test whether a variable is defined. It is substituted by
+TRUE or FALSE.
+Escaping the colon with a backslash protects it from substitution.
 
 
 
@@ -3813,6 +3817,12 @@ testdb= INSERT INTO my_table VALUES (:'content');
 
 
 
+The :{?name} special syntax returns TRUE
+or FALSE depending on whether the variable exists or not, thus is
+always substituted, unless the colon is backslash-escaped.
+
+
+
 The colon syntax for variables is standard SQL for
 embedded query languages, such as ECPG.
 The colon syntaxes for array slices and type casts are
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index db7a1b9..9a53cb3 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -281,6 +281,10 @@ other			.
 	unquoted_option_chars = 0;
 }
 
+:\{\?{variable_char}+\}	{
+	psqlscan_test_variable(cur_state, yytext, yyleng);
+}
+
 :'{variable_char}*	{
 	/* Throw back everything but the colon */
 	yyless(1);
@@ -295,6 +299,20 @@ other			.
 	ECHO;
 }
 
+:\{\?{variable_char}*	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	unquoted_option_chars++;
+	ECHO;
+}
+
+:\{		{
+	/* Throw back everything but the colon */
+	yyless(1);
+	unquoted_option_chars++;
+	ECHO;
+}
+
 {other}			{
 	unquoted_option_chars++;
 	ECHO;
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 27689d7..4375142a 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -745,9 +745,13 @@ other			.
 			 PQUOTE_SQL_IDENT);
 }
 
+:\{\?{variable_char}+\}	{
+	psqlscan_test_variable(cur_state, yytext, yyleng);
+}
+
 	/*
 	 * These rules just avoid the need for scanner backup if one of the
-	 * two rules above fails to match completely.
+	 * three rules above fails to match completely.
 	 */
 
 :'{variable_char}*	{
@@ -762,6 +766,17 @@ other			.
 	ECHO;
 }
 
+:\{\?{variable_char}*	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	ECHO;
+}
+:\{	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	ECHO;
+}
+
 	/*
 	 * Back to backend-compatible rules.
 	 */
@@ -1442,3 +1457,28 @@ psqlscan_escape_variable(PsqlScanState state, const char *txt, int len,
 		psqlscan_emit(state, txt, len);
 	}
 }
+
+void
+psqlscan_test_variable(PsqlScanState state, const char *txt, int len)
+{
+	char	*varname;
+	char	*value;
+
+	varname = psqlscan_extract_substring(state, txt + 3, len - 4);
+	if (state->callbacks->get_variable)
+		value = state->callbacks->get_variable(varname, PQUOTE_PLAIN,
+			   state->cb_passthrough);
+	else
+		value = NULL;
+	free(varname);
+
+	if (value != NULL)
+	{
+		psqlscan_emit(state, "TRUE", 4);
+		free(value);
+	}
+	else
+	{
+		psqlscan_emit(state, "FALSE", 5);
+	}
+}
diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h
index c70ff29..e9b3517 100644
--- a/src/include/fe_utils/psqlscan_int.h
+++ b/src/include/fe_utils/psqlscan_int.h
@@ -142,5 +142,7 @@ extern char *psqlscan_extract_substring(PsqlScanState state,
 extern void psqlscan_escape_variable(PsqlScanState state,
 		 const char *txt, int len,
 		 PsqlScanQuoteType quote);
+extern void psqlscan_test_variable(PsqlScanState state,
+   const char *txt, int len);
 
 #endif			/* PSQLSCAN_INT_H */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 4aaf4c1..2b2f23b 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2929,6 +2929,32 @@ bar 'bar' "bar"
 	\echo 'should print #8-1'
 should print #8-1
 \endif
+-- :{?...} defined variable test
+\set i 1
+\if :{?i}
+  \echo '#9-1 ok, variable i is defined'
+#9-1 ok, variable i is defined
+\else
+  \echo 'should not print #9-2'
+\endif
+\if :{?no_such_variable}
+  \echo 'should not print #10-1'
+\else
+  \echo '#10-2 ok, variable no_such_variable is not defined'
+#10-2 ok, variable no_such_variable is not defined
+\endif
+SELECT :{?i} AS i_is_defined;
+ i_is_defined 
+--
+ t

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

[Fabien COELHO]

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.


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

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

[Pavel Stehule]

2017-08-26 8:54 GMT+02:00 Fabien COELHO :

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