Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite  wrote:

In interactive mode, the warning in untaken branches is misleading
when \endif is on the same line as the commands that
are skipped. For instance:

  postgres=# \if false \echo NOK \endif
  \echo command ignored; use \endif or Ctrl-C to exit current \if block
  postgres=#

From the point of view of the user, the execution flow has exited
the branch already when this warning is displayed.
Of course issuing the recommended \endif at this point doesn't work:

  postgres=# \endif
  \endif: no matching \if

Maybe that part of the message:
"use \endif or Ctrl-C to exit current \if block"
should be displayed only when coming back at the prompt,
and if the flow is still in an untaken branch at this point?


Is this an open item, or do we not care about fixing it?


I would suggest that this is not important enough to block anything.

Otherwise, I agree that displaying this interactive message only when it 
is pertinent is desirable, but this might change the underlying logic 
significantly: it may mean holding somewhere a message to be shown at next 
prompt, and being able to decide when to clear it.


There is also the question of what happens if there are multiple such 
messages, should they all be shown? Only the first? The last? Should it 
avoid repeats?


So I propose to call it a feature for now, especially that we do not 
expect people to write a lot of one-liner multiple-backslash-commands in 
interactive mode.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Robert Haas]

On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite  wrote:
> In interactive mode, the warning in untaken branches is misleading
> when \endif is on the same line as the commands that
> are skipped. For instance:
>
>   postgres=# \if false \echo NOK \endif
>   \echo command ignored; use \endif or Ctrl-C to exit current \if block
>   postgres=#
>
> From the point of view of the user, the execution flow has exited
> the branch already when this warning is displayed.
> Of course issuing the recommended \endif at this point doesn't work:
>
>   postgres=# \endif
>   \endif: no matching \if
>
> Maybe that part of the message:
> "use \endif or Ctrl-C to exit current \if block"
> should be displayed only when coming back at the prompt,
> and if the flow is still in an untaken branch at this point?

Is this an open item, or do we not care about fixing it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Daniel Verite]

  Hi,

In interactive mode, the warning in untaken branches is misleading
when \endif is on the same line as the commands that
are skipped. For instance:

  postgres=# \if false \echo NOK \endif
  \echo command ignored; use \endif or Ctrl-C to exit current \if block
  postgres=# 

From the point of view of the user, the execution flow has exited
the branch already when this warning is displayed.
Of course issuing the recommended \endif at this point doesn't work:

  postgres=# \endif
  \endif: no matching \if

Maybe that part of the message: 
"use \endif or Ctrl-C to exit current \if block"
should be displayed only when coming back at the prompt,
and if the flow is still in an untaken branch at this point?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> If it actually is impossible to give pgbench equivalent behavior, we'll 
>> just have to live with the discrepancy,

> Yep.

>> but ISTM it could probably be made to work.

> Even if it could somehow, I do not see it as a useful feature for pgbench. 

Perhaps not.

> I also lack a good use case for psql for this feature.

It doesn't seem very outlandish to me: the alternative would be to repeat
all of a query that might span dozens of lines, in order to change one or
two lines.  That's not very readable or maintainable.

I'm prepared to believe that extremely long queries aren't ever going
to be common in pgbench scripts, so that there's not much need to support
the equivalent behavior in pgbench.  So maybe it's fine.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


pgbench (well, at least if I succeed in getting boolean expressions and
setting variables, which is just a maybe), but this kind of if in the
middle of expression does not make much sense for a pgbench script where
"if" must be evaluated at execution time, not parse time.


Well, it's not really clear to me why that would be true.


For example, how can you PREPARE a possibly combinatorial thing?

SELECT
  \if ... XX \else YY \endif
FROM
  \if ... ZZ \else WW \endif
WHERE
  \if ... AA \else BB \endif
 ;

Or the kind of operation:

  \if ...
SELECT *
  \else
DELETE
  \endif
  FROM table WHERE condition;

Even the structure can be changed somehow:

  SELECT
\if ...
  1 ;
  SELECT 2
\endif
  ;

If it actually is impossible to give pgbench equivalent behavior, we'll 
just have to live with the discrepancy,


Yep.


but ISTM it could probably be made to work.


Even if it could somehow, I do not see it as a useful feature for pgbench. 
I also lack a good use case for psql for this feature.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> [...] Aside from cosmetic changes, I've made it behave reasonably for 
>> cases where \if is used on portions of a query, for instance

> A small issue I see is that I was planning to add such an if syntax to 
> pgbench (well, at least if I succeed in getting boolean expressions and 
> setting variables, which is just a maybe), but this kind of if in the 
> middle of expression does not make much sense for a pgbench script where 
> "if" must be evaluated at execution time, not parse time.

Well, it's not really clear to me why that would be true.  If it actually
is impossible to give pgbench equivalent behavior, we'll just have to live
with the discrepancy, but ISTM it could probably be made to work.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


Patch applies cleanly. Make check ok. Feature still works!


Idem for v30.

[...] Aside from cosmetic changes, I've made it behave reasonably for 
cases where \if is used on portions of a query, for instance


SELECT
\if :something
   var1
\else
   var2
\endif
FROM table;


This is commendable, but I would not have bothered, although it is more 
cpp-like with it.


A small issue I see is that I was planning to add such an if syntax to 
pgbench (well, at least if I succeed in getting boolean expressions and 
setting variables, which is just a maybe), but this kind of if in the 
middle of expression does not make much sense for a pgbench script where 
"if" must be evaluated at execution time, not parse time.



which as I mentioned a long time ago is something that people will
certainly expect to work.


I would not have expected it to work, but indeed other people could. 
Sometimes I try something with pg and it does not work as I hoped. That is 
life.


