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

[Robert Haas]

On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO  wrote:
> What would be the mnemonic for "," an "@"?

Oh, I just picked it because control-@ is the nul character, and your
commands would be nullified.  I realize that's pretty weak, but we're
talking about finding a punctuation mark to represent the concept of
commands-are-currently-being-skipped, and it doesn't seem particularly
worse than ^ to represent single-line mode.  If somebody's got a
better idea, fine, but there aren't that many unused punctuation marks
to choose from, and I think it's better to use a punctuation mark
rather than, say, a letter, like 's' for skip.  Otherwise you might
have the prompt change from:

banana=>

to

bananas>

Which I think is less obvious than

banana@>

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

[Fabien COELHO]

For two states:
  * for being executed (beware, it is ***important***)


It does lend importance, but that's also the line continuation marker for
"comment". Would that be a problem?


Argh. Indeed, even if people seldom type C comments in psql interactive 
mode...


Remaining ASCII characters I can thing of, hopefully avoiding already used 
ones: +%,@$\`|&:;_


So, maybe consider these ones:
  "+" for it is "on"
  "`" which is a "sub-shell execution"
  "&" for "and the next command is ..."


  / for not (under the hood, and it is opposed to *)


+1, I was going to suggest '/' for a false state, with two possible
metaphors to justify it
 1. the slash in a "no" sign ("no smoking", ghostbusters, etc)
 2. the leading char of a c/java/javascript comment (what is written here
is just words, not code)


Great.

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

[Corey Huinker]

>
> @in't gonna execute it?
>>
>
> Hmmm... This is too much of an Americanism, IMHO.


The @ looks like a handwritten 'a'.  @in't gonna => ain't gonna => will
not. It's a bad joke, made as a way of saying that I also could not think
of a good mnemonic for '@' or ','.


> I'm here all week, try the veal.
>>
>
> Sorry, syntax error, you have lost me. Some googling suggests a reference
> to post WW2 "lounge entertainers", probably in the USA. I also do not
> understand why this would mean "yes".


It's a thing lounge entertainers said after they told a bad joke.


>   . for z (waiting for the end of the sentence, i.e. endif)
>

+1 ... if we end up displaying the not-true-and-not-evaluated 'z' state.


>   & for t (no real mnemonic)
>
> For two states:
>   * for being executed (beware, it is ***important***)
>

It does lend importance, but that's also the line continuation marker for
"comment". Would that be a problem?


>   / for not (under the hood, and it is opposed to *)
>

+1, I was going to suggest '/' for a false state, with two possible
metaphors to justify it
  1. the slash in a "no" sign ("no smoking", ghostbusters, etc)
  2. the leading char of a c/java/javascript comment (what is written here
is just words, not code)

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

[Fabien COELHO]

Hello Corey,


If I can find some simple mnemonic for "," vs "@" for being executed vs
ignored, I could live with that, but nothing obvious comes to my mind.


@in't gonna execute it?


Hmmm... This is too much of an Americanism, IMHO.


I'm here all week, try the veal.


Sorry, syntax error, you have lost me. Some googling suggests a reference 
to post WW2 "lounge entertainers", probably in the USA. I also do not 
understand why this would mean "yes".



I'd be fine with either of these on aesthetic grounds. On technical
grounds, 'z' is harder to show.


I'm not sure that this valid technical point should be a good reason for 
guiding what feedback should be provided to the user, but it makes it 
simpler to choose two states:-)


For three states with more culturally neutral mnemonics, I thought of:
  ? for f (waiting for a true answer...)
  . for z (waiting for the end of the sentence, i.e. endif)
  & for t (no real mnemonic)

For two states:
  * for being executed (beware, it is ***important***)
  / for not (under the hood, and it is opposed to *)

Otherwise I still like "?[tfz]", but it is two characters long.

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

[Corey Huinker]

On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO  wrote:

>
> Hello Robert,
>
> [...] I think we should try to make this REALLY simple.  We don't really
>> want to have everybody have to change their PROMPT1 and PROMPT2 strings for
>> this one feature.
>>
>
> Ok. I think that we agree that the stack was too much details.
>
> How about just introducing a new value for %R?
>>
>
> Yes. That is indeed one of the idea being discussed.
>
> [...] , or @ if commands are currently being ignored because of the result
>> of an \if test.
>>
>
,-or-@ has one advantage over t/f/z: we cannot infer the 'z' state purely
from pset.active_state, and the if-stack itself is sequestered in
scan_state, which is not visible to the get_prompt() function.

I suppose if somebody wanted it, a separate slash command that does a
verbose printing of the current if-stack would be nice, but mostly just to
explain to people how the if-stack works.


> If I can find some simple mnemonic for "," vs "@" for being executed vs
> ignored, I could live with that, but nothing obvious comes to my mind.
>

@in't gonna execute it?

I'm here all week, try the veal.

