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

> And here's the patch. I've changed the subject line and will be submitting
> a new entry to the commitfest.

A few comments:

Patch applies, make check is ok, but currently really too partial.

I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL 
has "ELSIF" like perl, that I do not like much, though. Given the syntax 
and behavioral proximity with cpp, I suggest that "elif" would be a better 
choice.

Documentation: I do not think that the systematic test-like example in the 
documentation is relevant, a better example should be shown. The list of 
what is considered true or false should be told explicitely, not end with 
"etc" that is everyone to guess.

Tests: On principle they should include echos in all non executed branches 
to reassure that they are indeed not executed.

Also, no error cases are tested. They should be. Maybe it is not very easy 
to test some cases which expect to make psql generate errors, but I feel 
they are absolutely necessary for such a feature.

1: unrecognized value "whatever" for "\if"; assuming "on"

I do not think that the script should continue with such an assumption.

2: encountered \else after \else ... "# ERROR BEFORE"

Even with ON_ERROR_STOP activated the execution continues.

3: no \endif

Error reported, but it does not stop even with ON_ERROR_STOP.

4: include with bad nesting...

Again, even with ON_ERROR_STOP, the execution continues...


Code:
  -       if (status != PSQL_CMD_ERROR)  +       if ((status != PSQL_CMD_ERROR) && psqlscan_branch_active(scan_state))

Why the added parenthesis?
  case ...: case ...:

The project rules are to add explicit /* PASSTHROUGH */ comment.

There are spaces at the end of one line in a comment about 
psqlscan_branch_end_state.

There are multiple "TODO" comments in the file... Is it done or work in 
progress?

Usually in pg comments about a function do not repeat the function name. 
For very short comment, they are /* inlined */ on one line. Use pg comment 
style.

-- 
Fabien.

> A few comments:

Argh, better with the attachements:-(

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

Hello,

>> I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
>> was the consensus. It's easy enough to change.
>
> Went with elsif to follow pl/pgsql. In reviewing the original thread it
> seemed that "elif" was linked to "fi" and that got some negative feedback.

As I understood it, the negative feeback was really about sh inspiration 
"if/fi", not about elif/elsif/elseif. I do not think that there was an
expressed consensus about the later.

Else-if variants from different languages include:
 VB: If ElseIf Else End If (with optional Then) PHP: if {} elseif {} else {} Tcl: if {} elseif {} else {}
 PL/pgSQL: IF ... THEN ... ELSIF ... ELSE ... END IF; Perl: if {} elsif {} else {} Ruby: if elsif else end
 CPP: #if #elif #else #endif Python: if : elif: else: bash: if [] then ... elif ... else ... fi

The closest case is CPP with its line-oriented #-prefix syntax, and I 
still think that we should do it like that.

> I went looking for other examples of explicit /* PASSTHROUGH */ comments
> and could find none. Could you explain what it is you want me to fix with
> the switch statements?

Sorry, I got it wrong. The comment (which is FALLTHROUGH or FALL THROUGH, 
not PASSTHROUGH) is required if there is specific code in a case and the 
case is expected to continue with executing the next case code as well. 
This is quite rare, and it is not the case for your code, which just has 
some comments in one case.

-- 
Fabien

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

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

> All valid issues. Will add those to the regression as well (with
> ON_ERROR_STOP disabled, obviously).

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

-- 
Fabien.

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

    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

"Daniel Verite" <daniel@manitou-mail.org> 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

    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

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.

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 
<command>\include</command>-ed file before all 
<command>\if</command>-<command>\endif</command> 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.

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=10001769width=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.

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,99999) 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)

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:
<quote> The -e setting shall be ignored when executing the compound list following the while, until, if, or elif
reservedword, a pipeline beginning with the ! reserved word, or any command of an AND-OR list other than the last 
</quote>

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

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.

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

Corey Huinker <corey.huinker@gmail.com> writes:
> On Tue, Jan 31, 2017 at 1:04 AM, Fabien COELHO <coelho@cri.ensmp.fr> 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.1485881533@sss.pgh.pa.us
        regards, tom lane

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.

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

> 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 "%<something>". 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)>   <ctrl-C>  calvin=(t)> <ctrl-C>  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.

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.

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

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

Corey Huinker <corey.huinker@gmail.com> 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

<sorry, resent, wrong from again *$%@#&!?>

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.

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.

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)

Corey Huinker <corey.huinker@gmail.com> 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

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.

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 <expr>": 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

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 <expr>": 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.

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

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

Corey Huinker <corey.huinker@gmail.com> 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

    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

On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker <corey.huinker@gmail.com> 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

On Fri, Feb 3, 2017 at 4:24 PM, Corey Huinker <corey.huinker@gmail.com> 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

On 2/2/17 4:39 PM, Corey Huinker wrote:
>
> On Wed, Feb 1, 2017 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com
> <mailto:Jim.Nasby@bluetreble.com>> 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)

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 <expr>    begin a new conditional block
  \elif <expr>  else if in the current conditional block
  \else         else in current conditional block
  \endif        end 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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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.

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

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

<Sigh>. It is better with the attachement.

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

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

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 <expr>": boolean expected # ok new \if is invalid,
ignoringcommands 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 <expr>": 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.

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 <expr>": 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.

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

On Mon, Feb 6, 2017 at 5:49 PM, Corey Huinker <corey.huinker@gmail.com> 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 <expr>": 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 <expr>": 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

Robert Haas <robertmhaas@gmail.com> 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 <expr>": 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

Corey Huinker <corey.huinker@gmail.com> writes:
> On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Thu, Feb 9, 2017 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On 2017-02-09 22:15, Tom Lane wrote:
> Corey Huinker <corey.huinker@gmail.com> 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

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.

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

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

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.

On 10 February 2017 at 21:36, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> 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.