I also cleaned up a lot of corner-case discrepancies between how much 
text is consumed in active-branch and inactive-branch cases (OT_FILEPIPE 
is a particularly nasty case in that regard :-()


Indeed.


I plan to read this over again tomorrow and then push it, if there are
not objections/corrections.


My small objection is that an eventual if in pgbench, with a separate 
parsing and execution, will not work in the middle of queries as this one. 
Do you think that such a discrepancy would be admissible.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> New Patch v29: Now with less coverage!

> Patch applies cleanly. Make check ok. Feature still works!

I've been hacking on this for about two full days now, and have gotten
it to a point where I think it's committable.  Aside from cosmetic
changes, I've made it behave reasonably for cases where \if is used
on portions of a query, for instance

SELECT
\if :something
var1
\else
var2
\endif
FROM table;

which as I mentioned a long time ago is something that people will
certainly expect to work.  I also cleaned up a lot of corner-case
discrepancies between how much text is consumed in active-branch and
inactive-branch cases (OT_FILEPIPE is a particularly nasty case in that
regard :-()

I plan to read this over again tomorrow and then push it, if there are
not objections/corrections.

regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..b51b11b 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** hello 10
*** 2064,2069 
--- 2064,2158 
  
  

+ \if expression
+ \elif expression
+ \else
+ \endif
+ 
+ 
+ This group of commands implements nestable conditional blocks.
+ A conditional block must begin with an \if and end
+ with an \endif.  In between there may be any number
+ of \elif clauses, which may optionally be followed
+ by a single \else clause.  Ordinary queries and
+ other types of backslash commands may (and usually do) appear between
+ the commands forming a conditional block.
+ 
+ 
+ The \if and \elif commands read
+ their argument(s) and evaluate them as a boolean expression.  If the
+ expression yields true then processing continues
+ normally; otherwise, lines are skipped until a
+ matching \elif, \else,
+ or \endif is reached.  Once
+ an \if or \elif test has
+ succeeded, the arguments of later \elif commands in
+ the same block are not evaluated but are treated as false.  Lines
+ following an \else are processed only if no earlier
+ matching \if or \elif succeeded.
+ 
+ 
+ The expression argument
+ of an \if or \elif command
+ is subject to variable interpolation and backquote expansion, just
+ like any other backslash command argument.  After that it is evaluated
+ like the value of an on/off option variable.  So a valid value
+ is any unambiguous case-insensitive match for one of:
+ true, false, 1,
+ 0, on, off,
+ yes, no.  For example,
+ t, T, and tR
+ will all be considered to be true.
+ 
+ 
+ Expressions that do not properly evaluate to true or false will
+ generate a warning and be treated as false.
+ 
+ 
+ Lines being skipped are parsed normally to identify queries and
+ backslash commands, but queries are not sent to the server, and
+ backslash commands other than conditionals
+ (\if, \elif,
+ \else, \endif) are
+ ignored.  Conditional commands are checked only for valid nesting.
+ Variable references in skipped lines are not expanded, and backquote
+ expansion is not performed either.
+ 
+ 
+ All the backslash commands of a given conditional block must appear in
+ the same source file. If EOF is reached on the main input file or an
+ \include-ed file before all local
+ \if-blocks have been closed,
+ then psql will raise an error.
+ 
+ 
+  Here is an example:
+ 
+ 
+ -- check for the existence of two separate records in the database and store
+ -- the results in separate psql variables
+ SELECT
+ EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+ EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+ \gset
+ \if :is_customer
+ SELECT * FROM customer WHERE customer_id = 123;
+ \elif :is_employee
+ \echo 'is not a customer but is an employee'
+ SELECT * FROM employee WHERE employee_id = 456;
+ \else
+ \if yes
+ \echo 'not a customer or employee'
+ \else
+ \echo 'this will never print'
+ \endif
+ \endif
+ 
+ 
+   
+ 
+ 
+   
  \l[+] or \list[+] [ pattern ]
  
  
*** testdb= INSERT INTO my_ta
*** 3715,3721 
  
  
  In prompt 1 normally =,
! but ^ if in single-line mode,
  or ! if the session is disconnected from the
  database (which can happen if \connect fails).
  In prompt 2 %R is replaced by a character that
--- 3804,3811 
  
  
  In prompt 1 normally =,
! but @ if 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

New Patch v29: Now with less coverage!


Patch applies cleanly. Make check ok. Feature still works!

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

New Patch v29: Now with less coverage!
(same as v28 minus the psql-on-error-stop.sql and associated changes)
Fabien raises some good points about if/then being a tremendous tool for
enhancing other existing regression tests.


On Wed, Mar 29, 2017 at 2:16 PM, Fabien COELHO  wrote:

>
> Hello Tom,
>
> If someone were to put together a TAP test suite that covered all that
>> and made for a meaningful improvement in psql's altogether-miserable
>> code coverage report[1], I would think that that would be a useful
>> expenditure of buildfarm time.
>>
>
> Ok, this is an interesting point.
>
> What I'm objecting to is paying the overhead for such a suite in order to
>> test just this one thing.
>>
>
> Well, it should start somewhere. Once something is running it is easier to
> add more tests.
>
> think that that passes the bang-for-buck test; or in other words, this
>> isn't the place I would start if I were creating a TAP suite for psql.
>>
>
> Sure, I would not have started with that either.
>
> Note that from this patch point of view, it is somehow logical to start
> testing a given feature when this very feature is being developed...
>
> The summary is that we agree that psql test coverage is abysmal, but you
> do not want to bootstrap a better test infrastructure for this particular
> and rather special new feature. Ok.
>
> Maybe Corey can submit another patch with the exit 3 test removed.
>
> --
> Fabien.
>
From 9caa338fa085bc1c0ee16f22c0c1c8d3c8fc1697 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Wed, 29 Mar 2017 15:30:22 -0400
Subject: [PATCH] psql if v29

---
 doc/src/sgml/ref/psql-ref.sgml |   90 ++-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1365 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 +++
 src/bin/psql/conditional.h |   62 ++
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql.out |  110 +++
 src/test/regress/sql/psql.sql  |  109 +++
 15 files changed, 1641 insertions(+), 290 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or \elif
+is subject to variable interpolation and backquote expansion, just
+like any other backslash command argument.  After that it is evaluated
+like the value of an on/off option variable.  So a valid value
+is any unambiguous case-insensitive match for one of:
+true, false, 1,
+0, on, off,
+yes, no.  For example,
+t, T, and tR
+will all be considered to be true.
+
+
+Expressions that do not properly evaluate to true or false will
+generate a warning and be treated as false.
+
+
+All the backslash commands of a given conditional block must appear in
+the same source file. If EOF is reached on 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


If someone were to put together a TAP test suite that covered all that
and made for a meaningful improvement in psql's altogether-miserable
code coverage report[1], I would think that that would be a useful
expenditure of buildfarm time.


Ok, this is an interesting point.

What I'm objecting to is paying the overhead for such a suite in order 
to test just this one thing.


Well, it should start somewhere. Once something is running it is easier to 
add more tests.



think that that passes the bang-for-buck test; or in other words, this
isn't the place I would start if I were creating a TAP suite for psql.


Sure, I would not have started with that either.

Note that from this patch point of view, it is somehow logical to start 
testing a given feature when this very feature is being developed...


The summary is that we agree that psql test coverage is abysmal, but you 
do not want to bootstrap a better test infrastructure for this particular 
and rather special new feature. Ok.


Maybe Corey can submit another patch with the exit 3 test removed.

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> If we're sufficiently dead set on it, we could go back to the TAP-based 
>> approach,

> Hmmm. You rejected it. I agree that TAP tests are not well suited for some 
> simple tests because of their initdb overhead.

>> but I still doubt that this test is worth the amount of overhead that 
>> would add.

> I think that there is an underlying issue with keeping on rejecting tests 
> which aim at having a reasonable code coverage of features by exercising 
> different paths.

There's certainly a fair amount of psql behavior that's not adequately
testable within the standard regression test infrastructure.  Parsing of
command line arguments and exit codes for unusual cases both fall into
that area, and then there's things like prompts and tab completion.
If someone were to put together a TAP test suite that covered all that
and made for a meaningful improvement in psql's altogether-miserable
code coverage report[1], I would think that that would be a useful
expenditure of buildfarm time.  What I'm objecting to is paying the
overhead for such a suite in order to test just this one thing.  I don't
think that that passes the bang-for-buck test; or in other words, this
isn't the place I would start if I were creating a TAP suite for psql.

regards, tom lane

[1] https://coverage.postgresql.org/src/bin/psql/index.html


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


psql_if_on_error_stop... ok (test process exited with exit code 3)

Don't think we can have that.  Even if pg_regress considers it a success,
every hacker is going to have to learn that that's a "pass",


Well, it says "ok"...

and I don't think I want to be answering that question every week till 
kingdom come.


Hmmm.

What if the test is renamed, say "psql_if_exit_code_3"? Maybe the clue 
would be clear enough to avoid questions? Or just remove the exit message 
but check the exit code is as expected?



I'm not really sure we need a test for this behavior.


My 0.02€:

I have learned the hard way over the years that what is not tested does 
not really work, including error behaviors. These tests (well, the initial 
TAP version at least) helped debug the feature, and would help keeping it 
alive when the code is updated.


Now if you do not want this test, it can be removed. The feature is worthy 
even without it.


If we're sufficiently dead set on it, we could go back to the TAP-based 
approach,


Hmmm. You rejected it. I agree that TAP tests are not well suited for some 
simple tests because of their initdb overhead.


but I still doubt that this test is worth the amount of overhead that 
would add.


I think that there is an underlying issue with keeping on rejecting tests 
which aim at having a reasonable code coverage of features by exercising 
different paths.


Maybe there could be some "larger but still reasonable tests" activated on 
demand so as to being able to keep tests and run them from time to time, 
which would not interfere too much with committers' work, and that some 
farm animals would run?


I thought that was one of the purpose of TAP tests, but obviously it is 
not.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
[ 0001-psql-if-v28.patch ]

Starting to look at this version, and what jumped out at me in testing
is that the regression output looks like this:

parallel group (12 tests):  psql_if_on_error_stop dbsize async misc_functions 
tidscan alter_operator tsrf psql alter_generic misc stats_ext sysviews
 alter_generic... ok
 alter_operator   ... ok
 misc ... ok
 psql ... ok
 psql_if_on_error_stop... ok (test process exited with exit code 3)
 async... ok
 dbsize   ... ok
 misc_functions   ... ok
 sysviews ... ok
 tsrf ... ok
 tidscan  ... ok
 stats_ext... ok

Don't think we can have that.  Even if pg_regress considers it a success,
every hacker is going to have to learn that that's a "pass", and I don't
think I want to be answering that question every week till kingdom come.

I'm not really sure we need a test for this behavior.  If we're
sufficiently dead set on it, we could go back to the TAP-based approach,
but I still doubt that this test is worth the amount of overhead that
would add.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

0001+0002 patch primarily for ease of review. will be following with a
single v28 patch shortly.


Applies cleanly. Make check ok. I think it behaves as committers required 
last. Let us try again with them...


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> 0001+0002 patch primarily for ease of review. will be following with a
> single v28 patch shortly.
>


0001-psql-if-v28.patch
Description: Binary data

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 27, 2017 at 3:25 PM, Fabien COELHO  wrote:

>
> And here you go
>>
>
> Patch applies cleany, make check ok. Looks pretty good.
>
> A minor detail I have just noticed, sorry: now that options are discarded
> by functions, some string variable declarations should be moved back inside
> the active branch. You moved them out because you where sharing the
> variables between the active & inactive branches, but this is no longer
> necessary, and the project practice seems to declare variables just where
> they are needed. That would be pattern in d, encoding in encoding, fname in
> f and g and include and out and s, prefix in gset, opt in help, opt* in lo
> and pset and set, arg* in prompt, env* in setenv... and maybe a few others.
>
> --
> Fabien.
>

done:
encoding f g gset help include lo out prompt pset s set setenv sf sv t T
timing unset watch x z ! ?

weird cases where they're both still needed:
d write

0001+0002 patch primarily for ease of review. will be following with a
single v28 patch shortly.


0001-psql-if-v27.patch
Description: Binary data


0002-fix-var-scoping.patch
Description: Binary data

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

And here you go


Patch applies cleany, make check ok. Looks pretty good.

A minor detail I have just noticed, sorry: now that options are discarded 
by functions, some string variable declarations should be moved back 
inside the active branch. You moved them out because you where sharing the 
variables between the active & inactive branches, but this is no longer 
necessary, and the project practice seems to declare variables just where 
they are needed. That would be pattern in d, encoding in encoding, fname 
in f and g and include and out and s, prefix in gset, opt in help, opt* in 
lo and pset and set, arg* in prompt, env* in setenv... and maybe a few 
others.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 27, 2017 at 10:34 AM, Fabien COELHO  wrote:

>
> Hello,
>
> I think that you could use another pattern where you init the
>>> PQExpBufferData structure instead of create it, so that only the string
>>> is
>>> malloced.
>>>
>>
>> In v26, I have the functions return PQExpBuffer. The two calling functions
>> then free it, which should solve any leak.
>>
>
> Yep, it works as well.
>
> Here's an addendum that does that. I can combine them in v27, but figured
>> this was quicker.
>>
>
> It works.
>
> However having just one full patch with a number would help so that I can
> say "ready to committer" or not on something.
>
> --
> Fabien.
>

And here you go (sorry for the delay, had errands to run).


0001-psql-if-v27.patch
Description: Binary data

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello,


I think that you could use another pattern where you init the
PQExpBufferData structure instead of create it, so that only the string is
malloced.


In v26, I have the functions return PQExpBuffer. The two calling functions
then free it, which should solve any leak.


Yep, it works as well.


Here's an addendum that does that. I can combine them in v27, but figured
this was quicker.


It works.

However having just one full patch with a number would help so that I can 
say "ready to committer" or not on something.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> I think that you could use another pattern where you init the
> PQExpBufferData structure instead of create it, so that only the string is
> malloced.


In v26, I have the functions return PQExpBuffer. The two calling functions
then free it, which should solve any leak.


>
>
> I think that you should use appendPQExpBufferChar and Str instead of
>>> relying on the format variant which is probably expensive. Something
>>> like:
>>>
>>>   if (num_options > 0)
>>> append...Char(buf, ' ');
>>>   append...Str(buf, ...);
>>>
>>
>> All flavors of appendPQExpBuffer*() I can find have a const *char format
>> string, so no way to append a naked string. If you know differently, I'm
>> listening. Not fixed.
>>
>
> These prototypes are from "pqexpbuffer.h", and do not seem to rely on a
> format:
>
>
Here's an addendum that does that. I can combine them in v27, but figured
this was quicker.
From 2fa083eb0115278f817a2d1d0439a47951a9c48b Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 27 Mar 2017 10:07:36 -0400
Subject: [PATCH 2/2] simpler append

---
 src/bin/psql/command.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index aa9dad4..a6168df 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -218,10 +218,9 @@ gather_boolean_expression(PsqlScanState scan_state, bool expansion, bool warn)
 	while ((value = psql_scan_slash_option(scan_state, slash_opt, NULL, false)))
 	{
 		/* add a space in between subsequent tokens */
-		if (num_options == 0)
-			appendPQExpBuffer(exp_buf, "%s", value);
-		else
-			appendPQExpBuffer(exp_buf, " %s", value);
+		if (num_options > 0)
+			appendPQExpBufferChar(exp_buf, ' ');
+		appendPQExpBufferStr(exp_buf, value);
 		num_options++;
 	}
 
-- 
2.7.4


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

ISTM that PQExpBuffer is partially a memory leak. Something should need 
to be freed?


I copied that pattern from somewhere else, so yeah, I duplicated whatever
leak was there.


Hmmm. Indeed some commands do not free, but there is a single use and the 
commands exits afterwards, eg "createuser".


I think that you could use another pattern where you init the 
PQExpBufferData structure instead of create it, so that only the string is 
malloced.



I think that you should use appendPQExpBufferChar and Str instead of
relying on the format variant which is probably expensive. Something like:

  if (num_options > 0)
append...Char(buf, ' ');
  append...Str(buf, ...);


All flavors of appendPQExpBuffer*() I can find have a const *char format 
string, so no way to append a naked string. If you know differently, I'm 
listening. Not fixed.


These prototypes are from "pqexpbuffer.h", and do not seem to rely on a 
format:


 extern void appendPQExpBufferChar(PQExpBuffer str, char ch);
 extern void appendPQExpBufferStr(PQExpBuffer str, const char *data);

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Patches do not apply cleanly.
> Part 1 gets:
>  error: patch failed: src/test/regress/parallel_schedule:89
>  error: src/test/regress/parallel_schedule: patch does not apply
>
> There is still the useless file, ok it is removed by part2. Could have
> been just one patch...
>

parallel_schedule failed because I hadn't rebased recently enough.

git format-patch did us no favors there. New patch is redone as one commit.

ISTM that PQExpBuffer is partially a memory leak. Something should need to
> be freed?
>

I copied that pattern from somewhere else, so yeah, I duplicated whatever
leak was there. Fixed.


> I think that you should use appendPQExpBufferChar and Str instead of
> relying on the format variant which is probably expensive. Something like:
>
>   if (num_options > 0)
> append...Char(buf, ' ');
>   append...Str(buf, ...);
>

All flavors of appendPQExpBuffer*() I can find have a const *char format
string, so no way to append a naked string. If you know differently, I'm
listening. Not fixed.


>
> is_true_boolean_expression: "return (success) ? tf : false;"
> Is this simply: "return success && tf;"?
>

Neat. Done.


>
> Some functions have opt1, opt2, but some start at opt0. This does not look
> too consistent, although the inconsistency may be preexisting from your
> patch. Basically, there is a need for some more restructuring in
> "command.c".


It is pre-existing. Maybe this patch will inspire someone else to make the
other more consistent.

v26 attached
From fc0c466b0331839efe722d089a8ead521e8a827e Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Sun, 26 Mar 2017 17:30:51 -0400
Subject: [PATCH] psql if v26

---
 doc/src/sgml/ref/psql-ref.sgml |   90 +-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1383 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 ++
 src/bin/psql/conditional.h |   62 +
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql.out |  110 ++
 .../regress/expected/psql_if_on_error_stop.out |   14 +
 src/test/regress/parallel_schedule |2 +-
 src/test/regress/serial_schedule   |1 +
 src/test/regress/sql/psql.sql  |  109 ++
 src/test/regress/sql/psql_if_on_error_stop.sql |   17 +
 19 files changed, 1699 insertions(+), 284 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h
 create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out
 create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or \elif
+is subject to variable interpolation and backquote expansion, just
+

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v25, try 2:

First file is what you were used to last time. 2nd and 3rd are changes
since then based on feedback.


Patches do not apply cleanly.
Part 1 gets:
 error: patch failed: src/test/regress/parallel_schedule:89
 error: src/test/regress/parallel_schedule: patch does not apply

There is still the useless file, ok it is removed by part2. Could have 
been just one patch...


After a manual fix in parallel_schedule, make check is ok.

gather_boolean_expression:

ISTM that PQExpBuffer is partially a memory leak. Something should need to 
be freed?


I think that you should use appendPQExpBufferChar and Str instead of 
relying on the format variant which is probably expensive. Something like:


  if (num_options > 0)
append...Char(buf, ' ');
  append...Str(buf, ...);

is_true_boolean_expression: "return (success) ? tf : false;"
Is this simply: "return success && tf;"?

Some functions have opt1, opt2, but some start at opt0. This does not look 
too consistent, although the inconsistency may be preexisting from your 
patch. Basically, there is a need for some more restructuring in 
"command.c".


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

As for a function for digested ignored slash options, it seems like I can
disregard the true/false value of the "semicolon" parameter. Is that
correct?


Dunno.


I do not see that as a significant issue, especially compared to the
benefit of having the automaton transition management in a single place.


I'm still struggling to see how this would add any clarity to the code
beyond what I can achieve by clustering the
exec_command_(if/elif/else/endif) near one another.


Hmmm... it is more cleanly encapsulated if in just one function?

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v25


ISTM that the attached file contents is identical to v24.

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

v25

- PQExpBuffer on gather_boolean_expression()
- convenience/clarity functions: ignore_slash_option(),
ignore_2_slash_options(), ignore_slash_line()
- remove inaccurate test of variable expansion in a false block
- added kitchen-sink test of parsing slash commands in a false block
- removed spurious file that shouldn't have been in v24
- removed any potential free(NULL) calls *that I introduced*, others remain
from master branch

NOT done:
- grouping all branching commands into one function - can be done in a
later patch for clarity
- combining _ef / _ev or _sf / _sv - can be done in a later patch for
clarity


On Fri, Mar 24, 2017 at 4:33 PM, Corey Huinker 
wrote:

> On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO 
> wrote:
>
>>
>> Hello Corey,
>>
>> I wished for the same thing, happy to use one if it is made known to me.
>>> I pulled that pattern from somewhere else in the code, and given that the
>>> max number of args for a command is around 4, I'm not too worried about
>>> scaling.
>>>
>>
>> If there are expressions one day like pgbench, the number of arguments
>> becomes arbitrary. Have you looked at PQExpBuffer?
>
>
> I will look into it.
>
>
>>
 There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
 make sense to create a function instead of keeping the initial
 copy-paste?

>>>
>>> Yes, and a few things like that, but I wanted this patch to keep as much
>>> code as-is as possible.
>>>
>>
>> If you put the generic function at the same place, one would be more or
>> less kept and the other would be just removed?
>
>
>> "git diff --patience -w" gives a rather convenient output for looking at
>> the patch.
>
>
> Good to know about that option.
>
> As for a function for digested ignored slash options, it seems like I can
> disregard the true/false value of the "semicolon" parameter. Is that
> correct?
>
>
>> I would suggest to put together all if-related backslash command, so that
 the stack management is all in one function instead of 4.

>>>
>>> I recognize the urge to group them together, but would there be any
>>> actual
>>> code sharing between them? Wouldn't I be either re-checking the string
>>> "cmd" again, or otherwise setting an enum that I immediately re-check
>>> inside the all_branching_commands() function?
>>>
>>
>> I do not see that as a significant issue, especially compared to the
>> benefit of having the automaton transition management in a single place.
>
>
> I'm still struggling to see how this would add any clarity to the code
> beyond what I can achieve by clustering the exec_command_(if/elif/else/endif)
> near one another.
>
>
>
>


0001.if_endif.v25.patch
Description: Binary data

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> I wished for the same thing, happy to use one if it is made known to me.
>> I pulled that pattern from somewhere else in the code, and given that the
>> max number of args for a command is around 4, I'm not too worried about
>> scaling.
>>
>
> If there are expressions one day like pgbench, the number of arguments
> becomes arbitrary. Have you looked at PQExpBuffer?


I will look into it.


>
>>> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
>>> make sense to create a function instead of keeping the initial
>>> copy-paste?
>>>
>>
>> Yes, and a few things like that, but I wanted this patch to keep as much
>> code as-is as possible.
>>
>
> If you put the generic function at the same place, one would be more or
> less kept and the other would be just removed?


> "git diff --patience -w" gives a rather convenient output for looking at
> the patch.


Good to know about that option.

As for a function for digested ignored slash options, it seems like I can
disregard the true/false value of the "semicolon" parameter. Is that
correct?


> I would suggest to put together all if-related backslash command, so that
>>> the stack management is all in one function instead of 4.
>>>
>>
>> I recognize the urge to group them together, but would there be any actual
>> code sharing between them? Wouldn't I be either re-checking the string
>> "cmd" again, or otherwise setting an enum that I immediately re-check
>> inside the all_branching_commands() function?
>>
>
> I do not see that as a significant issue, especially compared to the
> benefit of having the automaton transition management in a single place.


I'm still struggling to see how this would add any clarity to the code
beyond what I can achieve by clustering the
exec_command_(if/elif/else/endif) near one another.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


I wished for the same thing, happy to use one if it is made known to me.
I pulled that pattern from somewhere else in the code, and given that the
max number of args for a command is around 4, I'm not too worried about
scaling.


If there are expressions one day like pgbench, the number of arguments 
becomes arbitrary. Have you looked at PQExpBuffer?



However there is an impact on testing because of all these changes. ISTM
that test cases should reflect this situation and test that \cd, \edit, ...
are indeed ignored properly and taking account there expected args...


I think one grand

\if false
\a
\c some_connect_string
...
\z some_table_name
\endif
should do the trick,


Yes. Maybe some commands could be on the same line as well.


but it wouldn't detect memory leaks.


No miracle...


There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
make sense to create a function instead of keeping the initial copy-paste?


Yes, and a few things like that, but I wanted this patch to keep as much
code as-is as possible.


If you put the generic function at the same place, one would be more or 
less kept and the other would be just removed?


"git diff --patience -w" gives a rather convenient output for looking at 
the patch.



I would suggest to put together all if-related backslash command, so that
the stack management is all in one function instead of 4.


I recognize the urge to group them together, but would there be any actual
code sharing between them? Wouldn't I be either re-checking the string
"cmd" again, or otherwise setting an enum that I immediately re-check
inside the all_branching_commands() function?


I do not see that as a significant issue, especially compared to the 
benefit of having the automaton transition management in a single place.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> A few comments about the patch.
>
> Patch applies. "make check" ok.
>
> As already pointed out, there is one useless file in the patch.
>
> Although currently there is only one expected argument for boolean
> expressions, the n² concatenation algorithm in gather_boolean_expression is
> not very elegant. Is there some string buffer data structure which could be
> used instead?
>

I wished for the same thing, happy to use one if it is made known to me.
I pulled that pattern from somewhere else in the code, and given that the
max number of args for a command is around 4, I'm not too worried about
scaling.


>
> ISTM that ignore_boolean_expression may call free on a NULL pointer if the
> expression is empty?
>

True. The psql code is actually littered with a lot of un-checked free(p)
calls, so I started to wonder if maybe we had a wrapper on free() that
checked for NULL. I'll fix this one just to be consistent.


>
> Generally I find the per-command functions rather an improvement.
>

I did too. I tried to split this patch up into two parts, one that broke
out the functions, and one that added if-then, and found that the first
patch was just as unwieldily without the if-then stuff as with.


>
> However there is an impact on testing because of all these changes. ISTM
> that test cases should reflect this situation and test that \cd, \edit, ...
> are indeed ignored properly and taking account there expected args...
>

I think one grand

\if false
\a
\c some_connect_string
...
\z some_table_name
\endif

should do the trick, but it wouldn't detect memory leaks.


>
> In "exec_command_connect" an argument is changed from "-reuse-previous" to
> "-reuse-previous=", not sure why.
>

It shouldn't have been. Good catch. Most commands were able to be migrated
with simple changes (status => *status, strcmp() if-block becomes
active-if-block, etc), but that one was slightly different.


>
> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
> make sense to create a function instead of keeping the initial copy-paste?
>

Yes, and a few things like that, but I wanted this patch to keep as much
code as-is as possible.


>
> I think that some functions could be used for some repeated cases such as
> "discard one arg", "discard one or two arg", "discard whole line", for the
> various inactive branches, so as to factor out code.
>

I'd be in favor of that as well


>
> I would suggest to put together all if-related backslash command, so that
> the stack management is all in one function instead of 4.
>

I recognize the urge to group them together, but would there be any actual
code sharing between them? Wouldn't I be either re-checking the string
"cmd" again, or otherwise setting an enum that I immediately re-check
inside the all_branching_commands() function?


>
> For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not
> sure why.


An oversight. Good catch.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v24 highlights:

- finally using git format-patch
- all conditional slash commands broken out into their own functions
(exec_command_$NAME) , each one tests if it's in an active branch, and if
it's not it consumes the same number of parameters, but discards them.
comments for each slash-command family were left as-is, may need to be
bulked up.
- TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif
placement
- documentation recommending ON_ERROR_STOP removed, because invalid
expressions no longer throw off if-endif balance
- documentation updated to reflex that contextually-correct-but-invalid
boolean expressions are treated as false
- psql_get_variable has a passthrough void pointer now, but I ended up not
needing it. Instead, all slash commands in false blocks either fetch
OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know.


A few comments about the patch.

Patch applies. "make check" ok.

As already pointed out, there is one useless file in the patch.

Although currently there is only one expected argument for boolean 
expressions, the n² concatenation algorithm in gather_boolean_expression 
is not very elegant. Is there some string buffer data structure which 
could be used instead?


ISTM that ignore_boolean_expression may call free on a NULL pointer if the 
expression is empty?


Generally I find the per-command functions rather an improvement.

However there is an impact on testing because of all these changes. ISTM 
that test cases should reflect this situation and test that \cd, \edit, 
... are indeed ignored properly and taking account there expected args...


In "exec_command_connect" an argument is changed from "-reuse-previous" to 
"-reuse-previous=", not sure why.


There seems to be pattern repetition for _ev _ef and _sf _sv. Would it 
make sense to create a function instead of keeping the initial copy-paste?


I think that some functions could be used for some repeated cases such as 
"discard one arg", "discard one or two arg", "discard whole line", for the 
various inactive branches, so as to factor out code.


I would suggest to put together all if-related backslash command, 
so that the stack management is all in one function instead of 4.


For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not 
sure why.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Alvaro Herrera]

Tom Lane wrote:

> I'm not entirely convinced that function-per-command is an improvement
> though.  Seems like it would only help to the extent that you could do a
> simple "return" to implement early exit, and it looks to me like that
> doesn't work in a lot of places because you still have to clean up things
> like malloc'd argument strings before you can return.  So the question
> we have to answer is whether this way looks cleaner than what we'd get if
> we just changed the logic in-place.  For the purpose of answering that
> question, looking at the final state is the right thing to do.
> 
> I don't have a definite opinion on that core question yet, since I've not
> read this version of the patch.  Anybody else want to give an opinion?

Currently, exec_command is a 1500-line function.  If I had to see how a
single \-command worked, I would have to fold everything but the command
I'm interested in, in case there's something nontrivial at function
start or end (or even in between -- I would have to start by figuring
out whether there's anything other than "else if" somewhere in those
1500 lines).  I think splitting into command-specific functions makes
this much easier to follow, particularly if we want to add extra tricks
such as returning early etc.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 19, 2017 at 1:18 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> v24 highlights:
>>
>
> The v24 patch is twice larger that the previous submission. Sigh.
>
> If I'm reading headers correctly, it seems that it adds an
> "expected/psql-on-error-stop.out" file without a corresponding test
> source in "sql/". Is this file to be simply ignored, or is a source missing?


Ignore it. I created the new .sql/.out pair, realized that the file naming
convention was underscores not dashes, changed them and evidently forgot
that I had already added a dashed one to git.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 19, 2017 at 4:23 PM, Fabien COELHO  wrote:

>
> Hello Tom,
>
> I'm not entirely convinced that function-per-command is an improvement
>> though. [...]
>>
>
> I don't have a definite opinion on that core question yet, since I've not
>> read this version of the patch.  Anybody else want to give an opinion?
>>
>
> My 0.02€:
>
> I've already provided my view...
>
> Personnally I like good functions. Maybe a per-command-family set of
> functions could improve the code readability, but (1) I'm not sure this is
> achieved by this patch (eg the if-related state management is now
> dispatched in 4 functions) and (2) I'm not sure that this approach helps
> much with respect to trying to factor out backslash-command-related
> active-or-not argument management.
>
> However I have not looked at the patch in detail. I'm planing to do so
> later this week.


I offered to split the patch into two steps (1. break each "family" into
it's own function and 2. Do what's needed for \if-\endif) but got no
response. I can still do that if people think it's worthwhile.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


I'm not entirely convinced that function-per-command is an improvement
though. [...]



I don't have a definite opinion on that core question yet, since I've not
read this version of the patch.  Anybody else want to give an opinion?


My 0.02€:

I've already provided my view...

Personnally I like good functions. Maybe a per-command-family set of 
functions could improve the code readability, but (1) I'm not sure this is 
achieved by this patch (eg the if-related state management is now 
dispatched in 4 functions) and (2) I'm not sure that this approach helps 
much with respect to trying to factor out backslash-command-related 
active-or-not argument management.


However I have not looked at the patch in detail. I'm planing to do so 
later this week.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Alvaro Herrera  writes:
> The reason this is so large is that there is an entangled refactoring
> patch, splitting the exec_command() function from one giant switch()
> into one routine for each command.  It's up to the committer whether to
> do it all in one patch, or to request this to be split into a
> refactoring patch plus another adding functionality on top.

Assuming we want to do it that way at all, two steps would probably be
easier to review in detail.

I'm not entirely convinced that function-per-command is an improvement
though.  Seems like it would only help to the extent that you could do a
simple "return" to implement early exit, and it looks to me like that
doesn't work in a lot of places because you still have to clean up things
like malloc'd argument strings before you can return.  So the question
we have to answer is whether this way looks cleaner than what we'd get if
we just changed the logic in-place.  For the purpose of answering that
question, looking at the final state is the right thing to do.

I don't have a definite opinion on that core question yet, since I've not
read this version of the patch.  Anybody else want to give an opinion?

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Alvaro Herrera]