To sum up your points: just update %R (ok), keep it simple/short (ok... but
> how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to
> be too nice with the user beyond the vital (ok, that significantly
> simplifies things).


I'd be fine with either of these on aesthetic grounds. On technical
grounds, 'z' is harder to show.

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

[Corey Huinker]

On Mon, Feb 13, 2017 at 11:29 AM, Robert Haas  wrote:

> possible states here.  Just when you've figured out what tfzffft


I agree with what you've said, but wanted to point out that any condition
that follows a 'z' would itself be 'z'. Not that tfz is much better.

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

[Fabien COELHO]

Hello Robert,

[...] I think we should try to make this REALLY simple.  We don't really 
want to have everybody have to change their PROMPT1 and PROMPT2 strings 
for this one feature.


Ok. I think that we agree that the stack was too much details.


How about just introducing a new value for %R?


Yes. That is indeed one of the idea being discussed.

[...] , or @ if commands are currently being ignored because of the 
result of an \if test.


Currently I find that %R logic is quite good, with "=" for give me 
something, "^" is start line regular expression for one line, "!" for 
beware someting is amiss, and in prompt2 "-" for continuation, '"' for in 
double quotes, "(" for in parenthesis and so on.


What would be the mnemonic for "," an "@"?

By shortening one of the suggestion down to two characters, we may have 
three cases:


  "?t" for "in condition, in true block"
  "?f" for "in condition, in false block (but true yet to come)"
  "?z" for "in condition, waiting for the end (true has been executed)".

So no indication about the stack depth and contents. tfz for true false 
and sleeping seem quite easy to infer and understand. "?" is also needed 
as a separator with the previous field which is the database name 
sometimes:


  calvin=> \if false
  calvin?f=> \echo 1
  calvin?f=> \elif true
  calvin?t=> \echo 2
2
  calvin?t=> \else
  calvin?z=> \echo 3
  calvin?z=> \endif
  calvin=>

With the suggested , and @:

  calvin=> \if false
  calvin,=> \echo 1
  calvin,=> \elif true
  calvin@=> \echo 2
2
  calvin@=> \else
  calvin,=> \echo 3
  calvin,=> \endif
  calvin=>

If I can find some simple mnemonic for "," vs "@" for being executed vs 
ignored, I could live with that, but nothing obvious comes to my mind.


The "?" for condition and Corey's [tfz] looked quite intuitive/mnemonic to 
me. The drawback is that it is 2 chars vs one char in above.


[...] I think that's all you need here: a way to alert users as to 
whether commands are being ignored, or not.


Yep.


[...]


To sum up your points: just update %R (ok), keep it simple/short (ok... 
but how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need 
to be too nice with the user beyond the vital (ok, that significantly 
simplifies things).


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

[Robert Haas]

On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO  wrote:
>> Ok, so that's not just PROMPT_READY, that's every prompt...which might be
>> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
>> level always being '.'?
>
> Yep. The idea is to keep it short, but to still have something to say "there
> are more levels" in the stack, hence the one dot. Basically I just
> compressed your 4 level proposal, and added a separator to deal with the
> preceding stuff and suggest the conditional.

I think we should try to make this REALLY simple.  We don't really
want to have everybody have to change their PROMPT1 and PROMPT2
strings for this one feature. How about just introducing a new value
for %R?  The documentation currently says:

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

...and suppose we just extend that to add:

, or @ if commands are currently being ignored because of the result
of an \if test.

The latter would include being in the \if section when the conditional
was true as well as being in the \else section when the conditional
was false.  I think that's all you need here: a way to alert users as
to whether commands are being ignored, or not.  Putting details in
about precisely why they are being ignored seems like it's too
complicated; people won't remember how to decode some bizarre series
of glyphs that we output.  Telling them whether their next command is
set to be ignored or executed is good enough; if the answer isn't what
they expect, they can debug their script to figure out what they
screwed up.

Also, keep in mind that people don't need to know everything from the
current prompt. They can try to debug things by looking back at
previous prompts. They'll understand that \if is going to introduce a
new nesting level and \endif is going to end one, and that \else and
\elseif may change things.  Aside from keeping the code simple so we
can maintain it and the output simple so that users can remember what
it means, I just don't believe that it's really going to be helpful to
convey much detail here.   People aren't going to paste in a gigaton
of commands and then look only at the last line of the output and try
to understand what it's telling them, or if they do that and are
confused, I think nobody will really feel bad about giving them the
advice "scroll up" or "try a simpler test case first".

Further keep in mind that eventually somebody's going to code \while
or \for or something, and then there are going to be even more
possible states here.  Just when you've figured out what tfzffft
means, they'll be the case of a \while loop which is getting skipped
because the condition at the top turned out to be false on the first
iteration, or where (half-joking) we're skipping commands until we
find the label that matches an executed \goto.  Writing maintainable
code includes leaving room open for other people to do stuff we can't
even foresee today, and that means we need not to use up a
disproportionate number of the glyphs that can reasonably be used in a
psql prompt just on this.  This is one small feature out of many that
psql has, and one small hint to the user about whether it's currently
causing commands to be skipped seems sufficient.

All IMHO, of course.

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

[Corey Huinker]

On Sat, Feb 11, 2017 at 3:48 PM, Fabien COELHO  wrote:

>
> Just realized that '?' means "unknown transactional status" in %x. That
>> might cause confusion if a person had a prompt of %x%R. Is that enough
>> reason to pick a different cue?
>>
>
> Argh.
>
> "\?\.?[tfz]" seems distinctive enough. Note that %R uses "'=-*^!$( and %x
> uses *!?, which means that they already share 2 characters, so adding ?
> does not seem like a big issue if it was not one before.
>
> Otherwise, maybe "&" or "%", but it is less about a condition.


Fair enough, it shouldn't be too confusing then.

The get_prompt() function can see the global pset, obviously, but can't see
the scan_state, where the if-stack currently resides. I could give up on
the notion of a per-file if-stack and just have one in pset, but that might
make life difficult for whomever is brave enough to tackle \while loops. Or
I could give pset a pointer to the current if-stack inside the scan_state,
or I could have pset hold a stack of stacks. Unsure which way would be
best.

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

[Fabien COELHO]

Just realized that '?' means "unknown transactional status" in %x. That
might cause confusion if a person had a prompt of %x%R. Is that enough
reason to pick a different cue?


Argh.

"\?\.?[tfz]" seems distinctive enough. Note that %R uses "'=-*^!$( and %x 
uses *!?, which means that they already share 2 characters, so adding ? 
does not seem like a big issue if it was not one before.


Otherwise, maybe "&" or "%", but it is less about a condition.

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

[Corey Huinker]

On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO  wrote:

>
> Ok, so that's not just PROMPT_READY, that's every prompt...which might be
>> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
>> level always being '.'?
>>
>
> Yep. The idea is to keep it short, but to still have something to say
> "there are more levels" in the stack, hence the one dot. Basically I just
> compressed your 4 level proposal, and added a separator to deal with the
> preceding stuff and suggest the conditional.
>
> --
> Fabien.
>

Just realized that '?' means "unknown transactional status" in %x. That
might cause confusion if a person had a prompt of %x%R. Is that enough
reason to pick a different cue?

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

[Fabien COELHO]

Ok, so that's not just PROMPT_READY, that's every prompt...which might be
ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
level always being '.'?


Yep. The idea is to keep it short, but to still have something to say 
"there are more levels" in the stack, hence the one dot. Basically I just 
compressed your 4 level proposal, and added a separator to deal with the 
preceding stuff and suggest the conditional.


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

[Corey Huinker]

>
>   calvin=> \if true
>
  calvin?t=> SELECT 1 +
>   calvin?t->   2;
> 3
>   calvin?t=> \if true
>   calvin?t=>   \echo hello
> hello
>   calvin?t=> \endif
>   calvin?t=> \else
>   calvin?z=>   \echo ignored
>   calvin?t=> \endif
>   calvin=>
>

Ok, so that's not just PROMPT_READY, that's every prompt...which might be
ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
level always being '.'? I'll give that a shot.

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

[Fabien COELHO]

Hello,

I'm looking forward to the doc update.

My 0.02€ about prompting:


Given that there is no more barking, then having some prompt indication
that the code is inside a conditional branch becomes more important, so
ISTM that there should be some plan to add it.


Yeah, prompting just got more important. I see a few ways to go about this:

1. Add a new prompt type, either %T for true (heh, pun) or %Y for
branching. It would print a string of chained 't' (branch is true), 'f'
(branch is false), 'z' (branch already had its true section). The depth
traversal would have a limit, say 3 levels deep, and if the tree goes more
than that deep, then '...' would be printed in the stead of any deeper
values. So the prompt would change through a  session like:

command   prompt is now
---   ---
\echo bob ''   = initial state, no branch going on at all
\if yes   't' = inside a true branch
\if no'tf' = false inside a true
\endif't' = back to just the true branch
\if yes   'tt'
\if yes   'ttt'
\if yes   '...ttt' = only show the last 3, but let it be known that
there's at least one more'
\else '...ttz' = past the point of a true bit of this branch


I like the "tfz" idea. I'm not sure whether the up to 6 characters is a 
good, though.



2. The printing of #1 could be integrated into %R only in PROMPT_READY
cases, either prepended or appended to the !/=/^, possibly separated by a :


Hmmm. Logically I would say prepend, but the default prompt is with the 
dbname, which is mostly letters, so it means adding a separator as well.



3. Like #2, but prepended/appended in all circumstances


I would say yes.


4. Keep %T (or %Y), and reflect the state of pset.active_branch within %R,
a single t/f/z


Yep, but with a separator?


5. Like #4, but also printing the if-stack depth if > 1


Hmmm, not sure...

Based on the your ideas above, I would suggest the following:

  calvin=> \if true
  calvin?t=> SELECT 1 +
  calvin?t->   2;
3
  calvin?t=> \if true
  calvin?t=>   \echo hello
hello
  calvin?t=> \endif
  calvin?t=> \else
  calvin?z=>   \echo ignored
  calvin?t=> \endif
  calvin=>

Or maybe use "?.t" for the nested if...

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

[Corey Huinker]

>
> Shouldn't there be some documentation changes to reflect the behavior on
> errors? A precise paragraph about that would be welcome, IMHO.
>

Oddly enough, the documentation I wrote hadn't addressed invalid booleans,
only the error messages did that.

The new behavior certainly warrants a mention, and I'll add that.


> Given that there is no more barking, then having some prompt indication
> that the code is inside a conditional branch becomes more important, so
> ISTM that there should be some plan to add it.


Yeah, prompting just got more important. I see a few ways to go about this:

1. Add a new prompt type, either %T for true (heh, pun) or %Y for
branching. It would print a string of chained 't' (branch is true), 'f'
(branch is false), 'z' (branch already had its true section). The depth
traversal would have a limit, say 3 levels deep, and if the tree goes more
than that deep, then '...' would be printed in the stead of any deeper
values. So the prompt would change through a  session like:

command   prompt is now
---   ---
\echo bob ''   = initial state, no branch going on at all
\if yes   't' = inside a true branch
\if no'tf' = false inside a true
\endif't' = back to just the true branch
\if yes   'tt'
\if yes   'ttt'
\if yes   '...ttt' = only show the last 3, but let it be known that
there's at least one more'
\else '...ttz' = past the point of a true bit of this branch

2. The printing of #1 could be integrated into %R only in PROMPT_READY
cases, either prepended or appended to the !/=/^, possibly separated by a :
3. Like #2, but prepended/appended in all circumstances
4. Keep %T (or %Y), and reflect the state of pset.active_branch within %R,
a single t/f/z
5. Like #4, but also printing the if-stack depth if > 1

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

[Fabien COELHO]

Hello Corey,


Changes in this patch:

- invalid boolean expression on \if or \elif is treated as if the script 
had a bad \command, so it either stops the script (ON_ERROR_STOP, script 
mode), or just gives the ParseVariableBool error and continues.


- All interactive "barks" removed except for [...]

- remaining error messages are tersed: [...]


Patch applies, make check ok, psql tap test ok.

Yep. At least the code is significantly simpler.

There is a useless space on one end of line in the perl script.

Shouldn't there be some documentation changes to reflect the behavior on 
errors? A precise paragraph about that would be welcome, IMHO.


In particular, I suggest that given the somehow more risky "ignore and 
keep going whatever" behavior after a syntax error on if in a script, 
there should be some advice that on_error_stop should better be activated 
in scripts which use \if.


Given that there is no more barking, then having some prompt indication 
that the code is inside a conditional branch becomes more important, so 
ISTM that there should be some plan to add it.


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

[Corey Huinker]

On Thu, Feb 9, 2017 at 4:43 PM, Erik Rijkers  wrote:

> On 2017-02-09 22:15, Tom Lane wrote:
>
>> Corey Huinker  writes:
>>
>
> The feature now  ( at patch v10) lets you break off with Ctrl-C anywhere.
> I like it now much more.
>
> The main thing I still dislike somewhat about the patch is the verbose
> output. To be honest I would prefer to just remove /all/ the interactive
> output.
>
> I would vote to just make it remain silent if there is no error.   (and if
> there is an error, issue a message and exit)
>
> thanks,
>
> Erik Rijkers
>

Changes in this patch:
- invalid boolean expression on \if or \elif is treated as if the script
had a bad \command, so it either stops the script (ON_ERROR_STOP, script
mode), or just gives the ParseVariableBool error and continues.

- All interactive "barks" removed except for
"command ignored. use \endif or Ctrl-C to exit current branch" when the
user types a non-branching \command in a false branch
"query ignored. use \endif or Ctrl-C to exit current branch" when the
user types a non-branching \command in a false branch
"\if: escaped" when a user does press Ctrl-C and they escape a branch.

- remaining error messages are tersed:
 \elif: cannot occur after \else
 \elif: no matching \if
 \else: cannot occur after \else
 \else: no matching \if
 \endif: no matching \if
 found EOF before closing \endif(s)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..390bccd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ 

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

[Erik Rijkers]

On 2017-02-09 22:15, Tom Lane wrote:

Corey Huinker  writes:


The feature now  ( at patch v10) lets you break off with Ctrl-C 
anywhere.  I like it now much more.


The main thing I still dislike somewhat about the patch is the verbose 
output. To be honest I would prefer to just remove /all/ the interactive 
output.


I would vote to just make it remain silent if there is no error.   (and 
if there is an error, issue a message and exit)


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

[Robert Haas]

On Thu, Feb 9, 2017 at 4:15 PM, Tom Lane  wrote:
> Basically, I think you need to start removing complexity (in the sense of
> special cases), not adding more.  I think Robert was saying the same
> thing, though possibly I shouldn't put words in his mouth.

Yeah, I was definitely going in that direction, whatever the details.

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

[Tom Lane]

Corey Huinker  writes:
> On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane  wrote:
>> IMO, an erroneous backslash command should have no effect, period.

> One way around this is to make the small change: commands with invalid
> expressions are ignored in interactive mode.

> Another way around it would be to ignore branching commands in interactive
> mode altogether and give a message like "branching commands not supported
> in interactive mode".

Uh, neither of those seem to be responding to my point.  There is no case
in psql where a command with an invalid argument does something beyond
throwing an error.  I do not think that \if is the place to start.

Having it act differently in interactive and noninteractive modes is an
even worse idea.  AFAICS, the only real value of using \if interactively
is to test out something you are about to copy into a script.  If we go
that route we're destroying the ability to test that way.

Basically, I think you need to start removing complexity (in the sense of
special cases), not adding more.  I think Robert was saying the same
thing, though possibly I shouldn't put words in his mouth.

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

[Corey Huinker]

On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > I (still) think this is a bad design.  Even if you've got all the
> > messages just right as things stand today, some new feature that comes
> > along in the future can change things so that they're not right any
> > more, and nobody's going to relish maintaining this.
>
> FWIW, I tend to agree that this is way overboard in terms of the amount of
> complexity going into the messages.  Also, I do not like what seems to
> be happening here:
>
> >> $ psql test < test2.sql -v ON_ERROR_STOP=0
> >> unrecognized value "error" for "\if ": boolean expected
> >> new \if is invalid, ignoring commands until next \endif
>
> 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.
>
> regards, tom lane
>

One way around this is to make the small change: commands with invalid
expressions are ignored in interactive mode.

Another way around it would be to ignore branching commands in interactive
mode altogether and give a message like "branching commands not supported
in interactive mode". That'd get rid of a lot of complexity right there. I
for one wouldn't miss it. The only use I saw for it was debugging a script,
and in that case the user can be their own branching via selective
copy/paste.

Do either of those sound appealing?

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

[Tom Lane]

Robert Haas  writes:
> I (still) think this is a bad design.  Even if you've got all the
> messages just right as things stand today, some new feature that comes
> along in the future can change things so that they're not right any
> more, and nobody's going to relish maintaining this.

FWIW, I tend to agree that this is way overboard in terms of the amount of
complexity going into the messages.  Also, I do not like what seems to
be happening here:

>> $ psql test < test2.sql -v ON_ERROR_STOP=0
>> unrecognized value "error" for "\if ": boolean expected
>> new \if is invalid, ignoring commands until next \endif

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.

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

[Robert Haas]

On Mon, Feb 6, 2017 at 5:49 PM, Corey Huinker  wrote:
> I suppressed the endif-balance checking in cases where we're in an
> already-failed situation.
> In cases where there was a boolean parsing failure, and ON_ERROR_STOP is on,
> the error message no longer speak of a future which the session does not
> have. I could left the ParseVariableBool() message as the only one, but
> wasn't sure that that was enough of an error message on its own.
> Added the test case to the existing tap tests. Incidentally, the tap tests
> aren't presently fooled into thinking they're interactive.
>
> $ cat test2.sql
> \if error
> \echo NO
> \endif
> \echo NOPE
> $ psql test < test2.sql -v ON_ERROR_STOP=0
> unrecognized value "error" for "\if ": boolean expected
> new \if is invalid, ignoring commands until next \endif
> NOPE
> $ psql test < test2.sql -v ON_ERROR_STOP=1
> unrecognized value "error" for "\if ": boolean expected
> new \if is invalid.

I (still) think this is a bad design.  Even if you've got all the
messages just right as things stand today, some new feature that comes
along in the future can change things so that they're not right any
more, and nobody's going to relish maintaining this.  This sort of
thing seems fine to me:

new \if is invalid

But then further breaking it down by things like whether
ON_ERROR_STOP=1 is set, or breaking down the \endif output depending
on the surrounding context, seems terrifyingly complex to me.

Mind you, I'm not planning to commit this patch anyway, so feel free
to ignore me, but if I were planning to commit it, I would not commit
it with that level of chattiness.

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

[Fabien COELHO]

This was my previous understanding of ON_ERROR_STOP. Somewhere in the
course of developing this patch I lost that. Glad to have it back.

The only changes I made were to invalid booleans on if/elif, and the final
branch balancing check won't set status to EXIT_USER unless it's
non-interactive and ON_ERROR_STOP = on.


About v10: Patch applies, make check ok, psql tap test ok. Html doc 
generation ok.


Everything looks ok to me.

Interactive tests behave as expected, especially ctrl-C and with 
on_error_stop=1.


ISTM that everything has been addressed.

I've switched the patch to "Ready for Committers", let's what happens on 
their side...


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

[Corey Huinker]

>
>
> It seems that ON_ERROR_STOP is mostly ignored by design when in
> interactive mode, probably because it is nicer not to disconnect the user
> who is actually typing things on a terminal.
>
> """
>   ON_ERROR_STOP
>
> By default, command processing continues after an error. When this
> variable is set to on, processing will instead stop immediately. In
> interactive mode, psql will return to the command prompt; otherwise, psql
> will exit, returning error code 3 to distinguish this case from fatal error
> conditions, which are reported using error code 1.
> """
>

This was my previous understanding of ON_ERROR_STOP. Somewhere in the
course of developing this patch I lost that. Glad to have it back.

The only changes I made were to invalid booleans on if/elif, and the final
branch balancing check won't set status to EXIT_USER unless it's
non-interactive and ON_ERROR_STOP = on.

> \if true
new \if is true, executing commands
> \endif
exited \if, executing commands
> \if false
new \if is false, ignoring commands until next \elif, \else, or \endif
> \endif
exited \if, executing commands
> \if error
unrecognized value "error" for "\if ": boolean expected
new \if is invalid, ignoring commands until next \endif
> \echo foo
inside inactive branch, command ignored.
> ^C
escaped \if, executing commands
> \echo foo
foo
> \endif
encountered un-matched \endif
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..63ddf0a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read 

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

[Fabien COELHO]

Hello Corey,


In cases where there was a boolean parsing failure, and ON_ERROR_STOP is
on, the error message no longer speak of a future which the session does
not have. I could left the ParseVariableBool() message as the only one, but
wasn't sure that that was enough of an error message on its own.
Added the test case to the existing tap tests. Incidentally, the tap tests
aren't presently fooled into thinking they're interactive.


Yes.


Revised cumulative patch attached for those playing along at home.


Nearly there...

It seems that ON_ERROR_STOP is mostly ignored by design when in 
interactive mode, probably because it is nicer not to disconnect the user 
who is actually typing things on a terminal.


"""
  ON_ERROR_STOP

By default, command processing continues after an error. When this 
variable is set to on, processing will instead stop immediately. In 
interactive mode, psql will return to the command prompt; otherwise, psql 
will exit, returning error code 3 to distinguish this case from fatal 
error conditions, which are reported using error code 1.

"""

So, you must check for interactive as well when shortening the message, 
and adapting it accordingly, otherwise on gets the wrong message in 
interactive mode:


   bash> ./psql -v ON_ERROR_STOP=1
   psql (10devel, server 9.6.1)
   Type "help" for help.

   calvin=# \if error
   unrecognized value "error" for "\if ": boolean expected
   new \if is invalid.
   calvin=# -- not stopped, but the stack has been cleaned up, I think

Basically it seems that there are 4 cases and 2 behaviors:

 - on_error_stop && scripting:
 actually exit on error

 - on_error_stop && interactive, !on_error_stop whether scripting or not:
 keep going, possibly with nesting checks?

The problem is that currently interactive behavior cannot be tested 
automatically.


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

[Corey Huinker]

On Mon, Feb 6, 2017 at 3:43 PM, Corey Huinker 
wrote:

> I did some more tests. I found a subtlety that I missed before: when
>> running under ON_ERROR_STOP, messages are not fully consistent:
>>
>>  sh> cat test.sql
>>  \set ON_ERROR_STOP on
>>  \if error
>>\echo NO
>>  \endif
>>  \echo NO
>>
>>  sh> ./psql < test.sql
>>  SET
>>  # ok
>>  unrecognized value "error" for "\if ": boolean expected
>>  # ok
>
>
> That's straight from ParseVariableBool, and we can keep that or suppress
> it. Whatever we do, we should do it with the notion that more complex
> expressions will eventually be allowed, but they'll still have to resolve
> to something that's a text boolean.
>
>
>>
>>  new \if is invalid, ignoring commands until next \endif
>>  # hmmm... but it does not, it is really stopping immediately...
>
>  found EOF before closing \endif(s)
>>  # no, it has just stopped before EOF because of the error...
>>
>
> Yeah, chattiness caught up to us here. Both of these messages can be
> suppressed, I think.
>
>
>>
>> Also I'm not quite sure why psql decided that it is in interactive mode
>> above, its stdin is a file, but why not.
>>
>> The issue is made more explicit with -f:
>>
>>  sh> ./psql -f test.sql
>>  SET
>>  psql:test.sql:2: unrecognized value "error" for "\if ": boolean
>> expected
>>  psql:test.sql:2: new \if is invalid, ignoring commands until next \endif
>>  psql:test.sql:2: found EOF before closing \endif(s)
>>
>> It stopped on line 2, which is expected, but it was not on EOF.
>>
>> I think that the message when stopping should be ", stopping as required
>> by ON_ERROR_STOP" or something like that instead of ", ignoring...", and
>> the EOF message should not be printed at all in this case.
>
>
> I agree, and will look into making that happen. Thanks for the test case.
>
>

I suppressed the endif-balance checking in cases where we're in an
already-failed situation.
In cases where there was a boolean parsing failure, and ON_ERROR_STOP is
on, the error message no longer speak of a future which the session does
not have. I could left the ParseVariableBool() message as the only one, but
wasn't sure that that was enough of an error message on its own.
Added the test case to the existing tap tests. Incidentally, the tap tests
aren't presently fooled into thinking they're interactive.

$ cat test2.sql
\if error
\echo NO
\endif
\echo NOPE
$ psql test < test2.sql -v ON_ERROR_STOP=0
unrecognized value "error" for "\if ": boolean expected
new \if is invalid, ignoring commands until next \endif
NOPE
$ psql test < test2.sql -v ON_ERROR_STOP=1
unrecognized value "error" for "\if ": boolean expected
new \if is invalid.
$ psql test -f test2.sql -v ON_ERROR_STOP=0
psql:test2.sql:1: unrecognized value "error" for "\if ": boolean
expected
psql:test2.sql:1: new \if is invalid, ignoring commands until next \endif
NOPE
$ psql test -f test2.sql -v ON_ERROR_STOP=1
psql:test2.sql:1: unrecognized value "error" for "\if ": boolean
expected
psql:test2.sql:1: new \if is invalid.


Revised cumulative patch attached for those playing along at home.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+

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

[Corey Huinker]

>
> I did some more tests. I found a subtlety that I missed before: when
> running under ON_ERROR_STOP, messages are not fully consistent:
>
>  sh> cat test.sql
>  \set ON_ERROR_STOP on
>  \if error
>\echo NO
>  \endif
>  \echo NO
>
>  sh> ./psql < test.sql
>  SET
>  # ok
>  unrecognized value "error" for "\if ": boolean expected
>  # ok


That's straight from ParseVariableBool, and we can keep that or suppress
it. Whatever we do, we should do it with the notion that more complex
expressions will eventually be allowed, but they'll still have to resolve
to something that's a text boolean.


>
>  new \if is invalid, ignoring commands until next \endif
>  # hmmm... but it does not, it is really stopping immediately...

 found EOF before closing \endif(s)
>  # no, it has just stopped before EOF because of the error...
>

Yeah, chattiness caught up to us here. Both of these messages can be
suppressed, I think.


>
> Also I'm not quite sure why psql decided that it is in interactive mode
> above, its stdin is a file, but why not.
>
> The issue is made more explicit with -f:
>
>  sh> ./psql -f test.sql
>  SET
>  psql:test.sql:2: unrecognized value "error" for "\if ": boolean
> expected
>  psql:test.sql:2: new \if is invalid, ignoring commands until next \endif
>  psql:test.sql:2: found EOF before closing \endif(s)
>
> It stopped on line 2, which is expected, but it was not on EOF.
>
> I think that the message when stopping should be ", stopping as required
> by ON_ERROR_STOP" or something like that instead of ", ignoring...", and
> the EOF message should not be printed at all in this case.


I agree, and will look into making that happen. Thanks for the test case.

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

[Fabien COELHO]

Consolidated Fabien's TAP test additions with v7, in case anyone else wants
to be reviewing.


Patch applies (with "patch"), make check ok, psql tap test ok.

I did some more tests. I found a subtlety that I missed before: when 
running under ON_ERROR_STOP, messages are not fully consistent:


 sh> cat test.sql
 \set ON_ERROR_STOP on
 \if error
   \echo NO
 \endif
 \echo NO

 sh> ./psql < test.sql
 SET
 # ok
 unrecognized value "error" for "\if ": boolean expected
 # ok
 new \if is invalid, ignoring commands until next \endif
 # hmmm... but it does not, it is really stopping immediately...
 found EOF before closing \endif(s)
 # no, it has just stopped before EOF because of the error...

Also I'm not quite sure why psql decided that it is in interactive mode 
above, its stdin is a file, but why not.


The issue is made more explicit with -f:

 sh> ./psql -f test.sql
 SET
 psql:test.sql:2: unrecognized value "error" for "\if ": boolean expected
 psql:test.sql:2: new \if is invalid, ignoring commands until next \endif
 psql:test.sql:2: found EOF before closing \endif(s)

It stopped on line 2, which is expected, but it was not on EOF.

I think that the message when stopping should be ", stopping as required 
by ON_ERROR_STOP" or something like that instead of ", ignoring...", and 
the EOF message should not be printed at all in this case.


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

[Corey Huinker]

On Mon, Feb 6, 2017 at 2:32 PM, Fabien COELHO  wrote:

>
> Do you think the TAP tests would benefit from having the input described
>> in a q(...) block rather than a string?
>>
>
> My 0.02€: Not really, so I would not bother. It breaks perl indentation
> and logic for a limited benefit. Maybe add comments if you feel that a test
> case is unclear.
>
> --
> Fabien.


Consolidated Fabien's TAP test additions with v7, in case anyone else wants
to be reviewing.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..4a3e471 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool
+read_boolean_expression(PsqlScanState scan_state, char *action,
+   bool *result)
+{
+   char*value = psql_scan_slash_option(scan_state,
+   
OT_NORMAL, NULL, false);
+   boolsuccess = ParseVariableBool(value, action, result);
+   free(value);
+   return success;
+}
+
+/*
+ * Return true if the command given is a branching command.
+ */
+static bool
+is_branching_command(const char *cmd)
+{
+   return (strcmp(cmd, "if") == 0 || strcmp(cmd, "elif") == 0
+   || strcmp(cmd, "else") == 0 || strcmp(cmd, "endif") == 
0);
+}
 
 /*
  * Subroutine to actually try to execute a backslash 

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

[Fabien COELHO]

Do you think the TAP tests would benefit from having the input described 
in a q(...) block rather than a string?


My 0.02€: Not really, so I would not bother. It breaks perl indentation 
and logic for a limited benefit. Maybe add comments if you feel that a 
test case is unclear.


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

[Corey Huinker]

On Mon, Feb 6, 2017 at 11:21 AM, Corey Huinker 
wrote:

>
>> Find attached a small patch to improve tap tests, which also checks that
>>> psql really exited by checking that nothing is printed afterwards.
>>>
>>
>>
> Do you think the TAP tests would benefit from having the input described
> in a q(...) block rather than a string?
>
> q(\if false
> \echo a
> \elif invalid
> \echo b
> \endif
> \echo c
> )
>
>
> It's a lot more lines, obviously, but it might make what is being tested
> clearer.
>
>
It occurred to me that the part of this patch most important to casual
users would be the printed messages at various states. I've enumerated
those below, along with the circumstances under which the user would see
them.

The following messages are for interactive and script users. They are also
errors which respect ON_ERROR_STOP.
---

\if statement which had an invalid boolean expression:

new \if is invalid, ignoring commands until next \endif


\elif was in a proper \if block, and not after the true block, but boolean
expression was invalid:

\elif is invalid, ignoring commands until next \endif


\elif statement after an \else

encountered \elif after \else


\elif statement outside of an \if block [*]

encountered un-matched \elif


\else outside of an \if

encountered un-matched \else


\else after an \else

encountered \else after \else


\endif statement outside of an \if block

encountered un-matched \endif


Input file ends with unresolved \if blocks

found EOF before closing \endif(s)


The following are interactive-only non-error informational messages.
-

\if statement which parsed to true:

new \if is true, executing commands


\if statement which parsed to false:

new \if is false, ignoring commands until next \elif, \else, or \endif

\if statement while already in a false/invalid block:

new \if is inside ignored block, ignoring commands until next \endif


\elif statement immediately after the true \if or \elif

\elif is after true block, ignoring commands until next \endif


\elif statement within a false block or subsequent elif after the first
ignored elif

\elif is inside ignored block, ignoring commands until next \endif


\elif was evaluated, was true

\elif is true, executing commands


\elif was evaluated, was false

\elif is false, ignoring commands until next \elif, \else, or \endif


\else statement in an ignored block or after the true block was found:

\else after true condition or in ignored block, ignoring commands until
next \endif


\else statement and all previous blocks were false

\else after all previous conditions false, executing commands


\endif statement ending only \if on the stack

exited \if, executing commands


\endif statement where last block was false but parent block is also false:

exited \\if to false parent branch, ignoring commands until next \endif


\endif statement where last block was true and parent is true

exited \\if to true parent branch, continuing executing commands


\endif statement where last block was false but parent is true

exited \\if to true parent branch, resuming executing commands


Script is currently in a false (or invalid) branch, and user entered a
command other than if/elif/endif:

inside inactive branch, command ignored.


Script currently in a false branch, and user entered a query:

inside inactive branch, query ignored. use \endif to exit current branch.


User in an \if branch and pressed ^C, with no more branches remaining:

escaped \\if, executing commands


User in an \if branch and pressed ^C, but parent branch was false:

escaped \\if to false parent branch, ignoring commands until next \endif


User in a true \if branch and pressed ^C, parent branch true

escaped \\if to true parent branch, continuing executing commands


User in a false \if branch and pressed ^C, parent branch true

escaped \if to true parent branch, resuming executing commands



Notes:


The text for ignored commands vs ignored queries is different.

The text for all the Ctrl-C messages re-uses the \endif messages, but are
"escaped" instead of "exited".

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

[Corey Huinker]

>
>
> Find attached a small patch to improve tap tests, which also checks that
>> psql really exited by checking that nothing is printed afterwards.
>>
>
>
Do you think the TAP tests would benefit from having the input described in
a q(...) block rather than a string?

q(\if false
\echo a
\elif invalid
\echo b
\endif
\echo c
)


It's a lot more lines, obviously, but it might make what is being tested
clearer.

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

[Fabien COELHO]

Find attached a small patch to improve tap tests, which also checks that psql 
really exited by checking that nothing is printed afterwards.


. It is better with the attachement.

--
Fabien.diff --git a/src/bin/psql/t/001_if.pl b/src/bin/psql/t/001_if.pl
index 68c9b15..a703cab 100644
--- a/src/bin/psql/t/001_if.pl
+++ b/src/bin/psql/t/001_if.pl
@@ -4,7 +4,7 @@ use warnings;
 use Config;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 15;
+use Test::More tests => 18;
 
 #
 # test invalid \if respects ON_ERROR_STOP
@@ -14,12 +14,14 @@ $node->init;
 $node->start;
 
 my $tests = [
-	[ "\\if invalid_expression\n\\endif\n", '', 'boolean expected', 'syntax error' ],
+# syntax errors
+	[ "\\if invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', 'syntax error' ],
+	[ "\\if false\n\\elif invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', 'syntax error' ],
 	# unmatched checks
-	[ "\\if true\\n", '', 'found EOF before closing.*endif', 'unmatched \if' ],
-	[ "\\elif true\\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ],
-	[ "\\else\\n", '', 'encountered un-matched.*else', 'unmatched \else' ],
-	[ "\\endif\\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ],
+	[ "\\if true\n", '', 'found EOF before closing.*endif', 'unmatched \if' ],
+	[ "\\elif true\n\\echo NO\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ],
+	[ "\\else\n\\echo NO\n", '', 'encountered un-matched.*else', 'unmatched \else' ],
+	[ "\\endif\n\\echo NO\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ],
 ];
 
 # 3 checks per tests
@@ -29,7 +31,7 @@ for my $test (@$tests) {
   my $retcode = $node->psql('postgres', $script,
 		stdout => \$stdout, stderr => \$stderr,
 		on_error_stop => 1);
-  is($retcode,'3',"$name test respects ON_ERROR_STOP");
+  is($retcode,'3',"$name test ON_ERROR_STOP");
   is($stdout, $stdout_expect, "$name test STDOUT");
   like($stderr, qr/$stderr_re/, "$name test STDERR");
 }

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

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

[Fabien COELHO]

  # elif error
  "\\if false\n\\elif error\n\\endif\n"

  # ignore commands on error (stdout must be empty)
  "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n"


Those are already in the regression (around line 2763 of
expected/psql.out), are you saying we should have them in TAP as well?
Should we only do TAP tests?


Ok. so, maybe just the first one. The idea would be to cover more cases of 
on error stop and check that it indeed stopped.


Find attached a small patch to improve tap tests, which also checks that 
psql really exited by checking that nothing is printed afterwards.


Also, for some reason there were \\n instead of \n in some place, it was 
working because the first command induced the error.



Anyway, here's the Ctrl-C behavior:


Ok. Basically it moves up each time Ctrl-C is called. Fine.

The future improvement would be to do that if the current input line was 
empty, otherwise only the current input line would be cleaned up.



Ctrl-C exits do the same before/after state checks that \endif does, the
lone difference being that it "escaped" the \if rather than "exited" the
\if. Thanks to Daniel for pointing out where it should be handled, because
I wasn't going to figure that out on my own.

v7's only major difference from v6 is the Ctrl-C branch escaping.


Ok. Bar from minor tests improvements, this looks pretty much ok to 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Hmmm. ISTM that control-c must at least reset the stack, otherwise their
> is not easy way to get out. What can be left to another patch is doing a
> control-C for contents and then another one for the stack when there is no
> content.
>

And so it shall be.


> - put Fabien's tap test in place verbatim
>>
>
> Hmmm. That was really just a POC... I would suggest some more tests, eg:
>
>   # elif error
>   "\\if false\n\\elif error\n\\endif\n"
>
>   # ignore commands on error (stdout must be empty)
>   "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n"
>

Those are already in the regression (around line 2763 of
expected/psql.out), are you saying we should have them in TAP as well?
Should we only do TAP tests?

Anyway, here's the Ctrl-C behavior:

# \if true
new \if is true, executing commands
# \echo msg
msg
# ^C
escaped \if, executing commands
# \if false
new \if is false, ignoring commands until next \elif, \else, or \endif
# \echo msg
inside inactive branch, command ignored.
# ^C
escaped \if, executing commands
# \echo msg
msg
# \endif
encountered un-matched \endif
#


Ctrl-C exits do the same before/after state checks that \endif does, the
lone difference being that it "escaped" the \if rather than "exited" the
\if. Thanks to Daniel for pointing out where it should be handled, because
I wasn't going to figure that out on my own.

v7's only major difference from v6 is the Ctrl-C branch escaping.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..4a3e471 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState 

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

[Fabien COELHO]

Hello Corey,


The check I was suggesting on whether Ctrl+C has been pressed

on an empty line seems harder to implement, because get_interactive()
just calls readline() or fgets(), which block to return when a whole
line is ready. AFAICS psql can't know what was the edit-in-progress
when these functions are interrupted by a signal instead of
returning normally.
But I don't think this check is essential, it could be left to another
patch.


Glad I wasn't missing something obvious.
I suppose we could base the behavior on whether there's at least one full
line already buffered.
However, I agree that it can be left to another patch.


Hmmm. ISTM that control-c must at least reset the stack, otherwise their 
is not easy way to get out. What can be left to another patch is doing a 
control-C for contents and then another one for the stack when there is no 
content.


Comments about v6:


- error messages are now a bit more terse, following suggestions


Ok.


- help text is more terse and Conditionals section was moved below Input
Output


Ok.


- leverage IFSTATE_NONE a bit to fold some not-in-a-branch cases into
existing switch statements, giving flatter, slightly cleaner code and that
addresses expected cases before exceptional ones


Code looks ok.


- comments highlight which messages are printed in both interactive and
script mode.


Yep.


- put Fabien's tap test in place verbatim


Hmmm. That was really just a POC... I would suggest some more tests, eg:

  # elif error
  "\\if false\n\\elif error\n\\endif\n"

  # ignore commands on error (stdout must be empty)
  "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n"

Also there is an empty line before the closing } of the while loop.


- No mention of Ctrl-C or PROMPT. Those can be addressed in separate
patches.


I think that Ctrl-C resetting the stack must be addressed in this patch. 
Trying to be more intelligent/incremental on Ctrl-C can wait for another 
time.



There's probably some more consensus building to do over the interactive
messages and comments,


Barking is now quite more verbose (?), but at least it is clear about the 
status and what is expected. I noticed that it is now always on, whether 
an error occured or not, which is ok with me.


and if interactive-ish tests are possible with TAP, we should add those 
too.


Good point. It seems that it is decided based on "source == stdin" plus 
checking whether both stdin/stdout are on terminal. Allowing to work 
around the later requires some more infrastructure to force "notty" (yuk, 
a negative variable tested negatively...) to false whatever, which does 
not seem to exist. So this is for another 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Feb 4, 2017 at 11:53 AM, Corey Huinker 
wrote:

> The check I was suggesting on whether Ctrl+C has been pressed
>> on an empty line seems harder to implement, because get_interactive()
>> just calls readline() or fgets(), which block to return when a whole
>> line is ready. AFAICS psql can't know what was the edit-in-progress
>> when these functions are interrupted by a signal instead of
>> returning normally.
>> But I don't think this check is essential, it could be left to another
>> patch.
>>
>
> Glad I wasn't missing something obvious.
> I suppose we could base the behavior on whether there's at least one full
> line already buffered.
> However, I agree that it can be left to another patch.
>

v6 patch. highlights:
- error messages are now a bit more terse, following suggestions
- help text is more terse and Conditionals section was moved below Input
Output
- leverage IFSTATE_NONE a bit to fold some not-in-a-branch cases into
existing switch statements, giving flatter, slightly cleaner code and that
addresses expected cases before exceptional ones
- comments highlight which messages are printed in both interactive and
script mode.
- put Fabien's tap test in place verbatim
- No mention of Ctrl-C or PROMPT. Those can be addressed in separate
patches.

There's probably some more consensus building to do over the interactive
messages and comments, and if interactive-ish tests are possible with TAP,
we should add those too.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..9058897 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ 

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

[Corey Huinker]

>
> The check I was suggesting on whether Ctrl+C has been pressed
> on an empty line seems harder to implement, because get_interactive()
> just calls readline() or fgets(), which block to return when a whole
> line is ready. AFAICS psql can't know what was the edit-in-progress
> when these functions are interrupted by a signal instead of
> returning normally.
> But I don't think this check is essential, it could be left to another
> patch.
>

Glad I wasn't missing something obvious.
I suppose we could base the behavior on whether there's at least one full
line already buffered.
However, I agree that it can be left to another patch.

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

[Corey Huinker]

>
> I noticed that the "barking" is conditional to "success". ISTM that it
> should always "bark" in interactive mode, whether success or not.
>

"success" in those cases means "the expression was a valid boolean", and
non-success cases (should) result in an error being printed regardless of
interactive mode. If you see otherwise, let me know.


>
> While testing it and seeing the code, I agree that it is too
> verbose/redundant. At least remove "active/inactive, ".


Have done so, new patch pending "how-do-I-know-when-input-is-empty" in Ctrl
C.


> - Help text. New block in help text called "Conditionals"
>>
>
> Maybe it could be moved to "Input/Output" renamed as "Input/Output
> Control", or maybe the "Conditionals" section could be moved next to it, it
> seems more logical than after large objects.
>

I put it near the bottom, figuring someone would have a better idea of
where to put it. You did.



> I think that the descriptions are too long. The interactive user can be
> trusted to know what "if/elif/else/endif" mean, or to refer to the full
> documentation otherwise. The point is just to provide a syntax and function
> reminder, not a substitute for the doc. Thus I would suggest shorter
> one-line messages like:
>
>  \if begin a new conditional block
>  \elif   else if in the current conditional block
>  \else else in current conditional block
>  \endifend current conditional block
>

+1



>
>>
> Hmmm. Perl is perl. Attached an attempt at improving that, which is
> probably debatable, but at least it is easy to add further tests without
> massive copy-pasting.


+1 that's a good start.


>
>
> - regression tests now have comments to explain purpose
>>
>
> Ok.
>
> Small details about the code:
>
>   +   if (!pset.active_branch && !is_branching_command(cmd) )
>
> Not sure why there is a space before the last closing parenthesis.


+1

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

[Daniel Verite]

Corey Huinker wrote:

[about Ctrl-C]

> That does seem to be the consensus desired behavior. I'm just not sure
> where to handle that. The var "cancel_pressed" shows up in a lot of places.
> Advice?

Probably you don't need to care about cancel_pressed, and
the /if stack could be unwound at the point the SIGINT
handler longjumps to, in mainloop.c:

/* got here with longjmp */

/* reset parsing state */
psql_scan_finish(scan_state);
psql_scan_reset(scan_state);
resetPQExpBuffer(query_buf);
resetPQExpBuffer(history_buf);
count_eof = 0;
slashCmdStatus = PSQL_CMD_UNKNOWN;
prompt_status = PROMPT_READY;
pset.stmt_lineno = 1;
cancel_pressed = false;

The check I was suggesting on whether Ctrl+C has been pressed
on an empty line seems harder to implement, because get_interactive()
just calls readline() or fgets(), which block to return when a whole
line is ready. AFAICS psql can't know what was the edit-in-progress
when these functions are interrupted by a signal instead of
returning normally.
But I don't think this check is essential, it could be left to another patch.


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

[Fabien COELHO]

A few comments about v5.


New patch.


Patch applies (with patch, I gave up on "git apply").
Make check ok.
Psql tap test ok.


Highlights:
- Interactive barking on branching state changes, commands typed while in
inactive state


I noticed that the "barking" is conditional to "success". ISTM that it 
should always "bark" in interactive mode, whether success or not.


While testing it and seeing the code, I agree that it is too 
verbose/redundant. At least remove "active/inactive, ".



- Help text. New block in help text called "Conditionals"


Maybe it could be moved to "Input/Output" renamed as "Input/Output 
Control", or maybe the "Conditionals" section could be moved next to it, 
it seems more logical than after large objects.


I think that the descriptions are too long. The interactive user can be 
trusted to know what "if/elif/else/endif" mean, or to refer to the full 
documentation otherwise. The point is just to provide a syntax and 
function reminder, not a substitute for the doc. Thus I would suggest 
shorter one-line messages like:


 \if begin a new conditional block
 \elif   else if in the current conditional block
 \else else in current conditional block
 \endifend current conditional block

There should not be a \n at the end, I think, but just between sections.


- SendQuery calls in mainloop.c are all encapsulated in send_query() to
ensure the same if-active and if-interactive logic is used


Ok.


- Exactly one perl TAP test, testing ON_ERROR_STOP. I predict more will be
needed, but I'm not sure what coverage is desired


More that one:-)


- I also predict that my TAP test style is pathetic


Hmmm. Perl is perl. Attached an attempt at improving that, which is 
probably debatable, but at least it is easy to add further tests without 
massive copy-pasting.



- regression tests now have comments to explain purpose


Ok.

Small details about the code:

  +   if (!pset.active_branch && !is_branching_command(cmd) )

Not sure why there is a space before the last closing parenthesis.

--
Fabien.use strict;
use warnings;

use Config;
use PostgresNode;
use TestLib;
use Test::More tests => 15;

#
# test invalid \if respects ON_ERROR_STOP
#
my $node = get_new_node('master');
$node->init;
$node->start;

my $tests = [
  [ "\\if invalid_expression\n\\endif\n", '', 'boolean expected', 'syntax error' ],
  # unmatched checks
  [ "\\if true\\n", '', 'found EOF before closing.*endif', 'unmatched \if' ],
  [ "\\elif true\\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ],
  [ "\\else\\n", '', 'encountered un-matched.*else', 'unmatched \else' ],
  [ "\\endif\\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ],
];

# 3 checks per tests
for my $test (@$tests) {
  my ($script, $stdout_expect, $stderr_re, $name) = @$test;
  my ($stdout, $stderr);
  my $retcode = $node->psql('postgres', $script,
		stdout => \$stdout, stderr => \$stderr,
		on_error_stop => 1);
  is($retcode,'3',"$name test respects ON_ERROR_STOP");
  is($stdout, $stdout_expect, "$name test STDOUT");
  like($stderr, qr/$stderr_re/, "$name test STDERR");

}

$node->teardown_node;

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

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

[Corey Huinker]

On Fri, Feb 3, 2017 at 7:42 PM, Jim Nasby  wrote:

> Since the current consensus is to be very verbose about \if, this is
> obviously a non-issue. Maybe worth adding a 'I' case to %R, but no big deal
> if that doesn't happen.


I think we left the door open to a separate patch for a prompt change.

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

[Jim Nasby]

On 2/2/17 4:39 PM, Corey Huinker wrote:


On Wed, Feb 1, 2017 at 4:58 PM, Jim Nasby > wrote:

I think the issue here is that the original case for this is a user
accidentally getting into an \if and then having no clue what's
going on. That's similar to what happens when you miss a quote or a
semicolon. We handle those cases with %R, and I think %R needs to
support if as well.

Perhaps there's value to providing more info (active branch, etc),
but ISTM trying to do that will just confuse the original (%R) case.


Jim,

After spending a few minutes to familiarize myself with %R, I'm in
agreement with your second statement (adding if-else to %R will just
confuse %R). However, your first statement seems to indicate the
opposite. Can you elaborate?


My point was that we need a way for users to know if they're stuck in an 
\if block, and right now that's handled with %R (inside transaction, 
parens, etc). My other point is that adding all the extra info to %R 
would be folly.


Since the current consensus is to be very verbose about \if, this is 
obviously a non-issue. Maybe worth adding a 'I' case to %R, but no big 
deal if that doesn't happen.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

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

[Robert Haas]

On Fri, Feb 3, 2017 at 4:24 PM, Corey Huinker  wrote:
> That might be what we end up doing. I'm willing to see how unwieldy it gets
> before rolling back to "endif: peace out".

All I'm saying is, give peace a chance.  :-)

> The state logic has stuff to do anyway, so for the moment I've added
> psql_error() messages at each endpoint. My current (unsubmitted) work has:
>
> if you were in a true branch and leave it  (i.e yes->yes)
>
> +   psql_error("exited \\if to true
> parent branch, \n"
> +   "continuing
> executing commands\n");
>
> if you were in a false branch beneath a true branch and leave it  (no->yes)
>
> +   psql_error("exited \\if to true
> parent branch, \n"
> +   "resuming executing
> commands\n");
>
> And if you were in a branch that was a child of a false branch (no->no):
>
> +   psql_error("exited \\if to false parent
> branch, \n"
> +   "ignoring commands until
> next \\endif\n");
>
> And the (yes->no) is an impossibility, so no message there.
>
> I'm not too concerned about what wording we finally go with, and as the
> coder I realize I'm too close to know the wording that will be most helpful
> to an outsider, so I'm very much trusting others to guide me.

But by far the most likely case is that you are not under another \if
at all, and none of these messages are really apropos for that case.

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

[Corey Huinker]

On Fri, Feb 3, 2017 at 3:49 PM, Robert Haas  wrote:

> On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker 
> wrote:
> > I could bulk up the error message on if/elif like such:
> >
> > if: true, executing commands.
> > if: false, ignoring commands until next \else, \elif, or \endif.
> > if: error, ignoring all commands until next \endif
> > else: true, executing commands
> > else: false, ignoring commands until next \endif
> > else: error, ignoring commands until next \endif
> > endif: now executing commands
> > endif: ignoring commands until next [\else, [\elif [, \endif]]
> >
> > Basically, I'd tailor the message to as closely reflect what is possible
> for
> > the user at this moment.
>
> I think that this is kinda hairy.  When I see "endif: now executing
> commands", my reaction is "oh crap, which commands are you
> executing?".  What you really mean is that future command are expected
> to be executed unless things change (for example, due to another \if
> in the meantime), but somebody might have a different interpretation
> of these messages.
>
> I think that the messages you are proposing for "if" and "else" are
> reasonable, but for "endif" I would just say "endif: exiting if" or
> something like that.  If the user doesn't know to what state they are
> returning, c'est la vie.
>

That might be what we end up doing. I'm willing to see how unwieldy it gets
before rolling back to "endif: peace out".

The state logic has stuff to do anyway, so for the moment I've added
psql_error() messages at each endpoint. My current (unsubmitted) work has:

if you were in a true branch and leave it  (i.e yes->yes)

+   psql_error("exited \\if to true
parent branch, \n"
+   "continuing
executing commands\n");

if you were in a false branch beneath a true branch and leave it  (no->yes)

+   psql_error("exited \\if to true
parent branch, \n"
+   "resuming executing
commands\n");

And if you were in a branch that was a child of a false branch (no->no):

+   psql_error("exited \\if to false parent
branch, \n"
+   "ignoring commands until
next \\endif\n");

And the (yes->no) is an impossibility, so no message there.

I'm not too concerned about what wording we finally go with, and as the
coder I realize I'm too close to know the wording that will be most helpful
to an outsider, so I'm very much trusting others to guide me.

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

[Robert Haas]

On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker  wrote:
> I could bulk up the error message on if/elif like such:
>
> if: true, executing commands.
> if: false, ignoring commands until next \else, \elif, or \endif.
> if: error, ignoring all commands until next \endif
> else: true, executing commands
> else: false, ignoring commands until next \endif
> else: error, ignoring commands until next \endif
> endif: now executing commands
> endif: ignoring commands until next [\else, [\elif [, \endif]]
>
> Basically, I'd tailor the message to as closely reflect what is possible for
> the user at this moment.

I think that this is kinda hairy.  When I see "endif: now executing
commands", my reaction is "oh crap, which commands are you
executing?".  What you really mean is that future command are expected
to be executed unless things change (for example, due to another \if
in the meantime), but somebody might have a different interpretation
of these messages.

I think that the messages you are proposing for "if" and "else" are
reasonable, but for "endif" I would just say "endif: exiting if" or
something like that.  If the user doesn't know to what state they are
returning, c'est la vie.

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

[Corey Huinker]

On Fri, Feb 3, 2017 at 12:20 PM, Daniel Verite 
wrote:

> If we add the functionality that Ctrl+C also exits from branches,
> we could do it like the shell does Ctrl+D for logout, that is it
> logs out only if the input buffer is empty, otherwise it does
> the other functionality bound to this key (normally Delete).
> So if you're in the middle of an edit, the first Ctrl+C will
> cancel the edit and a second one will go back from the /if
>

That does seem to be the consensus desired behavior. I'm just not sure
where to handle that. The var "cancel_pressed" shows up in a lot of places.
Advice?

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

[Daniel Verite]

Corey Huinker wrote:

> I meant in the specific psql-context, does it do anything other
> than (attempt to) terminate sent-but-not-received SQL queries?

It cancels the current edit in the query buffer, including the
case when it spans multiple lines.
If we add the functionality that Ctrl+C also exits from branches,
we could do it like the shell does Ctrl+D for logout, that is it
logs out only if the input buffer is empty, otherwise it does
the other functionality bound to this key (normally Delete).
So if you're in the middle of an edit, the first Ctrl+C will
cancel the edit and a second one will go back from the /if

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

[Tom Lane]

Corey Huinker  writes:
> Well played (again). That one ranks up there with "and don't call me
> Shirley." I meant in the specific psql-context, does it do anything other
> than (attempt to) terminate sent-but-not-received SQL queries?

It also flushes the input buffer.  I think it is probably reasonable to
let it cancel interactive \if state as well.  A useful thought-experiment
is to ask what behavior you'd want if we had metacommand loops ... and
I think the answer there is pretty obvious: you'd want control-C to kill
a loop.

I'm less sure about what it ought to do when control is somewhere in
a script file.  I *think* we have things set up to kill execution of
script files altogether, in which case we have the answer a fortiori.
If not, there's room for 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll
>> start looking into it, as I'm not presently aware of what it is used for.
>>
>
> Not me.
>
> Wikipedia, which holds all the knowledge in the universe, says: "In many
> command-line interface environments, control-C is used to abort the current
> task and regain user control."
>

Well played (again). That one ranks up there with "and don't call me
Shirley." I meant in the specific psql-context, does it do anything other
than (attempt to) terminate sent-but-not-received SQL queries?

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

[Fabien COELHO]

I could bulk up the error message on if/elif like such: [...]


Looks ok to me.


Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll
start looking into it, as I'm not presently aware of what it is used for.


Not me.

Wikipedia, which holds all the knowledge in the universe, says: "In many 
command-line interface environments, control-C is used to abort the 
current task and regain user control."


I agree that it should do that.

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

[Corey Huinker]

On Fri, Feb 3, 2017 at 7:24 AM, Fabien COELHO  wrote:

>
> Hello Erik,
>
> Still, it would be an improvement to be able to break out of an inactive
>> \if-branch with Ctrl-C.
>>
>
> Yes.
>
> '\endif' is too long to type, /and/ you have to know it.
>>
>
> Yep. \if is shorter but has been rejected. Ctrl-C should be the way out.


I could bulk up the error message on if/elif like such:

if: true, executing commands.
if: false, ignoring commands until next \else, \elif, or \endif.
if: error, ignoring all commands until next \endif
else: true, executing commands
else: false, ignoring commands until next \endif
else: error, ignoring commands until next \endif
endif: now executing commands
endif: ignoring commands until next [\else, [\elif [, \endif]]


Basically, I'd tailor the message to as closely reflect what is possible
for the user at this moment.


Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll
start looking into it, as I'm not presently aware of what it is used for.


2. Inside an \if block \q should be given precedence and cause a direct
>> exit of psql (or at the very least exit the if block(s)), as in regular SQL
>> statements (compare: 'select * from t \q' which will immediately exit psql
>> -- this is good. )
>>
>
> One use case if to be able to write "\if ... \q \endif" in scripts. If \q
> is always executed, then the use case is blown. I cannot think of any
> language that would execute anything in a false branch. So Ctrl-C or Ctrl-D
> is the way out, and \if control must really have precedence over its
> contents.
>
> 3. I think the 'barking' is OK because interactive use is certainly not
>> the first use-case.
>> But nonetheless it could be made a bit more terse without losing its
>> function.
>>
>
> [...] It really is a bit too wordy, [...]
>>
>
> Maybe.
>
> (or alternatively, just  mention 'if: active' or 'elif: inactive', etc.,
>> which has the advantage of being shorter)
>>
>
> This last version is too terse I think. The point is that the user
> understands whether their commands are going to be executed or ignored, and
> "active/inactive" is not very clear.
>
> 5. A real bug, I think:
>> #\if asdasd
>> unrecognized value "asdasd" for "\if ": boolean expected
>> # \q;
>> inside inactive branch, command ignored.
>> #
>>
>> That 'unrecognized value' message is fair enough but it is
>> counterintuitive that after an erroneous opening \if-expression, the
>> if-modus should be entered into. ( and now I have to type \endif again... )
>>
>
> Hmmm.
>
> It should tell that it is in an unclosed if and that it is currently
> ignoring commands, so the "barking" is missing.
>

It does need a bespoke bark.

Also, I do not think that implementing a different behavior for interactive
> is a good idea, because then typing directly and reading a file would
> result in different behaviors, which would not help debugging.
>

+1


>
> So, as Tom suggested (I think), the feature is not designed for
> interactive use, so it does not need to be optimized for that purpose,
> although it should be sane enough.


+1

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

[Fabien COELHO]

2. Inside an \if block \q should be given precedence and cause a direct 
exit of psql (or at the very least exit the if block(s)), as in regular SQL 
statements (compare: 'select * from t \q' which will immediately exit psql 
-- this is good. )


One use case if to be able to write "\if ... \q \endif" in scripts. If \q is 
always executed, then the use case is blown. I cannot think of any language 
that would execute anything in a false branch. So Ctrl-C or Ctrl-D is the way 
out, and \if control must really have precedence over its contents.


After giving it some more thoughts, a possible solution could be to have a 
"\exit [status]" which could be ignored (there has been some talk about 
that one), and have \q which is not, but I would find it weird and error 
prone for the user. As already said, \if use-case is not about interactive 
psql.


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

[Fabien COELHO]

Hello Erik,

Still, it would be an improvement to be able to break out of an inactive 
\if-branch with Ctrl-C.


Yes.


'\endif' is too long to type, /and/ you have to know it.


Yep. \if is shorter but has been rejected. Ctrl-C should be the way out.

2. Inside an \if block \q should be given precedence and cause a direct 
exit of psql (or at the very least exit the if block(s)), as in regular 
SQL statements (compare: 'select * from t \q' which will immediately 
exit psql -- this is good. )


One use case if to be able to write "\if ... \q \endif" in scripts. If \q 
is always executed, then the use case is blown. I cannot think of any 
language that would execute anything in a false branch. So Ctrl-C or 
Ctrl-D is the way out, and \if control must really have precedence over 
its contents.


3. I think the 'barking' is OK because interactive use is certainly not the 
first use-case.
But nonetheless it could be made a bit more terse without losing its 
function.



[...] It really is a bit too wordy, [...]


Maybe.

(or alternatively, just  mention 'if: active' or 'elif: inactive', etc., 
which has the advantage of being shorter)


This last version is too terse I think. The point is that the user 
understands whether their commands are going to be executed or ignored, 
and "active/inactive" is not very clear.



5. A real bug, I think:
#\if asdasd
unrecognized value "asdasd" for "\if ": boolean expected
# \q;
inside inactive branch, command ignored.
#

That 'unrecognized value' message is fair enough but it is counterintuitive 
that after an erroneous opening \if-expression, the if-modus should be 
entered into. ( and now I have to type \endif again... )


Hmmm.

It should tell that it is in an unclosed if and that it is 
currently ignoring commands, so the "barking" is missing.


Otherwise that is really the currently desired behavior for scripting use:

  \if syntax-error...
DROP USER foo;
  \elif ...
DROP DATABASE foo;
  \else
...
  \endif

If the "\if" is simply ignored, then it is going to execute everything 
that appears after that, whereas the initial condition failed to be 
checked, and it will proceed to ignore all further \elif and \else in 
passing.


Also, I do not think that implementing a different behavior for 
interactive is a good idea, because then typing directly and reading a 
file would result in different behaviors, which would not help debugging.


So, as Tom suggested (I think), the feature is not designed for 
interactive use, so it does not need to be optimized for that purpose,

although it should be sane enough.

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

[Erik Rijkers]

On 2017-02-03 08:16, Corey Huinker wrote:


0001.if_endif.v5.diff


1. Well, with this amount of interactive output  it is impossible to get 
stuck without knowing :)
This is good. Still, it would be an improvement to be able to break out 
of an inactive \if-branch
with Ctrl-C.  (I noticed that inside an active branch it is already 
possible )

'\endif' is too long to type, /and/ you have to know it.

2. Inside an \if block  \q should be given precedence and cause a direct 
exit of psql (or at the

very least exit the if block(s)), as in regular SQL statements
(compare: 'select * from t  \q'  which will immediately exit psql -- 
this is good. )


3. I think the 'barking' is OK because interactive use is certainly not 
the first use-case.
But nonetheless it could be made a bit more terse without losing its 
function.

The interactive behavior is now:
# \if 1
entered if: active, executing commands
# \elif 0
entered elif: inactive, ignoring commands
# \else
entered else: inactive, ignoring commands
# \endif
exited if: active, executing commands

It really is a bit too wordy, IMHO; I would say, drop all 'entered', 
'active',  and 'inactive' words.

That leaves it plenty clear what's going on.
That would make those lines:
if: executing commands
elif: ignoring commands
else: ignoring commands
exited if
   (or alternatively, just  mention 'if: active' or 'elif: inactive', 
etc., which has the advantage of being shorter)


5. A real bug, I think:
#\if asdasd
unrecognized value "asdasd" for "\if ": boolean expected
# \q;
inside inactive branch, command ignored.
#

That 'unrecognized value' message is fair enough but it is 
counterintuitive that after an erroneous opening \if-expression, the 
if-modus should be entered into. ( and now I have to type \endif 
again... )


6. About the help screen:
There should be an empty line above 'Conditionals' to visually divide it 
from other help items.


The indenting of the new block is incorrect: the lines that start with
 fprintf(output, _("  \\
are indented to the correct level; the other lines are indented 1 place 
too much.


The help text has a few typos (some multiple times):
queires -> queries
exectue -> execute
subsequennt -> subsequent

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

[Corey Huinker]

On Fri, Feb 3, 2017 at 12:57 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> Glad you like the barking. I'm happy to let the prompt issue rest for now,
>> we can always add it later.
>>
>> If we DID want it, however, I don't think it'll be hard to add a special
>> prompt (Thinking %T or %Y because they both look like branches, heh),
>>
>
> Ah!:-) T may stand for Tree, but Y looks a little bit more like branches.
> Maybe Y for Yew.
>

Well played. The %Y prompt can be a separate patch.

New patch. Highlights:
- rebased to master as of ten minutes ago
- Interactive barking on branching state changes, commands typed while in
inactive state
- Help text. New block in help text called "Conditionals"
- SendQuery calls in mainloop.c are all encapsulated in send_query() to
ensure the same if-active and if-interactive logic is used
- Exactly one perl TAP test, testing ON_ERROR_STOP. I predict more will be
needed, but I'm not sure what coverage is desired
- I also predict that my TAP test style is pathetic
- regression tests now have comments to explain purpose
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..dcf567e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool
+read_boolean_expression(PsqlScanState scan_state, char *action,
+   bool *result)
+{
+   char*value = 

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

[Fabien COELHO]

Hello Corey,


Glad you like the barking. I'm happy to let the prompt issue rest for now,
we can always add it later.

If we DID want it, however, I don't think it'll be hard to add a special
prompt (Thinking %T or %Y because they both look like branches, heh),


Ah!:-) T may stand for Tree, but Y looks a little bit more like branches. 
Maybe Y for Yew.



With a prompt1 of '%T> ' Would then resolve to "ttfi" [...]
This is just idle musing, I'm perfectly happy to leave it out entirely.


I like it. I would prefer to have it available, but my advice is to follow 
committer' opinions. At the minimum, there must be some trace, either 
"barking" or "prompting".


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

[Corey Huinker]

>
>
> On reflection, it seems fairly improbable to me that people would use
> \if and friends interactively.  They're certainly useful for scripting,
> but would you really type commands that you know are going to be ignored?
>

I'm thinking the one use-case is where a person is debugging a
non-interactive script, cuts and pastes it into an interactive script, and
then scrolls through command history to fix the bits that didn't work. So,
no, you wouldn't *type* them per se, but you'd want the session as if you
had. The if-then barking might really be useful in that context.


>
> Therefore, I don't think we should stress out about fitting branching
> activity into the prompts.  That's just not the use-case.  (Note: we
> might well have to reconsider that if we get looping, but for now it's
> not a problem.)  Moreover, if someone is confused because they don't
> realize they're inside a failed \if, it's unlikely that a subtle change in
> the prompt would help them.  So your more in-the-face approach of printing
> messages seems good to me.
>

Glad you like the barking. I'm happy to let the prompt issue rest for now,
we can always add it later.

If we DID want it, however, I don't think it'll be hard to add a special
prompt (Thinking %T or %Y because they both look like branches, heh), and
it could print the if-state stack, maybe something like:

\if true
  \if true
 \if false
\if true

With a prompt1 of '%T> ' Would then resolve to

ttfi>

for true, true, false, ignored.

This is just idle musing, I'm perfectly happy to leave it out entirely.


> This seems more or less reasonable, although:
>
> > # \endif
> > active \endif, executing commands
>
> looks a bit weird.  Maybe instead say "exited \if, executing commands"?
>

+1


>
> BTW, what is your policy about nesting these things in include files?
> My immediate inclination is that if we hit EOF with open \if state,
> we should drop it and revert to the state in the surrounding file.
> Otherwise things will be way too confusing.
>

That's how it works now if you have ON_ERROR_STOP off, plus an error
message telling you about the imbalance. If you have ON_ERROR_STOP on, it's
fatal.

All \if-\endif pairs must be balanced within a file (well, within a
MainLoop, but to the user it looks like a file). Every new file opened via
\i or \ir starts a new if-stack. Because commands in an inactive branch are
never executed, we don't have to worry about the state of the parent stack
when we do a \i, because we know it's trues all the way down.

We chose this not so much because if-endif needed it (we could have thrown
it into the pset struct just as easily), but because of the issues that
might come up with a \while loop: needing to remember previous GOTO points
in a file (if the input even *is* a file...) is going to be hard enough,
remembering them across files would be harder, and further complicated by
the possibility that a file \included on one iteration might not be
included on the next (or vice versa)...and like you said, way too confusing.

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

[Tom Lane]

Corey Huinker  writes:
>> As it is, I've added interactive mode psql_error notifications about the
>> resulting branching state of any branching commands, and any attempt to
>> send non-branching commands or queries while in an inactive branch will
>> generate a psql_error saying that the command was ignored. Waiting til I
>> get what should or shouldn't be done about prompts before issuing the next
>> patch revision.

On reflection, it seems fairly improbable to me that people would use
\if and friends interactively.  They're certainly useful for scripting,
but would you really type commands that you know are going to be ignored?

Therefore, I don't think we should stress out about fitting branching
activity into the prompts.  That's just not the use-case.  (Note: we
might well have to reconsider that if we get looping, but for now it's
not a problem.)  Moreover, if someone is confused because they don't
realize they're inside a failed \if, it's unlikely that a subtle change in
the prompt would help them.  So your more in-the-face approach of printing
messages seems good to me.

> So far, interactive branching information will look like this (it prints on
> every branching command and on every ignored command):

This seems more or less reasonable, although:

> # \endif
> active \endif, executing commands

looks a bit weird.  Maybe instead say "exited \if, executing commands"?

BTW, what is your policy about nesting these things in include files?
My immediate inclination is that if we hit EOF with open \if state,
we should drop it and revert to the state in the surrounding file.
Otherwise things will be way too confusing.

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

[Corey Huinker]

>
>
> All,
> As it is, I've added interactive mode psql_error notifications about the
> resulting branching state of any branching commands, and any attempt to
> send non-branching commands or queries while in an inactive branch will
> generate a psql_error saying that the command was ignored. Waiting til I
> get what should or shouldn't be done about prompts before issuing the next
> patch revision.
>

So far, interactive branching information will look like this (it prints on
every branching command and on every ignored command):

# \if true
active \if, executing commands
# select 1;
 ?column?
--
1
(1 row)

Time: 0.282 ms
# \else
inactive \else, ignoring commands
# select 1;
inside inactive branch, query ignored.
# select
... # 1;
inside inactive branch, query ignored.
# \endif
active \endif, executing commands
# \if false
inactive \if, ignoring commands
# \i file_name
inside inactive branch, command ignored.
# \elif false
inactive \elif, ignoring commands
# \else
active \else, executing commands
# \endif
active \endif, executing commands


Comments welcome.

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

[Corey Huinker]

On Wed, Feb 1, 2017 at 4:58 PM, Jim Nasby  wrote:

> I think the issue here is that the original case for this is a user
> accidentally getting into an \if and then having no clue what's going on.
> That's similar to what happens when you miss a quote or a semicolon. We
> handle those cases with %R, and I think %R needs to support if as well.
>
> Perhaps there's value to providing more info (active branch, etc), but
> ISTM trying to do that will just confuse the original (%R) case.
>

Jim,

After spending a few minutes to familiarize myself with %R, I'm in
agreement with your second statement (adding if-else to %R will just
confuse %R). However, your first statement seems to indicate the opposite.
Can you elaborate?

All,
As it is, I've added interactive mode psql_error notifications about the
resulting branching state of any branching commands, and any attempt to
send non-branching commands or queries while in an inactive branch will
generate a psql_error saying that the command was ignored. Waiting til I
get what should or shouldn't be done about prompts before issuing the next
patch revision.

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

[Fabien COELHO]

Hello,


What do I need to do to get TAP tests running?


I misunderstood. You need to configure with "--enable-tap-tests".


There is a spurious empty line added at the very end of "mainloop.h":

  +
   #endif   /* MAINLOOP_H */


Not in my diff, but that's been coming and going in your diff reviews.


Strange. Maybe this is linked to the warning displayed with "git apply" 
when I apply the diff.



I would suggest to add a short one line comment before each test to
explain what is being tested, like "-- test \elif execution", "-- test
\else execution"...


Where are you suggesting this?


In "regress/sql/psql.sql", in front of each group which starts a test.


Debatable suggestion about "psql_branch_empty":


The name isn't great. Maybe psql_branch_stack_empty()?


Yep, maybe, or "empty_stack" or "stack_is_empty" or IDK...


"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop"

Yeah, we either need to go fully with telling the programmer that it's a
stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm
inclined to go full-stack, as it were.


Anything consistent is ok. I'm fine with calling a stack a 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Jim Nasby]

On 2/1/17 12:03 PM, Fabien COELHO wrote:

I'm unsure whether it is a good idea, I like terse interfaces, but this
is just an opinion.


I think the issue here is that the original case for this is a user 
accidentally getting into an \if and then having no clue what's going 
on. That's similar to what happens when you miss a quote or a semicolon. 
We handle those cases with %R, and I think %R needs to support if as well.


Perhaps there's value to providing more info (active branch, etc), but 
ISTM trying to do that will just confuse the original (%R) case.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

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

[Fabien COELHO]

Hello,


We could just issue interactive-only warnings when:
- A user types a branching condition command which sets the branch inactive
- A user types a command or query when the current branch is inactive.

The warnings could be specific about state, something like:

psql session is now in an inactive \if branch. No queries will be executed
and only branching commands (\if, \elif, \else, \endif) will be evaluated.
[...]


My 0.02€: it looks too verbose, should stay on one short line. Maybe:

  (in|)active (\if|\elif|\else), (execut|ignor)ing commands

Although there is some redundancy...

   calvin=>\if true
 active \if: executing commands
   calvin=(t)> \echo ok
 ok
   calvin=(t)> \else
 inactive \else: skipping commands
   calvin=(f)> ...

Maybe it could be controlled, say based on VERBOSITY setting (which really 
controls verbosity of error reports) or some other.


I'm unsure whether it is a good idea, I like terse interfaces, but this is 
just an opinion.


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

[Corey Huinker]

>
>
> You can also run "make check" directly in the "src/bin/psql" directory.


Previously, that didn't do anything, but now that I've created a TAP test
it does. It doesn't, however, run the psql regress tests. But at least I
can use the two commands in combination and not have to run *all* TAP tests
outside of psql.

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

[Corey Huinker]

On Wed, Feb 1, 2017 at 8:53 AM, Corey Huinker 
wrote:

>
>> Run   make check-world   (as opposed to  just  make check )
>
>
> I'll give that a shot.
>

That was it. Tests don't run if you don't invoke them. Thanks.

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

[Corey Huinker]

>
> However I would say yes, it should provide some feedback... This means
> probably adding a new prompt substitution "%". In the worst
> case, the prompt should reflect the current stack, or at least the top of
> the task...
>

We could just issue interactive-only warnings when:
- A user types a branching condition command which sets the branch inactive
- A user types a command or query when the current branch is inactive.

The warnings could be specific about state, something like:

psql session is now in an inactive \if branch. No queries will be executed
and only branching commands (\if, \elif, \else, \endif) will be evaluated.

psql session is now in an inactive \elif branch. No queries will be
executed and only branching commands (\if, \elif, \else, \endif) will be
evaluated.

psql session is now in an inactive \else branch. No queries will be
executed and only branching commands (\if, \endif) will be evaluated.



This could of course be done in addition to prompt changes, and is
orthogonal to the CTRL-c  option. I'd like more input before moving forward
on either of those, as they have a good chance to clobber other expected
behaviors.

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

[Fabien COELHO]

Hello Corey,


There is a spurious empty line added at the very end of "mainloop.h":

  +
   #endif   /* MAINLOOP_H */


Not in my diff, but that's been coming and going in your diff reviews.


Strange. Maybe this is linked to the warning displayed with "git apply" when 
I apply the diff.



I would suggest to add a short one line comment before each test to
explain what is being tested, like "-- test \elif execution", "-- test
\else execution"...


Where are you suggesting this?


In "regress/sql/psql.sql", in front of each group which starts a test.


Debatable suggestion about "psql_branch_empty":


The name isn't great. Maybe psql_branch_stack_empty()?


 Yep, maybe, or "empty_stack" or "stack_is_empty" or IDK...


"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop"

Yeah, we either need to go fully with telling the programmer that it's a
stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm
inclined to go full-stack, as it were.


Anything consistent is ok. I'm fine with calling a stack a 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Good find. I'll have to bulk up the help text.


Yes.


This raises a question: in interactive mode, should we give some feedback
as to the result of an \if or \elif test? (see below)


Obviously \if makes more sense for scripting.

However I would say yes, it should provide some feedback... This means 
probably adding a new prompt substitution "%". In the worst 
case, the prompt should reflect the current stack, or at least the top of 
the task...


Maybe use "%?" which could be substituted by:

empty stack   -> ""
ignore state  -> "." or "(i)"
*_true state  -> "t" or "(t)"
*_false state -> "f" or "(f)"

calvin=>   \if true
calvin=(t)>  \echo "running..."
  running...
calvin=(t)>  \else
calvin=(f)>   whatever
calvin=(f)>  \endif
calvin=>



Therefore making it possible to break out of \if-mode with Ctrl-C would be
an improvement, I think.
I would even prefer it when  \q would exit psql always, even from within
\if-mode.

So I don't think we can do that. At least not in non-interactive mode.


Yep.


As for CTRL-C, I've never looked into what psql does with CTRL-C, so I
don't know if it's possible, let alone desirable.


I think that ctrl-c should abandon current command, which is what the user 
expect when things go wrong. I would suggest to pop the stack on ctrl-C on 
an empty input, eg:


  calvin=> \if true
  calvin=(t)> SELECT
  calvin-(t)>   
  calvin=(t)> 
  calvin=>


Also, shouldn't  the prompt change inside an \if block?


That's a good question. I could see us finding ways to print the t/f of
whether a branch is active or not, but I'd like to hear from more people
before diving into something like that.


See above.

Adding a state indicator is probably ok, the key question is whether the 
default prompt is changed.


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

[Erik Rijkers]

On 2017-02-01 14:18, Corey Huinker wrote:

to do to get TAP tests running? I must be missing something.


Guesswork on my part:

Add  --enable-tap-tests option  on ./configure

Run   make check-world   (as opposed to  just  make check )


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

[Corey Huinker]

>
>
> Add  --enable-tap-tests option  on ./configure
>

This much I had already done.


>
> Run   make check-world   (as opposed to  just  make check )


I'll give that a shot.

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

[Fabien COELHO]

Run   make check-world   (as opposed to  just  make check )


I'll give that a shot.


You can also run "make check" directly in the "src/bin/psql" directory.

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

[Tom Lane]

Corey Huinker  writes:
> Right. What I meant was, no matter how I changed that test, I could not get
> it to fail, which made me think it was not executing at all. What do I need
> to do to get TAP tests running? I must be missing something.

configure --enable-tap-tests, perhaps?

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

[Corey Huinker]

>
> - wrote an intentionally failing TAP test to see if "make check" executes
>> it. it does not. need help.
>>
>
> A failing test expects code 3, not 0 as written in "t/001_if.pl". With
>
>   is($retcode,'3','Invalid \if respects ON_ERROR_STOP');
>
> instead, the tests succeeds because psql returned 3.
>

Right. What I meant was, no matter how I changed that test, I could not get
it to fail, which made me think it was not executing at all. What do I need
to do to get TAP tests running? I must be missing something.


> More check should be done about stdout to check that it failed for the
> expected reasons though. And maybe more tests could be added to trigger all
> possible state transition errors (eg else after else, elif after else,
> endif without if, if without endif, whatever...).


Agreed. But I couldn't build further on the test without being sure it was
being run.


>
> Other comments and suggestions:
>
> Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?
>

I think that's a merge error from rebasing. Will fix.


>
> There is a spurious empty line added at the very end of "mainloop.h":
>
>   +
>#endif   /* MAINLOOP_H */
>

Not in my diff, but that's been coming and going in your diff reviews.


>
>
> I would suggest to add a short one line comment before each test to
> explain what is being tested, like "-- test \elif execution", "-- test
> \else execution"...
>

Where are you suggesting this?


>
> Debatable suggestion about "psql_branch_empty": the function name somehow
> suggests an "empty branch", where it is really testing that the stack is
> empty. Maybe the function could be removed and "psql_branch_get_state(...)
> == IF_STATE_NONE" used instead. Not sure.
>

The name isn't great. Maybe psql_branch_stack_empty()?

"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop"
> which would be symmetrical to "psql_branch_push"? Or maybe push should be
> named "begin_state" or "new_state"...
>

Yeah, we either need to go fully with telling the programmer that it's a
stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm
inclined to go full-stack, as it were.



>
> C style details: I would avoid non mandatory parentheses, eg:
>
>   +   return ((strcmp(cmd, "if") == 0 || \
>   +   strcmp(cmd, "elif") == 0 || \
>   +   strcmp(cmd, "else") == 0 || \
>   +   strcmp(cmd, "endif") == 0));
>
> I would remove the external double parentheses (( ... )). Also I'm not
> sure why there are end-of-line backslashes on the first instance, maybe a
> macro turned into a function?
>

I copied that from somewhere, and I don't remember where. I think the test
was originally nested much further before being moved to its own function.
Can fix.


>
>   +   return ((s == IFSTATE_NONE) ||
>   +   (s == IFSTATE_TRUE) ||
>   +   (s == IFSTATE_ELSE_TRUE));
>
> I would remove all parenthenses.
>

+1


>
>  +   return (state->branch_stack == NULL);
>
> Idem.


+1

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

[Corey Huinker]

On Wed, Feb 1, 2017 at 6:31 AM, Erik Rijkers  wrote:

> On 2017-02-01 09:27, Corey Huinker wrote:
>
>>  0001.if_endif.v4.diff
>>
>
> A few thoughts after a quick try:
>
> I dislike the ease with which one gets stuck inside an \if block, in
> interactive mode.
>
> (for instance, in my very first session, I tried   '\? \if'  to see if
> there is more info in that help-screen, but it only displays the normal
> help screen.  But after that one cannot exit with  \q  anymore, and there
> is no feedback of any kind (prompt?) in which black hole one has ended up.
> Only a \endif  provides rescue.)
>

Good find. I'll have to bulk up the help text.
This raises a question: in interactive mode, should we give some feedback
as to the result of an \if or \elif test? (see below)


>
> Therefore making it possible to break out of \if-mode with Ctrl-C would be
> an improvement, I think.
> I would even prefer it when  \q would exit psql always, even from within
> \if-mode.
>

This whole thing got started with a \quit_if  command, and it was
pointed out that

\if :condition
   \q
\endif

SELECT ... FROM ...


would be preferable. So I don't think we can do that. At least not in
non-interactive mode.

As for CTRL-C, I've never looked into what psql does with CTRL-C, so I
don't know if it's possible, let alone desirable.


>
> Also, shouldn't  the prompt change inside an \if block?
>

That's a good question. I could see us finding ways to print the t/f of
whether a branch is active or not, but I'd like to hear from more people
before diving into something like that.

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

[Erik Rijkers]

On 2017-02-01 09:27, Corey Huinker wrote:

 0001.if_endif.v4.diff


A few thoughts after a quick try:

I dislike the ease with which one gets stuck inside an \if block, in 
interactive mode.


(for instance, in my very first session, I tried   '\? \if'  to see if 
there is more info in that help-screen, but it only displays the normal 
help screen.  But after that one cannot exit with  \q  anymore, and 
there is no feedback of any kind (prompt?) in which black hole one has 
ended up.  Only a \endif  provides rescue.)


Therefore making it possible to break out of \if-mode with Ctrl-C would 
be an improvement, I think.
I would even prefer it when  \q would exit psql always, even from within 
\if-mode.


Also, shouldn't  the prompt change inside an \if block?

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

[Fabien COELHO]

Hello Corey,

Some comments about v4:

Patch applies. "git apply" complained about a space or line somewhere, not 
sure why. make check ok.



- rebased on post-psql hooks master


Good.


- took nearly every suggestion for change to documentation


Indeed. Looks ok to me.


- \if ERROR will throw the entire \if..\endif into IGNORE mode


Ok. I think that it is the better behavior, but other people opinion may 
differ. Opinions are welcome.



- state is now pushed on all \ifs


Ok.


- reinstated leveraging of ParseVariableBool


Ok.


- history is now kept in interactive mode regardless of \if-truth


Ok.


- reworked coding example to cause less agita


Yep.


- removed MainLoop "are endifs balanced" variable in favor of in-place
check which respects ON_ERROR_STOP.


Ok.


- make changes to psql/Makefile to enable TAP tests and created t/ directory
- wrote an intentionally failing TAP test to see if "make check" executes
it. it does not. need help.


A failing test expects code 3, not 0 as written in "t/001_if.pl". With

  is($retcode,'3','Invalid \if respects ON_ERROR_STOP');

instead, the tests succeeds because psql returned 3.

More check should be done about stdout to check that it failed for the 
expected reasons though. And maybe more tests could be added to trigger 
all possible state transition errors (eg else after else, elif after else, 
endif without if, if without endif, whatever...).



I'm hoping my failure in that last bit is easy to spot/fix, so I can move
forward with testing unbalanced branching, etc.


Other comments and suggestions:

Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?

There is a spurious empty line added at the very end of "mainloop.h":

  +
   #endif   /* MAINLOOP_H */


I would suggest to add a short one line comment before each test to 
explain what is being tested, like "-- test \elif execution", "-- test 
\else execution"...


Debatable suggestion about "psql_branch_empty": the function name somehow 
suggests an "empty branch", where it is really testing that the stack is 
empty. Maybe the function could be removed and "psql_branch_get_state(...) 
== IF_STATE_NONE" used instead. Not sure.


"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" 
which would be symmetrical to "psql_branch_push"? Or maybe push should be 
named "begin_state" or "new_state"...


C style details: I would avoid non mandatory parentheses, eg:

  +   return ((strcmp(cmd, "if") == 0 || \
  +   strcmp(cmd, "elif") == 0 || \
  +   strcmp(cmd, "else") == 0 || \
  +   strcmp(cmd, "endif") == 0));

I would remove the external double parentheses (( ... )). Also I'm not 
sure why there are end-of-line backslashes on the first instance, maybe a 
macro turned into a function?


  +   return ((s == IFSTATE_NONE) ||
  +   (s == IFSTATE_TRUE) ||
  +   (s == IFSTATE_ELSE_TRUE));

I would remove all parenthenses.

 +   return (state->branch_stack == NULL);

Idem.

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

[Corey Huinker]

>
>
> I think ParseVariableBool is only likely to change to reject a NULL value
> rather than silently interpreting it as FALSE, which is the way it is in
> HEAD right now.  That behavior is a leftover hack, really, and moving the
> treatment of unset values upstream seems a lot cleaner.  See my draft
> patch at
> https://www.postgresql.org/message-id/30629.1485881...@sss.pgh.pa.us
>
> regards, tom lane
>


Updated patch:
- rebased on post-psql hooks master
- took nearly every suggestion for change to documentation
- \if ERROR will throw the entire \if..\endif into IGNORE mode
- state is now pushed on all \ifs
- reinstated leveraging of ParseVariableBool
- history is now kept in interactive mode regardless of \if-truth
- reworked coding example to cause less agita
- removed MainLoop "are endifs balanced" variable in favor of in-place
check which respects ON_ERROR_STOP.
- make changes to psql/Makefile to enable TAP tests and created t/ directory
- wrote an intentionally failing TAP test to see if "make check" executes
it. it does not. need help.

I'm hoping my failure in that last bit is easy to spot/fix, so I can move
forward with testing unbalanced branching, etc.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 4e51e90..aad62ac 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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.
+
+
+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..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -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..a916671 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,29 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool
+read_boolean_expression(PsqlScanState 

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

[Tom Lane]

Corey Huinker  writes:
> On Tue, Jan 31, 2017 at 1:04 AM, Fabien COELHO  wrote:
>> The ParseVariableBool function has been updated, and the new version is
>> much cleaner, including all fixes that I suggested in your copy, so you can
>> use it in your patch.

> I see there's still a lot of activity in the thread, I can't tell if it's
> directly related to ParseVariableBool() or in the way it is called. Should
> I wait for the dust to settle over there?

I think ParseVariableBool is only likely to change to reject a NULL value
rather than silently interpreting it as FALSE, which is the way it is in
HEAD right now.  That behavior is a leftover hack, really, and moving the
treatment of unset values upstream seems a lot cleaner.  See my draft
patch at
https://www.postgresql.org/message-id/30629.1485881...@sss.pgh.pa.us

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

[Corey Huinker]

On Tue, Jan 31, 2017 at 1:04 AM, Fabien COELHO  wrote:

>
> This is code lifted from variable.c's ParseVariableBool().  When the other
>>> patch for "psql hooks" is committed (the one that detects when the string
>>> wasn't a valid boolean), this code will go away and we'll just use
>>> ParseVariableBool() again.
>>>
>>
> The ParseVariableBool function has been updated, and the new version is
> much cleaner, including all fixes that I suggested in your copy, so you can
> use it in your patch.
>
> --
> Fabien.
>

I see there's still a lot of activity in the thread, I can't tell if it's
directly related to ParseVariableBool() or in the way it is called. Should
I wait for the dust to settle over there?

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

[Fabien COELHO]

This is code lifted from variable.c's ParseVariableBool().  When the other
patch for "psql hooks" is committed (the one that detects when the string
wasn't a valid boolean), this code will go away and we'll just use
ParseVariableBool() again.


The ParseVariableBool function has been updated, and the new version is 
much cleaner, including all fixes that I suggested in your copy, so you 
can use it in your patch.


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

[Fabien COELHO]

Hello Daniel,

[...] So psql is not following that model with ON_ERROR_STOP if it exits 
with an error when unable to evaluate an "if" expression. I'm not 
implying that we should necessarily adopt the shell behavior, but as 
these choices have certainly been made in POSIX for good reasons, we 
should make sure to think twice about why they don't apply to psql.


Interesting point.

The shell is about processes, and if relies on the status code returned, 
with 0 meaning true, and anything else, which is somehow a process error, 
meaning false. So there is no way to distinguish false from process error 
in this system, because the status is just one integer.


However, a syntax error, for instance with a shell internal test, leads to 
nothing being executed:


   bash> if [[ bad syntax ]] ; then echo then ; else echo else ; fi
   -bash: conditional binary operator expected
   -bash: syntax error near `syntax'
   # nothing is echoed

Another example with python in interactive mode:

   python> if 1+: print 1; else print 0
   SyntaxError: invalid syntax
   # nothing is printed

So rejecting execution altogether on syntax errors is somehow a common 
practice.


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

[Daniel Verite]

Corey Huinker wrote:

> >   \if ERROR
> >  \echo X
> >   \else
> >  \echo Y
> >   \endif
> >
> > having both X & Y printed and error reported on else and endif. I think
> > that an expression error should just put the stuff in ignore state.
> >
> 
> Not just false, but ignore the whole if-endif? interesting. I hadn't
> thought of that. Can do.

If we use the Unix shell as a model, in POSIX "test" and  "if"
are such that an evaluation error (exit status>1) leads to the same
flow than when evaluating to false (exit status=1).

References I can find:

test:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

if:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04_07

BTW, in "set -e" mode, it also says that a failure to evaluate an "if"
 expression does not lead to the script stopping:

  The -e setting shall be ignored when executing the compound list
  following the while, until, if, or elif reserved word, a pipeline
  beginning with the ! reserved word, or any command of an AND-OR list
  other than the last


So psql is not following that model with ON_ERROR_STOP if it exits
with an error when unable to evaluate an "if" expression.
I'm not implying that we should necessarily adopt the shell behavior,
but as these choices have certainly been made in POSIX for good
reasons, we should make sure to think twice about why they don't
apply to 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Jim Nasby]

On 1/29/17 2:35 AM, Fabien COELHO wrote:

I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if
there are many employees. [...]


I believe that the scan stops on the first row it finds, because the
EXITS() clause is met.


Hmmm... That is not so clear from "EXPLAIN" output:


You need to use a better test case...


explain analyze select exists(select 1 from generate_series(1,9) gs);
  QUERY PLAN
---
 Result  (cost=0.01..0.02 rows=1 width=1) (actual time=26.278..26.278 rows=1 
loops=1)
   InitPlan 1 (returns $0)
 ->  Function Scan on generate_series gs  (cost=0.00..10.00 rows=1000 
width=0) (actual time=26.271..26.271 rows=1 loops=1)
 Planning time: 6.568 ms
 Execution time: 48.917 ms
(5 rows)


In any case, +1 for not promoting count(*) <> 0; that's a really, really 
bad way to test for existence.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

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

[Corey Huinker]

>
> My point is that examples about one thing can be interpreted as example
> for other things which is also done in the example, so it is better to do
> everything right.
>

Fair enough. I'll rewrite the examples to use pk lookups. I doubt the query
plan for those will change much in the future.


> Hmmm. Copy-pasting is bad enough, and "when the other patch is committed"
> is an unknown, so I would still suggest to fix obvious defects at least (eg
> dead code which may result in compiler warnings, inconsistent comments...).
>

It was do that or pause this work until that unknown was resolved.



>
> My point was that you must at least push something, otherwise both
> branches are executed (!), and some commands could be attached to
> upper-level conditions:
>
>
> As for which state is pushed, it is indeed debatable. I do think that
> pushing ignore on errors is a better/less risky behavior, but other people'
> opinion may differ.


+1




>
>
> On "else" when in state ignored, ISTM that it should remain in state
>>> ignore, not switch to else-false.
>>>
>>
>> That's how I know if this is the first "else" I encountered.
>>
>
> Ok, my mistake. Maybe expand the comment a little bit if appropriate.


+1


>
> As I recall, history is only for interactive mode. If I really typed
> something, I'm expecting to get it by visiting previous commands, because I
> certainly do not want to retype it again.
>
> For your above example, maybe I would reedit the clause definition, then
> want to execute the delete.


Good points, and history does save the string with the variable in it, not
the resolved string that was sent (or not sent) to the server.

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

[Fabien COELHO]

Hello,


I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if
there are many employees. [...]


I believe that the scan stops on the first row it finds, because the
EXITS() clause is met.


Hmmm... That is not so clear from "EXPLAIN" output:

 Result  (cost=0.03..0.04 rows=1 width=1)
  InitPlan 1 (returns $0)
  ->  Seq Scan on ...  (cost=0.00..263981.69 rows=10001769 width=0)

There is a plan for the sub-query, so it looks like it is actually fully 
executed. Maybe adding "LIMIT 1" would be better?


But it's not relevant to the documentation, I simply wanted to 
demonstrate some results that couldn't be resolved at parse time, so 
that the \if tests were meaningful. If the query example is distracting 
from the point of the documentation, we should change it.


My point is that examples about one thing can be interpreted as example 
for other things which is also done in the example, so it is better to do 
everything right.




In "read_boolean_expression": [...]



This is code lifted from variable.c's ParseVariableBool().  When the other
patch for "psql hooks" is committed (the one that detects when the string
wasn't a valid boolean), this code will go away and we'll just use
ParseVariableBool() again.


Hmmm. Copy-pasting is bad enough, and "when the other patch is committed" 
is an unknown, so I would still suggest to fix obvious defects at least 
(eg dead code which may result in compiler warnings, inconsistent 
comments...).


[...] The second test on success may not rely on the value set above. 
That looks very strange. ISTM that the state should be pushed whether 
parsing succeeded or not. Moreover, it results in:


  \if ERROR
 \echo X
  \else
 \echo Y
  \endif

having both X & Y printed and error reported on else and endif. I think
that an expression error should just put the stuff in ignore state.


Not just false, but ignore the whole if-endif? interesting. I hadn't
thought of that. Can do.


My point was that you must at least push something, otherwise both 
branches are executed (!), and some commands could be attached to 
upper-level conditions:


  \if true
\if ERROR
  ...
\endif // this becomes "if true \endif"
...
  \endif // this becomes an error

As for which state is pushed, it is indeed debatable. I do think that 
pushing ignore on errors is a better/less risky behavior, but other 
people' opinion may differ.



On "else" when in state ignored, ISTM that it should remain in state
ignore, not switch to else-false.


That's how I know if this is the first "else" I encountered.


Ok, my mistake. Maybe expand the comment a little bit if appropriate.


History saving: I'm wondering whether all read line should be put into
history, whether executed or not.


Good question. I gave it some thought and I decided it shouldn't.  First,
because history is a set of statements that were attempted, and those
statements were not. But perhaps more importantly, because the statement
could have contained an expandable variable, and since that variable would
not be evaluated the SQL stored would be subtly altered from the original
intent, perhaps in ways that might drastically alter the meaning of the
statement. A highly contrived example:

\set clause 'where cust_id = 1'
\if false
delete from customers :clause;
\endif


Hmmm.


So yeah, it just seemed easier to not store in history.


Hmmm.

As I recall, history is only for interactive mode. If I really typed 
something, I'm expecting to get it by visiting previous commands, because 
I certainly do not want to retype it again.


For your above example, maybe I would reedit the clause definition, 
then want to execute the delete.


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

[Corey Huinker]

>
>
> I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if
> there are many employees. EXPLAIN suggests a seq_scan, which seems bad.
> With "(SELECT COUNT(*) FROM pgbench_accounts) <> 0" pg seems to generate an
> index only scan on a large table, so maybe it is a better way to do it. It
> seems strange that there is no better way to do that in a relational tool,
> the relational model being an extension of set theory... and there is no
> easy way (?) to check whether a set is empty.
>

I believe that the scan stops on the first row it finds, because the
EXITS() clause is met. But it's not relevant to the documentation, I simply
wanted to demonstrate some results that couldn't be resolved at parse time,
so that the \if tests were meaningful. If the query example is distracting
from the point of the documentation, we should change it.


>
> """If an EOF is reached on the main file or an
> \include-ed file before all
> \if-\endif are matched, then psql
> will raise an error."""
>
> In sentence above: "before all" -> "before all local"? "then" -> ""?
>

+1

>
> "other options booleans" -> "other booleans of options"? or "options'
> booleans" maybe, but that is for people?
>

+1

>
> "unabigous" -> "unambiguous"
>

+1


>
> I think that the three paragraph explanation about non evaluation could be
> factor into one, maybe something like: """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 nesting."""
>
> I would suggest to merge elif/else constraints by describing what is
> expected rather than what is not expected. """An \if command may contain
> any number of \elif clauses and may end with one \else clause""".
>

I'll give it another shot, as I forgot to mention the non-evaluation of
expressions in dead branches.



>
>
> ## CODE
>
> In "read_boolean_expression":
>
>  + if (value)
>
> "if (value != NULL)" is usually used, I think.
>
>  + if (value == NULL)
>  +   return false; /* not set -> assume "off" */
>
> This is dead code, because value has been checked to be non NULL a few
> lines above.
>
>  + (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0)
>  + (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0)
>
> Hmmm, not easy to parse. Maybe it deserves a comment?
> "check at least two chars to distinguish on & off"
>
> ",action" -> ", action" (space after commas).
>
> The "result" is not set on errors, but maybe it should be set to false
> anyway and explicitely, instead of relying on some prior initialization?
> Or document that the result is not set on errors.
>

This is code lifted from variable.c's ParseVariableBool().  When the other
patch for "psql hooks" is committed (the one that detects when the string
wasn't a valid boolean), this code will go away and we'll just use
ParseVariableBool() again.


>
> if command:
>
>   if (is active) {
> success = ...
> if (success) {
>   ...
> }
>   }
>   if (success) {
> ...
>   }
>
> The second test on success may not rely on the value set above. That looks
> very strange. ISTM that the state should be pushed whether parsing
> succeeded or not. Moreover, it results in:
>
>   \if ERROR
>  \echo X
>   \else
>  \echo Y
>   \endif
>
> having both X & Y printed and error reported on else and endif. I think
> that an expression error should just put the stuff in ignore state.
>

Not just false, but ignore the whole if-endif? interesting. I hadn't
thought of that. Can do.


>
>
> On "else" when in state ignored, ISTM that it should remain in state
> ignore, not switch to else-false.
>

That's how I know if this is the first "else" I encountered. I could split
the if-state back into a struct of booleans if you think that makes more
sense.



>
>
> Comment about "IFSTATE_FALSE" talks about the state being true, maybe a
> copy-paste error?
>

Yes.


>
> In comments: "... variables the branch" -> "variables if the branch"
>

Yes.


>
> The "if_endifs_balanced" variable is not very useful. Maybe just the test
> and error reporting in the right place:
>
>  if (... && !psqlscan_branch_empty(scan_state))
>psql_error("found EOF before closing \\endif(s)\n");
>

+1
I think I got the idea at some point that psql_error broke out of the
current execution block.


> History saving: I'm wondering whether all read line should be put into
> history, whether executed or not.
>

Good question. I gave it some thought and I decided it shouldn't.  First,
because history is a set of statements that were attempted, and those
statements were not. But perhaps more importantly, because the statement
could have contained an expandable variable, and since that variable would
not be evaluated the SQL stored would be subtly altered from the original
intent, perhaps in ways that might drastically alter the meaning of the
statement. A 

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

[Fabien COELHO]

Hello Corey,


And here it is


About the patch v3:

## DOCUMENTATION

I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if 
there are many employees. EXPLAIN suggests a seq_scan, which seems bad. 
With "(SELECT COUNT(*) FROM pgbench_accounts) <> 0" pg seems to generate 
an index only scan on a large table, so maybe it is a better way to do it. 
It seems strange that there is no better way to do that in a relational 
tool, the relational model being an extension of set theory... and there 
is no easy way (?) to check whether a set is empty.


"""If an EOF is reached on the main file or an 
\include-ed file before all 
\if-\endif are matched, then psql 
will raise an error."""


In sentence above: "before all" -> "before all local"? "then" -> ""?

"other options booleans" -> "other booleans of options"? or "options' 
booleans" maybe, but that is for people?


"unabigous" -> "unambiguous"

I think that the three paragraph explanation about non evaluation could be 
factor into one, maybe something like: """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 nesting."""


I would suggest to merge elif/else constraints by describing what is 
expected rather than what is not expected. """An \if command may contain 
any number of \elif clauses and may end with one \else clause""".



## CODE

In "read_boolean_expression":

 + if (value)

"if (value != NULL)" is usually used, I think.

 + if (value == NULL)
 +   return false; /* not set -> assume "off" */

This is dead code, because value has been checked to be non NULL a few 
lines above.


 + (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0)
 + (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0)

Hmmm, not easy to parse. Maybe it deserves a comment?
"check at least two chars to distinguish on & off"

",action" -> ", action" (space after commas).

The "result" is not set on errors, but maybe it should be set to false 
anyway and explicitely, instead of relying on some prior initialization?

Or document that the result is not set on errors.

if command:

  if (is active) {
success = ...
if (success) {
  ...
}
  }
  if (success) {
...
  }

The second test on success may not rely on the value set above. That looks 
very strange. ISTM that the state should be pushed whether parsing 
succeeded or not. Moreover, it results in:


  \if ERROR
 \echo X
  \else
 \echo Y
  \endif

having both X & Y printed and error reported on else and endif. I think 
that an expression error should just put the stuff in ignore state.



On "else" when in state ignored, ISTM that it should remain in state 
ignore, not switch to else-false.



Comment about "IFSTATE_FALSE" talks about the state being true, maybe a 
copy-paste error?


In comments: "... variables the branch" -> "variables if the branch"

The "if_endifs_balanced" variable is not very useful. Maybe just the test 
and error reporting in the right place:


 if (... && !psqlscan_branch_empty(scan_state))
   psql_error("found EOF before closing \\endif(s)\n");



 +
  #endif   /* MAINLOOP_H */


 -
 /*
  * Main processing loop for reading lines of input
  * and sending them to the backend.

Do not add/remove empty lines in places unrelated to the patch?


History saving: I'm wondering whether all read line should be put into 
history, whether executed or not.


Is it possible to make some of the added functions static? If so, do it.

I checked that it does stop on errors with -v ON_ERROR_STOP=1. However I 
would be more at ease if this was tested somewhere.


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

[Corey Huinker]

On Thu, Jan 26, 2017 at 4:06 PM, Corey Huinker 
wrote:

>
>
> On Thu, Jan 26, 2017 at 3:55 PM, Fabien COELHO 
> wrote:
>
>>
>> Hello Daniel,
>>
>> A comment about control flow and variables: in branches that are not
>>> taken, variables are expanded nonetheless, in a way that can be surprising.
>>> Case in point:
>>>
>>> \set var 'ab''cd'
>>> -- select :var;
>>> \if false
>>>  select :var ;
>>> \else
>>>  select 1;
>>> \endif
>>>
>>> To avoid that kind of trouble, would it make sense not to expand
>>> variables in untaken branches?
>>>
>>
>> Hmmm. This case is somehow contrived (for a string, :'var' could be used,
>> in which case escaping would avoid the hazard), but I think that what you
>> suggest is a better behavior, if easy to implement.
>>
>> --
>> Fabien.
>>
>
> Good question, Daniel. Variable expansion seems to be done via
> psql_get_variable which is invoked via callback, which means that I might
> have to move branch_block_active into PsqlSettings. That's slightly
> different because the existing boolean is scoped with MainLoop(), but
> there's no way to get to a new MainLoop unless you're in an executing
> branch, and no way to legally exit a MainLoop with an unbalanced if-endif
> state. In short, I think it's better behavior. It does prevent someone from
> setting a variable to '\endif' and expecting that to work, like this:
>
> select case
>  when random() < 0.5 then '\endif'
>  else E'\else\n\echo fooled you\n\endif'
> end as contrived_metaprogramming
> \gset
>
> \if false
>   :contrived_metaprogramming
>
>
> I'm sure someone will argue that preventing that is a good thing. Unless
> someone sees a good reason not to move that PsqlSettings, I'll make that
> change.
>
>
And here it is
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 640fe12..fcf265b 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,78 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer) as has_customers,
+EXISTS(SELECT 1 FROM employee) as has_employees
+\gset
+\if :has_users
+SELECT * FROM customer ORDER BY creation_date LIMIT 5;
+\elif :has_employees
+\echo 'no customers found'
+SELECT * FROM employee ORDER BY creation_date LIMIT 5;
+\else
+\if yes
+\echo 'No customers or employees'
+\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 
+\if-\endif are matched, then
+psql will raise an error.
+
+
+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 is evaluated like other options booleans, so the valid values
+are any unabiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+Queries within a false branch of a conditional block will not be
+sent to the server.
+
+
+Non-conditional \-commands within a false branch
+of a conditional block will not be evaluated for correctness. The
+command will be ignored along with all remaining input to the end
+of the line.
+
+
+Expressions on \if and \elif
+commands within a false branch of a conditional block will not be
+evaluated.
+
+
+A conditional block can at most one \else command.
+
+
+The \elif command cannot follow the
+\else command.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0c164a3..8235015 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,68 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*

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

[Corey Huinker]

On Thu, Jan 26, 2017 at 3:55 PM, Fabien COELHO  wrote:

>
> Hello Daniel,
>
> A comment about control flow and variables: in branches that are not
>> taken, variables are expanded nonetheless, in a way that can be surprising.
>> Case in point:
>>
>> \set var 'ab''cd'
>> -- select :var;
>> \if false
>>  select :var ;
>> \else
>>  select 1;
>> \endif
>>
>> To avoid that kind of trouble, would it make sense not to expand
>> variables in untaken branches?
>>
>
> Hmmm. This case is somehow contrived (for a string, :'var' could be used,
> in which case escaping would avoid the hazard), but I think that what you
> suggest is a better behavior, if easy to implement.
>
> --
> Fabien.
>

Good question, Daniel. Variable expansion seems to be done via
psql_get_variable which is invoked via callback, which means that I might
have to move branch_block_active into PsqlSettings. That's slightly
different because the existing boolean is scoped with MainLoop(), but
there's no way to get to a new MainLoop unless you're in an executing
branch, and no way to legally exit a MainLoop with an unbalanced if-endif
state. In short, I think it's better behavior. It does prevent someone from
setting a variable to '\endif' and expecting that to work, like this:

select case
 when random() < 0.5 then '\endif'
 else E'\else\n\echo fooled you\n\endif'
end as contrived_metaprogramming
\gset

\if false
  :contrived_metaprogramming


I'm sure someone will argue that preventing that is a good thing. Unless
someone sees a good reason not to move that PsqlSettings, I'll make that
change.

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

[Fabien COELHO]

Hello Daniel,

A comment about control flow and variables: in branches that are not 
taken, variables are expanded nonetheless, in a way that can be 
surprising. Case in point:


\set var 'ab''cd'
-- select :var;
\if false
 select :var ;
\else
 select 1;
\endif

To avoid that kind of trouble, would it make sense not to expand
variables in untaken branches?


Hmmm. This case is somehow contrived (for a string, :'var' could be used, 
in which case escaping would avoid the hazard), but I think that what you 
suggest is a better behavior, if easy to implement.


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

[Daniel Verite]

Corey Huinker wrote:

> Revised patch

A comment about control flow and variables:
in branches that are not taken, variables are expanded 
nonetheless, in a way that can be surprising.
Case in point:

\set var 'ab''cd'
-- select :var; 
\if false
  select :var ;
\else
  select 1;
\endif

The 2nd reference to :var has a quoting hazard, but since it's within
an "\if false" branch, at a glance it seems like this script might work.
In fact it barfs with:
  psql:script.sql:0: found EOF before closing \endif(s)

AFAICS what happens is that :var gets injected and starts a
runaway string, so that as far as the parser is concerned
the \else ..\endif block slips into the untaken branch, as a part of
that unfinished string.

This contrasts with line 2: -- select :var
where the reference to :var is inoffensive.

To avoid that kind of trouble, would it make sense not to expand
variables in untaken branches?

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

[Corey Huinker]

Revised patch, with one caveat: It contains copy/pasted code from
variable.c intended to bridge the gap until https://commitfest.postgresql.
org/12/799/  (changing ParseVariableBool to detect invalid boolean-ish
strings) is merged. We may want to pause full-review of this patch pending
resolution of that one. I'm happy to continue with the stop-gap in place.

Changes made:
- \elseif is now \elif
- Invalid boolean values now return an error
- ON_ERROR_STOP is respected in all errors raided by \if, \elsif, \else,
\endif commands.
- Documentation gives a more real-world example of usage.
- Documentation gives a more explicit list of valid boolean values
- Regression tests for out-of-place \endif, \else, and \endif
- Regression test for invalid boolean values
- Removal of debug detritus.

Changes not(yet) made:
- No TAP test for errors respecting ON_ERROR_STOP
- function comments in psqlscan.l follow the style found in other comments
there, which goes counter to global style.


On Tue, Jan 24, 2017 at 3:58 AM, Fabien COELHO  wrote:

>
> I would suggest to assume false on everything else, and/or maybe to ignore
>>> the whole if/endif section in such cases.
>>>
>>
>> +1, it also halves the number of values we have to support later.
>>
>
> After giving it some thought, I revise a little bit my opinion:
>
>
> I think that if the value is evaluated to TRUE or FALSE, then fine. If it
> is anything else, then an error is raised (error message shown), which
> should also stop the script on "ON_ERROR_STOP", and if not the script
> continues with assuming the value was FALSE.
>
> --
> Fabien.
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 9915731..20091e5 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2007,6 +2007,78 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer) as has_customers,
+EXISTS(SELECT 1 FROM employee) as has_employees
+\gset
+\if :has_users
+SELECT * FROM customer ORDER BY creation_date LIMIT 5;
+\elif :has_employees
+\echo 'no customers found'
+SELECT * FROM employee ORDER BY creation_date LIMIT 5;
+\else
+\if yes
+\echo 'No customers or employees'
+\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 
+\if-\endif are matched, then
+psql will raise an error.
+
+
+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 is evaluated like other options booleans, so the valid values
+are any unabiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+Queries within a false branch of a conditional block will not be
+sent to the server.
+
+
+Non-conditional \-commands within a false branch
+of a conditional block will not be evaluated for correctness. The
+command will be ignored along with all remaining input to the end
+of the line.
+
+
+Expressions on \if and \elif
+commands within a false branch of a conditional block will not be
+evaluated.
+
+
+A conditional block can at most one \else command.
+
+
+The \elif command cannot follow the
+\else command.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4139b77..feb9ddc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && psqlscan_branch_active(scan_state))
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,68 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool

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

[Tom Lane]

"Daniel Verite"  writes:
> ISTM that it's important that eventually ParseVariableBool()
> and \if agree on what evaluates to true and false (and the
> more straightforward way to achieve that is by \if calling
> directly ParseVariableBool), but that it's not productive that we
> discuss /if issues relatively to the behavior of ParseVariableBool()
> in HEAD at the moment, as it's likely to change.

AFAIK we do have consensus on changing its behavior to disallow
assignment of invalid values.  It's just a matter of getting the
patch to be stylistically nice.

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

[Corey Huinker]

On Tue, Jan 24, 2017 at 1:25 PM, Corey Huinker 
wrote:

> This might be asking a lot, but could we make a "strict" mode for
> ParseVariableBool() that returns a success boolean, and have the existing
> ParseVariableBool() signature call that new function, and issue the
> "assuming " warning if the strict function failed?


Nevermind. Looking at the v7 patch I see that it does everything I need and
more. I should have looked first.

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

[Corey Huinker]

>
> ISTM that it's important that eventually ParseVariableBool()
> and \if agree on what evaluates to true and false (and the
> more straightforward way to achieve that is by \if calling
> directly ParseVariableBool), but that it's not productive that we
> discuss /if issues relatively to the behavior of ParseVariableBool()
> in HEAD at the moment, as it's likely to change.
>

I'd like to keep in sync with ParseVariableBoolean(), but

Also, Fabien has made a good case for invalid parsed values being an
ON_ERROR_STOP-able error, and not defaulted to either true or false.

This might be asking a lot, but could we make a "strict" mode for
ParseVariableBool() that returns a success boolean, and have the existing
ParseVariableBool() signature call that new function, and issue the
"assuming " warning if the strict function failed?

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

[Daniel Verite]

Corey Huinker wrote:

> >
> > 1: unrecognized value "whatever" for "\if"; assuming "on"
> >
> > I do not think that the script should continue with such an assumption.
> >
> 
> I agree, and this means we can't use ParseVariableBool() as-is

The patch at https://commitfest.postgresql.org/12/799/
in the ongoing CF already changes ParseVariableBool()
to not assume that unrecognizable values should be set to
"on".

There's also the fact that ParseVariableBool() in HEAD assumes
that an empty value is valid and true, which I think leads to this
inconsistency in the current patch:

\set empty
\if :empty
select 1 as result \gset
\else
select 2 as result \gset
\endif
\echo 'result is' :result

produces: result is 1 (so an empty string evaluates to true)

Yet this sequence:

\if
select 1 as result \gset
\else
select 2 as result \gset
\endif
\echo 'result is' :result

produces: result is 2 (so an empty \if evaluates to false)

The equivalence between empty value and true in
ParseVariableBool() is also suppressed in the above-mentioned
patch.

ISTM that it's important that eventually ParseVariableBool()
and \if agree on what evaluates to true and false (and the
more straightforward way to achieve that is by \if calling
directly ParseVariableBool), but that it's not productive that we
discuss /if issues relatively to the behavior of ParseVariableBool()
in HEAD at the moment, as it's likely to change.


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

[Fabien COELHO]

I would suggest to assume false on everything else, and/or maybe to ignore
the whole if/endif section in such cases.


+1, it also halves the number of values we have to support later.


After giving it some thought, I revise a little bit my opinion:

I think that if the value is evaluated to TRUE or FALSE, then fine. If it 
is anything else, then an error is raised (error message shown), which 
should also stop the script on "ON_ERROR_STOP", and if not the script 
continues with assuming the value was FALSE.


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

[Corey Huinker]

On Tue, Jan 24, 2017 at 1:15 AM, Fabien COELHO  wrote:

>
> 1: unrecognized value "whatever" for "\if"; assuming "on"
>>>
>>> I do not think that the script should continue with such an assumption.
>>>
>>
>> I agree, and this means we can't use ParseVariableBool() as-is. I already
>> broke out argument reading to it's own function, knowing that it'd be the
>> stub for expressions. So I guess we start that now. What subset of true-ish
>> values do you think we should support? If we think that real expressions
>> are possible soon, we could only allow 'true' and 'false' for now, but if
>> we expect that expressions might not make it into v10, then perhaps we
>> should support the same text values that coerce to booleans on the server
>> side.
>>
>
> Hmmm. I would text value that coerce to true? I would also accept non-zero
> integers (eg SELECT 1::BOOL; -- t).
>


The docs (doc/src/sgml/datatype.sgml) list TRUE 't' 'true' 'y' 'yes' 'on'
'1'vs FALSE 'f' 'false' 'n' 'no' 'off' '0'

However, src/test/regress/expected/boolean.out shows that there's some
flexiblity there with spaces, even before you inspect parse_bool_with_len()
in src/backend/utils/adt/bool.c.

If we could bring src/backend/utils/adt/bool.c across the server/client
wall, I would just use parse_bool_with_len as-is.

As it is, given that whatever I do is temporary until real expressions come
into place, that wouldn't be a terrible amount of code copying, and we'd
still have a proto-expression that probably isn't going to conflict with
whatever expression syntax we do chose. Having said that, if anyone can see
ANY reason that some subset of the existing bool-friendly strings would
cause a problem with the expression syntax we do hope to use, speak now and
we can restrict the accepted values.


> I would suggest to assume false on everything else, and/or maybe to ignore
> the whole if/endif section in such cases.


+1, it also halves the number of values we have to support later.


> ISTM that with TAP test you can check for error returns, so maybe this can
> be done.


I'll have to educate myself on TAP tests.