I haven't been following this thread but just skimming through it for
the first time I thought this was more baroque than I was expecting. I
was expecting something like a { for each level of nested if you're in
so you can see how many deep you are. I didn't expect to see anything
more complex than that.

-- 
greg

On 11 February 2017 at 23:45, Corey Huinker <corey.huinker@gmail.com> wrote:
> So you'd just want to know nesting depth, with no indicator of true/false?

Even that's more than bash does, for example:

$ if true ; then
> if false ; then
> :
> fi
> fi

I'm a bit confused how the true/false is actually valuable. It doesn't
tell you how the expression actually evaluated, just where you are in
the code you're typing in which you can tell equally well by looking
at what code you're typing in. The reason nesting level is handy is
just to remind you in case you forget.

For debugging scripts it would be handy to have some way to tell
whether the \if expression actually evaluated to true or false but
that wouldn't be in the prompt I don't think.

-- 
greg

Hello Greg,

>> So you'd just want to know nesting depth, with no indicator of true/false?
>
> Even that's more than bash does, for example: [...]

Indeed, there is nothing in "bash" prompt about nesting control 
structures. However other shells have such indications: "zsh" has "%_", 
"tcsh" has "%R". In tcsh for example, there is mention of the structure 
type but none of nesting depth nor truth:
  >    if ( 0 ) then  if?    ...

> I'm a bit confused how the true/false is actually valuable.

The point is just to tell the user that the next command (1) is under an 
if control structure and (2) whether it is going to be executed or 
ignored. That is not too bad in 2 characters.

> It doesn't tell you how the expression actually evaluated,

I do not get your point... t tells that it was true, f that it was false?

> just where you are in the code you're typing in which you can tell 
> equally well by looking at what code you're typing in.
  SELECT ... AS condition \gset  \if :condition ...

The value of the condition is not obvious from the code, it depends on the 
database state.

> The reason nesting level is handy is just to remind you in case you 
> forget.

Sure, that can be useful too.

> For debugging scripts it would be handy to have some way to tell
> whether the \if expression actually evaluated to true or false but
> that wouldn't be in the prompt I don't think.

Are you suggest to add another command to display the current stack state, 
eg "\ifstate" or whatever?

"\if" is really about scripting, so the idea was to have something quite 
light for interactive debugging, especially to help the user not to be 
stuck into a false branch, hence the prompt information with t/f/z.

What should be in the prompt is indeed debatable: existence, nesting 
depth, current truth value, part of the stack... I think that something, 
whatever it is, is necessary.

Maybe this can be a discussed in a follow-up patch and Corey should 
proceed to finalize the if patch?

-- 
Fabien

>> Maybe this can be a discussed in a follow-up patch and Corey should
>> proceed to finalize the if patch?
>
> In the event that we can leave prompting to a later patch, here are the v12
> highlights:

My 0.02€ about v12: Patch applies, make check ok, psql make check ok.

> - created conditional.h and conditional.c which contain the functions with
> stack-ish push/pop/peek/poke names

Why not.

> - now all non-test, non-doc changes are in src/bin/psql

Hmmm, see below.

> - moved conditional stack out of scan_state, stack state maintained by
> mainloop.c/startup.c, passed to HandleSlashCommands

ISTM that it is kind of a regression, because logically this is about the 
scan state so it should be in the corresponding structure, and having two 
structures to pass the scan state is not an improvement...

> - documentation encourages the user to employ ON_ERROR_STOP when using
> conditionals

Indeed. The paragraph explanations are clear enough to me.

I would suggest to also apply the advice to the example shown, including a 
comment about why the variable is set on.

Also, the last element of the tap tests should be distinct: I suggest to 
use 'if syntax error' and 'elif syntax error' in place of 'syntax error' 
for the two first tests.

-- 
Fabien.

On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO <coelho@cri.ensmp.fr> 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

Hello Corey,

> That effort was creating as many headaches as it solved. Rolled back v12 
> except for doc paragraph, added more descriptive tap test names. Enjoy.

The TAP tests are missing from the patch, not sure why...

The stack cleanup is nicer with pop calls.

Two debatable suggestions about the example: - put the on_error_stop as the very first thing in the example? - add a
commentexplaining what the SELECT ... \gset does?
 

-- 
Fabien.

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=>
\echo3  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.

Hello Corey,

> I went back to master and re-applied v11, something must have gotten lost
> in translation.

Probably you need "git add" for added files?

About v14: patch applies, make check ok, psql tap tests ok.

All seems fine to me. Test coverage is better than a lot of other 
features. Code & comments seem fine. Doc and example are clear enough to 
me.

The level of messages in interactive is terse but informative when needed, 
on errors and when commands are ignored. The only missing point is about 
doing something to the prompt, but given the current messages ISTM that 
this can wait for a follow-up patch. Robert Haas advice is to keep it 
simple and short and in %R. There was also some suggestion to have a "show 
the stack" command for debug, I think that this can wait as well.

I've turned again the CF entry to "ready for committers", to see what 
committers thing about this new and simplified version.

-- 
Fabien.

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

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

On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO <coelho@cri.ensmp.fr> 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

Corey Huinker <corey.huinker@gmail.com> writes:
> So moving the conditional stack back into PsqlScanState has some side
> effects: conditional.[ch] have to move to the fe_utils/ dirs, and now
> pgbench, which does not use conditionals, would have to link to them. Is
> that a small price to pay for modularity and easier-to-find code? Or should
> I just tuck it back into psqlscan_int.[ch]?

Pardon me for coming in late, but what in the world has this to do with
the lexer's state at all?  IOW, I don't think I like either of what you're
suggesting ...
        regards, tom lane

Hello Tom,

>> So moving the conditional stack back into PsqlScanState has some side
>> effects: conditional.[ch] have to move to the fe_utils/ dirs, and now
>> pgbench, which does not use conditionals, would have to link to them. Is
>> that a small price to pay for modularity and easier-to-find code? Or should
>> I just tuck it back into psqlscan_int.[ch]?
>
> Pardon me for coming in late, but what in the world has this to do with
> the lexer's state at all?  IOW, I don't think I like either of what you're
> suggesting ...

The "lexer" state holds the stuff useful to psql to know where commands 
start and stop, to process backslash commands, including counting 
parenthesis and nested comments and so on... It seems logical to put the 
"if" stack there as well, but if you think that it should be somewhere 
else, please advise Corey about where to put it.

-- 
Fabien.

Corey Huinker <corey.huinker@gmail.com> writes:
>> but if you think that it should be somewhere else, please advise Corey
>> about where to put it.

> Just a reminder that I'm standing by for advice.

Sorry, I'd lost track of this thread.

> The issue at hand is whether the if-state should be a part of the
> PsqlScanState, or if it should be a separate state variable owned by
> MainLoop() and passed to HandleSlashCommands(), ... or some other solution.

My reaction to putting it in PsqlScanState is pretty much "over my dead
body".  That's just trashing any pretense of an arms-length separation
between psql and the lexer proper.  We only recently got done sweating
blood to create that separation, why would we toss it away already?

If you're concerned about the notational overhead of passing two arguments
rather than one, my druthers would be to invent a new struct type, perhaps
named along the lines of PsqlFileState or PsqlInputState, and pass that
around.  One of its fields would be a PsqlScanState pointer, the rest
would be for if-state and whatever else we think we need in per-input-file
state.

However, that way is doubling down on the assumption that the if-state is
exactly one-to-one with input file levels, isn't it?  We might be better
off to just live with the separate arguments to preserve some flexibility
in that regard.  The v12 patch doesn't look that awful in terms of what
it's adding to argument lists.

One thing I'm wondering is why the "active_branch" bool is in "pset"
and not in the conditional stack.  That seems, at best, pretty grotty.
_psqlSettings is meant for reasonably persistent state.
        regards, tom lane

Corey Huinker <corey.huinker@gmail.com> writes:
> On Wed, Feb 22, 2017 at 4:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> One thing I'm wondering is why the "active_branch" bool is in "pset"
>> and not in the conditional stack.  That seems, at best, pretty grotty.
>> _psqlSettings is meant for reasonably persistent state.

> With the if-stack moved to MainLoop(), nearly all the active_branch checks
> could be against a variable that lives in MainLoop(), with two big
> exceptions: GetVariable() needs to know when NOT to expand a variable
> because it's in a false-block, and get_prompt will need to know when it's
> in a false block for printing the '@' prompt hint or equivalent, and pset
> is the only global around I know of to do that.

Dunno, that sounds a lot like an "if the only tool I have is a hammer,
then this must be a nail" argument.  pset should not accrete every single
global variable in psql just because it's there.  Actually, there's a
pretty fair amount of stuff in it already that should not be there by any
reasonable interpretation of what it's for.  Inventing a PsqlFileState or
similar struct might be a good idea to help pull some of that cruft out of
pset and get it back to having a reasonably clearly defined purpose of
holding "current settings".

So I think that if you're intent on this being a global variable, it might
as well be a standalone global variable.  I was wondering more about
whether we shouldn't be passing the condition-stack top pointer around
to places that need to know about conditional execution.  get_prompt would
be one if we decide that the prompt might need to reflect this (a question
that still seems undecided to me --- I think we'd be better off with "this
command was ignored" warning messages).  I'm failing to follow why
GetVariable would need to care.
        regards, tom lane

Corey Huinker <corey.huinker@gmail.com> writes:
> After some research, GetVariable is called by psql_get_variable, which is
> one of the callback functions passed to psql_scan_create(). So passing a
> state variable around probably isn't going to work and PsqlFileState now
> looks like the best option.

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

Anyway, what this seems to point up is that maybe we should've allowed
for a passthrough "void *" argument to the psqlscan callback functions.
There wasn't one in the original design but it's a fairly standard part
of our usual approach to callback functions, so it's hard to see an
objection to adding one now.
        regards, tom lane

> Welcome to v15, highlights:

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

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

-- 
Fabien.

    Tom Lane wrote:

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

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

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

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


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

Hello Daniel,

>> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>> arguing that variable expansion shouldn't be able to insert, say, an \else
>> in a non-active branch?  Maybe, but if it can insert an \else in an active
>> branch, then why not non-active too?  Seems a bit inconsistent.
>
> Are we sold on the idea that conditionals should be implemented
> by meta-commands, rather than for example terminal symbols of
> a new grammar on top of the existing?

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

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

-- 
Fabien.

Hello Corey,

> v16 is everything v15 promised to be.

My 0.02€:

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

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

Indeed.

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

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

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

Same.

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

Same.

>> - PsqlScanState no longer has branching state

Indeed.

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

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

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

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

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

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

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

>> - ConditionalStack could morph into PsqlFileState without too much
>>   work.

Probably.

Code details:

Add space after comma when calling send_query.

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

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

-- 
Fabien.

About v17:

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

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

Ok.

>> It could be nice to keep test cases that show what may happen?
>
> Restored. It looks weird now, but it fixes the unterminated quoted 
> string.

Typo "unterminted".

I think I found an issue while testing in interactive:
 calvin=# \if false calvin@#   \if false calvin@#     \echo false-false   command ignored, use \endif or Ctrl-C to exit
currentbranch. calvin@#   \endif calvin=#   \echo in false   in false
 

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

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

>> Maybe the documentation could add some kind of warning about that?
>
> I changed the paragraph to

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

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

Maybe the sentence could be cut in shorter pieces.

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

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

Ok.

-- 
Fabien.

Hello Corey,

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


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

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

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

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


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

-- 
Fabien.

Hello Corey,

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

Hmmm.

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

My 0.02€ about v19:

If there are two errors, I do not care which one is shown, both will have 
to be fixed anyway in the end... So I would suggest to choose the simplest 
possible implementation:
  on elif:    always eval expression      => possible eval error    switch      => including detecting misplaced elif
errors

If the second error must absolutely be shown in all cases, then add a 
second misplaced elif detection in the eval expression failure branch:
  on elif    always eval    if (eval failed)      also checked for misplaced (hey user, you have 2 errors in fact...)
  bye bye...    // else eval was fine    switch      including misplaced elif detection
 

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

-- 
Fabien.

Hello Corey,

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

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

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

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

Hmmm.

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

The case we are discussing is for the user who decides to write code with 
*two* errors on the same line:
  \if good-condition  \else  \elif bad-condition  \endif

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

Maybe an alternative could be to write simpler code anyway, somehow like 
it was before:
  // on "elif"  switch (peek(state)) {  case NONE:       error;  case ELSE_TRUE:  error;  case ELSE_FALSE: error;  case
IGNORED:   break;  case TRUE:       poke IGNORED;  case FALSE:                   success = evaluate(&is_true)
       if (!success)                     error;                   else if (is_true)                       poke TRUE
default:        error;  }
 

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

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

-- 
Fabien.

Hello Corey,

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

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

>> Now if you want to require committer opinion on this one, fine with me.
>
> Rather than speculate on what a committer thinks of this edge case (and
> making a patch for each possible theory), I'd rather just ask them what
> their priorities are and which user experience they favor.

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

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

-- 
Fabien.

Hello Corey,

> Tom was pretty adamant that invalid commands are not executed. So in a case
> like this, with ON_ERROR_STOP off:
>
> \if false
> \echo 'a'
> \elif true
> \echo 'b'
> \elif invalid
> \echo 'c'
> \endif
>
> Both 'b' and 'c' should print, because "\elif invalid" should not execute.
> The code I had before was simpler, but it missed that.

Hmmm. You can still have it with one switch, by repeating the evaluation 
under true and ignore, even if the value is not used:
  switch(state)  {    case NONE: error;    case ELSE_TRUE: error;    case ELSE_FALSE: error;    case IF_TRUE:        if
(eval())         ...        else error;        break;    case IF_FALSE:        if (eval())          ...        else
error;       break;    case IGNORE:        if (eval())          ...        else error;        break;    }
 

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

For me, it is only slightly better: I think that for helping understanding 
and maintenance, the automaton state transitions should be all clear and 
loud in just one place, so I would really like to see a single common 
structure:
  if (is "if") switch on all states;  else if (is "elif") switch on all states;  else if (is "else") switch on all
states; else if (is "endif") switch on all states;
 

And minimal necessary error handling around that.

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

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

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

-- 
Fabien.

Hello Corey,

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

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

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

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

-- 
Fabien.

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

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

-- 
Fabien.

About v21:

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

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

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

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

I've turned the patch as ready, again.

-- 
Fabien.

Corey Huinker <corey.huinker@gmail.com> writes:
> [ 0001.if_endif.v21.diff ]

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

Couldn't we get close to the same coverage by adding a single-purpose
test script to the main regression tests?  Along the lines of
    \set ON_ERROR_STOP 1    \if invalid    \echo should not get here    \endif    \echo should not get here either

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

Corey Huinker <corey.huinker@gmail.com> writes:
> [ 0001.if_endif.v21.diff ]

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

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

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

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

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

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

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

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

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

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

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

(notice we're out of the conditional)

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

(notice we're not out of the conditional)

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

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

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

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

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

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

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

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

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

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

That happens because the \if code gratuituously resets the lexer,
as we can see from the unexpected change in the prompt.

* I'm not on board with having a bad expression result in failing
the \if or \elif altogether.  It was stated several times upthread
that that should be processed as though the result were "false",
and I agree with that.  As it stands, it's completely impossible to
write script code that can cope with possibly-failing expressions,
or even to reason very clearly about what will happen: you can't
know whether a following \else will be honored, for example.
We might as well replace the recommendation to use ON_ERROR_STOP with
a forced abort() for an invalid expression value, because trying to
continue a script with this behavior is entirely useless.


I did some work on the patch before reaching these conclusions,
mostly improving the documentation, getting rid of some unnecessary
#include's, etc.  I've attached that work as far as it went.

            regards, tom lane

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


        <varlistentry>
+         <term><literal>\if</literal> <replaceable class="parameter">expression</replaceable></term>
+         <term><literal>\elif</literal> <replaceable class="parameter">expression</replaceable></term>
+         <term><literal>\else</literal></term>
+         <term><literal>\endif</literal></term>
+         <listitem>
+         <para>
+         This group of commands implements nestable conditional blocks.
+         A conditional block must begin with an <command>\if</command> and end
+         with an <command>\endif</command>.  In between there may be any number
+         of <command>\elif</command> clauses, which may optionally be followed
+         by a single <command>\else</command> clause.  Ordinary queries and
+         other types of backslash commands may (and usually do) appear between
+         the commands forming a conditional block.
+         </para>
+         <para>
+         The <command>\if</command> and <command>\elif</command> commands read
+         the rest of the line and evaluate it as a boolean expression.  If the
+         expression is <literal>true</literal> then processing continues
+         normally; otherwise, lines are skipped until a
+         matching <command>\elif</command>, <command>\else</command>,
+         or <command>\endif</command> is reached.  Once
+         an <command>\if</command> or <command>\elif</command> has succeeded,
+         later matching <command>\elif</command> commands are not evaluated but
+         are treated as false.  Lines following an <command>\else</command> are
+         processed only if no earlier matching <command>\if</command>
+         or <command>\elif</command> succeeded.
+         </para>
+         <para>
+         Lines being skipped are parsed normally to identify queries and
+         backslash commands, but queries are not sent to the server, and
+         backslash commands other than conditionals
+         (<command>\if</command>, <command>\elif</command>,
+         <command>\else</command>, <command>\endif</command>) are
+         ignored.  Conditional commands are checked only for valid nesting.
+         </para>
+         <para>
+         The <replaceable class="parameter">expression</replaceable> argument
+         of <command>\if</command> or <command>\elif</command>
+         is subject to variable interpolation and backquote expansion, just
+         like any other backslash command argument.  After that it is evaluated
+         like the value of an on/off option variable.  So a valid value
+         is any unambiguous case-insensitive match for one of:
+         <literal>true</literal>, <literal>false</literal>, <literal>1</literal>,
+         <literal>0</literal>, <literal>on</literal>, <literal>off</literal>,
+         <literal>yes</literal>, <literal>no</literal>.  For example,
+         <literal>t</literal>, <literal>T</literal>, and <literal>tR</literal>
+         will all be considered to be <literal>true</literal>.
+         </para>
+         <para>
+         Expressions that do not properly evaluate to true or false will
+         generate an error and cause the <command>\if</command> or
+         <command>\elif</command> command to fail.  Because that behavior may
+         change branching context in undesirable ways (executing code which
+         was intended to be skipped, causing <command>\elif</command>,
+         <command>\else</command>, and <command>\endif</command> commands to
+         pair with the wrong <command>\if</command>, etc), it is
+         recommended that scripts that use conditionals also set
+         <varname>ON_ERROR_STOP</varname>.
+         </para>
+         <para>
+         All the backslash commands of a given conditional block must appear in
+         the same source file. If EOF is reached on the main input file or an
+         <command>\include</command>-ed file before all local
+         <command>\if</command>-blocks have been closed,
+         then <application>psql</> will raise an error.
+         </para>
+         <para>
+          Here is an example:
+         </para>
+ <programlisting>
+ -- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+ \set ON_ERROR_STOP on
+ -- check for the existence of two separate records in the database and store
+ -- the results in separate psql variables
+ SELECT
+     EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+     EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+ \gset
+ \if :is_customer
+     SELECT * FROM customer WHERE customer_id = 123;
+ \elif :is_employee
+     \echo 'is not a customer but is an employee'
+     SELECT * FROM employee WHERE employee_id = 456;
+ \else
+     \if yes
+         \echo 'not a customer or employee'
+     \else
+         \echo 'this will never print'
+     \endif
+ \endif
+ </programlisting>
+         </listitem>
+       </varlistentry>
+
+
+       <varlistentry>
          <term><literal>\l[+]</literal> or <literal>\list[+] [ <link linkend="APP-PSQL-patterns"><replaceable
class="parameter">pattern</replaceable></link>]</literal></term> 
          <listitem>
          <para>
*************** testdb=> <userinput>INSERT INTO my_ta
*** 3715,3721 ****
          <listitem>
          <para>
          In prompt 1 normally <literal>=</literal>,
!         but <literal>^</literal> if in single-line mode,
          or <literal>!</literal> if the session is disconnected from the
          database (which can happen if <command>\connect</command> fails).
          In prompt 2 <literal>%R</literal> is replaced by a character that
--- 3811,3818 ----
          <listitem>
          <para>
          In prompt 1 normally <literal>=</literal>,
!         but <literal>@</literal> if the session is in a false conditional
!         block, or <literal>^</literal> if in single-line mode,
          or <literal>!</literal> if the session is disconnected from the
          database (which can happen if <command>\connect</command> fails).
          In prompt 2 <literal>%R</literal> is replaced by a character that
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b1..5239013 100644
*** a/src/bin/psql/.gitignore
--- b/src/bin/psql/.gitignore
***************
*** 3,5 ****
--- 3,7 ----
  /sql_help.c

  /psql
+
+ /tmp_check/
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index f8e31ea..90ee85d 100644
*** a/src/bin/psql/Makefile
--- b/src/bin/psql/Makefile
*************** REFDOCDIR= $(top_srcdir)/doc/src/sgml/re
*** 21,30 ****
  override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
  LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq

! OBJS=    command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
      startup.o prompt.o variables.o large_obj.o describe.o \
      crosstabview.o tab-complete.o \
!     sql_help.o psqlscanslash.o \
      $(WIN32RES)


--- 21,30 ----
  override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
  LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq

! OBJS=    command.o common.o conditional.o copy.o help.o input.o mainloop.o \
      startup.o prompt.o variables.o large_obj.o describe.o \
      crosstabview.o tab-complete.o \
!     sql_help.o stringutils.o psqlscanslash.o \
      $(WIN32RES)


*************** uninstall:
*** 57,64 ****
--- 57,71 ----

  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 4f4a0aa..359da08 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** typedef enum EditableObjectType
*** 59,64 ****
--- 59,65 ----
  /* functions for use in this file */
  static backslashResult exec_command(const char *cmd,
               PsqlScanState scan_state,
+              ConditionalStack cstack,
               PQExpBuffer query_buf);
  static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
          int lineno, bool *edited);
*************** static void checkWin32Codepage(void);
*** 106,111 ****
--- 107,113 ----

  backslashResult
  HandleSlashCmds(PsqlScanState scan_state,
+                 ConditionalStack cstack,
                  PQExpBuffer query_buf)
  {
      backslashResult status = PSQL_CMD_SKIP_LINE;
*************** HandleSlashCmds(PsqlScanState scan_state
*** 118,124 ****
      cmd = psql_scan_slash_command(scan_state);

      /* And try to execute it */
!     status = exec_command(cmd, scan_state, query_buf);

      if (status == PSQL_CMD_UNKNOWN)
      {
--- 120,126 ----
      cmd = psql_scan_slash_command(scan_state);

      /* And try to execute it */
!     status = exec_command(cmd, scan_state, cstack, query_buf);

      if (status == PSQL_CMD_UNKNOWN)
      {
*************** HandleSlashCmds(PsqlScanState scan_state
*** 129,135 ****
          status = PSQL_CMD_ERROR;
      }

!     if (status != PSQL_CMD_ERROR)
      {
          /* eat any remaining arguments after a valid command */
          /* note we suppress evaluation of backticks here */
--- 131,137 ----
          status = PSQL_CMD_ERROR;
      }

!     if (status != PSQL_CMD_ERROR && conditional_active(cstack))
      {
          /* eat any remaining arguments after a valid command */
          /* note we suppress evaluation of backticks here */
*************** read_connect_arg(PsqlScanState scan_stat
*** 191,196 ****
--- 193,222 ----
      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);
+     bool    success = 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 command.
*************** read_connect_arg(PsqlScanState scan_stat
*** 198,209 ****
--- 224,248 ----
  static backslashResult
  exec_command(const char *cmd,
               PsqlScanState scan_state,
+              ConditionalStack cstack,
               PQExpBuffer query_buf)
  {
      bool        success = true; /* indicate here if the command ran ok or
                                   * failed */
      backslashResult status = PSQL_CMD_SKIP_LINE;

+     if (!conditional_active(cstack) && !is_branching_command(cmd))
+     {
+         if (pset.cur_cmd_interactive)
+             psql_error("command ignored, use \\endif or Ctrl-C to exit "
+                         "current branch.\n");
+
+         /* Continue with an empty buffer as if the command were never read */
+         resetPQExpBuffer(query_buf);
+         psql_scan_reset(scan_state);
+         return status;
+     }
+
      /*
       * \a -- toggle field alignment This makes little sense but we keep it
       * around.
*************** exec_command(const char *cmd,
*** 1008,1013 ****
--- 1047,1202 ----
          }
      }

+     /*
+      * \if <expr> is the beginning of an \if..\endif block. <expr> must be a
+      * valid boolean expression, or the command will be ignored. If this \if
+      * is itself a part of a branch that is false/ignored, the expression
+      * will be checked for validity but cannot override the outer block.
+      */
+     else if (strcmp(cmd, "if") == 0)
+     {
+         bool if_true = false;
+         ifState new_if_state;
+         success = read_boolean_expression(scan_state, "\\if <expr>",
+                                             &if_true);
+
+         switch (conditional_stack_peek(cstack))
+         {
+             case IFSTATE_IGNORED:
+             case IFSTATE_FALSE:
+             case IFSTATE_ELSE_FALSE:
+                 /* new if-block, expression result is ignored */
+                 new_if_state = IFSTATE_IGNORED;
+                 break;
+             default:
+                 /* new if-block, expression result matters */
+                 new_if_state = (if_true) ? IFSTATE_TRUE : IFSTATE_FALSE;
+                 break;
+         }
+
+         /* only start if a new if-block if the expression was valid */
+         if (success)
+             conditional_stack_push(cstack, new_if_state);
+
+         psql_scan_reset(scan_state);
+     }
+
+     /*
+      * \elif <expr> is part of an \if..\endif block. <expr> must be a valid
+      * boolean expression, or the command will be ignored.
+      */
+     else if (strcmp(cmd, "elif") == 0)
+     {
+         bool elif_true = false;
+         switch (conditional_stack_peek(cstack))
+         {
+             case IFSTATE_IGNORED:
+                 /*
+                  * inactive branch, only test for valid expression.
+                  * either if-endif already had a true block,
+                  * or whole parent block is false.
+                  */
+                 success = read_boolean_expression(scan_state, "\\elif <expr>",
+                                                     &elif_true);
+                 break;
+             case IFSTATE_TRUE:
+                 /*
+                  * just finished true section of this if-endif, test for valid
+                  * expression, but then ignore the rest until \endif
+                  */
+                 success = read_boolean_expression(scan_state, "\\elif <expr>",
+                                                     &elif_true);
+                 if (success)
+                     conditional_stack_poke(cstack, IFSTATE_IGNORED);
+                 break;
+             case IFSTATE_FALSE:
+                 /*
+                  * have not yet found a true block in this if-endif,
+                  * this might be the first.
+                  */
+                 success = read_boolean_expression(scan_state, "\\elif <expr>",
+                                                     &elif_true);
+                 if (success && elif_true)
+                     conditional_stack_poke(cstack, IFSTATE_TRUE);
+                 break;
+             case IFSTATE_NONE:
+                 /* no if to elif from */
+                 psql_error("\\elif: no matching \\if\n");
+                 success = false;
+                 break;
+             case IFSTATE_ELSE_TRUE:
+             case IFSTATE_ELSE_FALSE:
+                 psql_error("\\elif: cannot occur after \\else\n");
+                 success = false;
+                 break;
+             default:
+                 break;
+         }
+         psql_scan_reset(scan_state);
+     }
+
+     /*
+      * \else is part of an \if..\endif block
+      * the statements within an \else branch will only be executed if
+      * all previous \if and \endif expressions evaluated to false
+      * and the block was not itself being ignored.
+      */
+     else if (strcmp(cmd, "else") == 0)
+     {
+         switch (conditional_stack_peek(cstack))
+         {
+             case IFSTATE_FALSE:
+                 /* just finished false section of an active branch */
+                 conditional_stack_poke(cstack, IFSTATE_ELSE_TRUE);
+                 break;
+             case IFSTATE_TRUE:
+             case IFSTATE_IGNORED:
+                 /*
+                  * either just finished true section of an active branch,
+                  * or whole branch was inactive. either way, be on the
+                  * lookout for any invalid \endif or \else commands
+                  */
+                 conditional_stack_poke(cstack, IFSTATE_ELSE_FALSE);
+                 break;
+             case IFSTATE_NONE:
+                 /* no if to else from */
+                 psql_error("\\else: no matching \\if\n");
+                 success = false;
+                 break;
+             case IFSTATE_ELSE_TRUE:
+             case IFSTATE_ELSE_FALSE:
+                 psql_error("\\else: cannot occur after \\else\n");
+                 success = false;
+                 break;
+             default:
+                 break;
+         }
+         psql_scan_reset(scan_state);
+     }
+
+     /*
+      * \endif - closing statment of an \if...\endif block
+      */
+     else if (strcmp(cmd, "endif") == 0)
+     {
+         /*
+          * get rid of this ifstate element and look at the previous
+          * one, if any
+          */
+         switch (conditional_stack_peek(cstack))
+         {
+             case IFSTATE_NONE:
+                 psql_error("\\endif: no matching \\if\n");
+                 success = false;
+                 break;
+             default:
+                 success = conditional_stack_pop(cstack);
+                 Assert(success);
+                 break;
+         }
+         psql_scan_reset(scan_state);
+     }
+
      /* \l is list databases */
      else if (strcmp(cmd, "l") == 0 || strcmp(cmd, "list") == 0 ||
               strcmp(cmd, "l+") == 0 || strcmp(cmd, "list+") == 0)
diff --git a/src/bin/psql/command.h b/src/bin/psql/command.h
index d0c3264..a396f29 100644
*** a/src/bin/psql/command.h
--- b/src/bin/psql/command.h
***************
*** 10,15 ****
--- 10,16 ----

  #include "fe_utils/print.h"
  #include "fe_utils/psqlscan.h"
+ #include "conditional.h"


  typedef enum _backslashResult
*************** typedef enum _backslashResult
*** 25,30 ****
--- 26,32 ----


  extern backslashResult HandleSlashCmds(PsqlScanState scan_state,
+                 ConditionalStack cstack,
                  PQExpBuffer query_buf);

  extern int    process_file(char *filename, bool use_relative_path);
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 1aa56ab..83ac284 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*************** setQFout(const char *fname)
*** 119,124 ****
--- 119,129 ----
   * If "escape" is true, return the value suitably quoted and escaped,
   * as an identifier or string literal depending on "as_ident".
   * (Failure in escaping should lead to returning NULL.)
+  *
+  * Variables are not expanded if the current branch is inactive
+  * (part of an \if..\endif section which is false). \elif branches
+  * will need temporarily mark the branch active in order to
+  * properly evaluate conditionals.
   */
  char *
  psql_get_variable(const char *varname, bool escape, bool as_ident)
diff --git a/src/bin/psql/conditional.c b/src/bin/psql/conditional.c
index ...8643ff1 .
*** a/src/bin/psql/conditional.c
--- b/src/bin/psql/conditional.c
***************
*** 0 ****
--- 1,103 ----
+ /*
+  * psql - the PostgreSQL interactive terminal
+  *
+  * Copyright (c) 2000-2017, PostgreSQL Global Development Group
+  *
+  * src/bin/psql/conditional.c
+  */
+ #include "postgres_fe.h"
+
+ #include "conditional.h"
+
+ /*
+  * create stack
+  */
+ ConditionalStack
+ conditional_stack_create(void)
+ {
+     ConditionalStack cstack = pg_malloc(sizeof(ConditionalStackData));
+     cstack->head = NULL;
+     return cstack;
+ }
+
+ /*
+  * destroy stack
+  */
+ void
+ conditional_stack_destroy(ConditionalStack cstack)
+ {
+     while (conditional_stack_pop(cstack))
+         continue;
+     free(cstack);
+ }
+
+ /*
+  * Create a new conditional branch.
+  */
+ void
+ conditional_stack_push(ConditionalStack cstack,    ifState new_state)
+ {
+     IfStackElem *p = (IfStackElem*) pg_malloc(sizeof(IfStackElem));
+     p->if_state = new_state;
+     p->next = cstack->head;
+     cstack->head = p;
+ }
+
+ /*
+  * Destroy the topmost conditional branch.
+  * Returns false if there was no branch to end.
+  */
+ bool
+ conditional_stack_pop(ConditionalStack cstack)
+ {
+     IfStackElem *p = cstack->head;
+     if (!p)
+         return false;
+     cstack->head = cstack->head->next;
+     free(p);
+     return true;
+ }
+
+ /*
+  * Fetch the current state of the top of the stack
+  */
+ ifState
+ conditional_stack_peek(ConditionalStack cstack)
+ {
+     if (conditional_stack_empty(cstack))
+         return IFSTATE_NONE;
+     return cstack->head->if_state;
+ }
+
+ /*
+  * Change the state of the topmost branch.
+  * Returns false if there was no branch state to set.
+  */
+ bool
+ conditional_stack_poke(ConditionalStack cstack, ifState new_state)
+ {
+     if (conditional_stack_empty(cstack))
+         return false;
+     cstack->head->if_state = new_state;
+     return true;
+ }
+
+ /*
+  * True if there are no active \if-blocks
+  */
+ bool
+ conditional_stack_empty(ConditionalStack cstack)
+ {
+     return cstack->head == NULL;
+ }
+
+ /*
+  * True if the current conditional block is active, or if there is no
+  * open \if (ie, we should execute commands normally)
+  */
+ bool
+ conditional_active(ConditionalStack cstack)
+ {
+     ifState s = conditional_stack_peek(cstack);
+     return s == IFSTATE_NONE || s == IFSTATE_TRUE || s == IFSTATE_ELSE_TRUE;
+ }
diff --git a/src/bin/psql/conditional.h b/src/bin/psql/conditional.h
index ...96dbd7f .
*** a/src/bin/psql/conditional.h
--- b/src/bin/psql/conditional.h
***************
*** 0 ****
--- 1,62 ----
+ /*
+  * psql - the PostgreSQL interactive terminal
+  *
+  * Copyright (c) 2000-2017, PostgreSQL Global Development Group
+  *
+  * src/bin/psql/conditional.h
+  */
+ #ifndef CONDITIONAL_H
+ #define CONDITIONAL_H
+
+ /*
+  * Possible states of a single level of \if block.
+  */
+ typedef enum ifState
+ {
+     IFSTATE_NONE = 0,    /* Not currently in an \if block */
+     IFSTATE_TRUE,        /* currently in an \if or \elif which is true
+                          * and all parent branches (if any) are true */
+     IFSTATE_FALSE,        /* currently in an \if or \elif which is false
+                          * but no true branch has yet been seen,
+                          * and all parent branches (if any) are true */
+     IFSTATE_IGNORED,    /* currently in an \elif which follows a true \if
+                          * or the whole \if is a child of a false parent */
+     IFSTATE_ELSE_TRUE,    /* currently in an \else which is true
+                          * and all parent branches (if any) are true */
+     IFSTATE_ELSE_FALSE    /* currently in an \else which is false or ignored */
+ } ifState;
+
+ /*
+  * The state of nested \ifs is stored in a stack.
+  */
+ typedef struct IfStackElem
+ {
+     ifState        if_state;
+     struct IfStackElem *next;
+ } IfStackElem;
+
+ typedef struct ConditionalStackData
+ {
+     IfStackElem    *head;
+ } ConditionalStackData;
+
+ typedef struct ConditionalStackData *ConditionalStack;
+
+
+ extern ConditionalStack conditional_stack_create(void);
+
+ extern void conditional_stack_destroy(ConditionalStack cstack);
+
+ extern bool conditional_stack_empty(ConditionalStack cstack);
+
+ extern void conditional_stack_push(ConditionalStack cstack, ifState new_state);
+
+ extern bool conditional_stack_pop(ConditionalStack cstack);
+
+ extern ifState conditional_stack_peek(ConditionalStack cstack);
+
+ extern bool conditional_stack_poke(ConditionalStack cstack, ifState new_state);
+
+ extern bool conditional_active(ConditionalStack cstack);
+
+ #endif   /* CONDITIONAL_H */
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 481031a..2005b9a 100644
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
*************** handleCopyIn(PGconn *conn, FILE *copystr
*** 552,558 ****
          /* interactive input probably silly, but give one prompt anyway */
          if (showprompt)
          {
!             const char *prompt = get_prompt(PROMPT_COPY);

              fputs(prompt, stdout);
              fflush(stdout);
--- 552,558 ----
          /* interactive input probably silly, but give one prompt anyway */
          if (showprompt)
          {
!             const char *prompt = get_prompt(PROMPT_COPY, NULL);

              fputs(prompt, stdout);
              fflush(stdout);
*************** handleCopyIn(PGconn *conn, FILE *copystr
*** 590,596 ****

              if (showprompt)
              {
!                 const char *prompt = get_prompt(PROMPT_COPY);

                  fputs(prompt, stdout);
                  fflush(stdout);
--- 590,596 ----

              if (showprompt)
              {
!                 const char *prompt = get_prompt(PROMPT_COPY, NULL);

                  fputs(prompt, stdout);
                  fflush(stdout);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index ba14df0..79afafb 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*************** slashUsage(unsigned short int pager)
*** 210,215 ****
--- 210,222 ----
      fprintf(output, _("  \\qecho [STRING]        write string to query output stream (see \\o)\n"));
      fprintf(output, "\n");

+     fprintf(output, _("Conditionals\n"));
+     fprintf(output, _("  \\if <expr>             begin a conditional block\n"));
+     fprintf(output, _("  \\elif <expr>           else if in the current conditional block\n"));
+     fprintf(output, _("  \\else                  else in the current conditional block\n"));
+     fprintf(output, _("  \\endif                 end current conditional block\n"));
+     fprintf(output, "\n");
+
      fprintf(output, _("Informational\n"));
      fprintf(output, _("  (options: S = show system objects, + = additional detail)\n"));
      fprintf(output, _("  \\d[S+]                 list tables, views, and sequences\n"));
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 6e358e2..47d6c50 100644
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*************** const PsqlScanCallbacks psqlscan_callbac
*** 25,30 ****
--- 25,47 ----


  /*
+  * execute query if branch is active.
+  * warn interactive users about ignored queries.
+  */
+ static bool
+ send_query(const char *query, ConditionalStack cstack)
+ {
+     /* execute query if branch is active */
+     if (conditional_active(cstack))
+         return SendQuery(query);
+
+     if (pset.cur_cmd_interactive)
+         psql_error("query ignored; use \\endif or Ctrl-C to exit current \\if branch\n");
+
+     return true;
+ }
+
+ /*
   * Main processing loop for reading lines of input
   *    and sending them to the backend.
   *
*************** int
*** 35,40 ****
--- 52,58 ----
  MainLoop(FILE *source)
  {
      PsqlScanState scan_state;    /* lexer working state */
+     ConditionalStack cond_stack;    /* \if status stack */
      volatile PQExpBuffer query_buf;        /* buffer for query being accumulated */
      volatile PQExpBuffer previous_buf;    /* if there isn't anything in the new
                                           * buffer yet, use this one for \e,
*************** MainLoop(FILE *source)
*** 69,74 ****
--- 86,92 ----

      /* Create working state */
      scan_state = psql_scan_create(&psqlscan_callbacks);
+     cond_stack = conditional_stack_create();

      query_buf = createPQExpBuffer();
      previous_buf = createPQExpBuffer();
*************** MainLoop(FILE *source)
*** 122,128 ****
--- 140,157 ----
              cancel_pressed = false;

              if (pset.cur_cmd_interactive)
+             {
                  putc('\n', stdout);
+                 /*
+                  * if interactive user is in a branch, then Ctrl-C will exit
+                  * from the inner-most branch
+                  */
+                 if (!conditional_stack_empty(cond_stack))
+                 {
+                     psql_error("\\if: escaped\n");
+                     conditional_stack_pop(cond_stack);
+                 }
+             }
              else
              {
                  successResult = EXIT_USER;
*************** MainLoop(FILE *source)
*** 140,146 ****
              /* May need to reset prompt, eg after \r command */
              if (query_buf->len == 0)
                  prompt_status = PROMPT_READY;
!             line = gets_interactive(get_prompt(prompt_status), query_buf);
          }
          else
          {
--- 169,176 ----
              /* May need to reset prompt, eg after \r command */
              if (query_buf->len == 0)
                  prompt_status = PROMPT_READY;
!             line = gets_interactive(get_prompt(prompt_status, cond_stack),
!                                     query_buf);
          }
          else
          {
*************** MainLoop(FILE *source)
*** 297,303 ****
                  }

                  /* execute query */
!                 success = SendQuery(query_buf->data);
                  slashCmdStatus = success ? PSQL_CMD_SEND : PSQL_CMD_ERROR;
                  pset.stmt_lineno = 1;

--- 327,333 ----
                  }

                  /* execute query */
!                 success = send_query(query_buf->data, cond_stack);
                  slashCmdStatus = success ? PSQL_CMD_SEND : PSQL_CMD_ERROR;
                  pset.stmt_lineno = 1;

*************** MainLoop(FILE *source)
*** 343,348 ****
--- 373,379 ----

                  /* execute backslash command */
                  slashCmdStatus = HandleSlashCmds(scan_state,
+                                                  cond_stack,
                                                   query_buf->len > 0 ?
                                                   query_buf : previous_buf);

*************** MainLoop(FILE *source)
*** 358,364 ****

                  if (slashCmdStatus == PSQL_CMD_SEND)
                  {
!                     success = SendQuery(query_buf->data);

                      /* transfer query to previous_buf by pointer-swapping */
                      {
--- 389,395 ----

                  if (slashCmdStatus == PSQL_CMD_SEND)
                  {
!                     success = send_query(query_buf->data, cond_stack);

                      /* transfer query to previous_buf by pointer-swapping */
                      {
*************** MainLoop(FILE *source)
*** 430,436 ****
              pg_send_history(history_buf);

          /* execute query */
!         success = SendQuery(query_buf->data);

          if (!success && die_on_error)
              successResult = EXIT_USER;
--- 461,467 ----
              pg_send_history(history_buf);

          /* execute query */
!         success = send_query(query_buf->data, cond_stack);

          if (!success && die_on_error)
              successResult = EXIT_USER;
*************** MainLoop(FILE *source)
*** 439,444 ****
--- 470,488 ----
      }

      /*
+      * Check for unbalanced \if-\endifs unless user explicitly quit, or the
+      * script is erroring out
+      */
+     if (slashCmdStatus != PSQL_CMD_TERMINATE &&
+         successResult != EXIT_USER &&
+         !conditional_stack_empty(cond_stack))
+     {
+         psql_error("reached EOF without finding closing \\endif(s)\n");
+         if (die_on_error && !pset.cur_cmd_interactive)
+             successResult = EXIT_USER;
+     }
+
+     /*
       * Let's just make real sure the SIGINT handler won't try to use
       * sigint_interrupt_jmp after we exit this routine.  If there is an outer
       * MainLoop instance, it will reset sigint_interrupt_jmp to point to
*************** MainLoop(FILE *source)
*** 452,457 ****
--- 496,502 ----
      destroyPQExpBuffer(history_buf);

      psql_scan_destroy(scan_state);
+     conditional_stack_destroy(cond_stack);

      pset.cur_cmd_source = prev_cmd_source;
      pset.cur_cmd_interactive = prev_cmd_interactive;
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index f7930c4..e502ff3 100644
*** a/src/bin/psql/prompt.c
--- b/src/bin/psql/prompt.c
***************
*** 66,72 ****
   */

  char *
! get_prompt(promptStatus_t status)
  {
  #define MAX_PROMPT_SIZE 256
      static char destination[MAX_PROMPT_SIZE + 1];
--- 66,72 ----
   */

  char *
! get_prompt(promptStatus_t status, ConditionalStack cstack)
  {
  #define MAX_PROMPT_SIZE 256
      static char destination[MAX_PROMPT_SIZE + 1];
*************** get_prompt(promptStatus_t status)
*** 188,194 ****
                      switch (status)
                      {
                          case PROMPT_READY:
!                             if (!pset.db)
                                  buf[0] = '!';
                              else if (!pset.singleline)
                                  buf[0] = '=';
--- 188,196 ----
                      switch (status)
                      {
                          case PROMPT_READY:
!                             if (cstack != NULL && !conditional_active(cstack))
!                                 buf[0] = '@';
!                             else if (!pset.db)
                                  buf[0] = '!';
                              else if (!pset.singleline)
                                  buf[0] = '=';
diff --git a/src/bin/psql/prompt.h b/src/bin/psql/prompt.h
index 977e754..b3d2d98 100644
*** a/src/bin/psql/prompt.h
--- b/src/bin/psql/prompt.h
***************
*** 10,16 ****

  /* enum promptStatus_t is now defined by psqlscan.h */
  #include "fe_utils/psqlscan.h"

! char       *get_prompt(promptStatus_t status);

  #endif   /* PROMPT_H */
--- 10,17 ----

  /* enum promptStatus_t is now defined by psqlscan.h */
  #include "fe_utils/psqlscan.h"
+ #include "conditional.h"

! char       *get_prompt(promptStatus_t status, ConditionalStack cstack);

  #endif   /* PROMPT_H */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 694f0ef..353d6c8 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** main(int argc, char *argv[])
*** 331,349 ****
              else if (cell->action == ACT_SINGLE_SLASH)
              {
                  PsqlScanState scan_state;

                  if (pset.echo == PSQL_ECHO_ALL)
                      puts(cell->val);

                  scan_state = psql_scan_create(&psqlscan_callbacks);
                  psql_scan_setup(scan_state,
                                  cell->val, strlen(cell->val),
                                  pset.encoding, standard_strings());

!                 successResult = HandleSlashCmds(scan_state, NULL) != PSQL_CMD_ERROR
                      ? EXIT_SUCCESS : EXIT_FAILURE;

                  psql_scan_destroy(scan_state);
              }
              else if (cell->action == ACT_FILE)
              {
--- 331,354 ----
              else if (cell->action == ACT_SINGLE_SLASH)
              {
                  PsqlScanState scan_state;
+                 ConditionalStack cond_stack;

                  if (pset.echo == PSQL_ECHO_ALL)
                      puts(cell->val);

                  scan_state = psql_scan_create(&psqlscan_callbacks);
+                 cond_stack = conditional_stack_create();
                  psql_scan_setup(scan_state,
                                  cell->val, strlen(cell->val),
                                  pset.encoding, standard_strings());

!                 successResult = HandleSlashCmds(scan_state,
!                                                 cond_stack,
!                                                 NULL) != PSQL_CMD_ERROR
                      ? EXIT_SUCCESS : EXIT_FAILURE;

                  psql_scan_destroy(scan_state);
+                 conditional_stack_destroy(cond_stack);
              }
              else if (cell->action == ACT_FILE)
              {
diff --git a/src/bin/psql/t/001_if.pl b/src/bin/psql/t/001_if.pl
index ...8180dab .
*** a/src/bin/psql/t/001_if.pl
--- b/src/bin/psql/t/001_if.pl
***************
*** 0 ****
--- 1,42 ----
+ use strict;
+ use warnings;
+
+ use Config;
+ use PostgresNode;
+ use TestLib;
+ use Test::More tests => 21;
+
+ #
+ # test that invalid \if respects ON_ERROR_STOP
+ #
+ my $node = get_new_node('master');
+ $node->init;
+ $node->start;
+
+ my $tests = [
+     # syntax errors
+     [ "\\if invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', '\if syntax error' ],
+     [ "\\if false\n\\elif invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', '\elif syntax error' ],
+     # unmatched checks
+     [ "\\if true\n", '', 'reached EOF without finding closing .endif', 'unmatched \if' ],
+     [ "\\elif true\n\\echo NO\n", '', '.elif: no matching .if', 'unmatched \elif' ],
+     [ "\\else\n\\echo NO\n", '', '.else: no matching .if', 'unmatched \else' ],
+     [ "\\endif\n\\echo NO\n", '', '.endif: no matching .if', 'unmatched \endif' ],
+     # error stop messages
+     [ "\\set ON_ERROR_STOP on\n\\if error\n\\echo NO\n\\endif\n\\echo NO\n", '',
+         'unrecognized value "error" for ".if <expr>": boolean expected', 'if error'],
+ ];
+
+ # 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 ON_ERROR_STOP");
+   is($stdout, $stdout_expect, "$name test STDOUT");
+   like($stderr, qr/$stderr_re/, "$name test STDERR");
+ }
+
+ $node->teardown_node;
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index eb7f197..76a6d18 100644
*** a/src/test/regress/expected/psql.out
--- b/src/test/regress/expected/psql.out
*************** deallocate q;
*** 2735,2740 ****
--- 2735,2843 ----
  \pset format aligned
  \pset expanded off
  \pset border 1
+ -- test a large nested if using a variety of true-equivalents
+ \if true
+     \if 1
+         \if yes
+             \if on
+                 \echo 'all true'
+ all true
+             \else
+                 \echo 'should not print #1-1'
+             \endif
+         \else
+             \echo 'should not print #1-2'
+         \endif
+     \else
+         \echo 'should not print #1-3'
+     \endif
+ \else
+     \echo 'should not print #1-4'
+ \endif
+ -- test a variety of false-equivalents in an if/elif/else structure
+ \if false
+     \echo 'should not print #2-1'
+ \elif 0
+     \echo 'should not print #2-2'
+ \elif no
+     \echo 'should not print #2-3'
+ \elif off
+     \echo 'should not print #2-4'
+ \else
+     \echo 'all false'
+ all false
+ \endif
+ -- test simple true-then-else
+ \if true
+     \echo 'first thing true'
+ first thing true
+ \else
+     \echo 'should not print #3-1'
+ \endif
+ -- test simple false-true-else
+ \if false
+     \echo 'should not print #4-1'
+ \elif true
+     \echo 'second thing true'
+ second thing true
+ \else
+     \echo 'should not print #5-1'
+ \endif
+ -- invalid boolean expressions mean the \if is ignored
+ \if invalid_boolean_expression
+ unrecognized value "invalid_boolean_expression" for "\if <expr>": boolean expected
+     \echo 'will print anyway #6-1'
+ will print anyway #6-1
+ \else
+ \else: no matching \if
+     \echo 'will print anyway #6-2'
+ will print anyway #6-2
+ \endif
+ \endif: no matching \if
+ -- test un-matched endif
+ \endif
+ \endif: no matching \if
+ -- test un-matched else
+ \else
+ \else: no matching \if
+ -- test un-matched elif
+ \elif
+ \elif: no matching \if
+ -- test double-else error
+ \if true
+ \else
+ \else
+ \else: cannot occur after \else
+ \endif
+ -- test elif out-of-order
+ \if false
+ \else
+ \elif
+ \elif: cannot occur after \else
+ \endif
+ -- test if-endif matching in a false branch
+ \if false
+     \if false
+         \echo 'should not print #7-1'
+     \else
+         \echo 'should not print #7-2'
+     \endif
+     \echo 'should not print #7-3'
+ \else
+     \echo 'should print #7-4'
+ should print #7-4
+ \endif
+ -- show that variables still expand even in false blocks
+ \set var 'ab''cd'
+ -- select :var;
+ \if false
+   select :var;
+ -- this will be skipped because of an unterminated string
+ \endif
+ -- fix the unterminated string
+ ';
+ -- now the if block can be properly ended
+ \endif
  -- SHOW_CONTEXT
  \set SHOW_CONTEXT never
  do $$
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 8f8e17a..6d1a6b5 100644
*** a/src/test/regress/sql/psql.sql
--- b/src/test/regress/sql/psql.sql
*************** deallocate q;
*** 382,387 ****
--- 382,487 ----
  \pset expanded off
  \pset border 1

+ -- test a large nested if using a variety of true-equivalents
+ \if true
+     \if 1
+         \if yes
+             \if on
+                 \echo 'all true'
+             \else
+                 \echo 'should not print #1-1'
+             \endif
+         \else
+             \echo 'should not print #1-2'
+         \endif
+     \else
+         \echo 'should not print #1-3'
+     \endif
+ \else
+     \echo 'should not print #1-4'
+ \endif
+
+ -- test a variety of false-equivalents in an if/elif/else structure
+ \if false
+     \echo 'should not print #2-1'
+ \elif 0
+     \echo 'should not print #2-2'
+ \elif no
+     \echo 'should not print #2-3'
+ \elif off
+     \echo 'should not print #2-4'
+ \else
+     \echo 'all false'
+ \endif
+
+ -- test simple true-then-else
+ \if true
+     \echo 'first thing true'
+ \else
+     \echo 'should not print #3-1'
+ \endif
+
+ -- test simple false-true-else
+ \if false
+     \echo 'should not print #4-1'
+ \elif true
+     \echo 'second thing true'
+ \else
+     \echo 'should not print #5-1'
+ \endif
+
+ -- invalid boolean expressions mean the \if is ignored
+ \if invalid_boolean_expression
+     \echo 'will print anyway #6-1'
+ \else
+     \echo 'will print anyway #6-2'
+ \endif
+
+ -- test un-matched endif
+ \endif
+
+ -- test un-matched else
+ \else
+
+ -- test un-matched elif
+ \elif
+
+ -- test double-else error
+ \if true
+ \else
+ \else
+ \endif
+
+ -- test elif out-of-order
+ \if false
+ \else
+ \elif
+ \endif
+
+ -- test if-endif matching in a false branch
+ \if false
+     \if false
+         \echo 'should not print #7-1'
+     \else
+         \echo 'should not print #7-2'
+     \endif
+     \echo 'should not print #7-3'
+ \else
+     \echo 'should print #7-4'
+ \endif
+
+ -- show that variables still expand even in false blocks
+ \set var 'ab''cd'
+ -- select :var;
+ \if false
+   select :var;
+ -- this will be skipped because of an unterminated string
+ \endif
+ -- fix the unterminated string
+ ';
+ -- now the if block can be properly ended
+ \endif
+
  -- SHOW_CONTEXT

  \set SHOW_CONTEXT never

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

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

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

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

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

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

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

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

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

> Other positions are also possible.

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

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

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

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

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

But I'm not seriously advocating for that position.

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

True.

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

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

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

I personally don't, but others may.

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

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

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

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

-- 
Fabien.

Hello Tom,

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

ISTM that it is still there, but for \elif conditions which are currently 
always checked.
  fabien=# \if false  fabien@#   \echo `echo BAD`    command ignored, use \endif or Ctrl-C to exit current branch.
fabien@#\else  fabien=#   \echo `echo OK`    OK  fabien=# \endif
 


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

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

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

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

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

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

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

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

Indeed.

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

As a simple way out, I suggest to:

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

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

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

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


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

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

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

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

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

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

-- 
Fabien.

Corey Huinker <corey.huinker@gmail.com> writes:
> Reading this, I started to wonder "so how did I get that impression?" and I
> found this from Feb 9:

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

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

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

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

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

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

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

> For instance:

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

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

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

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

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

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

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

            regards, tom lane

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 1aa56ab..e9d4fe6 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*************** setQFout(const char *fname)
*** 119,127 ****
   * If "escape" is true, return the value suitably quoted and escaped,
   * as an identifier or string literal depending on "as_ident".
   * (Failure in escaping should lead to returning NULL.)
   */
  char *
! psql_get_variable(const char *varname, bool escape, bool as_ident)
  {
      char       *result;
      const char *value;
--- 119,131 ----
   * If "escape" is true, return the value suitably quoted and escaped,
   * as an identifier or string literal depending on "as_ident".
   * (Failure in escaping should lead to returning NULL.)
+  *
+  * "passthrough" is the pointer previously given to psql_scan_set_passthrough.
+  * psql currently doesn't use this.
   */
  char *
! psql_get_variable(const char *varname, bool escape, bool as_ident,
!                   void *passthrough)
  {
      char       *result;
      const char *value;
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index a83bc69..3d8b8da 100644
*** a/src/bin/psql/common.h
--- b/src/bin/psql/common.h
***************
*** 16,22 ****
  extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
  extern bool setQFout(const char *fname);

! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident);

  extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2);

--- 16,23 ----
  extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
  extern bool setQFout(const char *fname);

! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident,
!                   void *passthrough);

  extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2);

diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index 5b7953b..ba4a08d 100644
*** a/src/bin/psql/psqlscanslash.l
--- b/src/bin/psql/psqlscanslash.l
*************** other            .
*** 243,249 ****
                                                               yyleng - 1);
                          value = cur_state->callbacks->get_variable(varname,
                                                                     false,
!                                                                    false);
                          free(varname);

                          /*
--- 243,250 ----
                                                               yyleng - 1);
                          value = cur_state->callbacks->get_variable(varname,
                                                                     false,
!                                                                    false,
!                                                                    cur_state->cb_passthrough);
                          free(varname);

                          /*
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 1b29341..19b3e57 100644
*** a/src/fe_utils/psqlscan.l
--- b/src/fe_utils/psqlscan.l
*************** other            .
*** 700,706 ****
                      if (cur_state->callbacks->get_variable)
                          value = cur_state->callbacks->get_variable(varname,
                                                                     false,
!                                                                    false);
                      else
                          value = NULL;

--- 700,707 ----
                      if (cur_state->callbacks->get_variable)
                          value = cur_state->callbacks->get_variable(varname,
                                                                     false,
!                                                                    false,
!                                                                    cur_state->cb_passthrough);
                      else
                          value = NULL;

*************** psql_scan_destroy(PsqlScanState state)
*** 923,928 ****
--- 924,942 ----
  }

  /*
+  * Set the callback passthrough pointer for the lexer.
+  *
+  * This could have been integrated into psql_scan_create, but keeping it
+  * separate allows the application to change the pointer later, which might
+  * be useful.
+  */
+ void
+ psql_scan_set_passthrough(PsqlScanState state, void *passthrough)
+ {
+     state->cb_passthrough = passthrough;
+ }
+
+ /*
   * Set up to perform lexing of the given input line.
   *
   * The text at *line, extending for line_len bytes, will be scanned by
*************** psqlscan_escape_variable(PsqlScanState s
*** 1409,1415 ****
      /* Variable lookup. */
      varname = psqlscan_extract_substring(state, txt + 2, len - 3);
      if (state->callbacks->get_variable)
!         value = state->callbacks->get_variable(varname, true, as_ident);
      else
          value = NULL;
      free(varname);
--- 1423,1430 ----
      /* Variable lookup. */
      varname = psqlscan_extract_substring(state, txt + 2, len - 3);
      if (state->callbacks->get_variable)
!         value = state->callbacks->get_variable(varname, true, as_ident,
!                                                state->cb_passthrough);
      else
          value = NULL;
      free(varname);
diff --git a/src/include/fe_utils/psqlscan.h b/src/include/fe_utils/psqlscan.h
index 21c4f22..0cc632b 100644
*** a/src/include/fe_utils/psqlscan.h
--- b/src/include/fe_utils/psqlscan.h
*************** typedef struct PsqlScanCallbacks
*** 53,59 ****
  {
      /* Fetch value of a variable, as a pfree'able string; NULL if unknown */
      /* This pointer can be NULL if no variable substitution is wanted */
!     char       *(*get_variable) (const char *varname, bool escape, bool as_ident);
      /* Print an error message someplace appropriate */
      /* (very old gcc versions don't support attributes on function pointers) */
  #if defined(__GNUC__) && __GNUC__ < 4
--- 53,60 ----
  {
      /* Fetch value of a variable, as a pfree'able string; NULL if unknown */
      /* This pointer can be NULL if no variable substitution is wanted */
!     char       *(*get_variable) (const char *varname, bool escape,
!                                            bool as_ident, void *passthrough);
      /* Print an error message someplace appropriate */
      /* (very old gcc versions don't support attributes on function pointers) */
  #if defined(__GNUC__) && __GNUC__ < 4
*************** typedef struct PsqlScanCallbacks
*** 67,72 ****
--- 68,75 ----
  extern PsqlScanState psql_scan_create(const PsqlScanCallbacks *callbacks);
  extern void psql_scan_destroy(PsqlScanState state);

+ extern void psql_scan_set_passthrough(PsqlScanState state, void *passthrough);
+
  extern void psql_scan_setup(PsqlScanState state,
                  const char *line, int line_len,
                  int encoding, bool std_strings);
diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h
index 0fddc7a..b4044e8 100644
*** a/src/include/fe_utils/psqlscan_int.h
--- b/src/include/fe_utils/psqlscan_int.h
*************** typedef struct PsqlScanStateData
*** 115,123 ****
      char       *dolqstart;        /* current $foo$ quote start string */

      /*
!      * Callback functions provided by the program making use of the lexer.
       */
      const PsqlScanCallbacks *callbacks;
  } PsqlScanStateData;


--- 115,125 ----
      char       *dolqstart;        /* current $foo$ quote start string */

      /*
!      * Callback functions provided by the program making use of the lexer,
!      * plus a void* callback passthrough argument.
       */
      const PsqlScanCallbacks *callbacks;
+     void       *cb_passthrough;
  } PsqlScanStateData;



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

    Tom Lane wrote:

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

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

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

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

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

Corey Huinker <corey.huinker@gmail.com> writes:
>> Barring objection I'll push this so that Corey can rebase over it.

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

Hearing no objections, pushed.
        regards, tom lane

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

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

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

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

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

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

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

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

WFM.
        regards, tom lane

Hello Corey & Tom,

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

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

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

Tom?

-- 
Fabien.

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

> 0001.if_endif.v23.diff


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

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

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


Thanks,

Erik Rijkers

    Tom Lane wrote:

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

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

Also we could leave open the option to accept an SQL expression
here. I expect people will need SQL as the evaluator in a lot of cases.
So far we need to do that:
 SELECT sql_expr ... AS varname \gset \if :varname ... \endif

Surely users will wonder right away why they can't write it like this
instead:
 \if (sql_expr) ... \endif

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


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

"Daniel Verite" <daniel@manitou-mail.org> writes:
>     Tom Lane wrote:
>> OT_WHOLE_LINE is not what you want because that results in verbatim
>> copying, without variable expansion or anything

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

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

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

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

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

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

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

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

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

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

It's possible that we could keep the implementation that uses an early exit
from exec_command() if we were to move argument collection for all
backslash commands up to the start of the function.  It would still be
a bit invasive, but perhaps not too awful: I'm imagining that instead of
   else if (strcmp(cmd, "setenv") == 0)   {       char       *envvar = psql_scan_slash_option(scan_state,
                                   OT_NORMAL, NULL, false);       char       *envval =
psql_scan_slash_option(scan_state,                                                  OT_NORMAL, NULL, false);
 

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

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

Hello Tom,

>> ISTM that I've tried to suggest to work around that complexity by:
>>   - document that \if-related commands should only occur at line start
>>     (and extend to eol).
>>   - detect and complain when this is not the case.
>
> I think this is a lousy definition, and would never be considered if we
> were working in a green field.

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

> Moreover, preventing such cases would be pretty darn ugly/messy as well.
>
> I also fear that there are corner cases where the behavior would still
> be inconsistent.  Consider
>
>     \if ...
>     \set foo `echo \endif should not appear here`

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

Another more interesting one would be:
  \if ...    \unset foo \endif

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

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

> I'm imagining that instead of
>
> [...] char       *envvar = psql_scan_slash_option(scan_state,
>
> we'd write
>
> [...] char       *envvar = args[0];
>
> where the args array had been filled at the top of the function.
> The top-of-function code would have to know all the cases where
> commands didn't use basic OT_NORMAL processing, but there aren't
> that many of those, I think.

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

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

-- 
Fabien.

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

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

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

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

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

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

Hello Tom,

>>> I also fear that there are corner cases where the behavior would still
>>> be inconsistent.  Consider
>>>
>>> \if ...
>>> \set foo `echo \endif should not appear here`
>
>> In this instance, ISTM that there is no problem. On "\if true", set is
>> executed, all is well. On "\if false", the whole line would be skipped
>> because the if-related commands are only expected on their own line, all
>> is well again. No problem.
>
> AFAICS, you misunderstood the example completely, or else you're proposing
> syntax restrictions that are even more bizarre and unintelligible than
> I thought before.

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

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

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

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

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

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

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

-- 
Fabien.

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

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

> \set x 'arg1 arg2'

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

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