Fabien COELHO wrote:
> 
> Hello Corey,
> 
> > v24 highlights:
> 
> The v24 patch is twice larger that the previous submission. Sigh.

The reason this is so large is that there is an entangled refactoring
patch, splitting the exec_command() function from one giant switch()
into one routine for each command.  It's up to the committer whether to
do it all in one patch, or to request this to be split into a
refactoring patch plus another adding functionality on top.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v24 highlights:


The v24 patch is twice larger that the previous submission. Sigh.

If I'm reading headers correctly, it seems that it adds an 
"expected/psql-on-error-stop.out" file without a corresponding test source 
in "sql/". Is this file to be simply ignored, or is a source missing?


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 17, 2017 at 2:18 PM, Corey Huinker 
wrote:

>
>> > \set x 'arg1 arg2'
>>
>> > \if false
>> > \cmd_that_takes_exactly_two_args :x
>> > \endif
>>
>> Yeah, throwing errors for bad arguments would also need to be suppressed.
>>
>> regards, tom lane
>>
>
> Ok, barring other feedback, I'm going to take my marching orders as "make
> each slash command active-aware". To keep that sane, I'm probably going to
> break out each slash command family into it's own static function.
>

...and here it is.

v24 highlights:

- finally using git format-patch
- all conditional slash commands broken out into their own functions
(exec_command_$NAME) , each one tests if it's in an active branch, and if
it's not it consumes the same number of parameters, but discards them.
comments for each slash-command family were left as-is, may need to be
bulked up.
- TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif
placement
- documentation recommending ON_ERROR_STOP removed, because invalid
expressions no longer throw off if-endif balance
- documentation updated to reflex that contextually-correct-but-invalid
boolean expressions are treated as false
- psql_get_variable has a passthrough void pointer now, but I ended up not
needing it. Instead, all slash commands in false blocks either fetch
OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know.
From da8306acab0f0304d62f966c5217139bacfe722e Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Sat, 18 Mar 2017 12:47:25 -0400
Subject: [PATCH] Add \if...\endif blocks

---
 doc/src/sgml/ref/psql-ref.sgml |   90 +-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1513 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 ++
 src/bin/psql/conditional.h |   62 +
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql-on-error-stop.out   |  506 +++
 src/test/regress/expected/psql.out |  100 ++
 .../regress/expected/psql_if_on_error_stop.out |   14 +
 src/test/regress/parallel_schedule |2 +-
 src/test/regress/serial_schedule   |1 +
 src/test/regress/sql/psql.sql  |  100 ++
 src/test/regress/sql/psql_if_on_error_stop.sql |   17 +
 20 files changed, 2316 insertions(+), 284 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h
 create mode 100644 src/test/regress/expected/psql-on-error-stop.out
 create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out
 create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> > \set x 'arg1 arg2'
>
> > \if false
> > \cmd_that_takes_exactly_two_args :x
> > \endif
>
> Yeah, throwing errors for bad arguments would also need to be suppressed.
>
> regards, tom lane
>

Ok, barring other feedback, I'm going to take my marching orders as "make
each slash command active-aware". To keep that sane, I'm probably going to
break out each slash command family into it's own static function.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
>> In the end, I suspect that teaching all the backslash commands to do
>> nothing after absorbing their arguments is likely to be the least messy
>> way to tackle this, even if it makes for a rather bulky patch.

> Perhaps, but just glancing at \connect makes me think that for some
> commands (present or future) the number of args might depend on the value
> of the first arg, and variable expansion-or-not, backtick execution-or-not
> could alter the number of apparent args on the line, like this:

> \set x 'arg1 arg2'

> \if false
> \cmd_that_takes_exactly_two_args :x
> \endif

Yeah, throwing errors for bad arguments would also need to be suppressed.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> In the end, I suspect that teaching all the backslash commands to do
> nothing after absorbing their arguments is likely to be the least messy
> way to tackle this, even if it makes for a rather bulky patch.
>
>
Perhaps, but just glancing at \connect makes me think that for some
commands (present or future) the number of args might depend on the value
of the first arg, and variable expansion-or-not, backtick execution-or-not
could alter the number of apparent args on the line, like this:

\set x 'arg1 arg2'

\if false
\cmd_that_takes_exactly_two_args :x
\endif

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


I also fear that there are corner cases where the behavior would still
be inconsistent.  Consider

\if ...
\set foo `echo \endif should not appear here`



