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

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

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

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


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.


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

Reply via email to