Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) - Mailing list pgsql-hackers

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.