In this instance, ISTM that there is no problem. On "\if true", set is
executed, all is well. On "\if false", the whole line would be skipped
because the if-related commands are only expected on their own line, all
is well again. No problem.


AFAICS, you misunderstood the example completely, or else you're proposing
syntax restrictions that are even more bizarre and unintelligible than
I thought before.


Hmmm. The example you put forward does work as expected with the rule I 
suggested. It does not prove that the rules are good or sane, I'm just 
stating that the example would work consistently.


We cannot have a situation where the syntax rules for backslash commands 
inside an \if are fundamentally different from what they are elsewhere;


Indeed, I do not see an issue with requiring some new backslash commands 
to be on their own line: Any average programmer would put them like that 
anyway for readability. What is the point of trying to write code to 
handle strange unmaintainable oneliners?



that's just going to lead to confusion and bug reports.


Whatever is done, there will be some confusion and bug reports:-)

If someone writes a strange one-liner and see that it generates errors, 
then the error messages should be clear enough. Maybe they will complain 
and fill in bugs because they like backslash-command oneliners. That is 
life.


Now you are the committer and Corey is the developer. I'm just a reviewer 
trying to help. I can still review a larger patch which tries to be subtly 
compatible with a lack of previous clear design by adding code complexity, 
even if I think that this particular effort is a bad idea (i.e. mis-spent 
resource on a useless sub-feature which makes future maintenance harder). 
With some luck, Corey may find a way of doing it which is not too bad.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> I think Fabien was arguing that inside a false block there would be no
> syntax rules beyond "is the first non-space character on this line a '\'
> and if so is it followed with a if/elif/else/endif?". If the answer is no,
> skip the line. To me that seems somewhat similar to Tom's suggestion that a
> false branch just keeps consuming text until it encounters a \conditional
> or EOF.

Hmm.  If we can keep the syntax requirements down to "\if and friends
must be the first backslash command on the line", and not change the
apparent behavior for any other command type, it probably would be okay
from the user's standpoint.  I'm not really convinced that this approach
will accomplish that, though, and especially not that it will do so
without injecting some ugliness into the core lexer.

In the end, I suspect that teaching all the backslash commands to do
nothing after absorbing their arguments is likely to be the least messy
way to tackle this, even if it makes for a rather bulky patch.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 17, 2017 at 11:42 AM, Tom Lane  wrote:

> Fabien COELHO  writes:
> >> I also fear that there are corner cases where the behavior would still
> >> be inconsistent.  Consider
> >>
> >> \if ...
> >> \set foo `echo \endif should not appear here`
>
> > In this instance, ISTM that there is no problem. On "\if true", set is
> > executed, all is well. On "\if false", the whole line would be skipped
> > because the if-related commands are only expected on their own line, all
> > is well again. No problem.
>
> AFAICS, you misunderstood the example completely, or else you're proposing
> syntax restrictions that are even more bizarre and unintelligible than
> I thought before.  We cannot have a situation where the syntax rules for
> backslash commands inside an \if are fundamentally different from what
> they are elsewhere; that's just going to lead to confusion and bug
> reports.
>
> regards, tom lane
>

I think Fabien was arguing that inside a false block there would be no
syntax rules beyond "is the first non-space character on this line a '\'
and if so is it followed with a if/elif/else/endif?". If the answer is no,
skip the line. To me that seems somewhat similar to Tom's suggestion that a
false branch just keeps consuming text until it encounters a \conditional
or EOF.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> I also fear that there are corner cases where the behavior would still
>> be inconsistent.  Consider
>> 
>> \if ...
>> \set foo `echo \endif should not appear here`

> In this instance, ISTM that there is no problem. On "\if true", set is 
> executed, all is well. On "\if false", the whole line would be skipped 
> because the if-related commands are only expected on their own line, all 
> is well again. No problem.

AFAICS, you misunderstood the example completely, or else you're proposing
syntax restrictions that are even more bizarre and unintelligible than
I thought before.  We cannot have a situation where the syntax rules for
backslash commands inside an \if are fundamentally different from what
they are elsewhere; that's just going to lead to confusion and bug
reports.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> command.c:38:25: fatal error: conditional.h: No such file or directory
>  #include "conditional.h"
>

Odd, it's listed as a new file in git status. Anyway, my point of posting
the WIP patch was to give people a reference point and spark discussion
about the next step, and it succeeded at that.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


ISTM that I've tried to suggest to work around that complexity by:
  - document that \if-related commands should only occur at line start
(and extend to eol).
  - detect and complain when this is not the case.


I think this is a lousy definition, and would never be considered if we
were working in a green field.


Yes, sure. As you pointed out, the field is not green: there is no clean 
lexical convention, too bad. I'm trying to deal with that without too much 
fuss in the code.



Moreover, preventing such cases would be pretty darn ugly/messy as well.

I also fear that there are corner cases where the behavior would still
be inconsistent.  Consider

\if ...
\set foo `echo \endif should not appear here`


In this instance, ISTM that there is no problem. On "\if true", set is 
executed, all is well. On "\if false", the whole line would be skipped 
because the if-related commands are only expected on their own line, all 
is well again. No problem.


Another more interesting one would be:

  \if ...
\unset foo \endif

On true, unset get its argument, then endif is detected as a backslash 
command, but it would see that it is not on its own line, so it would 
error out *and* be ignored. On false, the whole line would be ignored, it 
would just not complain, but it would be the same, i.e. it is *not* an 
\endif again. The drawback is only that the wrong \endif is not detected 
when under a false branch. That is why I added a third bullet "call border 
cases a feature".


ISTM that the proposed simple rules allow to deal with the situation 
without having to dive into each command lexing rules, and changing the 
existing code significantly. The drawback is that misplaced \endif are not 
detected in false branch, but they are ignored anyway, which is fine.



I'm imagining that instead of

[...] char   *envvar = psql_scan_slash_option(scan_state,

we'd write

[...] char   *envvar = args[0];

where the args array had been filled at the top of the function.
The top-of-function code would have to know all the cases where
commands didn't use basic OT_NORMAL processing, but there aren't
that many of those, I think.


Yep, I understood the idea. There are a few of those, about 49 OT_* in 
"command.c", including 34 OT_NORMAL, 1 OT_NO_EVAL, 3 OT_FILEPIPE, 9 
OT_WHOLELINE, some OT_SQLHACKID & OT_SQLID. I'm not sure of the 
combinations.


It still means splitting command lexing knowledge in several places. I'm 
not convinced by the impact on the resulting code with regard to 
readability and maintainability, so if there could be a way to get 
something without taking that path that would be nice, hence my 
suggestions.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
> ISTM that I've tried to suggest to work around that complexity by:
>   - document that \if-related commands should only occur at line start
> (and extend to eol).
>   - detect and complain when this is not the case.

I think this is a lousy definition, and would never be considered if we
were working in a green field.  Moreover, preventing such cases would be
pretty darn ugly/messy as well.

I also fear that there are corner cases where the behavior would still
be inconsistent.  Consider

\if ...
\set foo `echo \endif should not appear here`

If the \if succeeds, the result of the second line would be to set foo
to "endif should not appear here" (and we'd remain in the \if block).
But if the \if fails and we need to skip the \set command, any approach
that involves changing the argument parsing rules will fail to recognize
the backtick construct, and then will see the \endif as a command.
Similar examples can be constructed using \copy.

It's possible that we could keep the implementation that uses an early exit
from exec_command() if we were to move argument collection for all
backslash commands up to the start of the function.  It would still be
a bit invasive, but perhaps not too awful: I'm imagining that instead of

else if (strcmp(cmd, "setenv") == 0)
{
char   *envvar = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);
char   *envval = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);

we'd write

else if (strcmp(cmd, "setenv") == 0)
{
char   *envvar = args[0];
char   *envval = args[1];

where the args array had been filled at the top of the function.
The top-of-function code would have to know all the cases where
commands didn't use basic OT_NORMAL processing, but there aren't
that many of those, I think.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

"Daniel Verite"  writes:
>   Tom Lane wrote:
>> OT_WHOLE_LINE is not what you want because that results in verbatim
>> copying, without variable expansion or anything

> But if we want to implement "\if defined :foo" in the future
> isn't it just what we need?

I don't think that should mean what you think.  I believe an appropriate
spelling of what you mean is "\if defined foo".  What you wrote should
result in foo being expanded and then a defined-ness test being performed
on whatever variable name results.

> Also we could leave open the option to accept an SQL expression
> here. I expect people will need SQL as the evaluator in a lot of cases.

Right, and they'll also want to insert variable references into that
SQL.  In the short term though, `expr ...` is going to be the solution,
and that means we'd better not throw away the behavior of expanding
back-ticks.

> There's a precedent with \copy accepting a query inside parentheses,
> using OT_WHOLE_LINE.

IMV, \copy is just about completely broken in this regard, precisely
because it fails to expand variable references.  I don't want to
emulate that brain-damage for \if.  (I believe, btw, that part
of the reason for \copy behaving this way is that we wanted to
preserve an ancient behavior whereby Windows users were not forced
to double backslashes in \windows\style\path\names.  Fortunately,
that bit of silliness need not be considered for \if.)

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Daniel Verite]

Tom Lane wrote:

> OT_WHOLE_LINE is not what you want because that results in verbatim
> copying, without variable expansion or anything

But if we want to implement "\if defined :foo" in the future
isn't it just what we need?

Also we could leave open the option to accept an SQL expression
here. I expect people will need SQL as the evaluator in a lot of cases.
So far we need to do that:

  SELECT sql_expr ... AS varname \gset
  \if :varname
  ...
  \endif

Surely users will wonder right away why they can't write it like this
instead:

  \if (sql_expr)
  ...
  \endif

There's a precedent with \copy accepting a query inside parentheses,
using OT_WHOLE_LINE.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Erik Rijkers]

On 2017-03-17 02:28, Corey Huinker wrote:
Attached is the latest work. Not everything is done yet. I post it 
because



0001.if_endif.v23.diff



This patch does not compile for me (gcc 6.3.0):

command.c:38:25: fatal error: conditional.h: No such file or directory
 #include "conditional.h"
 ^
