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

Corey Huinker <corey(dot)huinker(at)gmail(dot)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(at)#

(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(at)# \echo foo \endif
command ignored, use \endif or Ctrl-C to exit current branch.
regression(at)#

(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(at)# \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