Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO wrote: > What would be the mnemonic for "," an "@"? Oh, I just picked it because control-@ is the nul character, and your commands would be nullified. I realize that's pretty weak, but we're talking about finding a punctuation mark to represent the concept of commands-are-currently-being-skipped, and it doesn't seem particularly worse than ^ to represent single-line mode. If somebody's got a better idea, fine, but there aren't that many unused punctuation marks to choose from, and I think it's better to use a punctuation mark rather than, say, a letter, like 's' for skip. Otherwise you might have the prompt change from: banana=> to bananas> Which I think is less obvious than banana@> -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
For two states: * for being executed (beware, it is ***important***) It does lend importance, but that's also the line continuation marker for "comment". Would that be a problem? Argh. Indeed, even if people seldom type C comments in psql interactive mode... Remaining ASCII characters I can thing of, hopefully avoiding already used ones: +%,@$\`|&:;_ So, maybe consider these ones: "+" for it is "on" "`" which is a "sub-shell execution" "&" for "and the next command is ..." / for not (under the hood, and it is opposed to *) +1, I was going to suggest '/' for a false state, with two possible metaphors to justify it 1. the slash in a "no" sign ("no smoking", ghostbusters, etc) 2. the leading char of a c/java/javascript comment (what is written here is just words, not code) Great. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > @in't gonna execute it? >> > > Hmmm... This is too much of an Americanism, IMHO. The @ looks like a handwritten 'a'. @in't gonna => ain't gonna => will not. It's a bad joke, made as a way of saying that I also could not think of a good mnemonic for '@' or ','. > I'm here all week, try the veal. >> > > Sorry, syntax error, you have lost me. Some googling suggests a reference > to post WW2 "lounge entertainers", probably in the USA. I also do not > understand why this would mean "yes". It's a thing lounge entertainers said after they told a bad joke. > . for z (waiting for the end of the sentence, i.e. endif) > +1 ... if we end up displaying the not-true-and-not-evaluated 'z' state. > & for t (no real mnemonic) > > For two states: > * for being executed (beware, it is ***important***) > It does lend importance, but that's also the line continuation marker for "comment". Would that be a problem? > / for not (under the hood, and it is opposed to *) > +1, I was going to suggest '/' for a false state, with two possible metaphors to justify it 1. the slash in a "no" sign ("no smoking", ghostbusters, etc) 2. the leading char of a c/java/javascript comment (what is written here is just words, not code)
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, If I can find some simple mnemonic for "," vs "@" for being executed vs ignored, I could live with that, but nothing obvious comes to my mind. @in't gonna execute it? Hmmm... This is too much of an Americanism, IMHO. I'm here all week, try the veal. Sorry, syntax error, you have lost me. Some googling suggests a reference to post WW2 "lounge entertainers", probably in the USA. I also do not understand why this would mean "yes". I'd be fine with either of these on aesthetic grounds. On technical grounds, 'z' is harder to show. I'm not sure that this valid technical point should be a good reason for guiding what feedback should be provided to the user, but it makes it simpler to choose two states:-) For three states with more culturally neutral mnemonics, I thought of: ? for f (waiting for a true answer...) . for z (waiting for the end of the sentence, i.e. endif) & for t (no real mnemonic) For two states: * for being executed (beware, it is ***important***) / for not (under the hood, and it is opposed to *) Otherwise I still like "?[tfz]", but it is two characters long. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO wrote: > > Hello Robert, > > [...] I think we should try to make this REALLY simple. We don't really >> want to have everybody have to change their PROMPT1 and PROMPT2 strings for >> this one feature. >> > > Ok. I think that we agree that the stack was too much details. > > How about just introducing a new value for %R? >> > > Yes. That is indeed one of the idea being discussed. > > [...] , or @ if commands are currently being ignored because of the result >> of an \if test. >> > ,-or-@ has one advantage over t/f/z: we cannot infer the 'z' state purely from pset.active_state, and the if-stack itself is sequestered in scan_state, which is not visible to the get_prompt() function. I suppose if somebody wanted it, a separate slash command that does a verbose printing of the current if-stack would be nice, but mostly just to explain to people how the if-stack works. > If I can find some simple mnemonic for "," vs "@" for being executed vs > ignored, I could live with that, but nothing obvious comes to my mind. > @in't gonna execute it? I'm here all week, try the veal. To sum up your points: just update %R (ok), keep it simple/short (ok... but > how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to > be too nice with the user beyond the vital (ok, that significantly > simplifies things). I'd be fine with either of these on aesthetic grounds. On technical grounds, 'z' is harder to show.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 11:29 AM, Robert Haas wrote: > possible states here. Just when you've figured out what tfzffft I agree with what you've said, but wanted to point out that any condition that follows a 'z' would itself be 'z'. Not that tfz is much better.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Robert, [...] I think we should try to make this REALLY simple. We don't really want to have everybody have to change their PROMPT1 and PROMPT2 strings for this one feature. Ok. I think that we agree that the stack was too much details. How about just introducing a new value for %R? Yes. That is indeed one of the idea being discussed. [...] , or @ if commands are currently being ignored because of the result of an \if test. Currently I find that %R logic is quite good, with "=" for give me something, "^" is start line regular expression for one line, "!" for beware someting is amiss, and in prompt2 "-" for continuation, '"' for in double quotes, "(" for in parenthesis and so on. What would be the mnemonic for "," an "@"? By shortening one of the suggestion down to two characters, we may have three cases: "?t" for "in condition, in true block" "?f" for "in condition, in false block (but true yet to come)" "?z" for "in condition, waiting for the end (true has been executed)". So no indication about the stack depth and contents. tfz for true false and sleeping seem quite easy to infer and understand. "?" is also needed as a separator with the previous field which is the database name sometimes: calvin=> \if false calvin?f=> \echo 1 calvin?f=> \elif true calvin?t=> \echo 2 2 calvin?t=> \else calvin?z=> \echo 3 calvin?z=> \endif calvin=> With the suggested , and @: calvin=> \if false calvin,=> \echo 1 calvin,=> \elif true calvin@=> \echo 2 2 calvin@=> \else calvin,=> \echo 3 calvin,=> \endif calvin=> If I can find some simple mnemonic for "," vs "@" for being executed vs ignored, I could live with that, but nothing obvious comes to my mind. The "?" for condition and Corey's [tfz] looked quite intuitive/mnemonic to me. The drawback is that it is 2 chars vs one char in above. [...] I think that's all you need here: a way to alert users as to whether commands are being ignored, or not. Yep. [...] To sum up your points: just update %R (ok), keep it simple/short (ok... but how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to be too nice with the user beyond the vital (ok, that significantly simplifies things). -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO wrote: >> Ok, so that's not just PROMPT_READY, that's every prompt...which might be >> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd >> level always being '.'? > > Yep. The idea is to keep it short, but to still have something to say "there > are more levels" in the stack, hence the one dot. Basically I just > compressed your 4 level proposal, and added a separator to deal with the > preceding stuff and suggest the conditional. I think we should try to make this REALLY simple. We don't really want to have everybody have to change their PROMPT1 and PROMPT2 strings for this one feature. How about just introducing a new value for %R? The documentation currently says: In prompt 1 normally =, but ^ if in single-line mode, or ! if the session is disconnected from the database (which can happen if \connect fails). ...and suppose we just extend that to add: , or @ if commands are currently being ignored because of the result of an \if test. The latter would include being in the \if section when the conditional was true as well as being in the \else section when the conditional was false. I think that's all you need here: a way to alert users as to whether commands are being ignored, or not. Putting details in about precisely why they are being ignored seems like it's too complicated; people won't remember how to decode some bizarre series of glyphs that we output. Telling them whether their next command is set to be ignored or executed is good enough; if the answer isn't what they expect, they can debug their script to figure out what they screwed up. Also, keep in mind that people don't need to know everything from the current prompt. They can try to debug things by looking back at previous prompts. They'll understand that \if is going to introduce a new nesting level and \endif is going to end one, and that \else and \elseif may change things. Aside from keeping the code simple so we can maintain it and the output simple so that users can remember what it means, I just don't believe that it's really going to be helpful to convey much detail here. People aren't going to paste in a gigaton of commands and then look only at the last line of the output and try to understand what it's telling them, or if they do that and are confused, I think nobody will really feel bad about giving them the advice "scroll up" or "try a simpler test case first". Further keep in mind that eventually somebody's going to code \while or \for or something, and then there are going to be even more possible states here. Just when you've figured out what tfzffft means, they'll be the case of a \while loop which is getting skipped because the condition at the top turned out to be false on the first iteration, or where (half-joking) we're skipping commands until we find the label that matches an executed \goto. Writing maintainable code includes leaving room open for other people to do stuff we can't even foresee today, and that means we need not to use up a disproportionate number of the glyphs that can reasonably be used in a psql prompt just on this. This is one small feature out of many that psql has, and one small hint to the user about whether it's currently causing commands to be skipped seems sufficient. All IMHO, of course. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Sat, Feb 11, 2017 at 3:48 PM, Fabien COELHO wrote: > > Just realized that '?' means "unknown transactional status" in %x. That >> might cause confusion if a person had a prompt of %x%R. Is that enough >> reason to pick a different cue? >> > > Argh. > > "\?\.?[tfz]" seems distinctive enough. Note that %R uses "'=-*^!$( and %x > uses *!?, which means that they already share 2 characters, so adding ? > does not seem like a big issue if it was not one before. > > Otherwise, maybe "&" or "%", but it is less about a condition. Fair enough, it shouldn't be too confusing then. The get_prompt() function can see the global pset, obviously, but can't see the scan_state, where the if-stack currently resides. I could give up on the notion of a per-file if-stack and just have one in pset, but that might make life difficult for whomever is brave enough to tackle \while loops. Or I could give pset a pointer to the current if-stack inside the scan_state, or I could have pset hold a stack of stacks. Unsure which way would be best.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Just realized that '?' means "unknown transactional status" in %x. That might cause confusion if a person had a prompt of %x%R. Is that enough reason to pick a different cue? Argh. "\?\.?[tfz]" seems distinctive enough. Note that %R uses "'=-*^!$( and %x uses *!?, which means that they already share 2 characters, so adding ? does not seem like a big issue if it was not one before. Otherwise, maybe "&" or "%", but it is less about a condition. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO wrote: > > Ok, so that's not just PROMPT_READY, that's every prompt...which might be >> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd >> level always being '.'? >> > > Yep. The idea is to keep it short, but to still have something to say > "there are more levels" in the stack, hence the one dot. Basically I just > compressed your 4 level proposal, and added a separator to deal with the > preceding stuff and suggest the conditional. > > -- > Fabien. > Just realized that '?' means "unknown transactional status" in %x. That might cause confusion if a person had a prompt of %x%R. Is that enough reason to pick a different cue?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Ok, so that's not just PROMPT_READY, that's every prompt...which might be ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd level always being '.'? Yep. The idea is to keep it short, but to still have something to say "there are more levels" in the stack, hence the one dot. Basically I just compressed your 4 level proposal, and added a separator to deal with the preceding stuff and suggest the conditional. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > calvin=> \if true > calvin?t=> SELECT 1 + > calvin?t-> 2; > 3 > calvin?t=> \if true > calvin?t=> \echo hello > hello > calvin?t=> \endif > calvin?t=> \else > calvin?z=> \echo ignored > calvin?t=> \endif > calvin=> > Ok, so that's not just PROMPT_READY, that's every prompt...which might be ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd level always being '.'? I'll give that a shot.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello, I'm looking forward to the doc update. My 0.02€ about prompting: Given that there is no more barking, then having some prompt indication that the code is inside a conditional branch becomes more important, so ISTM that there should be some plan to add it. Yeah, prompting just got more important. I see a few ways to go about this: 1. Add a new prompt type, either %T for true (heh, pun) or %Y for branching. It would print a string of chained 't' (branch is true), 'f' (branch is false), 'z' (branch already had its true section). The depth traversal would have a limit, say 3 levels deep, and if the tree goes more than that deep, then '...' would be printed in the stead of any deeper values. So the prompt would change through a session like: command prompt is now --- --- \echo bob '' = initial state, no branch going on at all \if yes 't' = inside a true branch \if no'tf' = false inside a true \endif't' = back to just the true branch \if yes 'tt' \if yes 'ttt' \if yes '...ttt' = only show the last 3, but let it be known that there's at least one more' \else '...ttz' = past the point of a true bit of this branch I like the "tfz" idea. I'm not sure whether the up to 6 characters is a good, though. 2. The printing of #1 could be integrated into %R only in PROMPT_READY cases, either prepended or appended to the !/=/^, possibly separated by a : Hmmm. Logically I would say prepend, but the default prompt is with the dbname, which is mostly letters, so it means adding a separator as well. 3. Like #2, but prepended/appended in all circumstances I would say yes. 4. Keep %T (or %Y), and reflect the state of pset.active_branch within %R, a single t/f/z Yep, but with a separator? 5. Like #4, but also printing the if-stack depth if > 1 Hmmm, not sure... Based on the your ideas above, I would suggest the following: calvin=> \if true calvin?t=> SELECT 1 + calvin?t-> 2; 3 calvin?t=> \if true calvin?t=> \echo hello hello calvin?t=> \endif calvin?t=> \else calvin?z=> \echo ignored calvin?t=> \endif calvin=> Or maybe use "?.t" for the nested if... -- Fabien -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > Shouldn't there be some documentation changes to reflect the behavior on > errors? A precise paragraph about that would be welcome, IMHO. > Oddly enough, the documentation I wrote hadn't addressed invalid booleans, only the error messages did that. The new behavior certainly warrants a mention, and I'll add that. > Given that there is no more barking, then having some prompt indication > that the code is inside a conditional branch becomes more important, so > ISTM that there should be some plan to add it. Yeah, prompting just got more important. I see a few ways to go about this: 1. Add a new prompt type, either %T for true (heh, pun) or %Y for branching. It would print a string of chained 't' (branch is true), 'f' (branch is false), 'z' (branch already had its true section). The depth traversal would have a limit, say 3 levels deep, and if the tree goes more than that deep, then '...' would be printed in the stead of any deeper values. So the prompt would change through a session like: command prompt is now --- --- \echo bob '' = initial state, no branch going on at all \if yes 't' = inside a true branch \if no'tf' = false inside a true \endif't' = back to just the true branch \if yes 'tt' \if yes 'ttt' \if yes '...ttt' = only show the last 3, but let it be known that there's at least one more' \else '...ttz' = past the point of a true bit of this branch 2. The printing of #1 could be integrated into %R only in PROMPT_READY cases, either prepended or appended to the !/=/^, possibly separated by a : 3. Like #2, but prepended/appended in all circumstances 4. Keep %T (or %Y), and reflect the state of pset.active_branch within %R, a single t/f/z 5. Like #4, but also printing the if-stack depth if > 1
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, Changes in this patch: - invalid boolean expression on \if or \elif is treated as if the script had a bad \command, so it either stops the script (ON_ERROR_STOP, script mode), or just gives the ParseVariableBool error and continues. - All interactive "barks" removed except for [...] - remaining error messages are tersed: [...] Patch applies, make check ok, psql tap test ok. Yep. At least the code is significantly simpler. There is a useless space on one end of line in the perl script. Shouldn't there be some documentation changes to reflect the behavior on errors? A precise paragraph about that would be welcome, IMHO. In particular, I suggest that given the somehow more risky "ignore and keep going whatever" behavior after a syntax error on if in a script, there should be some advice that on_error_stop should better be activated in scripts which use \if. Given that there is no more barking, then having some prompt indication that the code is inside a conditional branch becomes more important, so ISTM that there should be some plan to add it. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Thu, Feb 9, 2017 at 4:43 PM, Erik Rijkers wrote: > On 2017-02-09 22:15, Tom Lane wrote: > >> Corey Huinker writes: >> > > The feature now ( at patch v10) lets you break off with Ctrl-C anywhere. > I like it now much more. > > The main thing I still dislike somewhat about the patch is the verbose > output. To be honest I would prefer to just remove /all/ the interactive > output. > > I would vote to just make it remain silent if there is no error. (and if > there is an error, issue a message and exit) > > thanks, > > Erik Rijkers > Changes in this patch: - invalid boolean expression on \if or \elif is treated as if the script had a bad \command, so it either stops the script (ON_ERROR_STOP, script mode), or just gives the ParseVariableBool error and continues. - All interactive "barks" removed except for "command ignored. use \endif or Ctrl-C to exit current branch" when the user types a non-branching \command in a false branch "query ignored. use \endif or Ctrl-C to exit current branch" when the user types a non-branching \command in a false branch "\if: escaped" when a user does press Ctrl-C and they escape a branch. - remaining error messages are tersed: \elif: cannot occur after \else \elif: no matching \if \else: cannot occur after \else \else: no matching \if \endif: no matching \if found EOF before closing \endif(s) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..390bccd 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state) return result
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2017-02-09 22:15, Tom Lane wrote: Corey Huinker writes: The feature now ( at patch v10) lets you break off with Ctrl-C anywhere. I like it now much more. The main thing I still dislike somewhat about the patch is the verbose output. To be honest I would prefer to just remove /all/ the interactive output. I would vote to just make it remain silent if there is no error. (and if there is an error, issue a message and exit) thanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Thu, Feb 9, 2017 at 4:15 PM, Tom Lane wrote: > Basically, I think you need to start removing complexity (in the sense of > special cases), not adding more. I think Robert was saying the same > thing, though possibly I shouldn't put words in his mouth. Yeah, I was definitely going in that direction, whatever the details. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane wrote: >> IMO, an erroneous backslash command should have no effect, period. > One way around this is to make the small change: commands with invalid > expressions are ignored in interactive mode. > Another way around it would be to ignore branching commands in interactive > mode altogether and give a message like "branching commands not supported > in interactive mode". Uh, neither of those seem to be responding to my point. There is no case in psql where a command with an invalid argument does something beyond throwing an error. I do not think that \if is the place to start. Having it act differently in interactive and noninteractive modes is an even worse idea. AFAICS, the only real value of using \if interactively is to test out something you are about to copy into a script. If we go that route we're destroying the ability to test that way. Basically, I think you need to start removing complexity (in the sense of special cases), not adding more. I think Robert was saying the same thing, though possibly I shouldn't put words in his mouth. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane wrote: > Robert Haas writes: > > I (still) think this is a bad design. Even if you've got all the > > messages just right as things stand today, some new feature that comes > > along in the future can change things so that they're not right any > > more, and nobody's going to relish maintaining this. > > FWIW, I tend to agree that this is way overboard in terms of the amount of > complexity going into the messages. Also, I do not like what seems to > be happening here: > > >> $ psql test < test2.sql -v ON_ERROR_STOP=0 > >> unrecognized value "error" for "\if ": boolean expected > >> new \if is invalid, ignoring commands until next \endif > > IMO, an erroneous backslash command should have no effect, period. > "It failed but we'll treat it as if it were valid" is a rathole > I don't want to descend into. It's particularly bad in interactive > mode, because the most natural thing to do is correct your spelling > and issue the command again --- but if psql already decided to do > something on the strength of the mistaken command, that doesn't work, > and you'll have to do something or other to unwind the unwanted > control state before you can get back to what you meant to do. > > regards, tom lane > One way around this is to make the small change: commands with invalid expressions are ignored in interactive mode. Another way around it would be to ignore branching commands in interactive mode altogether and give a message like "branching commands not supported in interactive mode". That'd get rid of a lot of complexity right there. I for one wouldn't miss it. The only use I saw for it was debugging a script, and in that case the user can be their own branching via selective copy/paste. Do either of those sound appealing?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Robert Haas writes: > I (still) think this is a bad design. Even if you've got all the > messages just right as things stand today, some new feature that comes > along in the future can change things so that they're not right any > more, and nobody's going to relish maintaining this. FWIW, I tend to agree that this is way overboard in terms of the amount of complexity going into the messages. Also, I do not like what seems to be happening here: >> $ psql test < test2.sql -v ON_ERROR_STOP=0 >> unrecognized value "error" for "\if ": boolean expected >> new \if is invalid, ignoring commands until next \endif IMO, an erroneous backslash command should have no effect, period. "It failed but we'll treat it as if it were valid" is a rathole I don't want to descend into. It's particularly bad in interactive mode, because the most natural thing to do is correct your spelling and issue the command again --- but if psql already decided to do something on the strength of the mistaken command, that doesn't work, and you'll have to do something or other to unwind the unwanted control state before you can get back to what you meant to do. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 6, 2017 at 5:49 PM, Corey Huinker wrote: > I suppressed the endif-balance checking in cases where we're in an > already-failed situation. > In cases where there was a boolean parsing failure, and ON_ERROR_STOP is on, > the error message no longer speak of a future which the session does not > have. I could left the ParseVariableBool() message as the only one, but > wasn't sure that that was enough of an error message on its own. > Added the test case to the existing tap tests. Incidentally, the tap tests > aren't presently fooled into thinking they're interactive. > > $ cat test2.sql > \if error > \echo NO > \endif > \echo NOPE > $ psql test < test2.sql -v ON_ERROR_STOP=0 > unrecognized value "error" for "\if ": boolean expected > new \if is invalid, ignoring commands until next \endif > NOPE > $ psql test < test2.sql -v ON_ERROR_STOP=1 > unrecognized value "error" for "\if ": boolean expected > new \if is invalid. I (still) think this is a bad design. Even if you've got all the messages just right as things stand today, some new feature that comes along in the future can change things so that they're not right any more, and nobody's going to relish maintaining this. This sort of thing seems fine to me: new \if is invalid But then further breaking it down by things like whether ON_ERROR_STOP=1 is set, or breaking down the \endif output depending on the surrounding context, seems terrifyingly complex to me. Mind you, I'm not planning to commit this patch anyway, so feel free to ignore me, but if I were planning to commit it, I would not commit it with that level of chattiness. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
This was my previous understanding of ON_ERROR_STOP. Somewhere in the course of developing this patch I lost that. Glad to have it back. The only changes I made were to invalid booleans on if/elif, and the final branch balancing check won't set status to EXIT_USER unless it's non-interactive and ON_ERROR_STOP = on. About v10: Patch applies, make check ok, psql tap test ok. Html doc generation ok. Everything looks ok to me. Interactive tests behave as expected, especially ctrl-C and with on_error_stop=1. ISTM that everything has been addressed. I've switched the patch to "Ready for Committers", let's what happens on their side... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > It seems that ON_ERROR_STOP is mostly ignored by design when in > interactive mode, probably because it is nicer not to disconnect the user > who is actually typing things on a terminal. > > """ > ON_ERROR_STOP > > By default, command processing continues after an error. When this > variable is set to on, processing will instead stop immediately. In > interactive mode, psql will return to the command prompt; otherwise, psql > will exit, returning error code 3 to distinguish this case from fatal error > conditions, which are reported using error code 1. > """ > This was my previous understanding of ON_ERROR_STOP. Somewhere in the course of developing this patch I lost that. Glad to have it back. The only changes I made were to invalid booleans on if/elif, and the final branch balancing check won't set status to EXIT_USER unless it's non-interactive and ON_ERROR_STOP = on. > \if true new \if is true, executing commands > \endif exited \if, executing commands > \if false new \if is false, ignoring commands until next \elif, \else, or \endif > \endif exited \if, executing commands > \if error unrecognized value "error" for "\if ": boolean expected new \if is invalid, ignoring commands until next \endif > \echo foo inside inactive branch, command ignored. > ^C escaped \if, executing commands > \echo foo foo > \endif encountered un-matched \endif > diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..63ddf0a 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read an
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, In cases where there was a boolean parsing failure, and ON_ERROR_STOP is on, the error message no longer speak of a future which the session does not have. I could left the ParseVariableBool() message as the only one, but wasn't sure that that was enough of an error message on its own. Added the test case to the existing tap tests. Incidentally, the tap tests aren't presently fooled into thinking they're interactive. Yes. Revised cumulative patch attached for those playing along at home. Nearly there... It seems that ON_ERROR_STOP is mostly ignored by design when in interactive mode, probably because it is nicer not to disconnect the user who is actually typing things on a terminal. """ ON_ERROR_STOP By default, command processing continues after an error. When this variable is set to on, processing will instead stop immediately. In interactive mode, psql will return to the command prompt; otherwise, psql will exit, returning error code 3 to distinguish this case from fatal error conditions, which are reported using error code 1. """ So, you must check for interactive as well when shortening the message, and adapting it accordingly, otherwise on gets the wrong message in interactive mode: bash> ./psql -v ON_ERROR_STOP=1 psql (10devel, server 9.6.1) Type "help" for help. calvin=# \if error unrecognized value "error" for "\if ": boolean expected new \if is invalid. calvin=# -- not stopped, but the stack has been cleaned up, I think Basically it seems that there are 4 cases and 2 behaviors: - on_error_stop && scripting: actually exit on error - on_error_stop && interactive, !on_error_stop whether scripting or not: keep going, possibly with nesting checks? The problem is that currently interactive behavior cannot be tested automatically. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 6, 2017 at 3:43 PM, Corey Huinker wrote: > I did some more tests. I found a subtlety that I missed before: when >> running under ON_ERROR_STOP, messages are not fully consistent: >> >> sh> cat test.sql >> \set ON_ERROR_STOP on >> \if error >>\echo NO >> \endif >> \echo NO >> >> sh> ./psql < test.sql >> SET >> # ok >> unrecognized value "error" for "\if ": boolean expected >> # ok > > > That's straight from ParseVariableBool, and we can keep that or suppress > it. Whatever we do, we should do it with the notion that more complex > expressions will eventually be allowed, but they'll still have to resolve > to something that's a text boolean. > > >> >> new \if is invalid, ignoring commands until next \endif >> # hmmm... but it does not, it is really stopping immediately... > > found EOF before closing \endif(s) >> # no, it has just stopped before EOF because of the error... >> > > Yeah, chattiness caught up to us here. Both of these messages can be > suppressed, I think. > > >> >> Also I'm not quite sure why psql decided that it is in interactive mode >> above, its stdin is a file, but why not. >> >> The issue is made more explicit with -f: >> >> sh> ./psql -f test.sql >> SET >> psql:test.sql:2: unrecognized value "error" for "\if ": boolean >> expected >> psql:test.sql:2: new \if is invalid, ignoring commands until next \endif >> psql:test.sql:2: found EOF before closing \endif(s) >> >> It stopped on line 2, which is expected, but it was not on EOF. >> >> I think that the message when stopping should be ", stopping as required >> by ON_ERROR_STOP" or something like that instead of ", ignoring...", and >> the EOF message should not be printed at all in this case. > > > I agree, and will look into making that happen. Thanks for the test case. > > I suppressed the endif-balance checking in cases where we're in an already-failed situation. In cases where there was a boolean parsing failure, and ON_ERROR_STOP is on, the error message no longer speak of a future which the session does not have. I could left the ParseVariableBool() message as the only one, but wasn't sure that that was enough of an error message on its own. Added the test case to the existing tap tests. Incidentally, the tap tests aren't presently fooled into thinking they're interactive. $ cat test2.sql \if error \echo NO \endif \echo NOPE $ psql test < test2.sql -v ON_ERROR_STOP=0 unrecognized value "error" for "\if ": boolean expected new \if is invalid, ignoring commands until next \endif NOPE $ psql test < test2.sql -v ON_ERROR_STOP=1 unrecognized value "error" for "\if ": boolean expected new \if is invalid. $ psql test -f test2.sql -v ON_ERROR_STOP=0 psql:test2.sql:1: unrecognized value "error" for "\if ": boolean expected psql:test2.sql:1: new \if is invalid, ignoring commands until next \endif NOPE $ psql test -f test2.sql -v ON_ERROR_STOP=1 psql:test2.sql:1: unrecognized value "error" for "\if ": boolean expected psql:test2.sql:1: new \if is invalid. Revised cumulative patch attached for those playing along at home. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > I did some more tests. I found a subtlety that I missed before: when > running under ON_ERROR_STOP, messages are not fully consistent: > > sh> cat test.sql > \set ON_ERROR_STOP on > \if error >\echo NO > \endif > \echo NO > > sh> ./psql < test.sql > SET > # ok > unrecognized value "error" for "\if ": boolean expected > # ok That's straight from ParseVariableBool, and we can keep that or suppress it. Whatever we do, we should do it with the notion that more complex expressions will eventually be allowed, but they'll still have to resolve to something that's a text boolean. > > new \if is invalid, ignoring commands until next \endif > # hmmm... but it does not, it is really stopping immediately... found EOF before closing \endif(s) > # no, it has just stopped before EOF because of the error... > Yeah, chattiness caught up to us here. Both of these messages can be suppressed, I think. > > Also I'm not quite sure why psql decided that it is in interactive mode > above, its stdin is a file, but why not. > > The issue is made more explicit with -f: > > sh> ./psql -f test.sql > SET > psql:test.sql:2: unrecognized value "error" for "\if ": boolean > expected > psql:test.sql:2: new \if is invalid, ignoring commands until next \endif > psql:test.sql:2: found EOF before closing \endif(s) > > It stopped on line 2, which is expected, but it was not on EOF. > > I think that the message when stopping should be ", stopping as required > by ON_ERROR_STOP" or something like that instead of ", ignoring...", and > the EOF message should not be printed at all in this case. I agree, and will look into making that happen. Thanks for the test case.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Consolidated Fabien's TAP test additions with v7, in case anyone else wants to be reviewing. Patch applies (with "patch"), make check ok, psql tap test ok. I did some more tests. I found a subtlety that I missed before: when running under ON_ERROR_STOP, messages are not fully consistent: sh> cat test.sql \set ON_ERROR_STOP on \if error \echo NO \endif \echo NO sh> ./psql < test.sql SET # ok unrecognized value "error" for "\if ": boolean expected # ok new \if is invalid, ignoring commands until next \endif # hmmm... but it does not, it is really stopping immediately... found EOF before closing \endif(s) # no, it has just stopped before EOF because of the error... Also I'm not quite sure why psql decided that it is in interactive mode above, its stdin is a file, but why not. The issue is made more explicit with -f: sh> ./psql -f test.sql SET psql:test.sql:2: unrecognized value "error" for "\if ": boolean expected psql:test.sql:2: new \if is invalid, ignoring commands until next \endif psql:test.sql:2: found EOF before closing \endif(s) It stopped on line 2, which is expected, but it was not on EOF. I think that the message when stopping should be ", stopping as required by ON_ERROR_STOP" or something like that instead of ", ignoring...", and the EOF message should not be printed at all in this case. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 6, 2017 at 2:32 PM, Fabien COELHO wrote: > > Do you think the TAP tests would benefit from having the input described >> in a q(...) block rather than a string? >> > > My 0.02€: Not really, so I would not bother. It breaks perl indentation > and logic for a limited benefit. Maybe add comments if you feel that a test > case is unclear. > > -- > Fabien. Consolidated Fabien's TAP test additions with v7, in case anyone else wants to be reviewing. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..4a3e471 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read and interpret argument as a boolean expression. + * Return true if a boolean value was successfully parsed. + */ +static bool +read_boolean_expression(PsqlScanState scan_state, char *action, + bool *result) +{ + char*value = psql_scan_slash_option(scan_state, + OT_NORMAL, NULL, false); + boolsuccess = ParseVariableBool(value, action, result); + free(value); + return success; +} + +/* + * Return true if the command given is a branching command. + */ +static bool +is_branching_command(const char *cmd) +{ + return (strcmp(cmd, "if") == 0 || strcmp(cmd, "elif") == 0 + || strcmp(cmd, "else") == 0 || strcmp(cmd, "endif") == 0); +} /* * Subroutine to actually try to execute a backslash command. @@ -207,6 +232,17 @
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Do you think the TAP tests would benefit from having the input described in a q(...) block rather than a string? My 0.02€: Not really, so I would not bother. It breaks perl indentation and logic for a limited benefit. Maybe add comments if you feel that a test case is unclear. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 6, 2017 at 11:21 AM, Corey Huinker wrote: > >> Find attached a small patch to improve tap tests, which also checks that >>> psql really exited by checking that nothing is printed afterwards. >>> >> >> > Do you think the TAP tests would benefit from having the input described > in a q(...) block rather than a string? > > q(\if false > \echo a > \elif invalid > \echo b > \endif > \echo c > ) > > > It's a lot more lines, obviously, but it might make what is being tested > clearer. > > It occurred to me that the part of this patch most important to casual users would be the printed messages at various states. I've enumerated those below, along with the circumstances under which the user would see them. The following messages are for interactive and script users. They are also errors which respect ON_ERROR_STOP. --- \if statement which had an invalid boolean expression: new \if is invalid, ignoring commands until next \endif \elif was in a proper \if block, and not after the true block, but boolean expression was invalid: \elif is invalid, ignoring commands until next \endif \elif statement after an \else encountered \elif after \else \elif statement outside of an \if block [*] encountered un-matched \elif \else outside of an \if encountered un-matched \else \else after an \else encountered \else after \else \endif statement outside of an \if block encountered un-matched \endif Input file ends with unresolved \if blocks found EOF before closing \endif(s) The following are interactive-only non-error informational messages. - \if statement which parsed to true: new \if is true, executing commands \if statement which parsed to false: new \if is false, ignoring commands until next \elif, \else, or \endif \if statement while already in a false/invalid block: new \if is inside ignored block, ignoring commands until next \endif \elif statement immediately after the true \if or \elif \elif is after true block, ignoring commands until next \endif \elif statement within a false block or subsequent elif after the first ignored elif \elif is inside ignored block, ignoring commands until next \endif \elif was evaluated, was true \elif is true, executing commands \elif was evaluated, was false \elif is false, ignoring commands until next \elif, \else, or \endif \else statement in an ignored block or after the true block was found: \else after true condition or in ignored block, ignoring commands until next \endif \else statement and all previous blocks were false \else after all previous conditions false, executing commands \endif statement ending only \if on the stack exited \if, executing commands \endif statement where last block was false but parent block is also false: exited \\if to false parent branch, ignoring commands until next \endif \endif statement where last block was true and parent is true exited \\if to true parent branch, continuing executing commands \endif statement where last block was false but parent is true exited \\if to true parent branch, resuming executing commands Script is currently in a false (or invalid) branch, and user entered a command other than if/elif/endif: inside inactive branch, command ignored. Script currently in a false branch, and user entered a query: inside inactive branch, query ignored. use \endif to exit current branch. User in an \if branch and pressed ^C, with no more branches remaining: escaped \\if, executing commands User in an \if branch and pressed ^C, but parent branch was false: escaped \\if to false parent branch, ignoring commands until next \endif User in a true \if branch and pressed ^C, parent branch true escaped \\if to true parent branch, continuing executing commands User in a false \if branch and pressed ^C, parent branch true escaped \if to true parent branch, resuming executing commands Notes: The text for ignored commands vs ignored queries is different. The text for all the Ctrl-C messages re-uses the \endif messages, but are "escaped" instead of "exited".
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > Find attached a small patch to improve tap tests, which also checks that >> psql really exited by checking that nothing is printed afterwards. >> > > Do you think the TAP tests would benefit from having the input described in a q(...) block rather than a string? q(\if false \echo a \elif invalid \echo b \endif \echo c ) It's a lot more lines, obviously, but it might make what is being tested clearer.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Find attached a small patch to improve tap tests, which also checks that psql really exited by checking that nothing is printed afterwards. . It is better with the attachement. -- Fabien.diff --git a/src/bin/psql/t/001_if.pl b/src/bin/psql/t/001_if.pl index 68c9b15..a703cab 100644 --- a/src/bin/psql/t/001_if.pl +++ b/src/bin/psql/t/001_if.pl @@ -4,7 +4,7 @@ use warnings; use Config; use PostgresNode; use TestLib; -use Test::More tests => 15; +use Test::More tests => 18; # # test invalid \if respects ON_ERROR_STOP @@ -14,12 +14,14 @@ $node->init; $node->start; my $tests = [ - [ "\\if invalid_expression\n\\endif\n", '', 'boolean expected', 'syntax error' ], +# syntax errors + [ "\\if invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', 'syntax error' ], + [ "\\if false\n\\elif invalid\n\\echo NO\n\\endif\n\\echo NO\n", '', 'boolean expected', 'syntax error' ], # unmatched checks - [ "\\if true\\n", '', 'found EOF before closing.*endif', 'unmatched \if' ], - [ "\\elif true\\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ], - [ "\\else\\n", '', 'encountered un-matched.*else', 'unmatched \else' ], - [ "\\endif\\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ], + [ "\\if true\n", '', 'found EOF before closing.*endif', 'unmatched \if' ], + [ "\\elif true\n\\echo NO\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ], + [ "\\else\n\\echo NO\n", '', 'encountered un-matched.*else', 'unmatched \else' ], + [ "\\endif\n\\echo NO\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ], ]; # 3 checks per tests @@ -29,7 +31,7 @@ for my $test (@$tests) { my $retcode = $node->psql('postgres', $script, stdout => \$stdout, stderr => \$stderr, on_error_stop => 1); - is($retcode,'3',"$name test respects ON_ERROR_STOP"); + is($retcode,'3',"$name test ON_ERROR_STOP"); is($stdout, $stdout_expect, "$name test STDOUT"); like($stderr, qr/$stderr_re/, "$name test STDERR"); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
# elif error "\\if false\n\\elif error\n\\endif\n" # ignore commands on error (stdout must be empty) "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n" Those are already in the regression (around line 2763 of expected/psql.out), are you saying we should have them in TAP as well? Should we only do TAP tests? Ok. so, maybe just the first one. The idea would be to cover more cases of on error stop and check that it indeed stopped. Find attached a small patch to improve tap tests, which also checks that psql really exited by checking that nothing is printed afterwards. Also, for some reason there were \\n instead of \n in some place, it was working because the first command induced the error. Anyway, here's the Ctrl-C behavior: Ok. Basically it moves up each time Ctrl-C is called. Fine. The future improvement would be to do that if the current input line was empty, otherwise only the current input line would be cleaned up. Ctrl-C exits do the same before/after state checks that \endif does, the lone difference being that it "escaped" the \if rather than "exited" the \if. Thanks to Daniel for pointing out where it should be handled, because I wasn't going to figure that out on my own. v7's only major difference from v6 is the Ctrl-C branch escaping. Ok. Bar from minor tests improvements, this looks pretty much ok to me. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > Hmmm. ISTM that control-c must at least reset the stack, otherwise their > is not easy way to get out. What can be left to another patch is doing a > control-C for contents and then another one for the stack when there is no > content. > And so it shall be. > - put Fabien's tap test in place verbatim >> > > Hmmm. That was really just a POC... I would suggest some more tests, eg: > > # elif error > "\\if false\n\\elif error\n\\endif\n" > > # ignore commands on error (stdout must be empty) > "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n" > Those are already in the regression (around line 2763 of expected/psql.out), are you saying we should have them in TAP as well? Should we only do TAP tests? Anyway, here's the Ctrl-C behavior: # \if true new \if is true, executing commands # \echo msg msg # ^C escaped \if, executing commands # \if false new \if is false, ignoring commands until next \elif, \else, or \endif # \echo msg inside inactive branch, command ignored. # ^C escaped \if, executing commands # \echo msg msg # \endif encountered un-matched \endif # Ctrl-C exits do the same before/after state checks that \endif does, the lone difference being that it "escaped" the \if rather than "exited" the \if. Thanks to Daniel for pointing out where it should be handled, because I wasn't going to figure that out on my own. v7's only major difference from v6 is the Ctrl-C branch escaping. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..4a3e471 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, The check I was suggesting on whether Ctrl+C has been pressed on an empty line seems harder to implement, because get_interactive() just calls readline() or fgets(), which block to return when a whole line is ready. AFAICS psql can't know what was the edit-in-progress when these functions are interrupted by a signal instead of returning normally. But I don't think this check is essential, it could be left to another patch. Glad I wasn't missing something obvious. I suppose we could base the behavior on whether there's at least one full line already buffered. However, I agree that it can be left to another patch. Hmmm. ISTM that control-c must at least reset the stack, otherwise their is not easy way to get out. What can be left to another patch is doing a control-C for contents and then another one for the stack when there is no content. Comments about v6: - error messages are now a bit more terse, following suggestions Ok. - help text is more terse and Conditionals section was moved below Input Output Ok. - leverage IFSTATE_NONE a bit to fold some not-in-a-branch cases into existing switch statements, giving flatter, slightly cleaner code and that addresses expected cases before exceptional ones Code looks ok. - comments highlight which messages are printed in both interactive and script mode. Yep. - put Fabien's tap test in place verbatim Hmmm. That was really just a POC... I would suggest some more tests, eg: # elif error "\\if false\n\\elif error\n\\endif\n" # ignore commands on error (stdout must be empty) "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n" Also there is an empty line before the closing } of the while loop. - No mention of Ctrl-C or PROMPT. Those can be addressed in separate patches. I think that Ctrl-C resetting the stack must be addressed in this patch. Trying to be more intelligent/incremental on Ctrl-C can wait for another time. There's probably some more consensus building to do over the interactive messages and comments, Barking is now quite more verbose (?), but at least it is clear about the status and what is expected. I noticed that it is now always on, whether an error occured or not, which is ok with me. and if interactive-ish tests are possible with TAP, we should add those too. Good point. It seems that it is decided based on "source == stdin" plus checking whether both stdin/stdout are on terminal. Allowing to work around the later requires some more infrastructure to force "notty" (yuk, a negative variable tested negatively...) to false whatever, which does not seem to exist. So this is for another time. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Sat, Feb 4, 2017 at 11:53 AM, Corey Huinker wrote: > The check I was suggesting on whether Ctrl+C has been pressed >> on an empty line seems harder to implement, because get_interactive() >> just calls readline() or fgets(), which block to return when a whole >> line is ready. AFAICS psql can't know what was the edit-in-progress >> when these functions are interrupted by a signal instead of >> returning normally. >> But I don't think this check is essential, it could be left to another >> patch. >> > > Glad I wasn't missing something obvious. > I suppose we could base the behavior on whether there's at least one full > line already buffered. > However, I agree that it can be left to another patch. > v6 patch. highlights: - error messages are now a bit more terse, following suggestions - help text is more terse and Conditionals section was moved below Input Output - leverage IFSTATE_NONE a bit to fold some not-in-a-branch cases into existing switch statements, giving flatter, slightly cleaner code and that addresses expected cases before exceptional ones - comments highlight which messages are printed in both interactive and script mode. - put Fabien's tap test in place verbatim - No mention of Ctrl-C or PROMPT. Those can be addressed in separate patches. There's probably some more consensus building to do over the interactive messages and comments, and if interactive-ish tests are possible with TAP, we should add those too. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..9058897 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_co
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > The check I was suggesting on whether Ctrl+C has been pressed > on an empty line seems harder to implement, because get_interactive() > just calls readline() or fgets(), which block to return when a whole > line is ready. AFAICS psql can't know what was the edit-in-progress > when these functions are interrupted by a signal instead of > returning normally. > But I don't think this check is essential, it could be left to another > patch. > Glad I wasn't missing something obvious. I suppose we could base the behavior on whether there's at least one full line already buffered. However, I agree that it can be left to another patch.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > I noticed that the "barking" is conditional to "success". ISTM that it > should always "bark" in interactive mode, whether success or not. > "success" in those cases means "the expression was a valid boolean", and non-success cases (should) result in an error being printed regardless of interactive mode. If you see otherwise, let me know. > > While testing it and seeing the code, I agree that it is too > verbose/redundant. At least remove "active/inactive, ". Have done so, new patch pending "how-do-I-know-when-input-is-empty" in Ctrl C. > - Help text. New block in help text called "Conditionals" >> > > Maybe it could be moved to "Input/Output" renamed as "Input/Output > Control", or maybe the "Conditionals" section could be moved next to it, it > seems more logical than after large objects. > I put it near the bottom, figuring someone would have a better idea of where to put it. You did. > I think that the descriptions are too long. The interactive user can be > trusted to know what "if/elif/else/endif" mean, or to refer to the full > documentation otherwise. The point is just to provide a syntax and function > reminder, not a substitute for the doc. Thus I would suggest shorter > one-line messages like: > > \if begin a new conditional block > \elif else if in the current conditional block > \else else in current conditional block > \endifend current conditional block > +1 > >> > Hmmm. Perl is perl. Attached an attempt at improving that, which is > probably debatable, but at least it is easy to add further tests without > massive copy-pasting. +1 that's a good start. > > > - regression tests now have comments to explain purpose >> > > Ok. > > Small details about the code: > > + if (!pset.active_branch && !is_branching_command(cmd) ) > > Not sure why there is a space before the last closing parenthesis. +1
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker wrote: [about Ctrl-C] > That does seem to be the consensus desired behavior. I'm just not sure > where to handle that. The var "cancel_pressed" shows up in a lot of places. > Advice? Probably you don't need to care about cancel_pressed, and the /if stack could be unwound at the point the SIGINT handler longjumps to, in mainloop.c: /* got here with longjmp */ /* reset parsing state */ psql_scan_finish(scan_state); psql_scan_reset(scan_state); resetPQExpBuffer(query_buf); resetPQExpBuffer(history_buf); count_eof = 0; slashCmdStatus = PSQL_CMD_UNKNOWN; prompt_status = PROMPT_READY; pset.stmt_lineno = 1; cancel_pressed = false; The check I was suggesting on whether Ctrl+C has been pressed on an empty line seems harder to implement, because get_interactive() just calls readline() or fgets(), which block to return when a whole line is ready. AFAICS psql can't know what was the edit-in-progress when these functions are interrupted by a signal instead of returning normally. But I don't think this check is essential, it could be left to another patch. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
A few comments about v5. New patch. Patch applies (with patch, I gave up on "git apply"). Make check ok. Psql tap test ok. Highlights: - Interactive barking on branching state changes, commands typed while in inactive state I noticed that the "barking" is conditional to "success". ISTM that it should always "bark" in interactive mode, whether success or not. While testing it and seeing the code, I agree that it is too verbose/redundant. At least remove "active/inactive, ". - Help text. New block in help text called "Conditionals" Maybe it could be moved to "Input/Output" renamed as "Input/Output Control", or maybe the "Conditionals" section could be moved next to it, it seems more logical than after large objects. I think that the descriptions are too long. The interactive user can be trusted to know what "if/elif/else/endif" mean, or to refer to the full documentation otherwise. The point is just to provide a syntax and function reminder, not a substitute for the doc. Thus I would suggest shorter one-line messages like: \if begin a new conditional block \elif else if in the current conditional block \else else in current conditional block \endifend current conditional block There should not be a \n at the end, I think, but just between sections. - SendQuery calls in mainloop.c are all encapsulated in send_query() to ensure the same if-active and if-interactive logic is used Ok. - Exactly one perl TAP test, testing ON_ERROR_STOP. I predict more will be needed, but I'm not sure what coverage is desired More that one:-) - I also predict that my TAP test style is pathetic Hmmm. Perl is perl. Attached an attempt at improving that, which is probably debatable, but at least it is easy to add further tests without massive copy-pasting. - regression tests now have comments to explain purpose Ok. Small details about the code: + if (!pset.active_branch && !is_branching_command(cmd) ) Not sure why there is a space before the last closing parenthesis. -- Fabien.use strict; use warnings; use Config; use PostgresNode; use TestLib; use Test::More tests => 15; # # test invalid \if respects ON_ERROR_STOP # my $node = get_new_node('master'); $node->init; $node->start; my $tests = [ [ "\\if invalid_expression\n\\endif\n", '', 'boolean expected', 'syntax error' ], # unmatched checks [ "\\if true\\n", '', 'found EOF before closing.*endif', 'unmatched \if' ], [ "\\elif true\\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ], [ "\\else\\n", '', 'encountered un-matched.*else', 'unmatched \else' ], [ "\\endif\\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ], ]; # 3 checks per tests for my $test (@$tests) { my ($script, $stdout_expect, $stderr_re, $name) = @$test; my ($stdout, $stderr); my $retcode = $node->psql('postgres', $script, stdout => \$stdout, stderr => \$stderr, on_error_stop => 1); is($retcode,'3',"$name test respects ON_ERROR_STOP"); is($stdout, $stdout_expect, "$name test STDOUT"); like($stderr, qr/$stderr_re/, "$name test STDERR"); } $node->teardown_node; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 7:42 PM, Jim Nasby wrote: > Since the current consensus is to be very verbose about \if, this is > obviously a non-issue. Maybe worth adding a 'I' case to %R, but no big deal > if that doesn't happen. I think we left the door open to a separate patch for a prompt change.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2/2/17 4:39 PM, Corey Huinker wrote: On Wed, Feb 1, 2017 at 4:58 PM, Jim Nasby mailto:jim.na...@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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 4:24 PM, Corey Huinker wrote: > That might be what we end up doing. I'm willing to see how unwieldy it gets > before rolling back to "endif: peace out". All I'm saying is, give peace a chance. :-) > The state logic has stuff to do anyway, so for the moment I've added > psql_error() messages at each endpoint. My current (unsubmitted) work has: > > if you were in a true branch and leave it (i.e yes->yes) > > + psql_error("exited \\if to true > parent branch, \n" > + "continuing > executing commands\n"); > > if you were in a false branch beneath a true branch and leave it (no->yes) > > + psql_error("exited \\if to true > parent branch, \n" > + "resuming executing > commands\n"); > > And if you were in a branch that was a child of a false branch (no->no): > > + psql_error("exited \\if to false parent > branch, \n" > + "ignoring commands until > next \\endif\n"); > > And the (yes->no) is an impossibility, so no message there. > > I'm not too concerned about what wording we finally go with, and as the > coder I realize I'm too close to know the wording that will be most helpful > to an outsider, so I'm very much trusting others to guide me. But by far the most likely case is that you are not under another \if at all, and none of these messages are really apropos for that case. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 3:49 PM, Robert Haas wrote: > On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker > wrote: > > I could bulk up the error message on if/elif like such: > > > > if: true, executing commands. > > if: false, ignoring commands until next \else, \elif, or \endif. > > if: error, ignoring all commands until next \endif > > else: true, executing commands > > else: false, ignoring commands until next \endif > > else: error, ignoring commands until next \endif > > endif: now executing commands > > endif: ignoring commands until next [\else, [\elif [, \endif]] > > > > Basically, I'd tailor the message to as closely reflect what is possible > for > > the user at this moment. > > I think that this is kinda hairy. When I see "endif: now executing > commands", my reaction is "oh crap, which commands are you > executing?". What you really mean is that future command are expected > to be executed unless things change (for example, due to another \if > in the meantime), but somebody might have a different interpretation > of these messages. > > I think that the messages you are proposing for "if" and "else" are > reasonable, but for "endif" I would just say "endif: exiting if" or > something like that. If the user doesn't know to what state they are > returning, c'est la vie. > That might be what we end up doing. I'm willing to see how unwieldy it gets before rolling back to "endif: peace out". The state logic has stuff to do anyway, so for the moment I've added psql_error() messages at each endpoint. My current (unsubmitted) work has: if you were in a true branch and leave it (i.e yes->yes) + psql_error("exited \\if to true parent branch, \n" + "continuing executing commands\n"); if you were in a false branch beneath a true branch and leave it (no->yes) + psql_error("exited \\if to true parent branch, \n" + "resuming executing commands\n"); And if you were in a branch that was a child of a false branch (no->no): + psql_error("exited \\if to false parent branch, \n" + "ignoring commands until next \\endif\n"); And the (yes->no) is an impossibility, so no message there. I'm not too concerned about what wording we finally go with, and as the coder I realize I'm too close to know the wording that will be most helpful to an outsider, so I'm very much trusting others to guide me.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker wrote: > I could bulk up the error message on if/elif like such: > > if: true, executing commands. > if: false, ignoring commands until next \else, \elif, or \endif. > if: error, ignoring all commands until next \endif > else: true, executing commands > else: false, ignoring commands until next \endif > else: error, ignoring commands until next \endif > endif: now executing commands > endif: ignoring commands until next [\else, [\elif [, \endif]] > > Basically, I'd tailor the message to as closely reflect what is possible for > the user at this moment. I think that this is kinda hairy. When I see "endif: now executing commands", my reaction is "oh crap, which commands are you executing?". What you really mean is that future command are expected to be executed unless things change (for example, due to another \if in the meantime), but somebody might have a different interpretation of these messages. I think that the messages you are proposing for "if" and "else" are reasonable, but for "endif" I would just say "endif: exiting if" or something like that. If the user doesn't know to what state they are returning, c'est la vie. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 12:20 PM, Daniel Verite wrote: > If we add the functionality that Ctrl+C also exits from branches, > we could do it like the shell does Ctrl+D for logout, that is it > logs out only if the input buffer is empty, otherwise it does > the other functionality bound to this key (normally Delete). > So if you're in the middle of an edit, the first Ctrl+C will > cancel the edit and a second one will go back from the /if > That does seem to be the consensus desired behavior. I'm just not sure where to handle that. The var "cancel_pressed" shows up in a lot of places. Advice?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker wrote: > I meant in the specific psql-context, does it do anything other > than (attempt to) terminate sent-but-not-received SQL queries? It cancels the current edit in the query buffer, including the case when it spans multiple lines. If we add the functionality that Ctrl+C also exits from branches, we could do it like the shell does Ctrl+D for logout, that is it logs out only if the input buffer is empty, otherwise it does the other functionality bound to this key (normally Delete). So if you're in the middle of an edit, the first Ctrl+C will cancel the edit and a second one will go back from the /if Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > Well played (again). That one ranks up there with "and don't call me > Shirley." I meant in the specific psql-context, does it do anything other > than (attempt to) terminate sent-but-not-received SQL queries? It also flushes the input buffer. I think it is probably reasonable to let it cancel interactive \if state as well. A useful thought-experiment is to ask what behavior you'd want if we had metacommand loops ... and I think the answer there is pretty obvious: you'd want control-C to kill a loop. I'm less sure about what it ought to do when control is somewhere in a script file. I *think* we have things set up to kill execution of script files altogether, in which case we have the answer a fortiori. If not, there's room for discussion. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll >> start looking into it, as I'm not presently aware of what it is used for. >> > > Not me. > > Wikipedia, which holds all the knowledge in the universe, says: "In many > command-line interface environments, control-C is used to abort the current > task and regain user control." > Well played (again). That one ranks up there with "and don't call me Shirley." I meant in the specific psql-context, does it do anything other than (attempt to) terminate sent-but-not-received SQL queries?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
I could bulk up the error message on if/elif like such: [...] Looks ok to me. Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll start looking into it, as I'm not presently aware of what it is used for. Not me. Wikipedia, which holds all the knowledge in the universe, says: "In many command-line interface environments, control-C is used to abort the current task and regain user control." I agree that it should do that. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 7:24 AM, Fabien COELHO wrote: > > Hello Erik, > > Still, it would be an improvement to be able to break out of an inactive >> \if-branch with Ctrl-C. >> > > Yes. > > '\endif' is too long to type, /and/ you have to know it. >> > > Yep. \if is shorter but has been rejected. Ctrl-C should be the way out. I could bulk up the error message on if/elif like such: if: true, executing commands. if: false, ignoring commands until next \else, \elif, or \endif. if: error, ignoring all commands until next \endif else: true, executing commands else: false, ignoring commands until next \endif else: error, ignoring commands until next \endif endif: now executing commands endif: ignoring commands until next [\else, [\elif [, \endif]] Basically, I'd tailor the message to as closely reflect what is possible for the user at this moment. Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll start looking into it, as I'm not presently aware of what it is used for. 2. Inside an \if block \q should be given precedence and cause a direct >> exit of psql (or at the very least exit the if block(s)), as in regular SQL >> statements (compare: 'select * from t \q' which will immediately exit psql >> -- this is good. ) >> > > One use case if to be able to write "\if ... \q \endif" in scripts. If \q > is always executed, then the use case is blown. I cannot think of any > language that would execute anything in a false branch. So Ctrl-C or Ctrl-D > is the way out, and \if control must really have precedence over its > contents. > > 3. I think the 'barking' is OK because interactive use is certainly not >> the first use-case. >> But nonetheless it could be made a bit more terse without losing its >> function. >> > > [...] It really is a bit too wordy, [...] >> > > Maybe. > > (or alternatively, just mention 'if: active' or 'elif: inactive', etc., >> which has the advantage of being shorter) >> > > This last version is too terse I think. The point is that the user > understands whether their commands are going to be executed or ignored, and > "active/inactive" is not very clear. > > 5. A real bug, I think: >> #\if asdasd >> unrecognized value "asdasd" for "\if ": boolean expected >> # \q; >> inside inactive branch, command ignored. >> # >> >> That 'unrecognized value' message is fair enough but it is >> counterintuitive that after an erroneous opening \if-expression, the >> if-modus should be entered into. ( and now I have to type \endif again... ) >> > > Hmmm. > > It should tell that it is in an unclosed if and that it is currently > ignoring commands, so the "barking" is missing. > It does need a bespoke bark. Also, I do not think that implementing a different behavior for interactive > is a good idea, because then typing directly and reading a file would > result in different behaviors, which would not help debugging. > +1 > > So, as Tom suggested (I think), the feature is not designed for > interactive use, so it does not need to be optimized for that purpose, > although it should be sane enough. +1
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
2. Inside an \if block \q should be given precedence and cause a direct exit of psql (or at the very least exit the if block(s)), as in regular SQL statements (compare: 'select * from t \q' which will immediately exit psql -- this is good. ) One use case if to be able to write "\if ... \q \endif" in scripts. If \q is always executed, then the use case is blown. I cannot think of any language that would execute anything in a false branch. So Ctrl-C or Ctrl-D is the way out, and \if control must really have precedence over its contents. After giving it some more thoughts, a possible solution could be to have a "\exit [status]" which could be ignored (there has been some talk about that one), and have \q which is not, but I would find it weird and error prone for the user. As already said, \if use-case is not about interactive psql. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Erik, Still, it would be an improvement to be able to break out of an inactive \if-branch with Ctrl-C. Yes. '\endif' is too long to type, /and/ you have to know it. Yep. \if is shorter but has been rejected. Ctrl-C should be the way out. 2. Inside an \if block \q should be given precedence and cause a direct exit of psql (or at the very least exit the if block(s)), as in regular SQL statements (compare: 'select * from t \q' which will immediately exit psql -- this is good. ) One use case if to be able to write "\if ... \q \endif" in scripts. If \q is always executed, then the use case is blown. I cannot think of any language that would execute anything in a false branch. So Ctrl-C or Ctrl-D is the way out, and \if control must really have precedence over its contents. 3. I think the 'barking' is OK because interactive use is certainly not the first use-case. But nonetheless it could be made a bit more terse without losing its function. [...] It really is a bit too wordy, [...] Maybe. (or alternatively, just mention 'if: active' or 'elif: inactive', etc., which has the advantage of being shorter) This last version is too terse I think. The point is that the user understands whether their commands are going to be executed or ignored, and "active/inactive" is not very clear. 5. A real bug, I think: #\if asdasd unrecognized value "asdasd" for "\if ": boolean expected # \q; inside inactive branch, command ignored. # That 'unrecognized value' message is fair enough but it is counterintuitive that after an erroneous opening \if-expression, the if-modus should be entered into. ( and now I have to type \endif again... ) Hmmm. It should tell that it is in an unclosed if and that it is currently ignoring commands, so the "barking" is missing. Otherwise that is really the currently desired behavior for scripting use: \if syntax-error... DROP USER foo; \elif ... DROP DATABASE foo; \else ... \endif If the "\if" is simply ignored, then it is going to execute everything that appears after that, whereas the initial condition failed to be checked, and it will proceed to ignore all further \elif and \else in passing. Also, I do not think that implementing a different behavior for interactive is a good idea, because then typing directly and reading a file would result in different behaviors, which would not help debugging. So, as Tom suggested (I think), the feature is not designed for interactive use, so it does not need to be optimized for that purpose, although it should be sane enough. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2017-02-03 08:16, Corey Huinker wrote: 0001.if_endif.v5.diff 1. Well, with this amount of interactive output it is impossible to get stuck without knowing :) This is good. Still, it would be an improvement to be able to break out of an inactive \if-branch with Ctrl-C. (I noticed that inside an active branch it is already possible ) '\endif' is too long to type, /and/ you have to know it. 2. Inside an \if block \q should be given precedence and cause a direct exit of psql (or at the very least exit the if block(s)), as in regular SQL statements (compare: 'select * from t \q' which will immediately exit psql -- this is good. ) 3. I think the 'barking' is OK because interactive use is certainly not the first use-case. But nonetheless it could be made a bit more terse without losing its function. The interactive behavior is now: # \if 1 entered if: active, executing commands # \elif 0 entered elif: inactive, ignoring commands # \else entered else: inactive, ignoring commands # \endif exited if: active, executing commands It really is a bit too wordy, IMHO; I would say, drop all 'entered', 'active', and 'inactive' words. That leaves it plenty clear what's going on. That would make those lines: if: executing commands elif: ignoring commands else: ignoring commands exited if (or alternatively, just mention 'if: active' or 'elif: inactive', etc., which has the advantage of being shorter) 5. A real bug, I think: #\if asdasd unrecognized value "asdasd" for "\if ": boolean expected # \q; inside inactive branch, command ignored. # That 'unrecognized value' message is fair enough but it is counterintuitive that after an erroneous opening \if-expression, the if-modus should be entered into. ( and now I have to type \endif again... ) 6. About the help screen: There should be an empty line above 'Conditionals' to visually divide it from other help items. The indenting of the new block is incorrect: the lines that start with fprintf(output, _(" \\ are indented to the correct level; the other lines are indented 1 place too much. The help text has a few typos (some multiple times): queires -> queries exectue -> execute subsequennt -> subsequent Thanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Fri, Feb 3, 2017 at 12:57 AM, Fabien COELHO wrote: > > Hello Corey, > > Glad you like the barking. I'm happy to let the prompt issue rest for now, >> we can always add it later. >> >> If we DID want it, however, I don't think it'll be hard to add a special >> prompt (Thinking %T or %Y because they both look like branches, heh), >> > > Ah!:-) T may stand for Tree, but Y looks a little bit more like branches. > Maybe Y for Yew. > Well played. The %Y prompt can be a separate patch. New patch. Highlights: - rebased to master as of ten minutes ago - Interactive barking on branching state changes, commands typed while in inactive state - Help text. New block in help text called "Conditionals" - SendQuery calls in mainloop.c are all encapsulated in send_query() to ensure the same if-active and if-interactive logic is used - Exactly one perl TAP test, testing ON_ERROR_STOP. I predict more will be needed, but I'm not sure what coverage is desired - I also predict that my TAP test style is pathetic - regression tests now have comments to explain purpose diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..c0ba4c4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..dcf567e 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read and interpret argument as a boolean expression. + * Return true if a boolean value was successfully parsed. + */ +static bool +read_boolean_expression(PsqlScanState scan_state, char *action, + bool *result) +{ + char*value = psql_scan_slash_option(scan_st
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, Glad you like the barking. I'm happy to let the prompt issue rest for now, we can always add it later. If we DID want it, however, I don't think it'll be hard to add a special prompt (Thinking %T or %Y because they both look like branches, heh), Ah!:-) T may stand for Tree, but Y looks a little bit more like branches. Maybe Y for Yew. With a prompt1 of '%T> ' Would then resolve to "ttfi" [...] This is just idle musing, I'm perfectly happy to leave it out entirely. I like it. I would prefer to have it available, but my advice is to follow committer' opinions. At the minimum, there must be some trace, either "barking" or "prompting". -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > On reflection, it seems fairly improbable to me that people would use > \if and friends interactively. They're certainly useful for scripting, > but would you really type commands that you know are going to be ignored? > I'm thinking the one use-case is where a person is debugging a non-interactive script, cuts and pastes it into an interactive script, and then scrolls through command history to fix the bits that didn't work. So, no, you wouldn't *type* them per se, but you'd want the session as if you had. The if-then barking might really be useful in that context. > > Therefore, I don't think we should stress out about fitting branching > activity into the prompts. That's just not the use-case. (Note: we > might well have to reconsider that if we get looping, but for now it's > not a problem.) Moreover, if someone is confused because they don't > realize they're inside a failed \if, it's unlikely that a subtle change in > the prompt would help them. So your more in-the-face approach of printing > messages seems good to me. > Glad you like the barking. I'm happy to let the prompt issue rest for now, we can always add it later. If we DID want it, however, I don't think it'll be hard to add a special prompt (Thinking %T or %Y because they both look like branches, heh), and it could print the if-state stack, maybe something like: \if true \if true \if false \if true With a prompt1 of '%T> ' Would then resolve to ttfi> for true, true, false, ignored. This is just idle musing, I'm perfectly happy to leave it out entirely. > This seems more or less reasonable, although: > > > # \endif > > active \endif, executing commands > > looks a bit weird. Maybe instead say "exited \if, executing commands"? > +1 > > BTW, what is your policy about nesting these things in include files? > My immediate inclination is that if we hit EOF with open \if state, > we should drop it and revert to the state in the surrounding file. > Otherwise things will be way too confusing. > That's how it works now if you have ON_ERROR_STOP off, plus an error message telling you about the imbalance. If you have ON_ERROR_STOP on, it's fatal. All \if-\endif pairs must be balanced within a file (well, within a MainLoop, but to the user it looks like a file). Every new file opened via \i or \ir starts a new if-stack. Because commands in an inactive branch are never executed, we don't have to worry about the state of the parent stack when we do a \i, because we know it's trues all the way down. We chose this not so much because if-endif needed it (we could have thrown it into the pset struct just as easily), but because of the issues that might come up with a \while loop: needing to remember previous GOTO points in a file (if the input even *is* a file...) is going to be hard enough, remembering them across files would be harder, and further complicated by the possibility that a file \included on one iteration might not be included on the next (or vice versa)...and like you said, way too confusing.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: >> As it is, I've added interactive mode psql_error notifications about the >> resulting branching state of any branching commands, and any attempt to >> send non-branching commands or queries while in an inactive branch will >> generate a psql_error saying that the command was ignored. Waiting til I >> get what should or shouldn't be done about prompts before issuing the next >> patch revision. On reflection, it seems fairly improbable to me that people would use \if and friends interactively. They're certainly useful for scripting, but would you really type commands that you know are going to be ignored? Therefore, I don't think we should stress out about fitting branching activity into the prompts. That's just not the use-case. (Note: we might well have to reconsider that if we get looping, but for now it's not a problem.) Moreover, if someone is confused because they don't realize they're inside a failed \if, it's unlikely that a subtle change in the prompt would help them. So your more in-the-face approach of printing messages seems good to me. > So far, interactive branching information will look like this (it prints on > every branching command and on every ignored command): This seems more or less reasonable, although: > # \endif > active \endif, executing commands looks a bit weird. Maybe instead say "exited \if, executing commands"? BTW, what is your policy about nesting these things in include files? My immediate inclination is that if we hit EOF with open \if state, we should drop it and revert to the state in the surrounding file. Otherwise things will be way too confusing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > All, > As it is, I've added interactive mode psql_error notifications about the > resulting branching state of any branching commands, and any attempt to > send non-branching commands or queries while in an inactive branch will > generate a psql_error saying that the command was ignored. Waiting til I > get what should or shouldn't be done about prompts before issuing the next > patch revision. > So far, interactive branching information will look like this (it prints on every branching command and on every ignored command): # \if true active \if, executing commands # select 1; ?column? -- 1 (1 row) Time: 0.282 ms # \else inactive \else, ignoring commands # select 1; inside inactive branch, query ignored. # select ... # 1; inside inactive branch, query ignored. # \endif active \endif, executing commands # \if false inactive \if, ignoring commands # \i file_name inside inactive branch, command ignored. # \elif false inactive \elif, ignoring commands # \else active \else, executing commands # \endif active \endif, executing commands Comments welcome.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Wed, Feb 1, 2017 at 4:58 PM, Jim Nasby wrote: > I think the issue here is that the original case for this is a user > accidentally getting into an \if and then having no clue what's going on. > That's similar to what happens when you miss a quote or a semicolon. We > handle those cases with %R, and I think %R needs to support if as well. > > Perhaps there's value to providing more info (active branch, etc), but > ISTM trying to do that will just confuse the original (%R) case. > Jim, After spending a few minutes to familiarize myself with %R, I'm in agreement with your second statement (adding if-else to %R will just confuse %R). However, your first statement seems to indicate the opposite. Can you elaborate? All, As it is, I've added interactive mode psql_error notifications about the resulting branching state of any branching commands, and any attempt to send non-branching commands or queries while in an inactive branch will generate a psql_error saying that the command was ignored. Waiting til I get what should or shouldn't be done about prompts before issuing the next patch revision.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello, What do I need to do to get TAP tests running? I misunderstood. You need to configure with "--enable-tap-tests". There is a spurious empty line added at the very end of "mainloop.h": + #endif /* MAINLOOP_H */ Not in my diff, but that's been coming and going in your diff reviews. Strange. Maybe this is linked to the warning displayed with "git apply" when I apply the diff. I would suggest to add a short one line comment before each test to explain what is being tested, like "-- test \elif execution", "-- test \else execution"... Where are you suggesting this? In "regress/sql/psql.sql", in front of each group which starts a test. Debatable suggestion about "psql_branch_empty": The name isn't great. Maybe psql_branch_stack_empty()? Yep, maybe, or "empty_stack" or "stack_is_empty" or IDK... "psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" Yeah, we either need to go fully with telling the programmer that it's a stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm inclined to go full-stack, as it were. Anything consistent is ok. I'm fine with calling a stack a stack:-) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2/1/17 12:03 PM, Fabien COELHO wrote: I'm unsure whether it is a good idea, I like terse interfaces, but this is just an opinion. I think the issue here is that the original case for this is a user accidentally getting into an \if and then having no clue what's going on. That's similar to what happens when you miss a quote or a semicolon. We handle those cases with %R, and I think %R needs to support if as well. Perhaps there's value to providing more info (active branch, etc), but ISTM trying to do that will just confuse the original (%R) case. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello, We could just issue interactive-only warnings when: - A user types a branching condition command which sets the branch inactive - A user types a command or query when the current branch is inactive. The warnings could be specific about state, something like: psql session is now in an inactive \if branch. No queries will be executed and only branching commands (\if, \elif, \else, \endif) will be evaluated. [...] My 0.02€: it looks too verbose, should stay on one short line. Maybe: (in|)active (\if|\elif|\else), (execut|ignor)ing commands Although there is some redundancy... calvin=>\if true active \if: executing commands calvin=(t)> \echo ok ok calvin=(t)> \else inactive \else: skipping commands calvin=(f)> ... Maybe it could be controlled, say based on VERBOSITY setting (which really controls verbosity of error reports) or some other. I'm unsure whether it is a good idea, I like terse interfaces, but this is just an opinion. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > You can also run "make check" directly in the "src/bin/psql" directory. Previously, that didn't do anything, but now that I've created a TAP test it does. It doesn't, however, run the psql regress tests. But at least I can use the two commands in combination and not have to run *all* TAP tests outside of psql.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Wed, Feb 1, 2017 at 8:53 AM, Corey Huinker wrote: > >> Run make check-world (as opposed to just make check ) > > > I'll give that a shot. > That was it. Tests don't run if you don't invoke them. Thanks.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > However I would say yes, it should provide some feedback... This means > probably adding a new prompt substitution "%". In the worst > case, the prompt should reflect the current stack, or at least the top of > the task... > We could just issue interactive-only warnings when: - A user types a branching condition command which sets the branch inactive - A user types a command or query when the current branch is inactive. The warnings could be specific about state, something like: psql session is now in an inactive \if branch. No queries will be executed and only branching commands (\if, \elif, \else, \endif) will be evaluated. psql session is now in an inactive \elif branch. No queries will be executed and only branching commands (\if, \elif, \else, \endif) will be evaluated. psql session is now in an inactive \else branch. No queries will be executed and only branching commands (\if, \endif) will be evaluated. This could of course be done in addition to prompt changes, and is orthogonal to the CTRL-c option. I'd like more input before moving forward on either of those, as they have a good chance to clobber other expected behaviors.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, There is a spurious empty line added at the very end of "mainloop.h": + #endif /* MAINLOOP_H */ Not in my diff, but that's been coming and going in your diff reviews. Strange. Maybe this is linked to the warning displayed with "git apply" when I apply the diff. I would suggest to add a short one line comment before each test to explain what is being tested, like "-- test \elif execution", "-- test \else execution"... Where are you suggesting this? In "regress/sql/psql.sql", in front of each group which starts a test. Debatable suggestion about "psql_branch_empty": The name isn't great. Maybe psql_branch_stack_empty()? Yep, maybe, or "empty_stack" or "stack_is_empty" or IDK... "psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" Yeah, we either need to go fully with telling the programmer that it's a stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm inclined to go full-stack, as it were. Anything consistent is ok. I'm fine with calling a stack a stack:-) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Good find. I'll have to bulk up the help text. Yes. This raises a question: in interactive mode, should we give some feedback as to the result of an \if or \elif test? (see below) Obviously \if makes more sense for scripting. However I would say yes, it should provide some feedback... This means probably adding a new prompt substitution "%". In the worst case, the prompt should reflect the current stack, or at least the top of the task... Maybe use "%?" which could be substituted by: empty stack -> "" ignore state -> "." or "(i)" *_true state -> "t" or "(t)" *_false state -> "f" or "(f)" calvin=> \if true calvin=(t)> \echo "running..." running... calvin=(t)> \else calvin=(f)> whatever calvin=(f)> \endif calvin=> Therefore making it possible to break out of \if-mode with Ctrl-C would be an improvement, I think. I would even prefer it when \q would exit psql always, even from within \if-mode. So I don't think we can do that. At least not in non-interactive mode. Yep. As for CTRL-C, I've never looked into what psql does with CTRL-C, so I don't know if it's possible, let alone desirable. I think that ctrl-c should abandon current command, which is what the user expect when things go wrong. I would suggest to pop the stack on ctrl-C on an empty input, eg: calvin=> \if true calvin=(t)> SELECT calvin-(t)> calvin=(t)> calvin=> Also, shouldn't the prompt change inside an \if block? That's a good question. I could see us finding ways to print the t/f of whether a branch is active or not, but I'd like to hear from more people before diving into something like that. See above. Adding a state indicator is probably ok, the key question is whether the default prompt is changed. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2017-02-01 14:18, Corey Huinker wrote: to do to get TAP tests running? I must be missing something. Guesswork on my part: Add --enable-tap-tests option on ./configure Run make check-world (as opposed to just make check ) Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > Add --enable-tap-tests option on ./configure > This much I had already done. > > Run make check-world (as opposed to just make check ) I'll give that a shot.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Run make check-world (as opposed to just make check ) I'll give that a shot. You can also run "make check" directly in the "src/bin/psql" directory. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > Right. What I meant was, no matter how I changed that test, I could not get > it to fail, which made me think it was not executing at all. What do I need > to do to get TAP tests running? I must be missing something. configure --enable-tap-tests, perhaps? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > - wrote an intentionally failing TAP test to see if "make check" executes >> it. it does not. need help. >> > > A failing test expects code 3, not 0 as written in "t/001_if.pl". With > > is($retcode,'3','Invalid \if respects ON_ERROR_STOP'); > > instead, the tests succeeds because psql returned 3. > Right. What I meant was, no matter how I changed that test, I could not get it to fail, which made me think it was not executing at all. What do I need to do to get TAP tests running? I must be missing something. > More check should be done about stdout to check that it failed for the > expected reasons though. And maybe more tests could be added to trigger all > possible state transition errors (eg else after else, elif after else, > endif without if, if without endif, whatever...). Agreed. But I couldn't build further on the test without being sure it was being run. > > Other comments and suggestions: > > Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"? > I think that's a merge error from rebasing. Will fix. > > There is a spurious empty line added at the very end of "mainloop.h": > > + >#endif /* MAINLOOP_H */ > Not in my diff, but that's been coming and going in your diff reviews. > > > I would suggest to add a short one line comment before each test to > explain what is being tested, like "-- test \elif execution", "-- test > \else execution"... > Where are you suggesting this? > > Debatable suggestion about "psql_branch_empty": the function name somehow > suggests an "empty branch", where it is really testing that the stack is > empty. Maybe the function could be removed and "psql_branch_get_state(...) > == IF_STATE_NONE" used instead. Not sure. > The name isn't great. Maybe psql_branch_stack_empty()? "psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" > which would be symmetrical to "psql_branch_push"? Or maybe push should be > named "begin_state" or "new_state"... > Yeah, we either need to go fully with telling the programmer that it's a stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm inclined to go full-stack, as it were. > > C style details: I would avoid non mandatory parentheses, eg: > > + return ((strcmp(cmd, "if") == 0 || \ > + strcmp(cmd, "elif") == 0 || \ > + strcmp(cmd, "else") == 0 || \ > + strcmp(cmd, "endif") == 0)); > > I would remove the external double parentheses (( ... )). Also I'm not > sure why there are end-of-line backslashes on the first instance, maybe a > macro turned into a function? > I copied that from somewhere, and I don't remember where. I think the test was originally nested much further before being moved to its own function. Can fix. > > + return ((s == IFSTATE_NONE) || > + (s == IFSTATE_TRUE) || > + (s == IFSTATE_ELSE_TRUE)); > > I would remove all parenthenses. > +1 > > + return (state->branch_stack == NULL); > > Idem. +1
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Wed, Feb 1, 2017 at 6:31 AM, Erik Rijkers wrote: > On 2017-02-01 09:27, Corey Huinker wrote: > >> 0001.if_endif.v4.diff >> > > A few thoughts after a quick try: > > I dislike the ease with which one gets stuck inside an \if block, in > interactive mode. > > (for instance, in my very first session, I tried '\? \if' to see if > there is more info in that help-screen, but it only displays the normal > help screen. But after that one cannot exit with \q anymore, and there > is no feedback of any kind (prompt?) in which black hole one has ended up. > Only a \endif provides rescue.) > Good find. I'll have to bulk up the help text. This raises a question: in interactive mode, should we give some feedback as to the result of an \if or \elif test? (see below) > > Therefore making it possible to break out of \if-mode with Ctrl-C would be > an improvement, I think. > I would even prefer it when \q would exit psql always, even from within > \if-mode. > This whole thing got started with a \quit_if command, and it was pointed out that \if :condition \q \endif SELECT ... FROM ... would be preferable. So I don't think we can do that. At least not in non-interactive mode. As for CTRL-C, I've never looked into what psql does with CTRL-C, so I don't know if it's possible, let alone desirable. > > Also, shouldn't the prompt change inside an \if block? > That's a good question. I could see us finding ways to print the t/f of whether a branch is active or not, but I'd like to hear from more people before diving into something like that.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 2017-02-01 09:27, Corey Huinker wrote: 0001.if_endif.v4.diff A few thoughts after a quick try: I dislike the ease with which one gets stuck inside an \if block, in interactive mode. (for instance, in my very first session, I tried '\? \if' to see if there is more info in that help-screen, but it only displays the normal help screen. But after that one cannot exit with \q anymore, and there is no feedback of any kind (prompt?) in which black hole one has ended up. Only a \endif provides rescue.) Therefore making it possible to break out of \if-mode with Ctrl-C would be an improvement, I think. I would even prefer it when \q would exit psql always, even from within \if-mode. Also, shouldn't the prompt change inside an \if block? thanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, Some comments about v4: Patch applies. "git apply" complained about a space or line somewhere, not sure why. make check ok. - rebased on post-psql hooks master Good. - took nearly every suggestion for change to documentation Indeed. Looks ok to me. - \if ERROR will throw the entire \if..\endif into IGNORE mode Ok. I think that it is the better behavior, but other people opinion may differ. Opinions are welcome. - state is now pushed on all \ifs Ok. - reinstated leveraging of ParseVariableBool Ok. - history is now kept in interactive mode regardless of \if-truth Ok. - reworked coding example to cause less agita Yep. - removed MainLoop "are endifs balanced" variable in favor of in-place check which respects ON_ERROR_STOP. Ok. - make changes to psql/Makefile to enable TAP tests and created t/ directory - wrote an intentionally failing TAP test to see if "make check" executes it. it does not. need help. A failing test expects code 3, not 0 as written in "t/001_if.pl". With is($retcode,'3','Invalid \if respects ON_ERROR_STOP'); instead, the tests succeeds because psql returned 3. More check should be done about stdout to check that it failed for the expected reasons though. And maybe more tests could be added to trigger all possible state transition errors (eg else after else, elif after else, endif without if, if without endif, whatever...). I'm hoping my failure in that last bit is easy to spot/fix, so I can move forward with testing unbalanced branching, etc. Other comments and suggestions: Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"? There is a spurious empty line added at the very end of "mainloop.h": + #endif /* MAINLOOP_H */ I would suggest to add a short one line comment before each test to explain what is being tested, like "-- test \elif execution", "-- test \else execution"... Debatable suggestion about "psql_branch_empty": the function name somehow suggests an "empty branch", where it is really testing that the stack is empty. Maybe the function could be removed and "psql_branch_get_state(...) == IF_STATE_NONE" used instead. Not sure. "psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" which would be symmetrical to "psql_branch_push"? Or maybe push should be named "begin_state" or "new_state"... C style details: I would avoid non mandatory parentheses, eg: + return ((strcmp(cmd, "if") == 0 || \ + strcmp(cmd, "elif") == 0 || \ + strcmp(cmd, "else") == 0 || \ + strcmp(cmd, "endif") == 0)); I would remove the external double parentheses (( ... )). Also I'm not sure why there are end-of-line backslashes on the first instance, maybe a macro turned into a function? + return ((s == IFSTATE_NONE) || + (s == IFSTATE_TRUE) || + (s == IFSTATE_ELSE_TRUE)); I would remove all parenthenses. + return (state->branch_stack == NULL); Idem. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > I think ParseVariableBool is only likely to change to reject a NULL value > rather than silently interpreting it as FALSE, which is the way it is in > HEAD right now. That behavior is a leftover hack, really, and moving the > treatment of unset values upstream seems a lot cleaner. See my draft > patch at > https://www.postgresql.org/message-id/30629.1485881...@sss.pgh.pa.us > > regards, tom lane > Updated patch: - rebased on post-psql hooks master - took nearly every suggestion for change to documentation - \if ERROR will throw the entire \if..\endif into IGNORE mode - state is now pushed on all \ifs - reinstated leveraging of ParseVariableBool - history is now kept in interactive mode regardless of \if-truth - reworked coding example to cause less agita - removed MainLoop "are endifs balanced" variable in favor of in-place check which respects ON_ERROR_STOP. - make changes to psql/Makefile to enable TAP tests and created t/ directory - wrote an intentionally failing TAP test to see if "make check" executes it. it does not. need help. I'm hoping my failure in that last bit is easy to spot/fix, so I can move forward with testing unbalanced branching, etc. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 4e51e90..aad62ac 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,67 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Lines within false branches are not evaluated in any way: queries are +not sent to the server, non-conditional commands are not evaluated but +bluntly ignored, nested if-expressions in such branches are also not +evaluated but are tallied to check for proper nesting. + + + \ir or \include_relative filename diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..7a418c6 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f17f610..a916671 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,29 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read and interpret argument as a boolean expression. + * Return true if a boolean value was successfully parsed. + */ +static bool +read_boolean_expression(PsqlScanState scan_
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > On Tue, Jan 31, 2017 at 1:04 AM, Fabien COELHO wrote: >> The ParseVariableBool function has been updated, and the new version is >> much cleaner, including all fixes that I suggested in your copy, so you can >> use it in your patch. > I see there's still a lot of activity in the thread, I can't tell if it's > directly related to ParseVariableBool() or in the way it is called. Should > I wait for the dust to settle over there? I think ParseVariableBool is only likely to change to reject a NULL value rather than silently interpreting it as FALSE, which is the way it is in HEAD right now. That behavior is a leftover hack, really, and moving the treatment of unset values upstream seems a lot cleaner. See my draft patch at https://www.postgresql.org/message-id/30629.1485881...@sss.pgh.pa.us regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Tue, Jan 31, 2017 at 1:04 AM, Fabien COELHO wrote: > > This is code lifted from variable.c's ParseVariableBool(). When the other >>> patch for "psql hooks" is committed (the one that detects when the string >>> wasn't a valid boolean), this code will go away and we'll just use >>> ParseVariableBool() again. >>> >> > The ParseVariableBool function has been updated, and the new version is > much cleaner, including all fixes that I suggested in your copy, so you can > use it in your patch. > > -- > Fabien. > I see there's still a lot of activity in the thread, I can't tell if it's directly related to ParseVariableBool() or in the way it is called. Should I wait for the dust to settle over there?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
This is code lifted from variable.c's ParseVariableBool(). When the other patch for "psql hooks" is committed (the one that detects when the string wasn't a valid boolean), this code will go away and we'll just use ParseVariableBool() again. The ParseVariableBool function has been updated, and the new version is much cleaner, including all fixes that I suggested in your copy, so you can use it in your patch. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Daniel, [...] So psql is not following that model with ON_ERROR_STOP if it exits with an error when unable to evaluate an "if" expression. I'm not implying that we should necessarily adopt the shell behavior, but as these choices have certainly been made in POSIX for good reasons, we should make sure to think twice about why they don't apply to psql. Interesting point. The shell is about processes, and if relies on the status code returned, with 0 meaning true, and anything else, which is somehow a process error, meaning false. So there is no way to distinguish false from process error in this system, because the status is just one integer. However, a syntax error, for instance with a shell internal test, leads to nothing being executed: bash> if [[ bad syntax ]] ; then echo then ; else echo else ; fi -bash: conditional binary operator expected -bash: syntax error near `syntax' # nothing is echoed Another example with python in interactive mode: python> if 1+: print 1; else print 0 SyntaxError: invalid syntax # nothing is printed So rejecting execution altogether on syntax errors is somehow a common practice. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker wrote: > > \if ERROR > > \echo X > > \else > > \echo Y > > \endif > > > > having both X & Y printed and error reported on else and endif. I think > > that an expression error should just put the stuff in ignore state. > > > > Not just false, but ignore the whole if-endif? interesting. I hadn't > thought of that. Can do. If we use the Unix shell as a model, in POSIX "test" and "if" are such that an evaluation error (exit status>1) leads to the same flow than when evaluating to false (exit status=1). References I can find: test: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html if: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04_07 BTW, in "set -e" mode, it also says that a failure to evaluate an "if" expression does not lead to the script stopping: The -e setting shall be ignored when executing the compound list following the while, until, if, or elif reserved word, a pipeline beginning with the ! reserved word, or any command of an AND-OR list other than the last So psql is not following that model with ON_ERROR_STOP if it exits with an error when unable to evaluate an "if" expression. I'm not implying that we should necessarily adopt the shell behavior, but as these choices have certainly been made in POSIX for good reasons, we should make sure to think twice about why they don't apply to psql. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 1/29/17 2:35 AM, Fabien COELHO wrote: I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if there are many employees. [...] I believe that the scan stops on the first row it finds, because the EXITS() clause is met. Hmmm... That is not so clear from "EXPLAIN" output: You need to use a better test case... explain analyze select exists(select 1 from generate_series(1,9) gs); QUERY PLAN --- Result (cost=0.01..0.02 rows=1 width=1) (actual time=26.278..26.278 rows=1 loops=1) InitPlan 1 (returns $0) -> Function Scan on generate_series gs (cost=0.00..10.00 rows=1000 width=0) (actual time=26.271..26.271 rows=1 loops=1) Planning time: 6.568 ms Execution time: 48.917 ms (5 rows) In any case, +1 for not promoting count(*) <> 0; that's a really, really bad way to test for existence. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > My point is that examples about one thing can be interpreted as example > for other things which is also done in the example, so it is better to do > everything right. > Fair enough. I'll rewrite the examples to use pk lookups. I doubt the query plan for those will change much in the future. > Hmmm. Copy-pasting is bad enough, and "when the other patch is committed" > is an unknown, so I would still suggest to fix obvious defects at least (eg > dead code which may result in compiler warnings, inconsistent comments...). > It was do that or pause this work until that unknown was resolved. > > My point was that you must at least push something, otherwise both > branches are executed (!), and some commands could be attached to > upper-level conditions: > > > As for which state is pushed, it is indeed debatable. I do think that > pushing ignore on errors is a better/less risky behavior, but other people' > opinion may differ. +1 > > > On "else" when in state ignored, ISTM that it should remain in state >>> ignore, not switch to else-false. >>> >> >> That's how I know if this is the first "else" I encountered. >> > > Ok, my mistake. Maybe expand the comment a little bit if appropriate. +1 > > As I recall, history is only for interactive mode. If I really typed > something, I'm expecting to get it by visiting previous commands, because I > certainly do not want to retype it again. > > For your above example, maybe I would reedit the clause definition, then > want to execute the delete. Good points, and history does save the string with the variable in it, not the resolved string that was sent (or not sent) to the server.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello, I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if there are many employees. [...] I believe that the scan stops on the first row it finds, because the EXITS() clause is met. Hmmm... That is not so clear from "EXPLAIN" output: Result (cost=0.03..0.04 rows=1 width=1) InitPlan 1 (returns $0) -> Seq Scan on ... (cost=0.00..263981.69 rows=10001769 width=0) There is a plan for the sub-query, so it looks like it is actually fully executed. Maybe adding "LIMIT 1" would be better? But it's not relevant to the documentation, I simply wanted to demonstrate some results that couldn't be resolved at parse time, so that the \if tests were meaningful. If the query example is distracting from the point of the documentation, we should change it. My point is that examples about one thing can be interpreted as example for other things which is also done in the example, so it is better to do everything right. In "read_boolean_expression": [...] This is code lifted from variable.c's ParseVariableBool(). When the other patch for "psql hooks" is committed (the one that detects when the string wasn't a valid boolean), this code will go away and we'll just use ParseVariableBool() again. Hmmm. Copy-pasting is bad enough, and "when the other patch is committed" is an unknown, so I would still suggest to fix obvious defects at least (eg dead code which may result in compiler warnings, inconsistent comments...). [...] The second test on success may not rely on the value set above. That looks very strange. ISTM that the state should be pushed whether parsing succeeded or not. Moreover, it results in: \if ERROR \echo X \else \echo Y \endif having both X & Y printed and error reported on else and endif. I think that an expression error should just put the stuff in ignore state. Not just false, but ignore the whole if-endif? interesting. I hadn't thought of that. Can do. My point was that you must at least push something, otherwise both branches are executed (!), and some commands could be attached to upper-level conditions: \if true \if ERROR ... \endif // this becomes "if true \endif" ... \endif // this becomes an error As for which state is pushed, it is indeed debatable. I do think that pushing ignore on errors is a better/less risky behavior, but other people' opinion may differ. On "else" when in state ignored, ISTM that it should remain in state ignore, not switch to else-false. That's how I know if this is the first "else" I encountered. Ok, my mistake. Maybe expand the comment a little bit if appropriate. History saving: I'm wondering whether all read line should be put into history, whether executed or not. Good question. I gave it some thought and I decided it shouldn't. First, because history is a set of statements that were attempted, and those statements were not. But perhaps more importantly, because the statement could have contained an expandable variable, and since that variable would not be evaluated the SQL stored would be subtly altered from the original intent, perhaps in ways that might drastically alter the meaning of the statement. A highly contrived example: \set clause 'where cust_id = 1' \if false delete from customers :clause; \endif Hmmm. So yeah, it just seemed easier to not store in history. Hmmm. As I recall, history is only for interactive mode. If I really typed something, I'm expecting to get it by visiting previous commands, because I certainly do not want to retype it again. For your above example, maybe I would reedit the clause definition, then want to execute the delete. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > > I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if > there are many employees. EXPLAIN suggests a seq_scan, which seems bad. > With "(SELECT COUNT(*) FROM pgbench_accounts) <> 0" pg seems to generate an > index only scan on a large table, so maybe it is a better way to do it. It > seems strange that there is no better way to do that in a relational tool, > the relational model being an extension of set theory... and there is no > easy way (?) to check whether a set is empty. > I believe that the scan stops on the first row it finds, because the EXITS() clause is met. But it's not relevant to the documentation, I simply wanted to demonstrate some results that couldn't be resolved at parse time, so that the \if tests were meaningful. If the query example is distracting from the point of the documentation, we should change it. > > """If an EOF is reached on the main file or an > \include-ed file before all > \if-\endif are matched, then psql > will raise an error.""" > > In sentence above: "before all" -> "before all local"? "then" -> ""? > +1 > > "other options booleans" -> "other booleans of options"? or "options' > booleans" maybe, but that is for people? > +1 > > "unabigous" -> "unambiguous" > +1 > > I think that the three paragraph explanation about non evaluation could be > factor into one, maybe something like: """Lines within false branches are > not evaluated in any way: queries are not sent to the server, non > conditional commands are not evaluated but bluntly ignored, nested if > expressions in such branches are also not evaluated but are tallied to > check for nesting.""" > > I would suggest to merge elif/else constraints by describing what is > expected rather than what is not expected. """An \if command may contain > any number of \elif clauses and may end with one \else clause""". > I'll give it another shot, as I forgot to mention the non-evaluation of expressions in dead branches. > > > ## CODE > > In "read_boolean_expression": > > + if (value) > > "if (value != NULL)" is usually used, I think. > > + if (value == NULL) > + return false; /* not set -> assume "off" */ > > This is dead code, because value has been checked to be non NULL a few > lines above. > > + (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0) > + (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0) > > Hmmm, not easy to parse. Maybe it deserves a comment? > "check at least two chars to distinguish on & off" > > ",action" -> ", action" (space after commas). > > The "result" is not set on errors, but maybe it should be set to false > anyway and explicitely, instead of relying on some prior initialization? > Or document that the result is not set on errors. > This is code lifted from variable.c's ParseVariableBool(). When the other patch for "psql hooks" is committed (the one that detects when the string wasn't a valid boolean), this code will go away and we'll just use ParseVariableBool() again. > > if command: > > if (is active) { > success = ... > if (success) { > ... > } > } > if (success) { > ... > } > > The second test on success may not rely on the value set above. That looks > very strange. ISTM that the state should be pushed whether parsing > succeeded or not. Moreover, it results in: > > \if ERROR > \echo X > \else > \echo Y > \endif > > having both X & Y printed and error reported on else and endif. I think > that an expression error should just put the stuff in ignore state. > Not just false, but ignore the whole if-endif? interesting. I hadn't thought of that. Can do. > > > On "else" when in state ignored, ISTM that it should remain in state > ignore, not switch to else-false. > That's how I know if this is the first "else" I encountered. I could split the if-state back into a struct of booleans if you think that makes more sense. > > > Comment about "IFSTATE_FALSE" talks about the state being true, maybe a > copy-paste error? > Yes. > > In comments: "... variables the branch" -> "variables if the branch" > Yes. > > The "if_endifs_balanced" variable is not very useful. Maybe just the test > and error reporting in the right place: > > if (... && !psqlscan_branch_empty(scan_state)) >psql_error("found EOF before closing \\endif(s)\n"); > +1 I think I got the idea at some point that psql_error broke out of the current execution block. > History saving: I'm wondering whether all read line should be put into > history, whether executed or not. > Good question. I gave it some thought and I decided it shouldn't. First, because history is a set of statements that were attempted, and those statements were not. But perhaps more importantly, because the statement could have contained an expandable variable, and since that variable would not be evaluated the SQL stored would be subtly altered from the original intent, perhaps in ways that might drastically alter the meaning of the statement. A highly
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, And here it is About the patch v3: ## DOCUMENTATION I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if there are many employees. EXPLAIN suggests a seq_scan, which seems bad. With "(SELECT COUNT(*) FROM pgbench_accounts) <> 0" pg seems to generate an index only scan on a large table, so maybe it is a better way to do it. It seems strange that there is no better way to do that in a relational tool, the relational model being an extension of set theory... and there is no easy way (?) to check whether a set is empty. """If an EOF is reached on the main file or an \include-ed file before all \if-\endif are matched, then psql will raise an error.""" In sentence above: "before all" -> "before all local"? "then" -> ""? "other options booleans" -> "other booleans of options"? or "options' booleans" maybe, but that is for people? "unabigous" -> "unambiguous" I think that the three paragraph explanation about non evaluation could be factor into one, maybe something like: """Lines within false branches are not evaluated in any way: queries are not sent to the server, non conditional commands are not evaluated but bluntly ignored, nested if expressions in such branches are also not evaluated but are tallied to check for nesting.""" I would suggest to merge elif/else constraints by describing what is expected rather than what is not expected. """An \if command may contain any number of \elif clauses and may end with one \else clause""". ## CODE In "read_boolean_expression": + if (value) "if (value != NULL)" is usually used, I think. + if (value == NULL) + return false; /* not set -> assume "off" */ This is dead code, because value has been checked to be non NULL a few lines above. + (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0) + (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0) Hmmm, not easy to parse. Maybe it deserves a comment? "check at least two chars to distinguish on & off" ",action" -> ", action" (space after commas). The "result" is not set on errors, but maybe it should be set to false anyway and explicitely, instead of relying on some prior initialization? Or document that the result is not set on errors. if command: if (is active) { success = ... if (success) { ... } } if (success) { ... } The second test on success may not rely on the value set above. That looks very strange. ISTM that the state should be pushed whether parsing succeeded or not. Moreover, it results in: \if ERROR \echo X \else \echo Y \endif having both X & Y printed and error reported on else and endif. I think that an expression error should just put the stuff in ignore state. On "else" when in state ignored, ISTM that it should remain in state ignore, not switch to else-false. Comment about "IFSTATE_FALSE" talks about the state being true, maybe a copy-paste error? In comments: "... variables the branch" -> "variables if the branch" The "if_endifs_balanced" variable is not very useful. Maybe just the test and error reporting in the right place: if (... && !psqlscan_branch_empty(scan_state)) psql_error("found EOF before closing \\endif(s)\n"); + #endif /* MAINLOOP_H */ - /* * Main processing loop for reading lines of input * and sending them to the backend. Do not add/remove empty lines in places unrelated to the patch? History saving: I'm wondering whether all read line should be put into history, whether executed or not. Is it possible to make some of the added functions static? If so, do it. I checked that it does stop on errors with -v ON_ERROR_STOP=1. However I would be more at ease if this was tested somewhere. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Thu, Jan 26, 2017 at 4:06 PM, Corey Huinker wrote: > > > On Thu, Jan 26, 2017 at 3:55 PM, Fabien COELHO > wrote: > >> >> Hello Daniel, >> >> A comment about control flow and variables: in branches that are not >>> taken, variables are expanded nonetheless, in a way that can be surprising. >>> Case in point: >>> >>> \set var 'ab''cd' >>> -- select :var; >>> \if false >>> select :var ; >>> \else >>> select 1; >>> \endif >>> >>> To avoid that kind of trouble, would it make sense not to expand >>> variables in untaken branches? >>> >> >> Hmmm. This case is somehow contrived (for a string, :'var' could be used, >> in which case escaping would avoid the hazard), but I think that what you >> suggest is a better behavior, if easy to implement. >> >> -- >> Fabien. >> > > Good question, Daniel. Variable expansion seems to be done via > psql_get_variable which is invoked via callback, which means that I might > have to move branch_block_active into PsqlSettings. That's slightly > different because the existing boolean is scoped with MainLoop(), but > there's no way to get to a new MainLoop unless you're in an executing > branch, and no way to legally exit a MainLoop with an unbalanced if-endif > state. In short, I think it's better behavior. It does prevent someone from > setting a variable to '\endif' and expecting that to work, like this: > > select case > when random() < 0.5 then '\endif' > else E'\else\n\echo fooled you\n\endif' > end as contrived_metaprogramming > \gset > > \if false > :contrived_metaprogramming > > > I'm sure someone will argue that preventing that is a good thing. Unless > someone sees a good reason not to move that PsqlSettings, I'll make that > change. > > And here it is diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 640fe12..fcf265b 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,78 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer) as has_customers, +EXISTS(SELECT 1 FROM employee) as has_employees +\gset +\if :has_users +SELECT * FROM customer ORDER BY creation_date LIMIT 5; +\elif :has_employees +\echo 'no customers found' +SELECT * FROM employee ORDER BY creation_date LIMIT 5; +\else +\if yes +\echo 'No customers or employees' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all +\if-\endif are matched, then +psql will raise an error. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which is evaluated like other options booleans, so the valid values +are any unabiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Queries within a false branch of a conditional block will not be +sent to the server. + + +Non-conditional \-commands within a false branch +of a conditional block will not be evaluated for correctness. The +command will be ignored along with all remaining input to the end +of the line. + + +Expressions on \if and \elif +commands within a false branch of a conditional block will not be +evaluated. + + +A conditional block can at most one \else command. + + +The \elif command cannot follow the +\else command. + + + \ir or \include_relative filename diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 0c164a3..8235015 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && pset.active_branch) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,68 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read and interpret argument as a boolean ex
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Thu, Jan 26, 2017 at 3:55 PM, Fabien COELHO wrote: > > Hello Daniel, > > A comment about control flow and variables: in branches that are not >> taken, variables are expanded nonetheless, in a way that can be surprising. >> Case in point: >> >> \set var 'ab''cd' >> -- select :var; >> \if false >> select :var ; >> \else >> select 1; >> \endif >> >> To avoid that kind of trouble, would it make sense not to expand >> variables in untaken branches? >> > > Hmmm. This case is somehow contrived (for a string, :'var' could be used, > in which case escaping would avoid the hazard), but I think that what you > suggest is a better behavior, if easy to implement. > > -- > Fabien. > Good question, Daniel. Variable expansion seems to be done via psql_get_variable which is invoked via callback, which means that I might have to move branch_block_active into PsqlSettings. That's slightly different because the existing boolean is scoped with MainLoop(), but there's no way to get to a new MainLoop unless you're in an executing branch, and no way to legally exit a MainLoop with an unbalanced if-endif state. In short, I think it's better behavior. It does prevent someone from setting a variable to '\endif' and expecting that to work, like this: select case when random() < 0.5 then '\endif' else E'\else\n\echo fooled you\n\endif' end as contrived_metaprogramming \gset \if false :contrived_metaprogramming I'm sure someone will argue that preventing that is a good thing. Unless someone sees a good reason not to move that PsqlSettings, I'll make that change.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Daniel, A comment about control flow and variables: in branches that are not taken, variables are expanded nonetheless, in a way that can be surprising. Case in point: \set var 'ab''cd' -- select :var; \if false select :var ; \else select 1; \endif To avoid that kind of trouble, would it make sense not to expand variables in untaken branches? Hmmm. This case is somehow contrived (for a string, :'var' could be used, in which case escaping would avoid the hazard), but I think that what you suggest is a better behavior, if easy to implement. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker wrote: > Revised patch A comment about control flow and variables: in branches that are not taken, variables are expanded nonetheless, in a way that can be surprising. Case in point: \set var 'ab''cd' -- select :var; \if false select :var ; \else select 1; \endif The 2nd reference to :var has a quoting hazard, but since it's within an "\if false" branch, at a glance it seems like this script might work. In fact it barfs with: psql:script.sql:0: found EOF before closing \endif(s) AFAICS what happens is that :var gets injected and starts a runaway string, so that as far as the parser is concerned the \else ..\endif block slips into the untaken branch, as a part of that unfinished string. This contrasts with line 2: -- select :var where the reference to :var is inoffensive. To avoid that kind of trouble, would it make sense not to expand variables in untaken branches? Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Revised patch, with one caveat: It contains copy/pasted code from variable.c intended to bridge the gap until https://commitfest.postgresql. org/12/799/ (changing ParseVariableBool to detect invalid boolean-ish strings) is merged. We may want to pause full-review of this patch pending resolution of that one. I'm happy to continue with the stop-gap in place. Changes made: - \elseif is now \elif - Invalid boolean values now return an error - ON_ERROR_STOP is respected in all errors raided by \if, \elsif, \else, \endif commands. - Documentation gives a more real-world example of usage. - Documentation gives a more explicit list of valid boolean values - Regression tests for out-of-place \endif, \else, and \endif - Regression test for invalid boolean values - Removal of debug detritus. Changes not(yet) made: - No TAP test for errors respecting ON_ERROR_STOP - function comments in psqlscan.l follow the style found in other comments there, which goes counter to global style. On Tue, Jan 24, 2017 at 3:58 AM, Fabien COELHO wrote: > > I would suggest to assume false on everything else, and/or maybe to ignore >>> the whole if/endif section in such cases. >>> >> >> +1, it also halves the number of values we have to support later. >> > > After giving it some thought, I revise a little bit my opinion: > > > I think that if the value is evaluated to TRUE or FALSE, then fine. If it > is anything else, then an error is raised (error message shown), which > should also stop the script on "ON_ERROR_STOP", and if not the script > continues with assuming the value was FALSE. > > -- > Fabien. > diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 9915731..20091e5 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2007,6 +2007,78 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +SELECT +EXISTS(SELECT 1 FROM customer) as has_customers, +EXISTS(SELECT 1 FROM employee) as has_employees +\gset +\if :has_users +SELECT * FROM customer ORDER BY creation_date LIMIT 5; +\elif :has_employees +\echo 'no customers found' +SELECT * FROM employee ORDER BY creation_date LIMIT 5; +\else +\if yes +\echo 'No customers or employees' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all +\if-\endif are matched, then +psql will raise an error. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which is evaluated like other options booleans, so the valid values +are any unabiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + + +Queries within a false branch of a conditional block will not be +sent to the server. + + +Non-conditional \-commands within a false branch +of a conditional block will not be evaluated for correctness. The +command will be ignored along with all remaining input to the end +of the line. + + +Expressions on \if and \elif +commands within a false branch of a conditional block will not be +evaluated. + + +A conditional block can at most one \else command. + + +The \elif command cannot follow the +\else command. + + + \ir or \include_relative filename diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 4139b77..feb9ddc 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -49,6 +49,7 @@ #include "psqlscanslash.h" #include "settings.h" #include "variables.h" +#include "fe_utils/psqlscan_int.h" /* * Editable database object types. @@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state, status = PSQL_CMD_ERROR; } - if (status != PSQL_CMD_ERROR) + if (status != PSQL_CMD_ERROR && psqlscan_branch_active(scan_state)) { /* eat any remaining arguments after a valid command */ /* note we suppress evaluation of backticks here */ @@ -194,6 +195,68 @@ read_connect_arg(PsqlScanState scan_state) return result; } +/* + * Read and interpret argument as a boolean expression. + * Return true if a boolean value was successfully parsed. + */ +static bool +read_boolean_expressio
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
"Daniel Verite" writes: > ISTM that it's important that eventually ParseVariableBool() > and \if agree on what evaluates to true and false (and the > more straightforward way to achieve that is by \if calling > directly ParseVariableBool), but that it's not productive that we > discuss /if issues relatively to the behavior of ParseVariableBool() > in HEAD at the moment, as it's likely to change. AFAIK we do have consensus on changing its behavior to disallow assignment of invalid values. It's just a matter of getting the patch to be stylistically nice. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Tue, Jan 24, 2017 at 1:25 PM, Corey Huinker wrote: > This might be asking a lot, but could we make a "strict" mode for > ParseVariableBool() that returns a success boolean, and have the existing > ParseVariableBool() signature call that new function, and issue the > "assuming " warning if the strict function failed? Nevermind. Looking at the v7 patch I see that it does everything I need and more. I should have looked first.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > ISTM that it's important that eventually ParseVariableBool() > and \if agree on what evaluates to true and false (and the > more straightforward way to achieve that is by \if calling > directly ParseVariableBool), but that it's not productive that we > discuss /if issues relatively to the behavior of ParseVariableBool() > in HEAD at the moment, as it's likely to change. > I'd like to keep in sync with ParseVariableBoolean(), but Also, Fabien has made a good case for invalid parsed values being an ON_ERROR_STOP-able error, and not defaulted to either true or false. This might be asking a lot, but could we make a "strict" mode for ParseVariableBool() that returns a success boolean, and have the existing ParseVariableBool() signature call that new function, and issue the "assuming " warning if the strict function failed?
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Corey Huinker wrote: > > > > 1: unrecognized value "whatever" for "\if"; assuming "on" > > > > I do not think that the script should continue with such an assumption. > > > > I agree, and this means we can't use ParseVariableBool() as-is The patch at https://commitfest.postgresql.org/12/799/ in the ongoing CF already changes ParseVariableBool() to not assume that unrecognizable values should be set to "on". There's also the fact that ParseVariableBool() in HEAD assumes that an empty value is valid and true, which I think leads to this inconsistency in the current patch: \set empty \if :empty select 1 as result \gset \else select 2 as result \gset \endif \echo 'result is' :result produces: result is 1 (so an empty string evaluates to true) Yet this sequence: \if select 1 as result \gset \else select 2 as result \gset \endif \echo 'result is' :result produces: result is 2 (so an empty \if evaluates to false) The equivalence between empty value and true in ParseVariableBool() is also suppressed in the above-mentioned patch. ISTM that it's important that eventually ParseVariableBool() and \if agree on what evaluates to true and false (and the more straightforward way to achieve that is by \if calling directly ParseVariableBool), but that it's not productive that we discuss /if issues relatively to the behavior of ParseVariableBool() in HEAD at the moment, as it's likely to change. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
I would suggest to assume false on everything else, and/or maybe to ignore the whole if/endif section in such cases. +1, it also halves the number of values we have to support later. After giving it some thought, I revise a little bit my opinion: I think that if the value is evaluated to TRUE or FALSE, then fine. If it is anything else, then an error is raised (error message shown), which should also stop the script on "ON_ERROR_STOP", and if not the script continues with assuming the value was FALSE. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Tue, Jan 24, 2017 at 1:15 AM, Fabien COELHO wrote: > > 1: unrecognized value "whatever" for "\if"; assuming "on" >>> >>> I do not think that the script should continue with such an assumption. >>> >> >> I agree, and this means we can't use ParseVariableBool() as-is. I already >> broke out argument reading to it's own function, knowing that it'd be the >> stub for expressions. So I guess we start that now. What subset of true-ish >> values do you think we should support? If we think that real expressions >> are possible soon, we could only allow 'true' and 'false' for now, but if >> we expect that expressions might not make it into v10, then perhaps we >> should support the same text values that coerce to booleans on the server >> side. >> > > Hmmm. I would text value that coerce to true? I would also accept non-zero > integers (eg SELECT 1::BOOL; -- t). > The docs (doc/src/sgml/datatype.sgml) list TRUE 't' 'true' 'y' 'yes' 'on' '1'vs FALSE 'f' 'false' 'n' 'no' 'off' '0' However, src/test/regress/expected/boolean.out shows that there's some flexiblity there with spaces, even before you inspect parse_bool_with_len() in src/backend/utils/adt/bool.c. If we could bring src/backend/utils/adt/bool.c across the server/client wall, I would just use parse_bool_with_len as-is. As it is, given that whatever I do is temporary until real expressions come into place, that wouldn't be a terrible amount of code copying, and we'd still have a proto-expression that probably isn't going to conflict with whatever expression syntax we do chose. Having said that, if anyone can see ANY reason that some subset of the existing bool-friendly strings would cause a problem with the expression syntax we do hope to use, speak now and we can restrict the accepted values. > I would suggest to assume false on everything else, and/or maybe to ignore > the whole if/endif section in such cases. +1, it also halves the number of values we have to support later. > ISTM that with TAP test you can check for error returns, so maybe this can > be done. I'll have to educate myself on TAP tests.