compilation terminated.
make[3]: *** [command.o] Error 1
make[2]: *** [all-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [all-bin-recurse] Error 2
make: *** [all-src-recurse] Error 2

Perhaps that is expected, as "Not everything is done yet",  but I can't 
tell from your email so I thought I'd report ir anyway. Ignore as 
appropriate...



Thanks,

Erik Rijkers


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey & Tom,


What is not done:
- skipped slash commands still consume the rest of the line

That last part is big, to quote Tom:

* More generally, I do not think that the approach of having exec_command
simply fall out immediately when in a false branch is going to work,
because it ignores the fact that different backslash commands have
different argument parsing rules.  Some will eat the rest of the line and
some won't.  I'm afraid that it might be necessary to remove that code
block and add a test to every single backslash command that decides
whether to actually perform its action after it's consumed its arguments.
That would be tedious :-(.  But as it stands, backslash commands will get
parsed differently (ie with potentially-different ending points) depending
on whether they're in a live branch or not, and that seems just way too
error-prone to be allowed to stand.


ISTM that I've tried to suggest to work around that complexity by:
 - document that \if-related commands should only occur at line start
   (and extend to eol).
 - detect and complain when this is not the case.
 - if some border cases are not detected, call it a feature.

ISTM that Tom did not respond to this possibly simpler approach... Maybe a 
"no" would be enough before starting heavy work which would touch all 
other commands...


Tom?

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

Attached is the latest work. Not everything is done yet. I post it because
the next step is likely to be "tedious" as Tom put it, and if there's a way
out of it, I want to avoid it.

What is done:
- all changes here built off the v22 patch
- any function which had scan_state and cond_stack passed in now only has
scan_state, and cond_stack is extracted from the cb_passthrough pointer.
- ConditonalStack is now only explictly passed to get_prompt ... which
doesn't have scan state
- Conditional commands no longer reset scan state, nor do they clear the
query buffer
- boolean expressions consume all options, but only evaluate variables and
backticks in situations where those would be active
- invalid boolean arguments are treated as false
- contextually wrong \else, \endif, \elif are still errors

What is not done:
- TAP tests are not converted to regular regression test(s)
- skipped slash commands still consume the rest of the line

That last part is big, to quote Tom:

* More generally, I do not think that the approach of having exec_command
simply fall out immediately when in a false branch is going to work,
because it ignores the fact that different backslash commands have
different argument parsing rules.  Some will eat the rest of the line and
some won't.  I'm afraid that it might be necessary to remove that code
block and add a test to every single backslash command that decides
whether to actually perform its action after it's consumed its arguments.
That would be tedious :-(.  But as it stands, backslash commands will get
parsed differently (ie with potentially-different ending points) depending
on whether they're in a live branch or not, and that seems just way too
error-prone to be allowed to stand.


If that's what needs to be done, does it make sense to first commit a
pre-patch that encapsulates each command family ( \c and \connect are a
family,  all \d* commands are one family) into its own static function? It
would make the follow-up patch to if-endif cleaner and easier to review.


On Thu, Mar 16, 2017 at 5:14 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> > Ok, I've got some time now and I'm starting to dig into this. I'd like to
> > restate what I *think* my feedback is, in case I missed or misunderstood
> > something.
> > ...
> > 3. Change command scans to scan the whole boolean expression, not just
> > OT_NORMAL.
> > There's a couple ways to go about this. My gut reaction is to create a
> new
> > scan type OT_BOOL_EXPR, which for the time being is the same as
> > OT_WHOLE_LINE, but could one day be something different.
>
> OT_WHOLE_LINE is not what you want because that results in verbatim
> copying, without variable expansion or anything.  My vote would be to
> repeatedly do OT_NORMAL until you get a NULL, thereby consuming as
> many regular arguments as the backslash command has.  (After which,
> if it wasn't exactly one argument, complain, for the moment.  But this
> leaves the door open for something like "\if :foo = :bar".)  Note that
> this implies that "\if some-expression \someothercommand" will be allowed,
> but I think that's fine, as I see no reason to allow backslashes in
> whatever if-expression syntax we invent later.  OT_WHOLE_LINE is a bit of
> a bastard child and I'd just as soon not define it as being the lexing
> behavior of any new commands.
>
> > 5. Allow contextually-correct invalid boolean expressions to map to
> false.
>
> > Out-of-context \endif, \else, and \elif commands remain as errors to be
> > ignored, invalid expressions in an \if or legallyl-placed \elif are just
> > treated as false.
>
> WFM.
>
> regards, tom lane
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..7743fb0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,102 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> Ok, I've got some time now and I'm starting to dig into this. I'd like to
> restate what I *think* my feedback is, in case I missed or misunderstood
> something.
> ...
> 3. Change command scans to scan the whole boolean expression, not just
> OT_NORMAL.
> There's a couple ways to go about this. My gut reaction is to create a new
> scan type OT_BOOL_EXPR, which for the time being is the same as
> OT_WHOLE_LINE, but could one day be something different.

OT_WHOLE_LINE is not what you want because that results in verbatim
copying, without variable expansion or anything.  My vote would be to
repeatedly do OT_NORMAL until you get a NULL, thereby consuming as
many regular arguments as the backslash command has.  (After which,
if it wasn't exactly one argument, complain, for the moment.  But this
leaves the door open for something like "\if :foo = :bar".)  Note that
this implies that "\if some-expression \someothercommand" will be allowed,
but I think that's fine, as I see no reason to allow backslashes in
whatever if-expression syntax we invent later.  OT_WHOLE_LINE is a bit of
a bastard child and I'd just as soon not define it as being the lexing
behavior of any new commands.

> 5. Allow contextually-correct invalid boolean expressions to map to false.

> Out-of-context \endif, \else, and \elif commands remain as errors to be
> ignored, invalid expressions in an \if or legallyl-placed \elif are just
> treated as false.

WFM.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 13, 2017 at 5:21 PM, Tom Lane  wrote:

> "Daniel Verite"  writes:
> > Tom Lane wrote:
> >> when we see \if is that we do nothing but absorb text
> >> until we see the matching \endif.  At that point we could bitch and
> throw
> >> everything away if, say, there's \elif after \else, or anything else you
> >> want to regard as a "compile time error".  Otherwise we start execution,
> >> and from there on it probably has to behave as we've been discussing.
> >> But this'd be pretty unfriendly from an interactive standpoint, and I'm
> >> not really convinced that it makes for significantly better error
> >> reporting.
>
> > On the whole, isn't that a reasonable model to follow for psql?
>
> One thing that occurs to me after more thought is that with such a model,
> we could not have different lexing rules for live vs not-live branches,
> since we would not have made those decisions before scanning the input.
> This seems problematic.  Even if you discount the question of whether
> variable expansion is allowed to change command-boundary decisions, we'd
> still not want backtick execution to happen everywhere in the block, ISTM.
>
> Maybe we could fix things so that backtick execution happens later, but
> it would be a pretty significant and invasive change to backslash command
> execution, I'm afraid.
>
> regards, tom lane
>

Ok, I've got some time now and I'm starting to dig into this. I'd like to
restate what I *think* my feedback is, in case I missed or misunderstood
something.

1. Convert perl tests to a single regular regression test.

2. Have MainLoop() pass the cond_stack to the lexer via
psql_scan_set_passthrough(scan_state, (void *) cond_stack);

3. Change command scans to scan the whole boolean expression, not just
OT_NORMAL.

There's a couple ways to go about this. My gut reaction is to create a new
scan type OT_BOOL_EXPR, which for the time being is the same as
OT_WHOLE_LINE, but could one day be something different.

4. Change variable expansion and backtick execution in false branches to
match new policy.

I've inferred that current preference would be for no expansion and no
execution.

5. Allow contextually-correct invalid boolean expressions to map to false.

Out-of-context \endif, \else, and \elif commands remain as errors to be
ignored, invalid expressions in an \if or legallyl-placed \elif are just
treated as false.

Did I miss anything?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

"Daniel Verite"  writes:
> Tom Lane wrote:
>> when we see \if is that we do nothing but absorb text
>> until we see the matching \endif.  At that point we could bitch and throw
>> everything away if, say, there's \elif after \else, or anything else you
>> want to regard as a "compile time error".  Otherwise we start execution,
>> and from there on it probably has to behave as we've been discussing.
>> But this'd be pretty unfriendly from an interactive standpoint, and I'm
>> not really convinced that it makes for significantly better error
>> reporting.

> On the whole, isn't that a reasonable model to follow for psql?

One thing that occurs to me after more thought is that with such a model,
we could not have different lexing rules for live vs not-live branches,
since we would not have made those decisions before scanning the input.
This seems problematic.  Even if you discount the question of whether
variable expansion is allowed to change command-boundary decisions, we'd
still not want backtick execution to happen everywhere in the block, ISTM.

Maybe we could fix things so that backtick execution happens later, but
it would be a pretty significant and invasive change to backslash command
execution, I'm afraid.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
>> Barring objection I'll push this so that Corey can rebase over it.

> Seems straightforward, and I appreciate you doing it for me!

Hearing no objections, pushed.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Daniel Verite]

Tom Lane wrote:

> when we see \if is that we do nothing but absorb text
> until we see the matching \endif.  At that point we could bitch and throw
> everything away if, say, there's \elif after \else, or anything else you
> want to regard as a "compile time error".  Otherwise we start execution,
> and from there on it probably has to behave as we've been discussing.
> But this'd be pretty unfriendly from an interactive standpoint, and I'm
> not really convinced that it makes for significantly better error
> reporting.

This is basically what bash does. In an if/else/fi block
in an interactive session, the second prompt is displayed at every new
line and nothing gets executed until it recognizes the end of the
block and it's valid as a whole. Otherwise, nothing of the block
gets executed. That doesn't strike me as unfriendly.

When non-interactive, in addition to the block not being executed,
the fact that it fails implies that the execution of the current script
is ended, independently of the errexit setting.
If errexit is set, the interpreter terminates. If it was
an included script and errexit is not set, the execution resumes
after the point of the inclusion.

On the whole, isn't that a reasonable model to follow for psql?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Barring objection I'll push this so that Corey can rebase over it.
>
> regards, tom lane
>

Seems straightforward, and I appreciate you doing it for me!

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

I wrote:
> IIRC, I objected to putting knowledge of ConditionalStack
> into the shared psqlscan.l lexer, and I still think that would be a bad
> idea; but we need some way to get the lexer to shut that off.  Probably
> the best way is to add a passthrough "void *" argument that would let the
> get_variable callback function mechanize the rule about not expanding
> in a false branch.

Here's a proposed patch that adds a passthrough of this sort.

The passthrough argument is passed only to the get_variable callback.
I dithered about whether to also pass it to the write_error callback,
but ultimately decided not to for now.  Neither psql nor pgbench wants it,
and in the case of psql we'd have to invent a separate wrapper function
because we would certainly not want to change the signature of
psql_error().

Barring objection I'll push this so that Corey can rebase over it.

regards, tom lane

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 1aa56ab..e9d4fe6 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*** setQFout(const char *fname)
*** 119,127 
   * If "escape" is true, return the value suitably quoted and escaped,
   * as an identifier or string literal depending on "as_ident".
   * (Failure in escaping should lead to returning NULL.)
   */
  char *
! psql_get_variable(const char *varname, bool escape, bool as_ident)
  {
  	char	   *result;
  	const char *value;
--- 119,131 
   * If "escape" is true, return the value suitably quoted and escaped,
   * as an identifier or string literal depending on "as_ident".
   * (Failure in escaping should lead to returning NULL.)
+  *
+  * "passthrough" is the pointer previously given to psql_scan_set_passthrough.
+  * psql currently doesn't use this.
   */
  char *
! psql_get_variable(const char *varname, bool escape, bool as_ident,
!   void *passthrough)
  {
  	char	   *result;
  	const char *value;
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index a83bc69..3d8b8da 100644
*** a/src/bin/psql/common.h
--- b/src/bin/psql/common.h
***
*** 16,22 
  extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
  extern bool setQFout(const char *fname);
  
! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident);
  
  extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2);
  
--- 16,23 
  extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
  extern bool setQFout(const char *fname);
  
! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident,
!   void *passthrough);
  
  extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2);
  
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index 5b7953b..ba4a08d 100644
*** a/src/bin/psql/psqlscanslash.l
--- b/src/bin/psql/psqlscanslash.l
*** other			.
*** 243,249 
  			 yyleng - 1);
  		value = cur_state->callbacks->get_variable(varname,
     false,
!    false);
  		free(varname);
  
  		/*
--- 243,250 
  			 yyleng - 1);
  		value = cur_state->callbacks->get_variable(varname,
     false,
!    false,
!    cur_state->cb_passthrough);
  		free(varname);
  
  		/*
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 1b29341..19b3e57 100644
*** a/src/fe_utils/psqlscan.l
--- b/src/fe_utils/psqlscan.l
*** other			.
*** 700,706 
  	if (cur_state->callbacks->get_variable)
  		value = cur_state->callbacks->get_variable(varname,
     false,
!    false);
  	else
  		value = NULL;
  
--- 700,707 
  	if (cur_state->callbacks->get_variable)
  		value = cur_state->callbacks->get_variable(varname,
     false,
!    false,
!    cur_state->cb_passthrough);
  	else
  		value = NULL;
  
*** psql_scan_destroy(PsqlScanState state)
*** 923,928 
--- 924,942 
  }
  
  /*
+  * Set the callback passthrough pointer for the lexer.
+  *
+  * This could have been integrated into psql_scan_create, but keeping it
+  * separate allows the application to change the pointer later, which might
+  * be useful.
+  */
+ void
+ psql_scan_set_passthrough(PsqlScanState state, void *passthrough)
+ {
+ 	state->cb_passthrough = passthrough;
+ }
+ 
+ /*
   * Set up to perform lexing of the given input line.
   *
   * The text at *line, extending for line_len bytes, will be scanned by
*** psqlscan_escape_variable(PsqlScanState s
*** 1409,1415 
  	/* Variable lookup. */
  	varname = psqlscan_extract_substring(state, txt + 2, len - 3);
  	if (state->callbacks->get_variable)
! 		value = state->callbacks->get_variable(varname, true, as_ident);
  	else
  		value = NULL;
  	free(varname);
--- 1423,1430 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

"David G. Johnston"  writes:
> There are only four commands and a finite number of usage permutations.
> Enumerating and figuring out the proper behavior for each should be done.

> Thus - ​If the expressions are bad they are considered false but the block
> is created

> If the flow-control command is bad the system will tell the user why and
> how to get back to a valid state - the entire machine state goes INVALID
> until a corrective command is encountered.

> For instance:

> \if
> \else
> \elif
> warning: elif block cannot occur directly within an \else block.  either
> start a new \if, \endif the current scope, or type \else to continue
> entering commands into the existing else block.  no expression evaluation
> has occurred.
> \echo 'c'
> warning: command ignored in broken \if block scope - see prior correction
> options

This is looking a whole lot like the overcomplicated error reporting that
we already considered and rejected.  I think it's sufficient to print
something like "\elif is not allowed to follow \else; command ignored"
and not change state.  We're not really helping anybody by going into
an "invalid machine state" AFAICS, and having such a thing complicates
the mental model more than I'd like.

A different way of looking at this problem, which will seem like overkill
right now but would absolutely not be once you consider looping, is that
what should happen when we see \if is that we do nothing but absorb text
until we see the matching \endif.  At that point we could bitch and throw
everything away if, say, there's \elif after \else, or anything else you
want to regard as a "compile time error".  Otherwise we start execution,
and from there on it probably has to behave as we've been discussing.
But this'd be pretty unfriendly from an interactive standpoint, and I'm
not really convinced that it makes for significantly better error
reporting.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[David G. Johnston]

On Sun, Mar 12, 2017 at 10:24 AM, Tom Lane  wrote:

>
> One point here is that we need to distinguish problems in the expression,
> which could arise from changing variable values, from some other types of
> mistakes like \elif with no preceding \if.  When you see something like
> that you pretty much have to treat it as a no-op; but I don't think that's
> a problem for scripting usage.
>

​One of my discarded write-ups from last night made a point that we don't
really distinguish between run-time and compile-time errors - possibly
because we haven't had to until now.

​If we detect what would be considered a compile-time error (\elif after
\else for instance) we could treat anything that isn't a conditional
meta-command as a no-op with a warning (and exit in stop-script mode)​.

There are only four commands and a finite number of usage permutations.
Enumerating and figuring out the proper behavior for each should be done.

Thus - ​If the expressions are bad they are considered false but the block
is created

If the flow-control command is bad the system will tell the user why and
how to get back to a valid state - the entire machine state goes INVALID
until a corrective command is encountered.

For instance:

\if
\else
\elif
warning: elif block cannot occur directly within an \else block.  either
start a new \if, \endif the current scope, or type \else to continue
entering commands into the existing else block.  no expression evaluation
has occurred.
\echo 'c'
warning: command ignored in broken \if block scope - see prior correction
options

Yes, that's wordy, but if that was shown the user would be able to
recognize their situation and be able to get back to their desired state.

Figuring out what the valid correction commands are for each invalid state,
and where the user is left, is tedious but mechanical.

So we'd need an INVALID state as well as the existing IGNORE state.

\endif would always work - but take you up one nesting level

The user shouldn't need to memorize the invalid state rules.  While we
could document them and point the reader there having them inline seems
preferable.
​

>
> We could imagine resolving this tension by treating failed \if expressions
> differently in interactive and noninteractive cases.  But I fear that cure
> would be worse than the disease.
>

​I don't think this becomes necessary - we should distinguish the error
types the same in both modes.​

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> (1) document that \if-related commands MUST be on their own
> line (i.e. like cpp #if directives?).
>

I have no opinion on whether \if-related comments must be on their own
line, though I coded as if that were the case.

I want to point out that the goal down the road is to allow rudimentary
expressions beyond just 'will this string cast to boolean true'.

For example, in the earlier thread "Undefined psql variables", I proposed a
slash command that would test if a named psql var were defined, and if not
then assign it a value.

Tom suggested leveraging if-then infrastructure like this

 \if not defined(x)
  \set x y
 \fi

Which would be great. I ask that whatever we decide in terms of how much
more input we read to digest the expression allow for constructs like the
one above.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> Reading this, I started to wonder "so how did I get that impression?" and I
> found this from Feb 9:

> IMO, an erroneous backslash command should have no effect, period.
> "It failed but we'll treat it as if it were valid" is a rathole
> I don't want to descend into.  It's particularly bad in interactive
> mode, because the most natural thing to do is correct your spelling
> and issue the command again --- but if psql already decided to do
> something on the strength of the mistaken command, that doesn't work,
> and you'll have to do something or other to unwind the unwanted
> control state before you can get back to what you meant to do.

Yeah, it's not the greatest thing for interactive usage, but as we
already discussed, this feature needs to be optimized for scripting not
interaction --- and even a bit of thought shows that the current behavior
is disastrous for scripting.  If your only suggestion for getting sane
behavior in a script is "set ON_ERROR_STOP", you've failed to provide
useful error handling.

One point here is that we need to distinguish problems in the expression,
which could arise from changing variable values, from some other types of
mistakes like \elif with no preceding \if.  When you see something like
that you pretty much have to treat it as a no-op; but I don't think that's
a problem for scripting usage.

We could imagine resolving this tension by treating failed \if expressions
differently in interactive and noninteractive cases.  But I fear that cure
would be worse than the disease.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 12, 2017 at 1:52 AM, David G. Johnston <
david.g.johns...@gmail.com> wrote:
>
> Oddly, Corey was using you as support for this position...though without
an actual quote:
>
> """

Reading this, I started to wonder "so how did I get that impression?" and I
found this from Feb 9:

IMO, an erroneous backslash command should have no effect, period.
"It failed but we'll treat it as if it were valid" is a rathole
I don't want to descend into.  It's particularly bad in interactive
mode, because the most natural thing to do is correct your spelling
and issue the command again --- but if psql already decided to do
something on the strength of the mistaken command, that doesn't work,
and you'll have to do something or other to unwind the unwanted
control state before you can get back to what you meant to do.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


* Daniel Verite previously pointed out the desirability of disabling
variable expansion while skipping script.  That doesn't seem to be here,


ISTM that it is still there, but for \elif conditions which are currently 
always checked.


  fabien=# \if false
  fabien@#   \echo `echo BAD`
command ignored, use \endif or Ctrl-C to exit current branch.
  fabien@# \else
  fabien=#   \echo `echo OK`
OK
  fabien=# \endif


IIRC, I objected to putting knowledge of ConditionalStack into the 
shared psqlscan.l lexer, and I still think that would be a bad idea; but 
we need some way to get the lexer to shut that off. Probably the best 
way is to add a passthrough "void *" argument that would let the 
get_variable callback function mechanize the rule about not expanding in 
a false branch.


Hmmm. I see this as a circumvolute way of providing the stack knowledge 
without actually giving the stack... it seems that would work, so why not.



* Whether or not you think it's important not to expand skipped variables,
I think that it's critical that skipped backtick expressions not be
executed.



\if something
\elif `expr :var1 + :var2 = :var3`
\endif



I think it's essential that expr not be called if the first if-condition
succeeded.


This was the behavior at some point, but it was changed because we 
understood that it was required that boolean errors were detected and the 
resulting command be simply ignored. I'm really fine with having that 
back.



* The documentation says that an \if or \elif expression extends to the
end of the line, but actually the code is just eating one OT_NORMAL
argument.  That means it's OK to do this: [...]



More generally, I do not think that the approach of having exec_command
simply fall out immediately when in a false branch is going to work,
because it ignores the fact that different backslash commands have
different argument parsing rules.  Some will eat the rest of the line and
some won't.  I'm afraid that it might be necessary to remove that code
block and add a test to every single backslash command that decides
whether to actually perform its action after it's consumed its arguments.
That would be tedious :-(.


Indeed.

IMO the very versatile lexing conventions of backslash commands, or rather 
their actual lack of any consistency, makes it hard to get something very 
sane out of this, especially with the "do not evaluate in false branch" 
argument.


As a simple way out, I suggest to:

(1) document that \if-related commands MUST be on their own
line (i.e. like cpp #if directives?).

(2) check that it is indeed the case when one \if-related
command detected.

(3) call it a feature if someone does not follow the rule and gets a
strange behavior as a result, as below:


regression=# \if 0
regression@# \echo foo \endif
  command ignored, use \endif or Ctrl-C to exit current branch.
  (notice we're not out of the conditional)




* I'm not on board with having a bad expression result in failing
the \if or \elif altogether.


This was understood as a requirement on previous versions which did not 
fail. I do agree that it seems better to keep the structure on errors, at 
least for script usage.


It was stated several times upthread that that should be processed as 
though the result were "false", and I agree with that.


I'm fine with that, if everyone could agree before Corey spends more time 
on this...


[...] We might as well replace the recommendation to use ON_ERROR_STOP 
with a forced abort() for an invalid expression value, because trying to 
continue a script with this behavior is entirely useless.


Hmmm. Maybe your remark is rhetorical. That could be for scripting use, 
but in interactive mode aborting coldly on syntax errors is not too nice 
for the user.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Starting to poke at this... the proposal to add prove checks for psql 
just to see whether \if respects ON_ERROR_STOP seems like an incredibly 
expensive way to test a rather minor point. On my machine, "make check" 
in bin/psql goes from zero time to close to 8 seconds.  I'm not really 
on board with adding that kind of time to every buildfarm run for the 
foreseeable future just for this.


ISTM that these tests allowed to find bugs in the implementation, so they 
were useful at some point. They are still useful in the short term if the 
implementation is to be changed significantly to respond to your various 
requirements. The underlying issue with TAP test is that it installs a new 
cluster on each script, which is quite costly.


In this case, the same result could be achieved with a number of small 
failing tests, which only launch "psql". Could that be acceptable? What 
you suggest is to keep only *one* failing test, which I find is kind of a 
regression from a testing coverage perspective, although obviously it is 
possible.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[David G. Johnston]

On Sat, Mar 11, 2017 at 5:45 PM, Tom Lane  wrote:

>
> * Whether or not you think it's important not to expand skipped variables,
> I think that it's critical that skipped backtick expressions not be
> executed.
> ​ [...] ​
> I do not think that a skipped \if or \elif
> should evaluate its argument at all.
>
>
​[...]
​

> * I'm not on board with having a bad expression result in failing
> the \if or \elif altogether.  It was stated several times upthread
> that that should be processed as though the result were "false",
> and I agree with that.


​+1​

​Oddly, Corey was using you as support for this position...though without
an actual quote:

"""
Tom was pretty adamant that invalid commands are not executed. So in a case
like this, with ON_ERROR_STOP off:

\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif

Both 'b' and 'c' should print, because "\elif invalid" should not execute.
The code I had before was simpler, but it missed that.
"""
https://www.postgresql.org/message-id/CADkLM%3De9BY_-
PT96mcs4qqiLtt8t-Fp%3De_AdycW-aS0OQvbC9Q%40mail.gmail.com

Also,

Robert made a comment somewhere along the line about users wanting to
simply re-type the intended line if the "invalid" was interactive and due
to a typo.  That concern is pretty much limited to just the "\if" situation
- if you typo an "\elif" block you can just type "\elif" again and begin
yet another "\elif" block.  I say we live with it and focus on the UX - if
you type \if no matter what happens after you hit enter you are in a
conditional block and will need to \endif at some point. Re-typing the
correct \if command will just make you need another one of them.

David J.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Robert Haas]

On Sat, Mar 11, 2017 at 9:40 PM, Tom Lane  wrote:
> I thought the same of the version you were complaining about, but
> the current patch seems to have dialed it back a good deal.  Do you
> still find the current error messages unmaintainable?

I haven't looked, but I had the impression this had been much improved.

>> But I have not taken a position on what should happen if the
>> condition for \if or \elsif evaluates to a baffling value.  Corey's
>> prior proposal was to treat it, essentially, as neither true nor
>> false, skipping both arms of the if.  Tom seems to want an invalid
>> value treated as false.  You could also imagine pretending that the
>> command never happened at all, likely leading to complete chaos.
>
> Hmm, if that "prior proposal" was indeed on the table, I missed it.
> The current patch, AFAICS, implements your third choice, which I quite
> agree would lead to complete chaos; there would be no way to write a
> script that did anything useful with that.

Well, other than: don't write a script with invalid commands in it.

But I'm not seriously advocating for that position.

> It is interesting to think about what would happen if "expr is neither
> true nor false" were defined as "skip immediately to \endif" (which
> I think is the natural generalization of what you said to apply to an
> intermediate \elif).  I believe that it'd be possible to work with it,
> but it's not very clear if it'd be easier or harder to work with than
> the rule of treating bogus results as false.  What is clear is that
> it'd be unlike any other conditional construct I ever worked with.

True.

> As was pointed out upthread, "treat error results as false" is what
> you get from "if" in a POSIX shell.  I think it's fair also to draw
> an analogy to what SQL does with null boolean values, which is to
> treat them as false when a decision is required (in, eg, WHERE or
> CASE).  So I think "treat bogus results as false" is the most
> conservative, least likely to cause unhappy surprises, solution here.

I don't mind that.  I was simply stating that I hadn't advocated for
anything in particular.

>> Other positions are also possible.
>
> If you've got concrete ideas about that, let's hear them.  I'm not
> trying to foreclose discussion.

I personally don't, but others may.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Robert Haas  writes:
> I think that I have not taken a firm position on what the behavior
> should be with respect to errors.I took the position that the
> messages being printed saying what happened were too detailed, because
> they not only described what had happened but also tried to
> prognosticate what would happen next, which was dissimilar to what we
> do elsewhere and likely to be hard to maintain - or even get right for
> v1.

I thought the same of the version you were complaining about, but
the current patch seems to have dialed it back a good deal.  Do you
still find the current error messages unmaintainable?

> But I have not taken a position on what should happen if the
> condition for \if or \elsif evaluates to a baffling value.  Corey's
> prior proposal was to treat it, essentially, as neither true nor
> false, skipping both arms of the if.  Tom seems to want an invalid
> value treated as false.  You could also imagine pretending that the
> command never happened at all, likely leading to complete chaos.

Hmm, if that "prior proposal" was indeed on the table, I missed it.
The current patch, AFAICS, implements your third choice, which I quite
agree would lead to complete chaos; there would be no way to write a
script that did anything useful with that.

It is interesting to think about what would happen if "expr is neither
true nor false" were defined as "skip immediately to \endif" (which
I think is the natural generalization of what you said to apply to an
intermediate \elif).  I believe that it'd be possible to work with it,
but it's not very clear if it'd be easier or harder to work with than
the rule of treating bogus results as false.  What is clear is that
it'd be unlike any other conditional construct I ever worked with.
As was pointed out upthread, "treat error results as false" is what
you get from "if" in a POSIX shell.  I think it's fair also to draw
an analogy to what SQL does with null boolean values, which is to
treat them as false when a decision is required (in, eg, WHERE or
CASE).  So I think "treat bogus results as false" is the most
conservative, least likely to cause unhappy surprises, solution here.

> Other positions are also possible.

If you've got concrete ideas about that, let's hear them.  I'm not
trying to foreclose discussion.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Robert Haas]

On Fri, Mar 3, 2017 at 3:18 AM, Fabien COELHO  wrote:
> I'm ok with this patch. I think that the very simple automaton code
> structure achieved is worth the very few code duplications. It is also
> significantly shorter than the nested if/switch variants, and it does
> exactly what Tom and Robert wished with respect to errors, so I think that
> this is a good compromise.

I think that I have not taken a firm position on what the behavior
should be with respect to errors.I took the position that the
messages being printed saying what happened were too detailed, because
they not only described what had happened but also tried to
prognosticate what would happen next, which was dissimilar to what we
do elsewhere and likely to be hard to maintain - or even get right for
v1.  But I have not taken a position on what should happen if the
condition for \if or \elsif evaluates to a baffling value.  Corey's
prior proposal was to treat it, essentially, as neither true nor
false, skipping both arms of the if.  Tom seems to want an invalid
value treated as false.  You could also imagine pretending that the
command never happened at all, likely leading to complete chaos.
Other positions are also possible.  I suggest that doing it the way
Tom likes may be the path of least resistance, but this isn't really
something I'm very animated about personally.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> [ 0001.if_endif.v21.diff ]

I had thought that this patch was pretty close to committable, but
the more I poke at it the less I think so.  The technology it uses
for skipping unexecuted script sections has got too many bugs.

* Daniel Verite previously pointed out the desirability of disabling
variable expansion while skipping script.  That doesn't seem to be here,
though there's an apparently-vestigial comment in psql/common.c claiming
that it is.  IIRC, I objected to putting knowledge of ConditionalStack
into the shared psqlscan.l lexer, and I still think that would be a bad
idea; but we need some way to get the lexer to shut that off.  Probably
the best way is to add a passthrough "void *" argument that would let the
get_variable callback function mechanize the rule about not expanding
in a false branch.

* Whether or not you think it's important not to expand skipped variables,
I think that it's critical that skipped backtick expressions not be
executed.  The specific use-case that I'm concerned about is backtick
evals in \if expressions, which are going to be all over the place as
long as we haven't got any native expression eval capability, and will
doubtless remain important even when/if we do.  So in a case like

\if something
\elif `expr :var1 + :var2 = :var3`
\endif

I think it's essential that expr not be called if the first if-condition
succeeded.  (That first condition might be checking whether the vars
contain valid integers, for example.)  The current patch gets this totally
wrong --- not only does it perform the backtick, but \elif complains if
the result isn't a valid bool.  I do not think that a skipped \if or \elif
should evaluate its argument at all.

* The documentation says that an \if or \elif expression extends to the
end of the line, but actually the code is just eating one OT_NORMAL
argument.  That means it's OK to do this:

regression=# \if 1 \echo foo \echo bar \endif
foo
bar
regression=# 

which doesn't seem insane, except that the inverse case is insane:

regression=# \if 0 \echo foo \echo bar \endif
regression@# 

(notice we're not out of the conditional).  Even if we change it to
eat the whole line as argument, this inconsistency will remain:

regression=# \if 1
regression=# \echo foo \endif
foo
regression=# 

(notice we're out of the conditional)

regression=# \if 0
regression@# \echo foo \endif
command ignored, use \endif or Ctrl-C to exit current branch.
regression@# 

(notice we're not out of the conditional)

This inconsistency seems to have to do with the code in HandleSlashCmds
that discards arguments until EOL after a failed backslash command.
You've modified that to also discard arguments after a non-executed
command, and I think that's broken.

* More generally, I do not think that the approach of having exec_command
simply fall out immediately when in a false branch is going to work,
because it ignores the fact that different backslash commands have
different argument parsing rules.  Some will eat the rest of the line and
some won't.  I'm afraid that it might be necessary to remove that code
block and add a test to every single backslash command that decides
whether to actually perform its action after it's consumed its arguments.
That would be tedious :-(.  But as it stands, backslash commands will get
parsed differently (ie with potentially-different ending points) depending
on whether they're in a live branch or not, and that seems just way too
error-prone to be allowed to stand.

* I think it's completely wrong to do either resetPQExpBuffer(query_buf)
or psql_scan_reset(scan_state) when deciding a branch is not to be
executed.  Compare these results:

regression=# select (1 +
regression(# \if 1
regression-# \echo foo
foo
regression-# \endif
regression-# 2);
 ?column? 
--
3
(1 row)

regression=# select (1 +
regression(# \if 0
regression-# \echo foo
command ignored, use \endif or Ctrl-C to exit current branch.
regression@# \endif
regression=# 2);
ERROR:  syntax error at or near "2"
LINE 1: 2);
^
regression=# 

If the first \if doesn't throw away an incomplete query buffer (which it
should not), then surely the second should not either.  Somebody who
actually wants to toss the query buffer can put \r into the appropriate
branch of their \if; it's not for us to force that on them.

* Also, the documentation for psql_scan_reset is pretty clear that it's to
be called when and only when the query buffer is reset, which makes your
calls in the bodies of the conditional commands wrong.  As an example:

regression=# select (1 +
regression(# 2;
regression(# 

(notice we've not sent the known-incomplete command to the server) vs

regression(# \r
Query buffer reset (cleared).
regression=# select (1 +
regression(# \if 1
regression-# \endif
regression-# 2;
ERROR:  syntax error at or near ";"
LINE 2: 2;
 ^
regression=# 

That happens because the \if code gratuituously 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Mar 11, 2017 at 4:17 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> > [ 0001.if_endif.v21.diff ]
>
> Starting to poke at this... the proposal to add prove checks for psql
> just to see whether \if respects ON_ERROR_STOP seems like an incredibly
> expensive way to test a rather minor point.  On my machine, "make check"
> in bin/psql goes from zero time to close to 8 seconds.  I'm not really
> on board with adding that kind of time to every buildfarm run for the
> foreseeable future just for this.
>
> Couldn't we get close to the same coverage by adding a single-purpose
> test script to the main regression tests?  Along the lines of
>
>  \set ON_ERROR_STOP 1
>  \if invalid
>  \echo should not get here
>  \endif
>  \echo should not get here either
>
> You could imagine just dropping that at the end of psql.sql, but I
> think probably a separate script is worth the trouble.
>
> regards, tom lane
>


I think I can manage that. Just to be clear, you're asking me to replace
the perl script with one new sql script? If so, there's probably a few
non-on-stop tests in there that might be worth preserving in regression
form.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> [ 0001.if_endif.v21.diff ]

Starting to poke at this... the proposal to add prove checks for psql
just to see whether \if respects ON_ERROR_STOP seems like an incredibly
expensive way to test a rather minor point.  On my machine, "make check"
in bin/psql goes from zero time to close to 8 seconds.  I'm not really
on board with adding that kind of time to every buildfarm run for the
foreseeable future just for this.

Couldn't we get close to the same coverage by adding a single-purpose
test script to the main regression tests?  Along the lines of

 \set ON_ERROR_STOP 1
 \if invalid
 \echo should not get here
 \endif
 \echo should not get here either

You could imagine just dropping that at the end of psql.sql, but I
think probably a separate script is worth the trouble.

regards, tom lane


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

About v21:

Patch applies with some offset, make check ok, psql tap tests ok.

I also did some interactive tests which behaved as I was expecting.

I'm ok with this patch. I think that the very simple automaton code 
structure achieved is worth the very few code duplications. It is also 
significantly shorter than the nested if/switch variants, and it does 
exactly what Tom and Robert wished with respect to errors, so I think that 
this is a good compromise.


A tiny detail about "default". I would have added a comment when it is 
expected to be dead code (else, elif), and I would have put the list of 
matching states explicitely otherwise (if, endif) otherwise the reader has 
to remember what the other states are. Probably it is me being really too 
peckish, if at all possible:-)


I've turned the patch as ready, again.

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 3, 2017 at 1:25 AM, Fabien COELHO  wrote:

>
>
> For endif, I really exagerated, "switch { defaut: " is too much, please
>> accept my apology. Maybe just do the pop & error reporting?
>>
>
It seemed like overkill, but I decided to roll with it.


>
> Or maybe be more explicit:
>
>   switch (current state)
>   case NONE:
>  error no matching if;
>   case ELSE_FALSE:
>   case ELSE_TRUE:
>   case ...:
>  pop;
>  Assert(success);


the pop() function tests for an empty stack, so this switch is
double-testing, but it's also no big deal, so here you go...
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

For endif, I really exagerated, "switch { defaut: " is too much, please 
accept my apology. Maybe just do the pop & error reporting?


Or maybe be more explicit:

  switch (current state)
  case NONE:
 error no matching if;
  case ELSE_FALSE:
  case ELSE_TRUE:
  case ...:
 pop;
 Assert(success);

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v20: attempt at implementing the switch-on-all-states style.


For the elif I think it is both simpler and better like that. Whether 
committer will agree is an unkown, as always.


For endif, I really exagerated, "switch { defaut: " is too much, please 
accept my apology. Maybe just do the pop & error reporting?


For if, the evaluation & error could be moved before the switch, which may 
contain only the new state setting decision, and the push after the 
switch? Also, I would suggest to use default only to detect an unexpected 
state error, and list all other states explicitely.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> For me, it is only slightly better: I think that for helping understanding
> and maintenance, the automaton state transitions should be all clear and
> loud in just one place, so I would really like to see a single common
> structure:
>
>   if (is "if") switch on all states;
>   else if (is "elif") switch on all states;
>   else if (is "else") switch on all states;
>   else if (is "endif") switch on all states;
>
> And minimal necessary error handling around that.
>
>
v20: attempt at implementing the switch-on-all-states style.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


Tom was pretty adamant that invalid commands are not executed. So in a case
like this, with ON_ERROR_STOP off:

\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif

Both 'b' and 'c' should print, because "\elif invalid" should not execute.
The code I had before was simpler, but it missed that.


Hmmm. You can still have it with one switch, by repeating the evaluation 
under true and ignore, even if the value is not used:


  switch(state)
  {
case NONE: error;
case ELSE_TRUE: error;
case ELSE_FALSE: error;
case IF_TRUE:
if (eval())
  ...
else error;
break;
case IF_FALSE:
if (eval())
  ...
else error;
break;
case IGNORE:
if (eval())
  ...
else error;
break;
}


Ok, so here's one idea I tossed around, maybe this will strike the right
balance for you.  If I create a function like this: [...]

Does that handle your objections?


For me, it is only slightly better: I think that for helping understanding 
and maintenance, the automaton state transitions should be all clear and 
loud in just one place, so I would really like to see a single common 
structure:


  if (is "if") switch on all states;
  else if (is "elif") switch on all states;
  else if (is "else") switch on all states;
  else if (is "endif") switch on all states;

And minimal necessary error handling around that.

Your suggestion does not achieve this, although I agree that the code 
structure would be cleaner thanks to the function.



p.s.  do we try to avoid constructs likeif (success = my_function(var1,
var2))   ?


I think it is allowed because I found some of them with grep (libpq, ecpg, 
postmaster, pg_dump, pg_upgrade...). They require added parentheses around 
the assignment:


  if ((success = eval())) ...

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> That is accurate. The only positive it has is that the user only
>> experiences one error, and it's the first error that was encountered if
>> reading top-to-bottom, left to right. It is an issue of which we
>> prioritize
>> - user experience or simpler code.
>>
>
> Hmmm. The last simpler structure I suggested, which is basically the one
> used in your code before the update, does check for the structure error
> first. The only drawback is that the condition is only evaluated when
> needed, which is an issue we can cope with, IMO.


Tom was pretty adamant that invalid commands are not executed. So in a case
like this, with ON_ERROR_STOP off:

\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif

Both 'b' and 'c' should print, because "\elif invalid" should not execute.
The code I had before was simpler, but it missed that.


>
> Now if you want to require committer opinion on this one, fine with me.
>>>
>>
>> Rather than speculate on what a committer thinks of this edge case (and
>> making a patch for each possible theory), I'd rather just ask them what
>> their priorities are and which user experience they favor.
>>
>
> ISTM that the consistent message by Robert & Tom was to provide simpler
> code even if the user experience is somehow degraded, as they required that
> user-friendly features were removed (eg trying to be nicer about structural
> syntax errors, barking in interactive mode so that the user always knows
> the current status, providing a detailed status indicator in the prompt...).
>

Ok, so here's one idea I tossed around, maybe this will strike the right
balance for you.

If I create a function like this:

static boolean
is_valid_else_context(IfState if_state, const char *cmd)
{
/* check for invalid \else / \elif contexts */

switch (if_state)

{
case IFSTATE_NONE:
/* not in an \if block */
psql_error("\\%s: no matching \\if\n", cmd);
return false;
break;
case IFSTATE_ELSE_TRUE:
case IFSTATE_ELSE_FALSE:
psql_error("\\%s: cannot occur after \\else\n", cmd);
return false;
break;
default:
break;
}
return true;
}



Then the elif block looks something like this:

else if (strcmp(cmd, "elif") == 0)
{
ifState if_state = conditional_stack_peek(cstack);

if (is_valid_else_context(if_state, "elif"))
{
/*
 * valid \elif context, check for valid expression
 */
bool elif_true = false;
success = read_boolean_expression(scan_state, "\\elif ",
_true);
if (success)
{
/*
 * got a valid boolean, what to do with it depends on
current
 * state
 */
switch (if_state)
{
case IFSTATE_IGNORED:
/*
 * inactive branch, do nothing.
 * either if-endif already had a true block,
 * or whole parent block is false.
 */
break;
case IFSTATE_TRUE:
/*
 * just finished true section of this if-endif,
 * must ignore the rest until \endif
 */
conditional_stack_poke(cstack, IFSTATE_IGNORED);
break;
case IFSTATE_FALSE:
/*
 * have not yet found a true block in this if-endif,
 * this might be the first.
 */
if (elif_true)
conditional_stack_poke(cstack, IFSTATE_TRUE);
break;
default:
/* error cases all previously ruled out */
break;
}
}
}
else
success = false;
psql_scan_reset(scan_state);
}


This is functionally the same as my latest patch, but the ugliness of
switching twice on if_state is hidden.

As an added benefit, the "else"-handling code gets pretty simple because it
can leverage that same function.

Does that handle your objections?

p.s.  do we try to avoid constructs likeif (success = my_function(var1,
var2))   ?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.


Hmmm. The last simpler structure I suggested, which is basically the one 
used in your code before the update, does check for the structure error 
first. The only drawback is that the condition is only evaluated when 
needed, which is an issue we can cope with, IMO.



Now if you want to require committer opinion on this one, fine with me.


Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.


ISTM that the consistent message by Robert & Tom was to provide simpler 
code even if the user experience is somehow degraded, as they required 
that user-friendly features were removed (eg trying to be nicer about 
structural syntax errors, barking in interactive mode so that the user 
always knows the current status, providing a detailed status indicator in 
the prompt...).


Now committers can change their opinions, it is their privilege:-)

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 12:23 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> on elif
>>  if misplaced elif
>> misplaced elif error
>>  else
>> eval expression
>>   => possible eval error
>> set new status if eval fine
>>
>
> Currently it is really:
>
>   switch (state) {
>   case NONE:
>   case ELSE_TRUE:
>   case ELSE_FALSE:
>  success = false;
>  show some error
>   default:
>   }
>   if (success) {
> success = evaluate_expression(...);
> if (success) {
>switch (state) {
>case ...:
>default:
>}
> }
>   }
>
> Which I do not find so neat. The previous one with nested switch-if-switch
> looked as bad.


That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.

Now if you want to require committer opinion on this one, fine with me.


Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


on elif
 if misplaced elif
misplaced elif error
 else
eval expression
  => possible eval error
set new status if eval fine


Currently it is really:

  switch (state) {
  case NONE:
  case ELSE_TRUE:
  case ELSE_FALSE:
 success = false;
 show some error
  default:
  }
  if (success) {
success = evaluate_expression(...);
if (success) {
   switch (state) {
   case ...:
   default:
   }
}
  }

Which I do not find so neat. The previous one with nested switch-if-switch 
looked as bad.



The issue at hand being the benefit to the user vs code complexity.


Hmmm.

One of my point is that I do not really see the user benefit... for me the 
issue is to have no user benefit and code complexity.


The case we are discussing is for the user who decides to write code with 
*two* errors on the same line:


  \if good-condition
  \else
  \elif bad-condition
  \endif

with an added complexity to show the elif bad position error first. Why 
should we care so much for such a special case?


Maybe an alternative could be to write simpler code anyway, somehow like 
it was before:


  // on "elif"
  switch (peek(state)) {
  case NONE:   error;
  case ELSE_TRUE:  error;
  case ELSE_FALSE: error;
  case IGNORED:break;
  case TRUE:   poke IGNORED;
  case FALSE:
   success = evaluate(_true)
   if (!success)
 error;
   else if (is_true)
   poke TRUE
  default: error;
  }

The only difference is that the evaluation is not done when it is not 
needed (what a draw back) but ISTM that it is significantly easier to 
understand and maintain.


Now if you want to require committer opinion on this one, fine with 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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 3:07 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> It doesn't strike me as much cleaner, but it's no worse, either.
>>
>
> Hmmm.
>
> The "if (x) { x = ... ; if (x) {" does not help much to improve
> readability and understandability...
>
> My 0.02€ about v19:
>
> If there are two errors, I do not care which one is shown, both will have
> to be fixed anyway in the end... So I would suggest to choose the simplest
> possible implementation:
>
>   on elif:
> always eval expression
>   => possible eval error
> switch
>   => including detecting misplaced elif errors
>
> If the second error must absolutely be shown in all cases, then add a
> second misplaced elif detection in the eval expression failure branch:
>
>   on elif
> always eval
> if (eval failed)
>   also checked for misplaced (hey user, you have 2 errors in fact...)
>   bye bye...
> // else eval was fine
> switch
>   including misplaced elif detection
>
> If the committer is angry at these simple approach, then revert to the
> strange looking and hard to understand switch-if-switch solution (~ v18, or
> some simplified? v19), but I do not think the be weak benefit is worth the
> code complexity.
>
> --
> Fabien.


Just to make things easy for the committer, the existing code only shows
the user one error:

on elif
  if misplaced elif
 misplaced elif error
  else
 eval expression
   => possible eval error
 set new status if eval fine


The issue at hand being the benefit to the user vs code complexity.

So, shall we send this off to the committers and let them decide?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


It doesn't strike me as much cleaner, but it's no worse, either.


Hmmm.

The "if (x) { x = ... ; if (x) {" does not help much to improve 
readability and understandability...


My 0.02€ about v19:

If there are two errors, I do not care which one is shown, both will have 
to be fixed anyway in the end... So I would suggest to choose the simplest 
possible implementation:


  on elif:
always eval expression
  => possible eval error
switch
  => including detecting misplaced elif errors

If the second error must absolutely be shown in all cases, then add a 
second misplaced elif detection in the eval expression failure branch:


  on elif
always eval
if (eval failed)
  also checked for misplaced (hey user, you have 2 errors in fact...)
  bye bye...
// else eval was fine
switch
  including misplaced elif detection

If the committer is angry at these simple approach, then revert to the 
strange looking and hard to understand switch-if-switch solution (~ v18, 
or some simplified? v19), but I do not think the be weak benefit is worth 
the code complexity.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
>>
>> Alternatively if the structure must really be kept, then deal with errors
>> in a first switch, read value *after* switch and deal with other errors
>> there, then start a second switch, and adjust the documentation accordingly?
>>
>>   switch
>> errors
>>   read
>>   if
>> errors
>>   // no error
>>   switch
>>
>
>
it's now something more like

switch
  error-conditions
if no-errors
  read
  if was a boolean
  switch last-state

It doesn't strike me as much cleaner, but it's no worse, either.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Feb 26, 2017 at 2:47 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> About v18: Patch applies, make check ok, psql tap tests ok.
>
>
> ISTM that contrary to the documentation "\elif something" is not evaluated
> in all cases, and the resulting code is harder to understand with a nested
> switch and condition structure:
>
>   switch
>   default
> read
> if
>switch
>
> I wish (this is a personal taste) it could avoid switch-nesting on the
> very same value. It should also conform to the documentation.
>

I wasn't too happy with it, but I figured it would spark discussion. I
succeeded.


>
> If there is no compelling reason for the switch-nesting, I would suggest
> to move the read_boolean_expression before the swich, to deal with error
> immediately there, and then to have just one switch.
>

I thought about doing it that way. However, in the case of:


\set x invalid

\if true

\else
\elif :x

\endif


The error has already "happened" at line 4, char 5, and it doesn't matter
what expression follows, you will get an error.  But because
read_boolean_expression() can emit errors, you would see the error saying
"invalid" isn't a valid boolean expression, and then see another error
saying that the \elif was out of place. If we suppress
read_boolean_expression()'s error reporting, then we have to re-create that
error message ourselves, or be fine with the error message on invalid
\elifs being inconsistent with invalid \ifs.

Similar to your suggestion below, we could encapsulate the first switch
into a function valid_elif_context(ConditionalStack), which might make the
code cleaner, but would make it harder to see that all swtich-cases are
covered between the two. That might be a tradeoff we want to make.


>
> Alternatively if the structure must really be kept, then deal with errors
> in a first switch, read value *after* switch and deal with other errors
> there, then start a second switch, and adjust the documentation accordingly?
>
>   switch
> errors
>   read
>   if
> errors
>   // no error
>   switch
>


How would the documentation have to change?

Also, the %R documentation has single line marker '^' before not executed
> '@', but it is the reverse in the code.


Noted and fixed in the next patch, good catch.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,

About v18: Patch applies, make check ok, psql tap tests ok.


ISTM that contrary to the documentation "\elif something" is not evaluated 
in all cases, and the resulting code is harder to understand with a nested 
switch and condition structure:


  switch
  default
read
if
   switch

I wish (this is a personal taste) it could avoid switch-nesting on the 
very same value. It should also conform to the documentation.


If there is no compelling reason for the switch-nesting, I would suggest 
to move the read_boolean_expression before the swich, to deal with error 
immediately there, and then to have just one switch.


Alternatively if the structure must really be kept, then deal with errors 
in a first switch, read value *after* switch and deal with other errors 
there, then start a second switch, and adjust the documentation 
accordingly?


  switch
errors
  read
  if
errors
  // no error
  switch


Also, the %R documentation has single line marker '^' before not executed 
'@', but it is the reverse in the code.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Typo "unterminted".
>

Fixed.


> The \if within the \if false branch is not tallied properly? Am I missing
> something?
>

Nope, you found a bug. FIxed. Test-case added.


> I changed the paragraph to
>>
>
>Lines within false branches are parsed normally, however, any
>> completed
>>queries are not sent to the server, and any completed commands
>> other
>>than conditionals (\if,
>> \elif,
>>\else, \endif) are ignored.
>>
>
> I'm not sure about the ", however, " commas, but I'm sure that I do not
> know English punctuation rules:-)
>

Re-worded it again for shorter sentences. Re-mentioned that conditionals
are still checked for proper nesting.

* Changed comments to reflect that \if always evalutes  even in a
false branch
* Changed \elif to first check if the command is in a proper \if block
before evaluating the expression. The invalid boolean message would mask
the bigger problem.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..27824d9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,7 +3780,8 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
+but ^ if in single-line mode, or
+@ if the session is in a false conditional block,
 or ! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

About v17:

Patch applies, "make check" & psql "make check" ok.


... '@' [...] I noticed that it takes precedence over '!'. [...]


My reasoning was this: if you're in a false block, and you're not connected
to a db, the \c isn't going to work for you until you get out of the false
block, so right now being in a false block is a bigger problem than not
being connected to a db. [...]


Ok.


It could be nice to keep test cases that show what may happen?


Restored. It looks weird now, but it fixes the unterminated quoted 
string.


Typo "unterminted".

I think I found an issue while testing in interactive:

 calvin=# \if false
 calvin@#   \if false
 calvin@# \echo false-false
   command ignored, use \endif or Ctrl-C to exit current branch.
 calvin@#   \endif
 calvin=#   \echo in false
   in false

The \if within the \if false branch is not tallied properly? Am I missing 
something?


Maybe more test cases should be added to check that nesting checks do work 
properly?



Maybe the documentation could add some kind of warning about that?


I changed the paragraph to



   Lines within false branches are parsed normally, however, any completed
   queries are not sent to the server, and any completed commands other
   than conditionals (\if, \elif,
   \else, \endif) are ignored.


I'm not sure about the ", however, " commas, but I'm sure that I do not 
know English punctuation rules:-)


Maybe the sentence could be cut in shorter pieces.

I think that the fact that "if" commands are checked for proper nesting 
could be kept in the explanation.



There's no mention that psql variables AREN'T expanded, so the user has
every expectation that they are.


Ok.

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> I'm not sure that '@' is the best choice, but this is just one char.


> I noticed that it takes precedence over '!'. Why not. ISTM that orthogonal
> features are not shown independently, but this is a preexisting state, and
> it allows to have a shorter prompt, so why not.
>

My reasoning was this: if you're in a false block, and you're not connected
to a db, the \c isn't going to work for you until you get out of the false
block, so right now being in a false block is a bigger problem than not
being connected to a db. I have no strong opinion about what should happen
here, so I welcome suggestions for what tops what.


>
> Anyway, the '%R' documentation needs to be updated.


Done.


> It could be nice to keep test cases that show what may happen?
>

Restored. It looks weird now, but it fixes the unterminated quoted string.


> The various simplifications required result in the feature being more
> error prone for the user. Maybe the documentation could add some kind of
> warning about that?


I changed the paragraph to
Lines within false branches are parsed normally, however, any
completed
queries are not sent to the server, and any completed commands other
than conditionals (\if, \elif,
\else, \endif) are ignored.

There's no mention that psql variables AREN'T expanded, so the user has
every expectation that they are.


>
> Add space after comma when calling send_query.
>

Done.


>
> I'm not sure why you removed the comments before \if in the doc example.
> Maybe keep a one liner?
>

Didn't mean to, restored.


> Why not reuse the pop loop trick to "destroy" the stack?


Forgot about that, restored.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..625c9a8 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,81 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally, however, any completed
+queries are not sent to the server, and any completed commands other
+than conditionals (\if, \elif,
+\else, \endif) are ignored.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,7 +3778,8 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
+but ^ if in single-line mode, or
+@ if the session is in a false conditional block,
 or ! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


v16 is everything v15 promised to be.


My 0.02€:

Patch applies, make check ok, psql make check ok as well.


Welcome to v15, highlights:
- all conditional data structure management moved to conditional.h and
  conditional.c


Indeed.

I cannot say that I find it better, but (1) Tom did required it and (2) it 
still works:-)


If the stack stuff had stayed in "fe_utils", it would have been easy to 
reuse them in pgbench where they might be useful... But who cares?



- conditional state lives in mainloop.c and is passed to
  HandleSlashCommands, exec_command and get_prompt as needed


Same.


- no more pset.active_branch, uses conditional_active(conditional_stack)
  instead


Same.


- PsqlScanState no longer has branching state


Indeed.


- Implements the %R '@' prompt on false branches.


I'm not sure that '@' is the best choice, but this is just one char.

I noticed that it takes precedence over '!'. Why not. ISTM that orthogonal 
features are not shown independently, but this is a preexisting state, and 
it allows to have a shorter prompt, so why not.


Anyway, the '%R' documentation needs to be updated.


- Variable expansion is never suppressed even in false blocks,
  regression test edited to reflect this.


It could be nice to keep test cases that show what may happen?

The various simplifications required result in the feature being more 
error prone for the user. Maybe the documentation could add some kind of 
warning about that?



- ConditionalStack could morph into PsqlFileState without too much
  work.


Probably.

Code details:

Add space after comma when calling send_query.

I'm not sure why you removed the comments before \if in the doc example. 
Maybe keep a one liner?


Why not reuse the pop loop trick to "destroy" the stack?

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Pavel Stehule]

2017-02-23 18:52 GMT+01:00 Fabien COELHO :

>
> Hello Daniel,
>
> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>>> arguing that variable expansion shouldn't be able to insert, say, an
>>> \else
>>> in a non-active branch?  Maybe, but if it can insert an \else in an
>>> active
>>> branch, then why not non-active too?  Seems a bit inconsistent.
>>>
>>
>> Are we sold on the idea that conditionals should be implemented
>> by meta-commands, rather than for example terminal symbols of
>> a new grammar on top of the existing?
>>
>
> I would say that this already exists server-side, and it is named
> PL/pgSQL:-)
>
> I think that once psql has started with \xxx commands, then client-side
> extensions must stick with it till the end of time.


+1

we don't need strong client side scripting language - it should be just
simple.

Pavel

>
>
> --
> 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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Daniel,


Ah, I see why *that* wants to know about it ... I think.  I suppose you're
arguing that variable expansion shouldn't be able to insert, say, an \else
in a non-active branch?  Maybe, but if it can insert an \else in an active
branch, then why not non-active too?  Seems a bit inconsistent.


Are we sold on the idea that conditionals should be implemented
by meta-commands, rather than for example terminal symbols of
a new grammar on top of the existing?


I would say that this already exists server-side, and it is named 
PL/pgSQL:-)


I think that once psql has started with \xxx commands, then client-side 
extensions must stick with it till the end of time.


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Daniel Verite]

Tom Lane wrote:

> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
> arguing that variable expansion shouldn't be able to insert, say, an \else
> in a non-active branch?  Maybe, but if it can insert an \else in an active
> branch, then why not non-active too?  Seems a bit inconsistent.

Are we sold on the idea that conditionals should be implemented
by meta-commands, rather than for example terminal symbols of
a new grammar on top of the existing?

To recall the context, psql variables are really macros that may
contain meta-commands, and when they do they're essentially
injected and executed at the point of interpolation. That's more
or less what started this thread: demo'ing how we could exit
conditionally by injecting '\q' or nothing into a variable, and
saying that even if doable it was pretty weird, and it would be
better to have real conditional structures instead.

But when conditional structures are implemented as
meta-commands, there's the problem that this structure
can be generated on the fly too, which in a way is no less weird.
While I think that the introduction of conditionals in
psql is great, I'm getting doubtful about that part.
Are there popular script languages or preprocessors
that accept variables/macros instead of symbols to structure
the flow of instructions? I can't think of any myself.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Files "conditional.h" and "conditional.c" are missing from the patch.
>
> Also, is there a particular reason why tap test have been removed?


That would be because I diffed against my last commit, not the master
branch, sigh.

v16 is everything v15 promised to be.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are not evaluated in any way: queries are
+not sent to the server, non-conditional commands are not evaluated but
+bluntly ignored, nested if-expressions in such branches are also not
+evaluated but are tallied to check for proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..6c15c01 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -38,6 +38,7 @@
 #include "fe_utils/string_utils.h"
 
 #include "common.h"
+#include "conditional.h"
 #include "copy.h"
 #include "crosstabview.h"
 #include "describe.h"
@@ -62,6 +63,7 @@ typedef enum EditableObjectType
 /* functions for use in this file */
 static backslashResult exec_command(const char *cmd,
 PsqlScanState scan_state,
+ConditionalStack cstack,
 PQExpBuffer query_buf);
 static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
int lineno, bool *edited);
@@ -109,6 +111,7 @@ static void checkWin32Codepage(void);
 
 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Welcome to v15, highlights:


Files "conditional.h" and "conditional.c" are missing from the patch.

Also, is there a particular reason why tap test have been removed?

--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 6:15 PM, Corey Huinker 
wrote:

> On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane  wrote:
>
>> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>> arguing that variable expansion shouldn't be able to insert, say, an \else
>> in a non-active branch?  Maybe, but if it can insert an \else in an active
>> branch, then why not non-active too?  Seems a bit inconsistent.
>>
>
> The major reason was avoiding situations like what Daniel showed: where
> value of a variable that is meaningless/undefined in the current
> false-block context gets expanded anyway, and thus code inside a false
> block has effects outside of that block. Granted, his example was
> contrived. I'm open to removing that feature and seeing what breaks in the
> test cases.
>


Welcome to v15, highlights:
- all conditional data structure management moved to conditional.h and
conditional.c
- conditional state lives in mainloop.c and is passed to
HandleSlashCommands, exec_command and get_prompt as needed
- no more pset.active_branch, uses conditional_active(conditional_stack)
instead
- PsqlScanState no longer has branching state
- Implements the %R '@' prompt on false branches.
- Variable expansion is never suppressed even in false blocks, regression
test edited to reflect this.
- ConditionalStack could morph into PsqlFileState without too much work.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are not evaluated in any way: queries are
+not sent to the server, non-conditional commands are not evaluated but
+bluntly ignored, nested if-expressions in such branches are also not
+evaluated but are tallied to check for proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean: