Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite wrote: In interactive mode, the warning in untaken branches is misleading when \endif is on the same line as the commands that are skipped. For instance: postgres=# \if false \echo NOK \endif \echo command ignored; use \endif or Ctrl-C to exit current \if block postgres=# From the point of view of the user, the execution flow has exited the branch already when this warning is displayed. Of course issuing the recommended \endif at this point doesn't work: postgres=# \endif \endif: no matching \if Maybe that part of the message: "use \endif or Ctrl-C to exit current \if block" should be displayed only when coming back at the prompt, and if the flow is still in an untaken branch at this point? Is this an open item, or do we not care about fixing it? I would suggest that this is not important enough to block anything. Otherwise, I agree that displaying this interactive message only when it is pertinent is desirable, but this might change the underlying logic significantly: it may mean holding somewhere a message to be shown at next prompt, and being able to decide when to clear it. There is also the question of what happens if there are multiple such messages, should they all be shown? Only the first? The last? Should it avoid repeats? So I propose to call it a feature for now, especially that we do not expect people to write a lot of one-liner multiple-backslash-commands in interactive mode. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite wrote: > In interactive mode, the warning in untaken branches is misleading > when \endif is on the same line as the commands that > are skipped. For instance: > > postgres=# \if false \echo NOK \endif > \echo command ignored; use \endif or Ctrl-C to exit current \if block > postgres=# > > From the point of view of the user, the execution flow has exited > the branch already when this warning is displayed. > Of course issuing the recommended \endif at this point doesn't work: > > postgres=# \endif > \endif: no matching \if > > Maybe that part of the message: > "use \endif or Ctrl-C to exit current \if block" > should be displayed only when coming back at the prompt, > and if the flow is still in an untaken branch at this point? Is this an open item, or do we not care about fixing it? -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hi, In interactive mode, the warning in untaken branches is misleading when \endif is on the same line as the commands that are skipped. For instance: postgres=# \if false \echo NOK \endif \echo command ignored; use \endif or Ctrl-C to exit current \if block postgres=# From the point of view of the user, the execution flow has exited the branch already when this warning is displayed. Of course issuing the recommended \endif at this point doesn't work: postgres=# \endif \endif: no matching \if Maybe that part of the message: "use \endif or Ctrl-C to exit current \if block" should be displayed only when coming back at the prompt, and if the flow is still in an untaken branch at this point? 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: >> If it actually is impossible to give pgbench equivalent behavior, we'll >> just have to live with the discrepancy, > Yep. >> but ISTM it could probably be made to work. > Even if it could somehow, I do not see it as a useful feature for pgbench. Perhaps not. > I also lack a good use case for psql for this feature. It doesn't seem very outlandish to me: the alternative would be to repeat all of a query that might span dozens of lines, in order to change one or two lines. That's not very readable or maintainable. I'm prepared to believe that extremely long queries aren't ever going to be common in pgbench scripts, so that there's not much need to support the equivalent behavior in pgbench. So maybe it's fine. 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, pgbench (well, at least if I succeed in getting boolean expressions and setting variables, which is just a maybe), but this kind of if in the middle of expression does not make much sense for a pgbench script where "if" must be evaluated at execution time, not parse time. Well, it's not really clear to me why that would be true. For example, how can you PREPARE a possibly combinatorial thing? SELECT \if ... XX \else YY \endif FROM \if ... ZZ \else WW \endif WHERE \if ... AA \else BB \endif ; Or the kind of operation: \if ... SELECT * \else DELETE \endif FROM table WHERE condition; Even the structure can be changed somehow: SELECT \if ... 1 ; SELECT 2 \endif ; If it actually is impossible to give pgbench equivalent behavior, we'll just have to live with the discrepancy, Yep. but ISTM it could probably be made to work. Even if it could somehow, I do not see it as a useful feature for pgbench. I also lack a good use case for psql for this feature. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: >> [...] Aside from cosmetic changes, I've made it behave reasonably for >> cases where \if is used on portions of a query, for instance > A small issue I see is that I was planning to add such an if syntax to > pgbench (well, at least if I succeed in getting boolean expressions and > setting variables, which is just a maybe), but this kind of if in the > middle of expression does not make much sense for a pgbench script where > "if" must be evaluated at execution time, not parse time. Well, it's not really clear to me why that would be true. If it actually is impossible to give pgbench equivalent behavior, we'll just have to live with the discrepancy, but ISTM it could probably be made to work. 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, Patch applies cleanly. Make check ok. Feature still works! Idem for v30. [...] Aside from cosmetic changes, I've made it behave reasonably for cases where \if is used on portions of a query, for instance SELECT \if :something var1 \else var2 \endif FROM table; This is commendable, but I would not have bothered, although it is more cpp-like with it. A small issue I see is that I was planning to add such an if syntax to pgbench (well, at least if I succeed in getting boolean expressions and setting variables, which is just a maybe), but this kind of if in the middle of expression does not make much sense for a pgbench script where "if" must be evaluated at execution time, not parse time. which as I mentioned a long time ago is something that people will certainly expect to work. I would not have expected it to work, but indeed other people could. Sometimes I try something with pg and it does not work as I hoped. That is life. I also cleaned up a lot of corner-case discrepancies between how much text is consumed in active-branch and inactive-branch cases (OT_FILEPIPE is a particularly nasty case in that regard :-() Indeed. I plan to read this over again tomorrow and then push it, if there are not objections/corrections. My small objection is that an eventual if in pgbench, with a separate parsing and execution, will not work in the middle of queries as this one. Do you think that such a discrepancy would be admissible. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: >> New Patch v29: Now with less coverage! > Patch applies cleanly. Make check ok. Feature still works! I've been hacking on this for about two full days now, and have gotten it to a point where I think it's committable. Aside from cosmetic changes, I've made it behave reasonably for cases where \if is used on portions of a query, for instance SELECT \if :something var1 \else var2 \endif FROM table; which as I mentioned a long time ago is something that people will certainly expect to work. I also cleaned up a lot of corner-case discrepancies between how much text is consumed in active-branch and inactive-branch cases (OT_FILEPIPE is a particularly nasty case in that regard :-() I plan to read this over again tomorrow and then push it, if there are not objections/corrections. regards, tom lane diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 2a9c412..b51b11b 100644 *** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** hello 10 *** 2064,2069 --- 2064,2158 + \if expression + \elif expression + \else + \endif + + + This group of commands implements nestable conditional blocks. + A conditional block must begin with an \if and end + with an \endif. In between there may be any number + of \elif clauses, which may optionally be followed + by a single \else clause. Ordinary queries and + other types of backslash commands may (and usually do) appear between + the commands forming a conditional block. + + + The \if and \elif commands read + their argument(s) and evaluate them as a boolean expression. If the + expression yields true then processing continues + normally; otherwise, lines are skipped until a + matching \elif, \else, + or \endif is reached. Once + an \if or \elif test has + succeeded, the arguments of later \elif commands in + the same block are not evaluated but are treated as false. Lines + following an \else are processed only if no earlier + matching \if or \elif succeeded. + + + The expression argument + of an \if or \elif command + is subject to variable interpolation and backquote expansion, just + like any other backslash command argument. After that it is evaluated + like the value of an on/off option variable. So a valid value + is any unambiguous case-insensitive match for one of: + true, false, 1, + 0, on, off, + yes, no. For example, + t, T, and tR + will all be considered to be true. + + + Expressions that do not properly evaluate to true or false will + generate a warning and be treated as false. + + + Lines being skipped are parsed normally to identify queries and + backslash commands, but queries are not sent to the server, and + backslash commands other than conditionals + (\if, \elif, + \else, \endif) are + ignored. Conditional commands are checked only for valid nesting. + Variable references in skipped lines are not expanded, and backquote + expansion is not performed either. + + + All the backslash commands of a given conditional block must appear in + the same source file. If EOF is reached on the main input file or an + \include-ed file before all local + \if-blocks have been closed, + then psql will raise an error. + + + Here is an example: + + + -- check for the existence of two separate records in the database and store + -- the results in separate psql variables + SELECT + EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, + EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee + \gset + \if :is_customer + SELECT * FROM customer WHERE customer_id = 123; + \elif :is_employee + \echo 'is not a customer but is an employee' + SELECT * FROM employee WHERE employee_id = 456; + \else + \if yes + \echo 'not a customer or employee' + \else + \echo 'this will never print' + \endif + \endif + + + + + + \l[+] or \list[+] [ pattern ] *** testdb=> INSERT INTO my_ta *** 3715,3721 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). In prompt 2 %R is replaced by a character that --- 3804,3811 In prompt 1 normally =, ! but @ if the session is in an
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
New Patch v29: Now with less coverage! Patch applies cleanly. Make check ok. Feature still works! -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
New Patch v29: Now with less coverage! (same as v28 minus the psql-on-error-stop.sql and associated changes) Fabien raises some good points about if/then being a tremendous tool for enhancing other existing regression tests. On Wed, Mar 29, 2017 at 2:16 PM, Fabien COELHO wrote: > > Hello Tom, > > If someone were to put together a TAP test suite that covered all that >> and made for a meaningful improvement in psql's altogether-miserable >> code coverage report[1], I would think that that would be a useful >> expenditure of buildfarm time. >> > > Ok, this is an interesting point. > > What I'm objecting to is paying the overhead for such a suite in order to >> test just this one thing. >> > > Well, it should start somewhere. Once something is running it is easier to > add more tests. > > think that that passes the bang-for-buck test; or in other words, this >> isn't the place I would start if I were creating a TAP suite for psql. >> > > Sure, I would not have started with that either. > > Note that from this patch point of view, it is somehow logical to start > testing a given feature when this very feature is being developed... > > The summary is that we agree that psql test coverage is abysmal, but you > do not want to bootstrap a better test infrastructure for this particular > and rather special new feature. Ok. > > Maybe Corey can submit another patch with the exit 3 test removed. > > -- > Fabien. > From 9caa338fa085bc1c0ee16f22c0c1c8d3c8fc1697 Mon Sep 17 00:00:00 2001 From: Corey Huinker Date: Wed, 29 Mar 2017 15:30:22 -0400 Subject: [PATCH] psql if v29 --- doc/src/sgml/ref/psql-ref.sgml | 90 ++- src/bin/psql/.gitignore|2 + src/bin/psql/Makefile |4 +- src/bin/psql/command.c | 1365 src/bin/psql/common.c |4 +- src/bin/psql/conditional.c | 103 +++ src/bin/psql/conditional.h | 62 ++ src/bin/psql/copy.c|4 +- src/bin/psql/help.c|7 + src/bin/psql/mainloop.c| 54 +- src/bin/psql/prompt.c |6 +- src/bin/psql/prompt.h |3 +- src/bin/psql/startup.c |8 +- src/test/regress/expected/psql.out | 110 +++ src/test/regress/sql/psql.sql | 109 +++ 15 files changed, 1641 insertions(+), 290 deletions(-) create mode 100644 src/bin/psql/conditional.c create mode 100644 src/bin/psql/conditional.h diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 2a9c412..18f8bfe 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2064,6 +2064,93 @@ hello 10 +\if expression +\elif expression +\else +\endif + + +This group of commands implements nestable conditional blocks. +A conditional block must begin with an \if and end +with an \endif. In between there may be any number +of \elif clauses, which may optionally be followed +by a single \else clause. Ordinary queries and +other types of backslash commands may (and usually do) appear between +the commands forming a conditional block. + + +The \if and \elif commands read +the rest of the line and evaluate it as a boolean expression. If the +expression is true then processing continues +normally; otherwise, lines are skipped until a +matching \elif, \else, +or \endif is reached. Once +an \if or \elif has succeeded, +later matching \elif commands are not evaluated but +are treated as false. Lines following an \else are +processed only if no earlier matching \if +or \elif succeeded. + + +Lines being skipped are parsed normally to identify queries and +backslash commands, but queries are not sent to the server, and +backslash commands other than conditionals +(\if, \elif, +\else, \endif) are +ignored. Conditional commands are checked only for valid nesting. + + +The expression argument +of \if or \elif +is subject to variable interpolation and backquote expansion, just +like any other backslash command argument. After that it is evaluated +like the value of an on/off option variable. So a valid value +is any unambiguous case-insensitive match for one of: +true, false, 1, +0, on, off, +yes, no. For example, +t, T, and tR +will all be considered to be true. + + +Expressions that do not properly evaluate to true or false will +generate a warning and be treated as false. + + +All the backslash commands of a given conditional block must appear in +the same source file. If EOF is reached on the main input file or an +\include-ed
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, If someone were to put together a TAP test suite that covered all that and made for a meaningful improvement in psql's altogether-miserable code coverage report[1], I would think that that would be a useful expenditure of buildfarm time. Ok, this is an interesting point. What I'm objecting to is paying the overhead for such a suite in order to test just this one thing. Well, it should start somewhere. Once something is running it is easier to add more tests. think that that passes the bang-for-buck test; or in other words, this isn't the place I would start if I were creating a TAP suite for psql. Sure, I would not have started with that either. Note that from this patch point of view, it is somehow logical to start testing a given feature when this very feature is being developed... The summary is that we agree that psql test coverage is abysmal, but you do not want to bootstrap a better test infrastructure for this particular and rather special new feature. Ok. Maybe Corey can submit another patch with the exit 3 test removed. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: >> If we're sufficiently dead set on it, we could go back to the TAP-based >> approach, > Hmmm. You rejected it. I agree that TAP tests are not well suited for some > simple tests because of their initdb overhead. >> but I still doubt that this test is worth the amount of overhead that >> would add. > I think that there is an underlying issue with keeping on rejecting tests > which aim at having a reasonable code coverage of features by exercising > different paths. There's certainly a fair amount of psql behavior that's not adequately testable within the standard regression test infrastructure. Parsing of command line arguments and exit codes for unusual cases both fall into that area, and then there's things like prompts and tab completion. If someone were to put together a TAP test suite that covered all that and made for a meaningful improvement in psql's altogether-miserable code coverage report[1], I would think that that would be a useful expenditure of buildfarm time. What I'm objecting to is paying the overhead for such a suite in order to test just this one thing. I don't think that that passes the bang-for-buck test; or in other words, this isn't the place I would start if I were creating a TAP suite for psql. regards, tom lane [1] https://coverage.postgresql.org/src/bin/psql/index.html -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, psql_if_on_error_stop... ok (test process exited with exit code 3) Don't think we can have that. Even if pg_regress considers it a success, every hacker is going to have to learn that that's a "pass", Well, it says "ok"... and I don't think I want to be answering that question every week till kingdom come. Hmmm. What if the test is renamed, say "psql_if_exit_code_3"? Maybe the clue would be clear enough to avoid questions? Or just remove the exit message but check the exit code is as expected? I'm not really sure we need a test for this behavior. My 0.02€: I have learned the hard way over the years that what is not tested does not really work, including error behaviors. These tests (well, the initial TAP version at least) helped debug the feature, and would help keeping it alive when the code is updated. Now if you do not want this test, it can be removed. The feature is worthy even without it. If we're sufficiently dead set on it, we could go back to the TAP-based approach, Hmmm. You rejected it. I agree that TAP tests are not well suited for some simple tests because of their initdb overhead. but I still doubt that this test is worth the amount of overhead that would add. I think that there is an underlying issue with keeping on rejecting tests which aim at having a reasonable code coverage of features by exercising different paths. Maybe there could be some "larger but still reasonable tests" activated on demand so as to being able to keep tests and run them from time to time, which would not interfere too much with committers' work, and that some farm animals would run? I thought that was one of the purpose of TAP tests, but obviously it is not. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: [ 0001-psql-if-v28.patch ] Starting to look at this version, and what jumped out at me in testing is that the regression output looks like this: parallel group (12 tests): psql_if_on_error_stop dbsize async misc_functions tidscan alter_operator tsrf psql alter_generic misc stats_ext sysviews alter_generic... ok alter_operator ... ok misc ... ok psql ... ok psql_if_on_error_stop... ok (test process exited with exit code 3) async... ok dbsize ... ok misc_functions ... ok sysviews ... ok tsrf ... ok tidscan ... ok stats_ext... ok Don't think we can have that. Even if pg_regress considers it a success, every hacker is going to have to learn that that's a "pass", and I don't think I want to be answering that question every week till kingdom come. I'm not really sure we need a test for this behavior. If we're sufficiently dead set on it, we could go back to the TAP-based approach, but I still doubt that this test is worth the amount of overhead that would add. 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
0001+0002 patch primarily for ease of review. will be following with a single v28 patch shortly. Applies cleanly. Make check ok. I think it behaves as committers required last. Let us try again with them... -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > 0001+0002 patch primarily for ease of review. will be following with a > single v28 patch shortly. > 0001-psql-if-v28.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Mon, Mar 27, 2017 at 3:25 PM, Fabien COELHO wrote: > > And here you go >> > > Patch applies cleany, make check ok. Looks pretty good. > > A minor detail I have just noticed, sorry: now that options are discarded > by functions, some string variable declarations should be moved back inside > the active branch. You moved them out because you where sharing the > variables between the active & inactive branches, but this is no longer > necessary, and the project practice seems to declare variables just where > they are needed. That would be pattern in d, encoding in encoding, fname in > f and g and include and out and s, prefix in gset, opt in help, opt* in lo > and pset and set, arg* in prompt, env* in setenv... and maybe a few others. > > -- > Fabien. > done: encoding f g gset help include lo out prompt pset s set setenv sf sv t T timing unset watch x z ! ? weird cases where they're both still needed: d write 0001+0002 patch primarily for ease of review. will be following with a single v28 patch shortly. 0001-psql-if-v27.patch Description: Binary data 0002-fix-var-scoping.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
And here you go Patch applies cleany, make check ok. Looks pretty good. A minor detail I have just noticed, sorry: now that options are discarded by functions, some string variable declarations should be moved back inside the active branch. You moved them out because you where sharing the variables between the active & inactive branches, but this is no longer necessary, and the project practice seems to declare variables just where they are needed. That would be pattern in d, encoding in encoding, fname in f and g and include and out and s, prefix in gset, opt in help, opt* in lo and pset and set, arg* in prompt, env* in setenv... and maybe a few others. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Mon, Mar 27, 2017 at 10:34 AM, Fabien COELHO wrote: > > Hello, > > I think that you could use another pattern where you init the >>> PQExpBufferData structure instead of create it, so that only the string >>> is >>> malloced. >>> >> >> In v26, I have the functions return PQExpBuffer. The two calling functions >> then free it, which should solve any leak. >> > > Yep, it works as well. > > Here's an addendum that does that. I can combine them in v27, but figured >> this was quicker. >> > > It works. > > However having just one full patch with a number would help so that I can > say "ready to committer" or not on something. > > -- > Fabien. > And here you go (sorry for the delay, had errands to run). 0001-psql-if-v27.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello, I think that you could use another pattern where you init the PQExpBufferData structure instead of create it, so that only the string is malloced. In v26, I have the functions return PQExpBuffer. The two calling functions then free it, which should solve any leak. Yep, it works as well. Here's an addendum that does that. I can combine them in v27, but figured this was quicker. It works. However having just one full patch with a number would help so that I can say "ready to committer" or not on something. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > I think that you could use another pattern where you init the > PQExpBufferData structure instead of create it, so that only the string is > malloced. In v26, I have the functions return PQExpBuffer. The two calling functions then free it, which should solve any leak. > > > I think that you should use appendPQExpBufferChar and Str instead of >>> relying on the format variant which is probably expensive. Something >>> like: >>> >>> if (num_options > 0) >>> append...Char(buf, ' '); >>> append...Str(buf, ...); >>> >> >> All flavors of appendPQExpBuffer*() I can find have a const *char format >> string, so no way to append a naked string. If you know differently, I'm >> listening. Not fixed. >> > > These prototypes are from "pqexpbuffer.h", and do not seem to rely on a > format: > > Here's an addendum that does that. I can combine them in v27, but figured this was quicker. From 2fa083eb0115278f817a2d1d0439a47951a9c48b Mon Sep 17 00:00:00 2001 From: Corey Huinker Date: Mon, 27 Mar 2017 10:07:36 -0400 Subject: [PATCH 2/2] simpler append --- src/bin/psql/command.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index aa9dad4..a6168df 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -218,10 +218,9 @@ gather_boolean_expression(PsqlScanState scan_state, bool expansion, bool warn) while ((value = psql_scan_slash_option(scan_state, slash_opt, NULL, false))) { /* add a space in between subsequent tokens */ - if (num_options == 0) - appendPQExpBuffer(exp_buf, "%s", value); - else - appendPQExpBuffer(exp_buf, " %s", value); + if (num_options > 0) + appendPQExpBufferChar(exp_buf, ' '); + appendPQExpBufferStr(exp_buf, value); num_options++; } -- 2.7.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
ISTM that PQExpBuffer is partially a memory leak. Something should need to be freed? I copied that pattern from somewhere else, so yeah, I duplicated whatever leak was there. Hmmm. Indeed some commands do not free, but there is a single use and the commands exits afterwards, eg "createuser". I think that you could use another pattern where you init the PQExpBufferData structure instead of create it, so that only the string is malloced. I think that you should use appendPQExpBufferChar and Str instead of relying on the format variant which is probably expensive. Something like: if (num_options > 0) append...Char(buf, ' '); append...Str(buf, ...); All flavors of appendPQExpBuffer*() I can find have a const *char format string, so no way to append a naked string. If you know differently, I'm listening. Not fixed. These prototypes are from "pqexpbuffer.h", and do not seem to rely on a format: extern void appendPQExpBufferChar(PQExpBuffer str, char ch); extern void appendPQExpBufferStr(PQExpBuffer str, const char *data); -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > Patches do not apply cleanly. > Part 1 gets: > error: patch failed: src/test/regress/parallel_schedule:89 > error: src/test/regress/parallel_schedule: patch does not apply > > There is still the useless file, ok it is removed by part2. Could have > been just one patch... > parallel_schedule failed because I hadn't rebased recently enough. git format-patch did us no favors there. New patch is redone as one commit. ISTM that PQExpBuffer is partially a memory leak. Something should need to > be freed? > I copied that pattern from somewhere else, so yeah, I duplicated whatever leak was there. Fixed. > I think that you should use appendPQExpBufferChar and Str instead of > relying on the format variant which is probably expensive. Something like: > > if (num_options > 0) > append...Char(buf, ' '); > append...Str(buf, ...); > All flavors of appendPQExpBuffer*() I can find have a const *char format string, so no way to append a naked string. If you know differently, I'm listening. Not fixed. > > is_true_boolean_expression: "return (success) ? tf : false;" > Is this simply: "return success && tf;"? > Neat. Done. > > Some functions have opt1, opt2, but some start at opt0. This does not look > too consistent, although the inconsistency may be preexisting from your > patch. Basically, there is a need for some more restructuring in > "command.c". It is pre-existing. Maybe this patch will inspire someone else to make the other more consistent. v26 attached From fc0c466b0331839efe722d089a8ead521e8a827e Mon Sep 17 00:00:00 2001 From: Corey Huinker Date: Sun, 26 Mar 2017 17:30:51 -0400 Subject: [PATCH] psql if v26 --- doc/src/sgml/ref/psql-ref.sgml | 90 +- src/bin/psql/.gitignore|2 + src/bin/psql/Makefile |4 +- src/bin/psql/command.c | 1383 src/bin/psql/common.c |4 +- src/bin/psql/conditional.c | 103 ++ src/bin/psql/conditional.h | 62 + src/bin/psql/copy.c|4 +- src/bin/psql/help.c|7 + src/bin/psql/mainloop.c| 54 +- src/bin/psql/prompt.c |6 +- src/bin/psql/prompt.h |3 +- src/bin/psql/startup.c |8 +- src/test/regress/expected/psql.out | 110 ++ .../regress/expected/psql_if_on_error_stop.out | 14 + src/test/regress/parallel_schedule |2 +- src/test/regress/serial_schedule |1 + src/test/regress/sql/psql.sql | 109 ++ src/test/regress/sql/psql_if_on_error_stop.sql | 17 + 19 files changed, 1699 insertions(+), 284 deletions(-) create mode 100644 src/bin/psql/conditional.c create mode 100644 src/bin/psql/conditional.h create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 2a9c412..18f8bfe 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2064,6 +2064,93 @@ hello 10 +\if expression +\elif expression +\else +\endif + + +This group of commands implements nestable conditional blocks. +A conditional block must begin with an \if and end +with an \endif. In between there may be any number +of \elif clauses, which may optionally be followed +by a single \else clause. Ordinary queries and +other types of backslash commands may (and usually do) appear between +the commands forming a conditional block. + + +The \if and \elif commands read +the rest of the line and evaluate it as a boolean expression. If the +expression is true then processing continues +normally; otherwise, lines are skipped until a +matching \elif, \else, +or \endif is reached. Once +an \if or \elif has succeeded, +later matching \elif commands are not evaluated but +are treated as false. Lines following an \else are +processed only if no earlier matching \if +or \elif succeeded. + + +Lines being skipped are parsed normally to identify queries and +backslash commands, but queries are not sent to the server, and +backslash commands other than conditionals +(\if, \elif, +\else, \endif) are +ignored. Conditional commands are checked only for valid nesting. + + +The expression argument +of \if or \elif +is subject to variable interpolation and backquote expansion, just +like any other backslash
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v25, try 2: First file is what you were used to last time. 2nd and 3rd are changes since then based on feedback. Patches do not apply cleanly. Part 1 gets: error: patch failed: src/test/regress/parallel_schedule:89 error: src/test/regress/parallel_schedule: patch does not apply There is still the useless file, ok it is removed by part2. Could have been just one patch... After a manual fix in parallel_schedule, make check is ok. gather_boolean_expression: ISTM that PQExpBuffer is partially a memory leak. Something should need to be freed? I think that you should use appendPQExpBufferChar and Str instead of relying on the format variant which is probably expensive. Something like: if (num_options > 0) append...Char(buf, ' '); append...Str(buf, ...); is_true_boolean_expression: "return (success) ? tf : false;" Is this simply: "return success && tf;"? Some functions have opt1, opt2, but some start at opt0. This does not look too consistent, although the inconsistency may be preexisting from your patch. Basically, there is a need for some more restructuring in "command.c". -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
As for a function for digested ignored slash options, it seems like I can disregard the true/false value of the "semicolon" parameter. Is that correct? Dunno. I do not see that as a significant issue, especially compared to the benefit of having the automaton transition management in a single place. I'm still struggling to see how this would add any clarity to the code beyond what I can achieve by clustering the exec_command_(if/elif/else/endif) near one another. Hmmm... it is more cleanly encapsulated if in just one function? -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v25 ISTM that the attached file contents is identical to v24. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
v25 - PQExpBuffer on gather_boolean_expression() - convenience/clarity functions: ignore_slash_option(), ignore_2_slash_options(), ignore_slash_line() - remove inaccurate test of variable expansion in a false block - added kitchen-sink test of parsing slash commands in a false block - removed spurious file that shouldn't have been in v24 - removed any potential free(NULL) calls *that I introduced*, others remain from master branch NOT done: - grouping all branching commands into one function - can be done in a later patch for clarity - combining _ef / _ev or _sf / _sv - can be done in a later patch for clarity On Fri, Mar 24, 2017 at 4:33 PM, Corey Huinker wrote: > On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO > wrote: > >> >> Hello Corey, >> >> I wished for the same thing, happy to use one if it is made known to me. >>> I pulled that pattern from somewhere else in the code, and given that the >>> max number of args for a command is around 4, I'm not too worried about >>> scaling. >>> >> >> If there are expressions one day like pgbench, the number of arguments >> becomes arbitrary. Have you looked at PQExpBuffer? > > > I will look into it. > > >> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it make sense to create a function instead of keeping the initial copy-paste? >>> >>> Yes, and a few things like that, but I wanted this patch to keep as much >>> code as-is as possible. >>> >> >> If you put the generic function at the same place, one would be more or >> less kept and the other would be just removed? > > >> "git diff --patience -w" gives a rather convenient output for looking at >> the patch. > > > Good to know about that option. > > As for a function for digested ignored slash options, it seems like I can > disregard the true/false value of the "semicolon" parameter. Is that > correct? > > >> I would suggest to put together all if-related backslash command, so that the stack management is all in one function instead of 4. >>> >>> I recognize the urge to group them together, but would there be any >>> actual >>> code sharing between them? Wouldn't I be either re-checking the string >>> "cmd" again, or otherwise setting an enum that I immediately re-check >>> inside the all_branching_commands() function? >>> >> >> I do not see that as a significant issue, especially compared to the >> benefit of having the automaton transition management in a single place. > > > I'm still struggling to see how this would add any clarity to the code > beyond what I can achieve by clustering the exec_command_(if/elif/else/endif) > near one another. > > > > 0001.if_endif.v25.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO wrote: > > Hello Corey, > > I wished for the same thing, happy to use one if it is made known to me. >> I pulled that pattern from somewhere else in the code, and given that the >> max number of args for a command is around 4, I'm not too worried about >> scaling. >> > > If there are expressions one day like pgbench, the number of arguments > becomes arbitrary. Have you looked at PQExpBuffer? I will look into it. > >>> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it >>> make sense to create a function instead of keeping the initial >>> copy-paste? >>> >> >> Yes, and a few things like that, but I wanted this patch to keep as much >> code as-is as possible. >> > > If you put the generic function at the same place, one would be more or > less kept and the other would be just removed? > "git diff --patience -w" gives a rather convenient output for looking at > the patch. Good to know about that option. As for a function for digested ignored slash options, it seems like I can disregard the true/false value of the "semicolon" parameter. Is that correct? > I would suggest to put together all if-related backslash command, so that >>> the stack management is all in one function instead of 4. >>> >> >> I recognize the urge to group them together, but would there be any actual >> code sharing between them? Wouldn't I be either re-checking the string >> "cmd" again, or otherwise setting an enum that I immediately re-check >> inside the all_branching_commands() function? >> > > I do not see that as a significant issue, especially compared to the > benefit of having the automaton transition management in a single place. I'm still struggling to see how this would add any clarity to the code beyond what I can achieve by clustering the exec_command_(if/elif/else/endif) near one another.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, I wished for the same thing, happy to use one if it is made known to me. I pulled that pattern from somewhere else in the code, and given that the max number of args for a command is around 4, I'm not too worried about scaling. If there are expressions one day like pgbench, the number of arguments becomes arbitrary. Have you looked at PQExpBuffer? However there is an impact on testing because of all these changes. ISTM that test cases should reflect this situation and test that \cd, \edit, ... are indeed ignored properly and taking account there expected args... I think one grand \if false \a \c some_connect_string ... \z some_table_name \endif should do the trick, Yes. Maybe some commands could be on the same line as well. but it wouldn't detect memory leaks. No miracle... There seems to be pattern repetition for _ev _ef and _sf _sv. Would it make sense to create a function instead of keeping the initial copy-paste? Yes, and a few things like that, but I wanted this patch to keep as much code as-is as possible. If you put the generic function at the same place, one would be more or less kept and the other would be just removed? "git diff --patience -w" gives a rather convenient output for looking at the patch. I would suggest to put together all if-related backslash command, so that the stack management is all in one function instead of 4. I recognize the urge to group them together, but would there be any actual code sharing between them? Wouldn't I be either re-checking the string "cmd" again, or otherwise setting an enum that I immediately re-check inside the all_branching_commands() function? I do not see that as a significant issue, especially compared to the benefit of having the automaton transition management in a single place. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > A few comments about the patch. > > Patch applies. "make check" ok. > > As already pointed out, there is one useless file in the patch. > > Although currently there is only one expected argument for boolean > expressions, the n² concatenation algorithm in gather_boolean_expression is > not very elegant. Is there some string buffer data structure which could be > used instead? > I wished for the same thing, happy to use one if it is made known to me. I pulled that pattern from somewhere else in the code, and given that the max number of args for a command is around 4, I'm not too worried about scaling. > > ISTM that ignore_boolean_expression may call free on a NULL pointer if the > expression is empty? > True. The psql code is actually littered with a lot of un-checked free(p) calls, so I started to wonder if maybe we had a wrapper on free() that checked for NULL. I'll fix this one just to be consistent. > > Generally I find the per-command functions rather an improvement. > I did too. I tried to split this patch up into two parts, one that broke out the functions, and one that added if-then, and found that the first patch was just as unwieldily without the if-then stuff as with. > > However there is an impact on testing because of all these changes. ISTM > that test cases should reflect this situation and test that \cd, \edit, ... > are indeed ignored properly and taking account there expected args... > I think one grand \if false \a \c some_connect_string ... \z some_table_name \endif should do the trick, but it wouldn't detect memory leaks. > > In "exec_command_connect" an argument is changed from "-reuse-previous" to > "-reuse-previous=", not sure why. > It shouldn't have been. Good catch. Most commands were able to be migrated with simple changes (status => *status, strcmp() if-block becomes active-if-block, etc), but that one was slightly different. > > There seems to be pattern repetition for _ev _ef and _sf _sv. Would it > make sense to create a function instead of keeping the initial copy-paste? > Yes, and a few things like that, but I wanted this patch to keep as much code as-is as possible. > > I think that some functions could be used for some repeated cases such as > "discard one arg", "discard one or two arg", "discard whole line", for the > various inactive branches, so as to factor out code. > I'd be in favor of that as well > > I would suggest to put together all if-related backslash command, so that > the stack management is all in one function instead of 4. > I recognize the urge to group them together, but would there be any actual code sharing between them? Wouldn't I be either re-checking the string "cmd" again, or otherwise setting an enum that I immediately re-check inside the all_branching_commands() function? > > For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not > sure why. An oversight. Good catch.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v24 highlights: - finally using git format-patch - all conditional slash commands broken out into their own functions (exec_command_$NAME) , each one tests if it's in an active branch, and if it's not it consumes the same number of parameters, but discards them. comments for each slash-command family were left as-is, may need to be bulked up. - TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif placement - documentation recommending ON_ERROR_STOP removed, because invalid expressions no longer throw off if-endif balance - documentation updated to reflex that contextually-correct-but-invalid boolean expressions are treated as false - psql_get_variable has a passthrough void pointer now, but I ended up not needing it. Instead, all slash commands in false blocks either fetch OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know. A few comments about the patch. Patch applies. "make check" ok. As already pointed out, there is one useless file in the patch. Although currently there is only one expected argument for boolean expressions, the n² concatenation algorithm in gather_boolean_expression is not very elegant. Is there some string buffer data structure which could be used instead? ISTM that ignore_boolean_expression may call free on a NULL pointer if the expression is empty? Generally I find the per-command functions rather an improvement. However there is an impact on testing because of all these changes. ISTM that test cases should reflect this situation and test that \cd, \edit, ... are indeed ignored properly and taking account there expected args... In "exec_command_connect" an argument is changed from "-reuse-previous" to "-reuse-previous=", not sure why. There seems to be pattern repetition for _ev _ef and _sf _sv. Would it make sense to create a function instead of keeping the initial copy-paste? I think that some functions could be used for some repeated cases such as "discard one arg", "discard one or two arg", "discard whole line", for the various inactive branches, so as to factor out code. I would suggest to put together all if-related backslash command, so that the stack management is all in one function instead of 4. For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not sure why. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Tom Lane wrote: > I'm not entirely convinced that function-per-command is an improvement > though. Seems like it would only help to the extent that you could do a > simple "return" to implement early exit, and it looks to me like that > doesn't work in a lot of places because you still have to clean up things > like malloc'd argument strings before you can return. So the question > we have to answer is whether this way looks cleaner than what we'd get if > we just changed the logic in-place. For the purpose of answering that > question, looking at the final state is the right thing to do. > > I don't have a definite opinion on that core question yet, since I've not > read this version of the patch. Anybody else want to give an opinion? Currently, exec_command is a 1500-line function. If I had to see how a single \-command worked, I would have to fold everything but the command I'm interested in, in case there's something nontrivial at function start or end (or even in between -- I would have to start by figuring out whether there's anything other than "else if" somewhere in those 1500 lines). I think splitting into command-specific functions makes this much easier to follow, particularly if we want to add extra tricks such as returning early etc. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sun, Mar 19, 2017 at 1:18 PM, Fabien COELHO wrote: > > Hello Corey, > > v24 highlights: >> > > The v24 patch is twice larger that the previous submission. Sigh. > > If I'm reading headers correctly, it seems that it adds an > "expected/psql-on-error-stop.out" file without a corresponding test > source in "sql/". Is this file to be simply ignored, or is a source missing? Ignore it. I created the new .sql/.out pair, realized that the file naming convention was underscores not dashes, changed them and evidently forgot that I had already added a dashed one to git.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sun, Mar 19, 2017 at 4:23 PM, Fabien COELHO wrote: > > Hello Tom, > > I'm not entirely convinced that function-per-command is an improvement >> though. [...] >> > > I don't have a definite opinion on that core question yet, since I've not >> read this version of the patch. Anybody else want to give an opinion? >> > > My 0.02€: > > I've already provided my view... > > Personnally I like good functions. Maybe a per-command-family set of > functions could improve the code readability, but (1) I'm not sure this is > achieved by this patch (eg the if-related state management is now > dispatched in 4 functions) and (2) I'm not sure that this approach helps > much with respect to trying to factor out backslash-command-related > active-or-not argument management. > > However I have not looked at the patch in detail. I'm planing to do so > later this week. I offered to split the patch into two steps (1. break each "family" into it's own function and 2. Do what's needed for \if-\endif) but got no response. I can still do that if people think it's worthwhile.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, I'm not entirely convinced that function-per-command is an improvement though. [...] I don't have a definite opinion on that core question yet, since I've not read this version of the patch. Anybody else want to give an opinion? My 0.02€: I've already provided my view... Personnally I like good functions. Maybe a per-command-family set of functions could improve the code readability, but (1) I'm not sure this is achieved by this patch (eg the if-related state management is now dispatched in 4 functions) and (2) I'm not sure that this approach helps much with respect to trying to factor out backslash-command-related active-or-not argument management. However I have not looked at the patch in detail. I'm planing to do so later this week. -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Alvaro Herrera writes: > The reason this is so large is that there is an entangled refactoring > patch, splitting the exec_command() function from one giant switch() > into one routine for each command. It's up to the committer whether to > do it all in one patch, or to request this to be split into a > refactoring patch plus another adding functionality on top. Assuming we want to do it that way at all, two steps would probably be easier to review in detail. I'm not entirely convinced that function-per-command is an improvement though. Seems like it would only help to the extent that you could do a simple "return" to implement early exit, and it looks to me like that doesn't work in a lot of places because you still have to clean up things like malloc'd argument strings before you can return. So the question we have to answer is whether this way looks cleaner than what we'd get if we just changed the logic in-place. For the purpose of answering that question, looking at the final state is the right thing to do. I don't have a definite opinion on that core question yet, since I've not read this version of the patch. Anybody else want to give an opinion? 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO wrote: > > Hello Corey, > > > v24 highlights: > > The v24 patch is twice larger that the previous submission. Sigh. The reason this is so large is that there is an entangled refactoring patch, splitting the exec_command() function from one giant switch() into one routine for each command. It's up to the committer whether to do it all in one patch, or to request this to be split into a refactoring patch plus another adding functionality on top. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v24 highlights: The v24 patch is twice larger that the previous submission. Sigh. If I'm reading headers correctly, it seems that it adds an "expected/psql-on-error-stop.out" file without a corresponding test source in "sql/". Is this file to be simply ignored, or is a source missing? -- 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Fri, Mar 17, 2017 at 2:18 PM, Corey Huinker wrote: > >> > \set x 'arg1 arg2' >> >> > \if false >> > \cmd_that_takes_exactly_two_args :x >> > \endif >> >> Yeah, throwing errors for bad arguments would also need to be suppressed. >> >> regards, tom lane >> > > Ok, barring other feedback, I'm going to take my marching orders as "make > each slash command active-aware". To keep that sane, I'm probably going to > break out each slash command family into it's own static function. > ...and here it is. v24 highlights: - finally using git format-patch - all conditional slash commands broken out into their own functions (exec_command_$NAME) , each one tests if it's in an active branch, and if it's not it consumes the same number of parameters, but discards them. comments for each slash-command family were left as-is, may need to be bulked up. - TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif placement - documentation recommending ON_ERROR_STOP removed, because invalid expressions no longer throw off if-endif balance - documentation updated to reflex that contextually-correct-but-invalid boolean expressions are treated as false - psql_get_variable has a passthrough void pointer now, but I ended up not needing it. Instead, all slash commands in false blocks either fetch OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know. From da8306acab0f0304d62f966c5217139bacfe722e Mon Sep 17 00:00:00 2001 From: Corey Huinker Date: Sat, 18 Mar 2017 12:47:25 -0400 Subject: [PATCH] Add \if...\endif blocks --- doc/src/sgml/ref/psql-ref.sgml | 90 +- src/bin/psql/.gitignore|2 + src/bin/psql/Makefile |4 +- src/bin/psql/command.c | 1513 src/bin/psql/common.c |4 +- src/bin/psql/conditional.c | 103 ++ src/bin/psql/conditional.h | 62 + src/bin/psql/copy.c|4 +- src/bin/psql/help.c|7 + src/bin/psql/mainloop.c| 54 +- src/bin/psql/prompt.c |6 +- src/bin/psql/prompt.h |3 +- src/bin/psql/startup.c |8 +- src/test/regress/expected/psql-on-error-stop.out | 506 +++ src/test/regress/expected/psql.out | 100 ++ .../regress/expected/psql_if_on_error_stop.out | 14 + src/test/regress/parallel_schedule |2 +- src/test/regress/serial_schedule |1 + src/test/regress/sql/psql.sql | 100 ++ src/test/regress/sql/psql_if_on_error_stop.sql | 17 + 20 files changed, 2316 insertions(+), 284 deletions(-) create mode 100644 src/bin/psql/conditional.c create mode 100644 src/bin/psql/conditional.h create mode 100644 src/test/regress/expected/psql-on-error-stop.out create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 2a9c412..18f8bfe 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2064,6 +2064,93 @@ hello 10 +\if expression +\elif expression +\else +\endif + + +This group of commands implements nestable conditional blocks. +A conditional block must begin with an \if and end +with an \endif. In between there may be any number +of \elif clauses, which may optionally be followed +by a single \else clause. Ordinary queries and +other types of backslash commands may (and usually do) appear between +the commands forming a conditional block. + + +The \if and \elif commands read +the rest of the line and evaluate it as a boolean expression. If the +expression is true then processing continues +normally; otherwise, lines are skipped until a +matching \elif, \else, +or \endif is reached. Once +an \if or \elif has succeeded, +later matching \elif commands are not evaluated but +are treated as false. Lines following an \else are +processed only if no earlier matching \if +or \elif succeeded. + + +Lines being skipped are parsed normally to identify queries and +backslash commands, but queries are not sent to the server, and +backslash commands other than conditionals +(\if, \elif, +\else, \endif) are +ignored. Conditional commands are checked only for valid nesting. + + +The expression argument +of \if or \elif +is subject to variable interpolati
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > > \set x 'arg1 arg2' > > > \if false > > \cmd_that_takes_exactly_two_args :x > > \endif > > Yeah, throwing errors for bad arguments would also need to be suppressed. > > regards, tom lane > Ok, barring other feedback, I'm going to take my marching orders as "make each slash command active-aware". To keep that sane, I'm probably going to break out each slash command family into it's own static function.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: >> In the end, I suspect that teaching all the backslash commands to do >> nothing after absorbing their arguments is likely to be the least messy >> way to tackle this, even if it makes for a rather bulky patch. > Perhaps, but just glancing at \connect makes me think that for some > commands (present or future) the number of args might depend on the value > of the first arg, and variable expansion-or-not, backtick execution-or-not > could alter the number of apparent args on the line, like this: > \set x 'arg1 arg2' > \if false > \cmd_that_takes_exactly_two_args :x > \endif Yeah, throwing errors for bad arguments would also need to be suppressed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > In the end, I suspect that teaching all the backslash commands to do > nothing after absorbing their arguments is likely to be the least messy > way to tackle this, even if it makes for a rather bulky patch. > > Perhaps, but just glancing at \connect makes me think that for some commands (present or future) the number of args might depend on the value of the first arg, and variable expansion-or-not, backtick execution-or-not could alter the number of apparent args on the line, like this: \set x 'arg1 arg2' \if false \cmd_that_takes_exactly_two_args :x \endif
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, I also fear that there are corner cases where the behavior would still be inconsistent. Consider \if ... \set foo `echo \endif should not appear here` In this instance, ISTM that there is no problem. On "\if true", set is executed, all is well. On "\if false", the whole line would be skipped because the if-related commands are only expected on their own line, all is well again. No problem. AFAICS, you misunderstood the example completely, or else you're proposing syntax restrictions that are even more bizarre and unintelligible than I thought before. Hmmm. The example you put forward does work as expected with the rule I suggested. It does not prove that the rules are good or sane, I'm just stating that the example would work consistently. We cannot have a situation where the syntax rules for backslash commands inside an \if are fundamentally different from what they are elsewhere; Indeed, I do not see an issue with requiring some new backslash commands to be on their own line: Any average programmer would put them like that anyway for readability. What is the point of trying to write code to handle strange unmaintainable oneliners? that's just going to lead to confusion and bug reports. Whatever is done, there will be some confusion and bug reports:-) If someone writes a strange one-liner and see that it generates errors, then the error messages should be clear enough. Maybe they will complain and fill in bugs because they like backslash-command oneliners. That is life. Now you are the committer and Corey is the developer. I'm just a reviewer trying to help. I can still review a larger patch which tries to be subtly compatible with a lack of previous clear design by adding code complexity, even if I think that this particular effort is a bad idea (i.e. mis-spent resource on a useless sub-feature which makes future maintenance harder). With some luck, Corey may find a way of doing it which is not too bad. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > I think Fabien was arguing that inside a false block there would be no > syntax rules beyond "is the first non-space character on this line a '\' > and if so is it followed with a if/elif/else/endif?". If the answer is no, > skip the line. To me that seems somewhat similar to Tom's suggestion that a > false branch just keeps consuming text until it encounters a \conditional > or EOF. Hmm. If we can keep the syntax requirements down to "\if and friends must be the first backslash command on the line", and not change the apparent behavior for any other command type, it probably would be okay from the user's standpoint. I'm not really convinced that this approach will accomplish that, though, and especially not that it will do so without injecting some ugliness into the core lexer. In the end, I suspect that teaching all the backslash commands to do nothing after absorbing their arguments is likely to be the least messy way to tackle this, even if it makes for a rather bulky patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Fri, Mar 17, 2017 at 11:42 AM, Tom Lane wrote: > Fabien COELHO writes: > >> I also fear that there are corner cases where the behavior would still > >> be inconsistent. Consider > >> > >> \if ... > >> \set foo `echo \endif should not appear here` > > > In this instance, ISTM that there is no problem. On "\if true", set is > > executed, all is well. On "\if false", the whole line would be skipped > > because the if-related commands are only expected on their own line, all > > is well again. No problem. > > AFAICS, you misunderstood the example completely, or else you're proposing > syntax restrictions that are even more bizarre and unintelligible than > I thought before. We cannot have a situation where the syntax rules for > backslash commands inside an \if are fundamentally different from what > they are elsewhere; that's just going to lead to confusion and bug > reports. > > regards, tom lane > I think Fabien was arguing that inside a false block there would be no syntax rules beyond "is the first non-space character on this line a '\' and if so is it followed with a if/elif/else/endif?". If the answer is no, skip the line. To me that seems somewhat similar to Tom's suggestion that a false branch just keeps consuming text until it encounters a \conditional or EOF.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: >> I also fear that there are corner cases where the behavior would still >> be inconsistent. Consider >> >> \if ... >> \set foo `echo \endif should not appear here` > In this instance, ISTM that there is no problem. On "\if true", set is > executed, all is well. On "\if false", the whole line would be skipped > because the if-related commands are only expected on their own line, all > is well again. No problem. AFAICS, you misunderstood the example completely, or else you're proposing syntax restrictions that are even more bizarre and unintelligible than I thought before. We cannot have a situation where the syntax rules for backslash commands inside an \if are fundamentally different from what they are elsewhere; that's just going to lead to confusion and bug reports. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > command.c:38:25: fatal error: conditional.h: No such file or directory > #include "conditional.h" > Odd, it's listed as a new file in git status. Anyway, my point of posting the WIP patch was to give people a reference point and spark discussion about the next step, and it succeeded at that.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, ISTM that I've tried to suggest to work around that complexity by: - document that \if-related commands should only occur at line start (and extend to eol). - detect and complain when this is not the case. I think this is a lousy definition, and would never be considered if we were working in a green field. Yes, sure. As you pointed out, the field is not green: there is no clean lexical convention, too bad. I'm trying to deal with that without too much fuss in the code. Moreover, preventing such cases would be pretty darn ugly/messy as well. I also fear that there are corner cases where the behavior would still be inconsistent. Consider \if ... \set foo `echo \endif should not appear here` In this instance, ISTM that there is no problem. On "\if true", set is executed, all is well. On "\if false", the whole line would be skipped because the if-related commands are only expected on their own line, all is well again. No problem. Another more interesting one would be: \if ... \unset foo \endif On true, unset get its argument, then endif is detected as a backslash command, but it would see that it is not on its own line, so it would error out *and* be ignored. On false, the whole line would be ignored, it would just not complain, but it would be the same, i.e. it is *not* an \endif again. The drawback is only that the wrong \endif is not detected when under a false branch. That is why I added a third bullet "call border cases a feature". ISTM that the proposed simple rules allow to deal with the situation without having to dive into each command lexing rules, and changing the existing code significantly. The drawback is that misplaced \endif are not detected in false branch, but they are ignored anyway, which is fine. I'm imagining that instead of [...] char *envvar = psql_scan_slash_option(scan_state, we'd write [...] char *envvar = args[0]; where the args array had been filled at the top of the function. The top-of-function code would have to know all the cases where commands didn't use basic OT_NORMAL processing, but there aren't that many of those, I think. Yep, I understood the idea. There are a few of those, about 49 OT_* in "command.c", including 34 OT_NORMAL, 1 OT_NO_EVAL, 3 OT_FILEPIPE, 9 OT_WHOLELINE, some OT_SQLHACKID & OT_SQLID. I'm not sure of the combinations. It still means splitting command lexing knowledge in several places. I'm not convinced by the impact on the resulting code with regard to readability and maintainability, so if there could be a way to get something without taking that path that would be nice, hence my suggestions. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Fabien COELHO writes: > ISTM that I've tried to suggest to work around that complexity by: > - document that \if-related commands should only occur at line start > (and extend to eol). > - detect and complain when this is not the case. I think this is a lousy definition, and would never be considered if we were working in a green field. Moreover, preventing such cases would be pretty darn ugly/messy as well. I also fear that there are corner cases where the behavior would still be inconsistent. Consider \if ... \set foo `echo \endif should not appear here` If the \if succeeds, the result of the second line would be to set foo to "endif should not appear here" (and we'd remain in the \if block). But if the \if fails and we need to skip the \set command, any approach that involves changing the argument parsing rules will fail to recognize the backtick construct, and then will see the \endif as a command. Similar examples can be constructed using \copy. It's possible that we could keep the implementation that uses an early exit from exec_command() if we were to move argument collection for all backslash commands up to the start of the function. It would still be a bit invasive, but perhaps not too awful: I'm imagining that instead of else if (strcmp(cmd, "setenv") == 0) { char *envvar = psql_scan_slash_option(scan_state, OT_NORMAL, NULL, false); char *envval = psql_scan_slash_option(scan_state, OT_NORMAL, NULL, false); we'd write else if (strcmp(cmd, "setenv") == 0) { char *envvar = args[0]; char *envval = args[1]; where the args array had been filled at the top of the function. The top-of-function code would have to know all the cases where commands didn't use basic OT_NORMAL processing, but there aren't that many of those, I think. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
"Daniel Verite" writes: > Tom Lane wrote: >> OT_WHOLE_LINE is not what you want because that results in verbatim >> copying, without variable expansion or anything > But if we want to implement "\if defined :foo" in the future > isn't it just what we need? I don't think that should mean what you think. I believe an appropriate spelling of what you mean is "\if defined foo". What you wrote should result in foo being expanded and then a defined-ness test being performed on whatever variable name results. > Also we could leave open the option to accept an SQL expression > here. I expect people will need SQL as the evaluator in a lot of cases. Right, and they'll also want to insert variable references into that SQL. In the short term though, `expr ...` is going to be the solution, and that means we'd better not throw away the behavior of expanding back-ticks. > There's a precedent with \copy accepting a query inside parentheses, > using OT_WHOLE_LINE. IMV, \copy is just about completely broken in this regard, precisely because it fails to expand variable references. I don't want to emulate that brain-damage for \if. (I believe, btw, that part of the reason for \copy behaving this way is that we wanted to preserve an ancient behavior whereby Windows users were not forced to double backslashes in \windows\style\path\names. Fortunately, that bit of silliness need not be considered for \if.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Tom Lane wrote: > OT_WHOLE_LINE is not what you want because that results in verbatim > copying, without variable expansion or anything But if we want to implement "\if defined :foo" in the future isn't it just what we need? Also we could leave open the option to accept an SQL expression here. I expect people will need SQL as the evaluator in a lot of cases. So far we need to do that: SELECT sql_expr ... AS varname \gset \if :varname ... \endif Surely users will wonder right away why they can't write it like this instead: \if (sql_expr) ... \endif There's a precedent with \copy accepting a query inside parentheses, using OT_WHOLE_LINE. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On 2017-03-17 02:28, Corey Huinker wrote: Attached is the latest work. Not everything is done yet. I post it because 0001.if_endif.v23.diff This patch does not compile for me (gcc 6.3.0): command.c:38:25: fatal error: conditional.h: No such file or directory #include "conditional.h" ^ compilation terminated. make[3]: *** [command.o] Error 1 make[2]: *** [all-psql-recurse] Error 2 make[2]: *** Waiting for unfinished jobs make[1]: *** [all-bin-recurse] Error 2 make: *** [all-src-recurse] Error 2 Perhaps that is expected, as "Not everything is done yet", but I can't tell from your email so I thought I'd report ir anyway. Ignore as appropriate... Thanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey & Tom, What is not done: - skipped slash commands still consume the rest of the line That last part is big, to quote Tom: * More generally, I do not think that the approach of having exec_command simply fall out immediately when in a false branch is going to work, because it ignores the fact that different backslash commands have different argument parsing rules. Some will eat the rest of the line and some won't. I'm afraid that it might be necessary to remove that code block and add a test to every single backslash command that decides whether to actually perform its action after it's consumed its arguments. That would be tedious :-(. But as it stands, backslash commands will get parsed differently (ie with potentially-different ending points) depending on whether they're in a live branch or not, and that seems just way too error-prone to be allowed to stand. ISTM that I've tried to suggest to work around that complexity by: - document that \if-related commands should only occur at line start (and extend to eol). - detect and complain when this is not the case. - if some border cases are not detected, call it a feature. ISTM that Tom did not respond to this possibly simpler approach... Maybe a "no" would be enough before starting heavy work which would touch all other commands... Tom? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Attached is the latest work. Not everything is done yet. I post it because the next step is likely to be "tedious" as Tom put it, and if there's a way out of it, I want to avoid it. What is done: - all changes here built off the v22 patch - any function which had scan_state and cond_stack passed in now only has scan_state, and cond_stack is extracted from the cb_passthrough pointer. - ConditonalStack is now only explictly passed to get_prompt ... which doesn't have scan state - Conditional commands no longer reset scan state, nor do they clear the query buffer - boolean expressions consume all options, but only evaluate variables and backticks in situations where those would be active - invalid boolean arguments are treated as false - contextually wrong \else, \endif, \elif are still errors What is not done: - TAP tests are not converted to regular regression test(s) - skipped slash commands still consume the rest of the line That last part is big, to quote Tom: * More generally, I do not think that the approach of having exec_command simply fall out immediately when in a false branch is going to work, because it ignores the fact that different backslash commands have different argument parsing rules. Some will eat the rest of the line and some won't. I'm afraid that it might be necessary to remove that code block and add a test to every single backslash command that decides whether to actually perform its action after it's consumed its arguments. That would be tedious :-(. But as it stands, backslash commands will get parsed differently (ie with potentially-different ending points) depending on whether they're in a live branch or not, and that seems just way too error-prone to be allowed to stand. If that's what needs to be done, does it make sense to first commit a pre-patch that encapsulates each command family ( \c and \connect are a family, all \d* commands are one family) into its own static function? It would make the follow-up patch to if-endif cleaner and easier to review. On Thu, Mar 16, 2017 at 5:14 PM, Tom Lane wrote: > Corey Huinker writes: > > Ok, I've got some time now and I'm starting to dig into this. I'd like to > > restate what I *think* my feedback is, in case I missed or misunderstood > > something. > > ... > > 3. Change command scans to scan the whole boolean expression, not just > > OT_NORMAL. > > There's a couple ways to go about this. My gut reaction is to create a > new > > scan type OT_BOOL_EXPR, which for the time being is the same as > > OT_WHOLE_LINE, but could one day be something different. > > OT_WHOLE_LINE is not what you want because that results in verbatim > copying, without variable expansion or anything. My vote would be to > repeatedly do OT_NORMAL until you get a NULL, thereby consuming as > many regular arguments as the backslash command has. (After which, > if it wasn't exactly one argument, complain, for the moment. But this > leaves the door open for something like "\if :foo = :bar".) Note that > this implies that "\if some-expression \someothercommand" will be allowed, > but I think that's fine, as I see no reason to allow backslashes in > whatever if-expression syntax we invent later. OT_WHOLE_LINE is a bit of > a bastard child and I'd just as soon not define it as being the lexing > behavior of any new commands. > > > 5. Allow contextually-correct invalid boolean expressions to map to > false. > > > Out-of-context \endif, \else, and \elif commands remain as errors to be > > ignored, invalid expressions in an \if or legallyl-placed \elif are just > > treated as false. > > WFM. > > regards, tom lane > diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 2a9c412..7743fb0 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2064,6 +2064,102 @@ hello 10 +\if expression +\elif expression +\else +\endif + + +This group of commands implements nestable conditional blocks. +A conditional block must begin with an \if and end +with an \endif. In between there may be any number +of \elif clauses, which may optionally be followed +by a single \else clause. Ordinary queries and +other types of backslash commands may (and usually do) appear between +the commands forming a conditional block. + + +The \if and \elif commands read +the rest of the line and evaluate it as a boolean expression. If the +expression is true then processing continues +normally; otherwise, lines are skipped until a +matching \elif, \else, +or \endif is reached. Once +an \if or \elif has succeeded, +later matching \elif commands are not evaluated but +are treated as false. Lines following an \else are +processed only if no earlier matching \if +or \elif succeeded. + +
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > Ok, I've got some time now and I'm starting to dig into this. I'd like to > restate what I *think* my feedback is, in case I missed or misunderstood > something. > ... > 3. Change command scans to scan the whole boolean expression, not just > OT_NORMAL. > There's a couple ways to go about this. My gut reaction is to create a new > scan type OT_BOOL_EXPR, which for the time being is the same as > OT_WHOLE_LINE, but could one day be something different. OT_WHOLE_LINE is not what you want because that results in verbatim copying, without variable expansion or anything. My vote would be to repeatedly do OT_NORMAL until you get a NULL, thereby consuming as many regular arguments as the backslash command has. (After which, if it wasn't exactly one argument, complain, for the moment. But this leaves the door open for something like "\if :foo = :bar".) Note that this implies that "\if some-expression \someothercommand" will be allowed, but I think that's fine, as I see no reason to allow backslashes in whatever if-expression syntax we invent later. OT_WHOLE_LINE is a bit of a bastard child and I'd just as soon not define it as being the lexing behavior of any new commands. > 5. Allow contextually-correct invalid boolean expressions to map to false. > Out-of-context \endif, \else, and \elif commands remain as errors to be > ignored, invalid expressions in an \if or legallyl-placed \elif are just > treated as false. WFM. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Mon, Mar 13, 2017 at 5:21 PM, Tom Lane wrote: > "Daniel Verite" writes: > > Tom Lane wrote: > >> when we see \if is that we do nothing but absorb text > >> until we see the matching \endif. At that point we could bitch and > throw > >> everything away if, say, there's \elif after \else, or anything else you > >> want to regard as a "compile time error". Otherwise we start execution, > >> and from there on it probably has to behave as we've been discussing. > >> But this'd be pretty unfriendly from an interactive standpoint, and I'm > >> not really convinced that it makes for significantly better error > >> reporting. > > > On the whole, isn't that a reasonable model to follow for psql? > > One thing that occurs to me after more thought is that with such a model, > we could not have different lexing rules for live vs not-live branches, > since we would not have made those decisions before scanning the input. > This seems problematic. Even if you discount the question of whether > variable expansion is allowed to change command-boundary decisions, we'd > still not want backtick execution to happen everywhere in the block, ISTM. > > Maybe we could fix things so that backtick execution happens later, but > it would be a pretty significant and invasive change to backslash command > execution, I'm afraid. > > regards, tom lane > Ok, I've got some time now and I'm starting to dig into this. I'd like to restate what I *think* my feedback is, in case I missed or misunderstood something. 1. Convert perl tests to a single regular regression test. 2. Have MainLoop() pass the cond_stack to the lexer via psql_scan_set_passthrough(scan_state, (void *) cond_stack); 3. Change command scans to scan the whole boolean expression, not just OT_NORMAL. There's a couple ways to go about this. My gut reaction is to create a new scan type OT_BOOL_EXPR, which for the time being is the same as OT_WHOLE_LINE, but could one day be something different. 4. Change variable expansion and backtick execution in false branches to match new policy. I've inferred that current preference would be for no expansion and no execution. 5. Allow contextually-correct invalid boolean expressions to map to false. Out-of-context \endif, \else, and \elif commands remain as errors to be ignored, invalid expressions in an \if or legallyl-placed \elif are just treated as false. Did I miss anything?
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
"Daniel Verite" writes: > Tom Lane wrote: >> when we see \if is that we do nothing but absorb text >> until we see the matching \endif. At that point we could bitch and throw >> everything away if, say, there's \elif after \else, or anything else you >> want to regard as a "compile time error". Otherwise we start execution, >> and from there on it probably has to behave as we've been discussing. >> But this'd be pretty unfriendly from an interactive standpoint, and I'm >> not really convinced that it makes for significantly better error >> reporting. > On the whole, isn't that a reasonable model to follow for psql? One thing that occurs to me after more thought is that with such a model, we could not have different lexing rules for live vs not-live branches, since we would not have made those decisions before scanning the input. This seems problematic. Even if you discount the question of whether variable expansion is allowed to change command-boundary decisions, we'd still not want backtick execution to happen everywhere in the block, ISTM. Maybe we could fix things so that backtick execution happens later, but it would be a pretty significant and invasive change to backslash command execution, I'm afraid. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: >> Barring objection I'll push this so that Corey can rebase over it. > Seems straightforward, and I appreciate you doing it for me! Hearing no objections, pushed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Tom Lane wrote: > when we see \if is that we do nothing but absorb text > until we see the matching \endif. At that point we could bitch and throw > everything away if, say, there's \elif after \else, or anything else you > want to regard as a "compile time error". Otherwise we start execution, > and from there on it probably has to behave as we've been discussing. > But this'd be pretty unfriendly from an interactive standpoint, and I'm > not really convinced that it makes for significantly better error > reporting. This is basically what bash does. In an if/else/fi block in an interactive session, the second prompt is displayed at every new line and nothing gets executed until it recognizes the end of the block and it's valid as a whole. Otherwise, nothing of the block gets executed. That doesn't strike me as unfriendly. When non-interactive, in addition to the block not being executed, the fact that it fails implies that the execution of the current script is ended, independently of the errexit setting. If errexit is set, the interpreter terminates. If it was an included script and errexit is not set, the execution resumes after the point of the inclusion. On the whole, isn't that a reasonable model to follow for psql? Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > Barring objection I'll push this so that Corey can rebase over it. > > regards, tom lane > Seems straightforward, and I appreciate you doing it for me!
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
I wrote: > IIRC, I objected to putting knowledge of ConditionalStack > into the shared psqlscan.l lexer, and I still think that would be a bad > idea; but we need some way to get the lexer to shut that off. Probably > the best way is to add a passthrough "void *" argument that would let the > get_variable callback function mechanize the rule about not expanding > in a false branch. Here's a proposed patch that adds a passthrough of this sort. The passthrough argument is passed only to the get_variable callback. I dithered about whether to also pass it to the write_error callback, but ultimately decided not to for now. Neither psql nor pgbench wants it, and in the case of psql we'd have to invent a separate wrapper function because we would certainly not want to change the signature of psql_error(). Barring objection I'll push this so that Corey can rebase over it. regards, tom lane diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 1aa56ab..e9d4fe6 100644 *** a/src/bin/psql/common.c --- b/src/bin/psql/common.c *** setQFout(const char *fname) *** 119,127 * If "escape" is true, return the value suitably quoted and escaped, * as an identifier or string literal depending on "as_ident". * (Failure in escaping should lead to returning NULL.) */ char * ! psql_get_variable(const char *varname, bool escape, bool as_ident) { char *result; const char *value; --- 119,131 * If "escape" is true, return the value suitably quoted and escaped, * as an identifier or string literal depending on "as_ident". * (Failure in escaping should lead to returning NULL.) + * + * "passthrough" is the pointer previously given to psql_scan_set_passthrough. + * psql currently doesn't use this. */ char * ! psql_get_variable(const char *varname, bool escape, bool as_ident, ! void *passthrough) { char *result; const char *value; diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h index a83bc69..3d8b8da 100644 *** a/src/bin/psql/common.h --- b/src/bin/psql/common.h *** *** 16,22 extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe); extern bool setQFout(const char *fname); ! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident); extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2); --- 16,23 extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe); extern bool setQFout(const char *fname); ! extern char *psql_get_variable(const char *varname, bool escape, bool as_ident, ! void *passthrough); extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2); diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l index 5b7953b..ba4a08d 100644 *** a/src/bin/psql/psqlscanslash.l --- b/src/bin/psql/psqlscanslash.l *** other . *** 243,249 yyleng - 1); value = cur_state->callbacks->get_variable(varname, false, ! false); free(varname); /* --- 243,250 yyleng - 1); value = cur_state->callbacks->get_variable(varname, false, ! false, ! cur_state->cb_passthrough); free(varname); /* diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l index 1b29341..19b3e57 100644 *** a/src/fe_utils/psqlscan.l --- b/src/fe_utils/psqlscan.l *** other . *** 700,706 if (cur_state->callbacks->get_variable) value = cur_state->callbacks->get_variable(varname, false, ! false); else value = NULL; --- 700,707 if (cur_state->callbacks->get_variable) value = cur_state->callbacks->get_variable(varname, false, ! false, ! cur_state->cb_passthrough); else value = NULL; *** psql_scan_destroy(PsqlScanState state) *** 923,928 --- 924,942 } /* + * Set the callback passthrough pointer for the lexer. + * + * This could have been integrated into psql_scan_create, but keeping it + * separate allows the application to change the pointer later, which might + * be useful. + */ + void + psql_scan_set_passthrough(PsqlScanState state, void *passthrough) + { + state->cb_passthrough = passthrough; + } + + /* * Set up to perform lexing of the given input line. * * The text at *line, extending for line_len bytes, will be scanned by *** psqlscan_escape_variable(PsqlScanState s *** 1409,1415 /* Variable lookup. */ varname = psqlscan_extract_substring(state, txt + 2, len - 3); if (state->callbacks->get_variable) ! value = state->callbacks->get_variable(varname, true, as_ident); else value = NULL; free(varname); --- 1423,1430 -
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
"David G. Johnston" writes: > There are only four commands and a finite number of usage permutations. > Enumerating and figuring out the proper behavior for each should be done. > Thus - If the expressions are bad they are considered false but the block > is created > If the flow-control command is bad the system will tell the user why and > how to get back to a valid state - the entire machine state goes INVALID > until a corrective command is encountered. > For instance: > \if > \else > \elif > warning: elif block cannot occur directly within an \else block. either > start a new \if, \endif the current scope, or type \else to continue > entering commands into the existing else block. no expression evaluation > has occurred. > \echo 'c' > warning: command ignored in broken \if block scope - see prior correction > options This is looking a whole lot like the overcomplicated error reporting that we already considered and rejected. I think it's sufficient to print something like "\elif is not allowed to follow \else; command ignored" and not change state. We're not really helping anybody by going into an "invalid machine state" AFAICS, and having such a thing complicates the mental model more than I'd like. A different way of looking at this problem, which will seem like overkill right now but would absolutely not be once you consider looping, is that what should happen when we see \if is that we do nothing but absorb text until we see the matching \endif. At that point we could bitch and throw everything away if, say, there's \elif after \else, or anything else you want to regard as a "compile time error". Otherwise we start execution, and from there on it probably has to behave as we've been discussing. But this'd be pretty unfriendly from an interactive standpoint, and I'm not really convinced that it makes for significantly better error reporting. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sun, Mar 12, 2017 at 10:24 AM, Tom Lane wrote: > > One point here is that we need to distinguish problems in the expression, > which could arise from changing variable values, from some other types of > mistakes like \elif with no preceding \if. When you see something like > that you pretty much have to treat it as a no-op; but I don't think that's > a problem for scripting usage. > One of my discarded write-ups from last night made a point that we don't really distinguish between run-time and compile-time errors - possibly because we haven't had to until now. If we detect what would be considered a compile-time error (\elif after \else for instance) we could treat anything that isn't a conditional meta-command as a no-op with a warning (and exit in stop-script mode). There are only four commands and a finite number of usage permutations. Enumerating and figuring out the proper behavior for each should be done. Thus - If the expressions are bad they are considered false but the block is created If the flow-control command is bad the system will tell the user why and how to get back to a valid state - the entire machine state goes INVALID until a corrective command is encountered. For instance: \if \else \elif warning: elif block cannot occur directly within an \else block. either start a new \if, \endif the current scope, or type \else to continue entering commands into the existing else block. no expression evaluation has occurred. \echo 'c' warning: command ignored in broken \if block scope - see prior correction options Yes, that's wordy, but if that was shown the user would be able to recognize their situation and be able to get back to their desired state. Figuring out what the valid correction commands are for each invalid state, and where the user is left, is tedious but mechanical. So we'd need an INVALID state as well as the existing IGNORE state. \endif would always work - but take you up one nesting level The user shouldn't need to memorize the invalid state rules. While we could document them and point the reader there having them inline seems preferable. > > We could imagine resolving this tension by treating failed \if expressions > differently in interactive and noninteractive cases. But I fear that cure > would be worse than the disease. > I don't think this becomes necessary - we should distinguish the error types the same in both modes.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > (1) document that \if-related commands MUST be on their own > line (i.e. like cpp #if directives?). > I have no opinion on whether \if-related comments must be on their own line, though I coded as if that were the case. I want to point out that the goal down the road is to allow rudimentary expressions beyond just 'will this string cast to boolean true'. For example, in the earlier thread "Undefined psql variables", I proposed a slash command that would test if a named psql var were defined, and if not then assign it a value. Tom suggested leveraging if-then infrastructure like this \if not defined(x) \set x y \fi Which would be great. I ask that whatever we decide in terms of how much more input we read to digest the expression allow for constructs like the one above.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > Reading this, I started to wonder "so how did I get that impression?" and I > found this from Feb 9: > IMO, an erroneous backslash command should have no effect, period. > "It failed but we'll treat it as if it were valid" is a rathole > I don't want to descend into. It's particularly bad in interactive > mode, because the most natural thing to do is correct your spelling > and issue the command again --- but if psql already decided to do > something on the strength of the mistaken command, that doesn't work, > and you'll have to do something or other to unwind the unwanted > control state before you can get back to what you meant to do. Yeah, it's not the greatest thing for interactive usage, but as we already discussed, this feature needs to be optimized for scripting not interaction --- and even a bit of thought shows that the current behavior is disastrous for scripting. If your only suggestion for getting sane behavior in a script is "set ON_ERROR_STOP", you've failed to provide useful error handling. One point here is that we need to distinguish problems in the expression, which could arise from changing variable values, from some other types of mistakes like \elif with no preceding \if. When you see something like that you pretty much have to treat it as a no-op; but I don't think that's a problem for scripting usage. We could imagine resolving this tension by treating failed \if expressions differently in interactive and noninteractive cases. But I fear that cure would be worse than the disease. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sun, Mar 12, 2017 at 1:52 AM, David G. Johnston < david.g.johns...@gmail.com> wrote: > > Oddly, Corey was using you as support for this position...though without an actual quote: > > """ Reading this, I started to wonder "so how did I get that impression?" and I found this from Feb 9: IMO, an erroneous backslash command should have no effect, period. "It failed but we'll treat it as if it were valid" is a rathole I don't want to descend into. It's particularly bad in interactive mode, because the most natural thing to do is correct your spelling and issue the command again --- but if psql already decided to do something on the strength of the mistaken command, that doesn't work, and you'll have to do something or other to unwind the unwanted control state before you can get back to what you meant to do.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Tom, * Daniel Verite previously pointed out the desirability of disabling variable expansion while skipping script. That doesn't seem to be here, ISTM that it is still there, but for \elif conditions which are currently always checked. fabien=# \if false fabien@# \echo `echo BAD` command ignored, use \endif or Ctrl-C to exit current branch. fabien@# \else fabien=# \echo `echo OK` OK fabien=# \endif IIRC, I objected to putting knowledge of ConditionalStack into the shared psqlscan.l lexer, and I still think that would be a bad idea; but we need some way to get the lexer to shut that off. Probably the best way is to add a passthrough "void *" argument that would let the get_variable callback function mechanize the rule about not expanding in a false branch. Hmmm. I see this as a circumvolute way of providing the stack knowledge without actually giving the stack... it seems that would work, so why not. * Whether or not you think it's important not to expand skipped variables, I think that it's critical that skipped backtick expressions not be executed. \if something \elif `expr :var1 + :var2 = :var3` \endif I think it's essential that expr not be called if the first if-condition succeeded. This was the behavior at some point, but it was changed because we understood that it was required that boolean errors were detected and the resulting command be simply ignored. I'm really fine with having that back. * The documentation says that an \if or \elif expression extends to the end of the line, but actually the code is just eating one OT_NORMAL argument. That means it's OK to do this: [...] More generally, I do not think that the approach of having exec_command simply fall out immediately when in a false branch is going to work, because it ignores the fact that different backslash commands have different argument parsing rules. Some will eat the rest of the line and some won't. I'm afraid that it might be necessary to remove that code block and add a test to every single backslash command that decides whether to actually perform its action after it's consumed its arguments. That would be tedious :-(. Indeed. IMO the very versatile lexing conventions of backslash commands, or rather their actual lack of any consistency, makes it hard to get something very sane out of this, especially with the "do not evaluate in false branch" argument. As a simple way out, I suggest to: (1) document that \if-related commands MUST be on their own line (i.e. like cpp #if directives?). (2) check that it is indeed the case when one \if-related command detected. (3) call it a feature if someone does not follow the rule and gets a strange behavior as a result, as below: regression=# \if 0 regression@# \echo foo \endif command ignored, use \endif or Ctrl-C to exit current branch. (notice we're not out of the conditional) * I'm not on board with having a bad expression result in failing the \if or \elif altogether. This was understood as a requirement on previous versions which did not fail. I do agree that it seems better to keep the structure on errors, at least for script usage. It was stated several times upthread that that should be processed as though the result were "false", and I agree with that. I'm fine with that, if everyone could agree before Corey spends more time on this... [...] We might as well replace the recommendation to use ON_ERROR_STOP with a forced abort() for an invalid expression value, because trying to continue a script with this behavior is entirely useless. Hmmm. Maybe your remark is rhetorical. That could be for scripting use, but in interactive mode aborting coldly on syntax errors is not too nice for the user. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Starting to poke at this... the proposal to add prove checks for psql just to see whether \if respects ON_ERROR_STOP seems like an incredibly expensive way to test a rather minor point. On my machine, "make check" in bin/psql goes from zero time to close to 8 seconds. I'm not really on board with adding that kind of time to every buildfarm run for the foreseeable future just for this. ISTM that these tests allowed to find bugs in the implementation, so they were useful at some point. They are still useful in the short term if the implementation is to be changed significantly to respond to your various requirements. The underlying issue with TAP test is that it installs a new cluster on each script, which is quite costly. In this case, the same result could be achieved with a number of small failing tests, which only launch "psql". Could that be acceptable? What you suggest is to keep only *one* failing test, which I find is kind of a regression from a testing coverage perspective, although obviously it is possible. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sat, Mar 11, 2017 at 5:45 PM, Tom Lane wrote: > > * Whether or not you think it's important not to expand skipped variables, > I think that it's critical that skipped backtick expressions not be > executed. > [...] > I do not think that a skipped \if or \elif > should evaluate its argument at all. > > [...] > * I'm not on board with having a bad expression result in failing > the \if or \elif altogether. It was stated several times upthread > that that should be processed as though the result were "false", > and I agree with that. +1 Oddly, Corey was using you as support for this position...though without an actual quote: """ Tom was pretty adamant that invalid commands are not executed. So in a case like this, with ON_ERROR_STOP off: \if false \echo 'a' \elif true \echo 'b' \elif invalid \echo 'c' \endif Both 'b' and 'c' should print, because "\elif invalid" should not execute. The code I had before was simpler, but it missed that. """ https://www.postgresql.org/message-id/CADkLM%3De9BY_- PT96mcs4qqiLtt8t-Fp%3De_AdycW-aS0OQvbC9Q%40mail.gmail.com Also, Robert made a comment somewhere along the line about users wanting to simply re-type the intended line if the "invalid" was interactive and due to a typo. That concern is pretty much limited to just the "\if" situation - if you typo an "\elif" block you can just type "\elif" again and begin yet another "\elif" block. I say we live with it and focus on the UX - if you type \if no matter what happens after you hit enter you are in a conditional block and will need to \endif at some point. Re-typing the correct \if command will just make you need another one of them. David J.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sat, Mar 11, 2017 at 9:40 PM, Tom Lane wrote: > I thought the same of the version you were complaining about, but > the current patch seems to have dialed it back a good deal. Do you > still find the current error messages unmaintainable? I haven't looked, but I had the impression this had been much improved. >> But I have not taken a position on what should happen if the >> condition for \if or \elsif evaluates to a baffling value. Corey's >> prior proposal was to treat it, essentially, as neither true nor >> false, skipping both arms of the if. Tom seems to want an invalid >> value treated as false. You could also imagine pretending that the >> command never happened at all, likely leading to complete chaos. > > Hmm, if that "prior proposal" was indeed on the table, I missed it. > The current patch, AFAICS, implements your third choice, which I quite > agree would lead to complete chaos; there would be no way to write a > script that did anything useful with that. Well, other than: don't write a script with invalid commands in it. But I'm not seriously advocating for that position. > It is interesting to think about what would happen if "expr is neither > true nor false" were defined as "skip immediately to \endif" (which > I think is the natural generalization of what you said to apply to an > intermediate \elif). I believe that it'd be possible to work with it, > but it's not very clear if it'd be easier or harder to work with than > the rule of treating bogus results as false. What is clear is that > it'd be unlike any other conditional construct I ever worked with. True. > As was pointed out upthread, "treat error results as false" is what > you get from "if" in a POSIX shell. I think it's fair also to draw > an analogy to what SQL does with null boolean values, which is to > treat them as false when a decision is required (in, eg, WHERE or > CASE). So I think "treat bogus results as false" is the most > conservative, least likely to cause unhappy surprises, solution here. I don't mind that. I was simply stating that I hadn't advocated for anything in particular. >> Other positions are also possible. > > If you've got concrete ideas about that, let's hear them. I'm not > trying to foreclose discussion. I personally don't, but others may. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Robert Haas writes: > I think that I have not taken a firm position on what the behavior > should be with respect to errors.I took the position that the > messages being printed saying what happened were too detailed, because > they not only described what had happened but also tried to > prognosticate what would happen next, which was dissimilar to what we > do elsewhere and likely to be hard to maintain - or even get right for > v1. I thought the same of the version you were complaining about, but the current patch seems to have dialed it back a good deal. Do you still find the current error messages unmaintainable? > But I have not taken a position on what should happen if the > condition for \if or \elsif evaluates to a baffling value. Corey's > prior proposal was to treat it, essentially, as neither true nor > false, skipping both arms of the if. Tom seems to want an invalid > value treated as false. You could also imagine pretending that the > command never happened at all, likely leading to complete chaos. Hmm, if that "prior proposal" was indeed on the table, I missed it. The current patch, AFAICS, implements your third choice, which I quite agree would lead to complete chaos; there would be no way to write a script that did anything useful with that. It is interesting to think about what would happen if "expr is neither true nor false" were defined as "skip immediately to \endif" (which I think is the natural generalization of what you said to apply to an intermediate \elif). I believe that it'd be possible to work with it, but it's not very clear if it'd be easier or harder to work with than the rule of treating bogus results as false. What is clear is that it'd be unlike any other conditional construct I ever worked with. As was pointed out upthread, "treat error results as false" is what you get from "if" in a POSIX shell. I think it's fair also to draw an analogy to what SQL does with null boolean values, which is to treat them as false when a decision is required (in, eg, WHERE or CASE). So I think "treat bogus results as false" is the most conservative, least likely to cause unhappy surprises, solution here. > Other positions are also possible. If you've got concrete ideas about that, let's hear them. I'm not trying to foreclose discussion. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Fri, Mar 3, 2017 at 3:18 AM, Fabien COELHO wrote: > I'm ok with this patch. I think that the very simple automaton code > structure achieved is worth the very few code duplications. It is also > significantly shorter than the nested if/switch variants, and it does > exactly what Tom and Robert wished with respect to errors, so I think that > this is a good compromise. I think that I have not taken a firm position on what the behavior should be with respect to errors.I took the position that the messages being printed saying what happened were too detailed, because they not only described what had happened but also tried to prognosticate what would happen next, which was dissimilar to what we do elsewhere and likely to be hard to maintain - or even get right for v1. But I have not taken a position on what should happen if the condition for \if or \elsif evaluates to a baffling value. Corey's prior proposal was to treat it, essentially, as neither true nor false, skipping both arms of the if. Tom seems to want an invalid value treated as false. You could also imagine pretending that the command never happened at all, likely leading to complete chaos. Other positions are also possible. I suggest that doing it the way Tom likes may be the path of least resistance, but this isn't really something I'm very animated about personally. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > [ 0001.if_endif.v21.diff ] I had thought that this patch was pretty close to committable, but the more I poke at it the less I think so. The technology it uses for skipping unexecuted script sections has got too many bugs. * Daniel Verite previously pointed out the desirability of disabling variable expansion while skipping script. That doesn't seem to be here, though there's an apparently-vestigial comment in psql/common.c claiming that it is. IIRC, I objected to putting knowledge of ConditionalStack into the shared psqlscan.l lexer, and I still think that would be a bad idea; but we need some way to get the lexer to shut that off. Probably the best way is to add a passthrough "void *" argument that would let the get_variable callback function mechanize the rule about not expanding in a false branch. * Whether or not you think it's important not to expand skipped variables, I think that it's critical that skipped backtick expressions not be executed. The specific use-case that I'm concerned about is backtick evals in \if expressions, which are going to be all over the place as long as we haven't got any native expression eval capability, and will doubtless remain important even when/if we do. So in a case like \if something \elif `expr :var1 + :var2 = :var3` \endif I think it's essential that expr not be called if the first if-condition succeeded. (That first condition might be checking whether the vars contain valid integers, for example.) The current patch gets this totally wrong --- not only does it perform the backtick, but \elif complains if the result isn't a valid bool. I do not think that a skipped \if or \elif should evaluate its argument at all. * The documentation says that an \if or \elif expression extends to the end of the line, but actually the code is just eating one OT_NORMAL argument. That means it's OK to do this: regression=# \if 1 \echo foo \echo bar \endif foo bar regression=# which doesn't seem insane, except that the inverse case is insane: regression=# \if 0 \echo foo \echo bar \endif regression@# (notice we're not out of the conditional). Even if we change it to eat the whole line as argument, this inconsistency will remain: regression=# \if 1 regression=# \echo foo \endif foo regression=# (notice we're out of the conditional) regression=# \if 0 regression@# \echo foo \endif command ignored, use \endif or Ctrl-C to exit current branch. regression@# (notice we're not out of the conditional) This inconsistency seems to have to do with the code in HandleSlashCmds that discards arguments until EOL after a failed backslash command. You've modified that to also discard arguments after a non-executed command, and I think that's broken. * More generally, I do not think that the approach of having exec_command simply fall out immediately when in a false branch is going to work, because it ignores the fact that different backslash commands have different argument parsing rules. Some will eat the rest of the line and some won't. I'm afraid that it might be necessary to remove that code block and add a test to every single backslash command that decides whether to actually perform its action after it's consumed its arguments. That would be tedious :-(. But as it stands, backslash commands will get parsed differently (ie with potentially-different ending points) depending on whether they're in a live branch or not, and that seems just way too error-prone to be allowed to stand. * I think it's completely wrong to do either resetPQExpBuffer(query_buf) or psql_scan_reset(scan_state) when deciding a branch is not to be executed. Compare these results: regression=# select (1 + regression(# \if 1 regression-# \echo foo foo regression-# \endif regression-# 2); ?column? -- 3 (1 row) regression=# select (1 + regression(# \if 0 regression-# \echo foo command ignored, use \endif or Ctrl-C to exit current branch. regression@# \endif regression=# 2); ERROR: syntax error at or near "2" LINE 1: 2); ^ regression=# If the first \if doesn't throw away an incomplete query buffer (which it should not), then surely the second should not either. Somebody who actually wants to toss the query buffer can put \r into the appropriate branch of their \if; it's not for us to force that on them. * Also, the documentation for psql_scan_reset is pretty clear that it's to be called when and only when the query buffer is reset, which makes your calls in the bodies of the conditional commands wrong. As an example: regression=# select (1 + regression(# 2; regression(# (notice we've not sent the known-incomplete command to the server) vs regression(# \r Query buffer reset (cleared). regression=# select (1 + regression(# \if 1 regression-# \endif regression-# 2; ERROR: syntax error at or near ";" LINE 2: 2; ^ regression=# That happens because the \if code gratuituously resets the lexer, as we can
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sat, Mar 11, 2017 at 4:17 PM, Tom Lane wrote: > Corey Huinker writes: > > [ 0001.if_endif.v21.diff ] > > Starting to poke at this... the proposal to add prove checks for psql > just to see whether \if respects ON_ERROR_STOP seems like an incredibly > expensive way to test a rather minor point. On my machine, "make check" > in bin/psql goes from zero time to close to 8 seconds. I'm not really > on board with adding that kind of time to every buildfarm run for the > foreseeable future just for this. > > Couldn't we get close to the same coverage by adding a single-purpose > test script to the main regression tests? Along the lines of > > \set ON_ERROR_STOP 1 > \if invalid > \echo should not get here > \endif > \echo should not get here either > > You could imagine just dropping that at the end of psql.sql, but I > think probably a separate script is worth the trouble. > > regards, tom lane > I think I can manage that. Just to be clear, you're asking me to replace the perl script with one new sql script? If so, there's probably a few non-on-stop tests in there that might be worth preserving in regression form.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Corey Huinker writes: > [ 0001.if_endif.v21.diff ] Starting to poke at this... the proposal to add prove checks for psql just to see whether \if respects ON_ERROR_STOP seems like an incredibly expensive way to test a rather minor point. On my machine, "make check" in bin/psql goes from zero time to close to 8 seconds. I'm not really on board with adding that kind of time to every buildfarm run for the foreseeable future just for this. Couldn't we get close to the same coverage by adding a single-purpose test script to the main regression tests? Along the lines of \set ON_ERROR_STOP 1 \if invalid \echo should not get here \endif \echo should not get here either You could imagine just dropping that at the end of psql.sql, but I think probably a separate script is worth the trouble. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
About v21: Patch applies with some offset, make check ok, psql tap tests ok. I also did some interactive tests which behaved as I was expecting. I'm ok with this patch. I think that the very simple automaton code structure achieved is worth the very few code duplications. It is also significantly shorter than the nested if/switch variants, and it does exactly what Tom and Robert wished with respect to errors, so I think that this is a good compromise. A tiny detail about "default". I would have added a comment when it is expected to be dead code (else, elif), and I would have put the list of matching states explicitely otherwise (if, endif) otherwise the reader has to remember what the other states are. Probably it is me being really too peckish, if at all possible:-) I've turned the patch as ready, again. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Fri, Mar 3, 2017 at 1:25 AM, Fabien COELHO wrote: > > > For endif, I really exagerated, "switch { defaut: " is too much, please >> accept my apology. Maybe just do the pop & error reporting? >> > It seemed like overkill, but I decided to roll with it. > > Or maybe be more explicit: > > switch (current state) > case NONE: > error no matching if; > case ELSE_FALSE: > case ELSE_TRUE: > case ...: > pop; > Assert(success); the pop() function tests for an empty stack, so this switch is double-testing, but it's also no big deal, so here you go... diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..9d245a9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,83 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this 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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally for query/command +completion, but any queries are not sent to the server, and any +commands other than conditionals (\if, +\elif, \else, +\endif) are ignored. Conditional commands are +still checked for valid nesting. + + + \ir or \include_relative filename @@ -3703,8 +3780,9 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, -or ! if the session is disconnected from the +but @ if the session is in a false conditional +block, or ^ if in single-line mode, or +! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2 %R is replaced by a character that depends on why psql expects more input: diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_help.o psqlscanslash.o \ @@ -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/d
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
For endif, I really exagerated, "switch { defaut: " is too much, please accept my apology. Maybe just do the pop & error reporting? Or maybe be more explicit: switch (current state) case NONE: error no matching if; case ELSE_FALSE: case ELSE_TRUE: case ...: pop; Assert(success); -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v20: attempt at implementing the switch-on-all-states style. For the elif I think it is both simpler and better like that. Whether committer will agree is an unkown, as always. For endif, I really exagerated, "switch { defaut: " is too much, please accept my apology. Maybe just do the pop & error reporting? For if, the evaluation & error could be moved before the switch, which may contain only the new state setting decision, and the push after the switch? Also, I would suggest to use default only to detect an unexpected state error, and list all other states explicitely. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > For me, it is only slightly better: I think that for helping understanding > and maintenance, the automaton state transitions should be all clear and > loud in just one place, so I would really like to see a single common > structure: > > if (is "if") switch on all states; > else if (is "elif") switch on all states; > else if (is "else") switch on all states; > else if (is "endif") switch on all states; > > And minimal necessary error handling around that. > > v20: attempt at implementing the switch-on-all-states style. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..9d245a9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,83 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this 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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally for query/command +completion, but any queries are not sent to the server, and any +commands other than conditionals (\if, +\elif, \else, +\endif) are ignored. Conditional commands are +still checked for valid nesting. + + + \ir or \include_relative filename @@ -3703,8 +3780,9 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, -or ! if the session is disconnected from the +but @ if the session is in a false conditional +block, or ^ if in single-line mode, or +! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2 %R is replaced by a character that depends on why psql expects more input: diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_help.o psqlscanslash.o \ @@ -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: distcl
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, Tom was pretty adamant that invalid commands are not executed. So in a case like this, with ON_ERROR_STOP off: \if false \echo 'a' \elif true \echo 'b' \elif invalid \echo 'c' \endif Both 'b' and 'c' should print, because "\elif invalid" should not execute. The code I had before was simpler, but it missed that. Hmmm. You can still have it with one switch, by repeating the evaluation under true and ignore, even if the value is not used: switch(state) { case NONE: error; case ELSE_TRUE: error; case ELSE_FALSE: error; case IF_TRUE: if (eval()) ... else error; break; case IF_FALSE: if (eval()) ... else error; break; case IGNORE: if (eval()) ... else error; break; } Ok, so here's one idea I tossed around, maybe this will strike the right balance for you. If I create a function like this: [...] Does that handle your objections? For me, it is only slightly better: I think that for helping understanding and maintenance, the automaton state transitions should be all clear and loud in just one place, so I would really like to see a single common structure: if (is "if") switch on all states; else if (is "elif") switch on all states; else if (is "else") switch on all states; else if (is "endif") switch on all states; And minimal necessary error handling around that. Your suggestion does not achieve this, although I agree that the code structure would be cleaner thanks to the function. p.s. do we try to avoid constructs likeif (success = my_function(var1, var2)) ? I think it is allowed because I found some of them with grep (libpq, ecpg, postmaster, pg_dump, pg_upgrade...). They require added parentheses around the assignment: if ((success = eval())) ... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO wrote: > > Hello Corey, > > That is accurate. The only positive it has is that the user only >> experiences one error, and it's the first error that was encountered if >> reading top-to-bottom, left to right. It is an issue of which we >> prioritize >> - user experience or simpler code. >> > > Hmmm. The last simpler structure I suggested, which is basically the one > used in your code before the update, does check for the structure error > first. The only drawback is that the condition is only evaluated when > needed, which is an issue we can cope with, IMO. Tom was pretty adamant that invalid commands are not executed. So in a case like this, with ON_ERROR_STOP off: \if false \echo 'a' \elif true \echo 'b' \elif invalid \echo 'c' \endif Both 'b' and 'c' should print, because "\elif invalid" should not execute. The code I had before was simpler, but it missed that. > > Now if you want to require committer opinion on this one, fine with me. >>> >> >> Rather than speculate on what a committer thinks of this edge case (and >> making a patch for each possible theory), I'd rather just ask them what >> their priorities are and which user experience they favor. >> > > ISTM that the consistent message by Robert & Tom was to provide simpler > code even if the user experience is somehow degraded, as they required that > user-friendly features were removed (eg trying to be nicer about structural > syntax errors, barking in interactive mode so that the user always knows > the current status, providing a detailed status indicator in the prompt...). > Ok, so here's one idea I tossed around, maybe this will strike the right balance for you. If I create a function like this: static boolean is_valid_else_context(IfState if_state, const char *cmd) { /* check for invalid \else / \elif contexts */ switch (if_state) { case IFSTATE_NONE: /* not in an \if block */ psql_error("\\%s: no matching \\if\n", cmd); return false; break; case IFSTATE_ELSE_TRUE: case IFSTATE_ELSE_FALSE: psql_error("\\%s: cannot occur after \\else\n", cmd); return false; break; default: break; } return true; } Then the elif block looks something like this: else if (strcmp(cmd, "elif") == 0) { ifState if_state = conditional_stack_peek(cstack); if (is_valid_else_context(if_state, "elif")) { /* * valid \elif context, check for valid expression */ bool elif_true = false; success = read_boolean_expression(scan_state, "\\elif ", &elif_true); if (success) { /* * got a valid boolean, what to do with it depends on current * state */ switch (if_state) { case IFSTATE_IGNORED: /* * inactive branch, do nothing. * either if-endif already had a true block, * or whole parent block is false. */ break; case IFSTATE_TRUE: /* * just finished true section of this if-endif, * must ignore the rest until \endif */ conditional_stack_poke(cstack, IFSTATE_IGNORED); break; case IFSTATE_FALSE: /* * have not yet found a true block in this if-endif, * this might be the first. */ if (elif_true) conditional_stack_poke(cstack, IFSTATE_TRUE); break; default: /* error cases all previously ruled out */ break; } } } else success = false; psql_scan_reset(scan_state); } This is functionally the same as my latest patch, but the ugliness of switching twice on if_state is hidden. As an added benefit, the "else"-handling code gets pretty simple because it can leverage that same function. Does that handle your objections? p.s. do we try to avoid constructs likeif (success = my_function(var1, var2)) ?
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, That is accurate. The only positive it has is that the user only experiences one error, and it's the first error that was encountered if reading top-to-bottom, left to right. It is an issue of which we prioritize - user experience or simpler code. Hmmm. The last simpler structure I suggested, which is basically the one used in your code before the update, does check for the structure error first. The only drawback is that the condition is only evaluated when needed, which is an issue we can cope with, IMO. Now if you want to require committer opinion on this one, fine with me. Rather than speculate on what a committer thinks of this edge case (and making a patch for each possible theory), I'd rather just ask them what their priorities are and which user experience they favor. ISTM that the consistent message by Robert & Tom was to provide simpler code even if the user experience is somehow degraded, as they required that user-friendly features were removed (eg trying to be nicer about structural syntax errors, barking in interactive mode so that the user always knows the current status, providing a detailed status indicator in the prompt...). Now committers can change their opinions, it is their privilege:-) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Wed, Mar 1, 2017 at 12:23 PM, Fabien COELHO wrote: > > Hello Corey, > > on elif >> if misplaced elif >> misplaced elif error >> else >> eval expression >> => possible eval error >> set new status if eval fine >> > > Currently it is really: > > switch (state) { > case NONE: > case ELSE_TRUE: > case ELSE_FALSE: > success = false; > show some error > default: > } > if (success) { > success = evaluate_expression(...); > if (success) { >switch (state) { >case ...: >default: >} > } > } > > Which I do not find so neat. The previous one with nested switch-if-switch > looked as bad. That is accurate. The only positive it has is that the user only experiences one error, and it's the first error that was encountered if reading top-to-bottom, left to right. It is an issue of which we prioritize - user experience or simpler code. Now if you want to require committer opinion on this one, fine with me. Rather than speculate on what a committer thinks of this edge case (and making a patch for each possible theory), I'd rather just ask them what their priorities are and which user experience they favor.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, on elif if misplaced elif misplaced elif error else eval expression => possible eval error set new status if eval fine Currently it is really: switch (state) { case NONE: case ELSE_TRUE: case ELSE_FALSE: success = false; show some error default: } if (success) { success = evaluate_expression(...); if (success) { switch (state) { case ...: default: } } } Which I do not find so neat. The previous one with nested switch-if-switch looked as bad. The issue at hand being the benefit to the user vs code complexity. Hmmm. One of my point is that I do not really see the user benefit... for me the issue is to have no user benefit and code complexity. The case we are discussing is for the user who decides to write code with *two* errors on the same line: \if good-condition \else \elif bad-condition \endif with an added complexity to show the elif bad position error first. Why should we care so much for such a special case? Maybe an alternative could be to write simpler code anyway, somehow like it was before: // on "elif" switch (peek(state)) { case NONE: error; case ELSE_TRUE: error; case ELSE_FALSE: error; case IGNORED:break; case TRUE: poke IGNORED; case FALSE: success = evaluate(&is_true) if (!success) error; else if (is_true) poke TRUE default: error; } The only difference is that the evaluation is not done when it is not needed (what a draw back) but ISTM that it is significantly easier to understand and maintain. Now if you want to require committer opinion on this one, fine with me. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Wed, Mar 1, 2017 at 3:07 AM, Fabien COELHO wrote: > > Hello Corey, > > It doesn't strike me as much cleaner, but it's no worse, either. >> > > Hmmm. > > The "if (x) { x = ... ; if (x) {" does not help much to improve > readability and understandability... > > My 0.02€ about v19: > > If there are two errors, I do not care which one is shown, both will have > to be fixed anyway in the end... So I would suggest to choose the simplest > possible implementation: > > on elif: > always eval expression > => possible eval error > switch > => including detecting misplaced elif errors > > If the second error must absolutely be shown in all cases, then add a > second misplaced elif detection in the eval expression failure branch: > > on elif > always eval > if (eval failed) > also checked for misplaced (hey user, you have 2 errors in fact...) > bye bye... > // else eval was fine > switch > including misplaced elif detection > > If the committer is angry at these simple approach, then revert to the > strange looking and hard to understand switch-if-switch solution (~ v18, or > some simplified? v19), but I do not think the be weak benefit is worth the > code complexity. > > -- > Fabien. Just to make things easy for the committer, the existing code only shows the user one error: on elif if misplaced elif misplaced elif error else eval expression => possible eval error set new status if eval fine The issue at hand being the benefit to the user vs code complexity. So, shall we send this off to the committers and let them decide?
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, It doesn't strike me as much cleaner, but it's no worse, either. Hmmm. The "if (x) { x = ... ; if (x) {" does not help much to improve readability and understandability... My 0.02€ about v19: If there are two errors, I do not care which one is shown, both will have to be fixed anyway in the end... So I would suggest to choose the simplest possible implementation: on elif: always eval expression => possible eval error switch => including detecting misplaced elif errors If the second error must absolutely be shown in all cases, then add a second misplaced elif detection in the eval expression failure branch: on elif always eval if (eval failed) also checked for misplaced (hey user, you have 2 errors in fact...) bye bye... // else eval was fine switch including misplaced elif detection If the committer is angry at these simple approach, then revert to the strange looking and hard to understand switch-if-switch solution (~ v18, or some simplified? v19), but I do not think the be weak benefit is worth the code complexity. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > >> >> Alternatively if the structure must really be kept, then deal with errors >> in a first switch, read value *after* switch and deal with other errors >> there, then start a second switch, and adjust the documentation accordingly? >> >> switch >> errors >> read >> if >> errors >> // no error >> switch >> > > it's now something more like switch error-conditions if no-errors read if was a boolean switch last-state It doesn't strike me as much cleaner, but it's no worse, either. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..9d245a9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,83 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this 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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally for query/command +completion, but any queries are not sent to the server, and any +commands other than conditionals (\if, +\elif, \else, +\endif) are ignored. Conditional commands are +still checked for valid nesting. + + + \ir or \include_relative filename @@ -3703,8 +3780,9 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, -or ! if the session is disconnected from the +but @ if the session is in a false conditional +block, or ^ if in single-line mode, or +! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2 %R is replaced by a character that depends on why psql expects more input: diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_help.o psqlscanslash.o \ @@ -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_h
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Sun, Feb 26, 2017 at 2:47 AM, Fabien COELHO wrote: > > Hello Corey, > > About v18: Patch applies, make check ok, psql tap tests ok. > > > ISTM that contrary to the documentation "\elif something" is not evaluated > in all cases, and the resulting code is harder to understand with a nested > switch and condition structure: > > switch > default > read > if >switch > > I wish (this is a personal taste) it could avoid switch-nesting on the > very same value. It should also conform to the documentation. > I wasn't too happy with it, but I figured it would spark discussion. I succeeded. > > If there is no compelling reason for the switch-nesting, I would suggest > to move the read_boolean_expression before the swich, to deal with error > immediately there, and then to have just one switch. > I thought about doing it that way. However, in the case of: \set x invalid \if true \else \elif :x \endif The error has already "happened" at line 4, char 5, and it doesn't matter what expression follows, you will get an error. But because read_boolean_expression() can emit errors, you would see the error saying "invalid" isn't a valid boolean expression, and then see another error saying that the \elif was out of place. If we suppress read_boolean_expression()'s error reporting, then we have to re-create that error message ourselves, or be fine with the error message on invalid \elifs being inconsistent with invalid \ifs. Similar to your suggestion below, we could encapsulate the first switch into a function valid_elif_context(ConditionalStack), which might make the code cleaner, but would make it harder to see that all swtich-cases are covered between the two. That might be a tradeoff we want to make. > > Alternatively if the structure must really be kept, then deal with errors > in a first switch, read value *after* switch and deal with other errors > there, then start a second switch, and adjust the documentation accordingly? > > switch > errors > read > if > errors > // no error > switch > How would the documentation have to change? Also, the %R documentation has single line marker '^' before not executed > '@', but it is the reverse in the code. Noted and fixed in the next patch, good catch.
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, About v18: Patch applies, make check ok, psql tap tests ok. ISTM that contrary to the documentation "\elif something" is not evaluated in all cases, and the resulting code is harder to understand with a nested switch and condition structure: switch default read if switch I wish (this is a personal taste) it could avoid switch-nesting on the very same value. It should also conform to the documentation. If there is no compelling reason for the switch-nesting, I would suggest to move the read_boolean_expression before the swich, to deal with error immediately there, and then to have just one switch. Alternatively if the structure must really be kept, then deal with errors in a first switch, read value *after* switch and deal with other errors there, then start a second switch, and adjust the documentation accordingly? switch errors read if errors // no error switch Also, the %R documentation has single line marker '^' before not executed '@', but it is the reverse in the code. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > Typo "unterminted". > Fixed. > The \if within the \if false branch is not tallied properly? Am I missing > something? > Nope, you found a bug. FIxed. Test-case added. > I changed the paragraph to >> > >Lines within false branches are parsed normally, however, any >> completed >>queries are not sent to the server, and any completed commands >> other >>than conditionals (\if, >> \elif, >>\else, \endif) are ignored. >> > > I'm not sure about the ", however, " commas, but I'm sure that I do not > know English punctuation rules:-) > Re-worded it again for shorter sentences. Re-mentioned that conditionals are still checked for proper nesting. * Changed comments to reflect that \if always evalutes even in a false branch * Changed \elif to first check if the command is in a proper \if block before evaluating the expression. The invalid boolean message would mask the bigger problem. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..27824d9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,83 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this 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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally for query/command +completion, but any queries are not sent to the server, and any +commands other than conditionals (\if, +\elif, \else, +\endif) are ignored. Conditional commands are +still checked for valid nesting. + + + \ir or \include_relative filename @@ -3703,7 +3780,8 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, +but ^ if in single-line mode, or +@ if the session is in a false conditional block, or ! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2 %R is replaced by a character that diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_hel
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
About v17: Patch applies, "make check" & psql "make check" ok. ... '@' [...] I noticed that it takes precedence over '!'. [...] My reasoning was this: if you're in a false block, and you're not connected to a db, the \c isn't going to work for you until you get out of the false block, so right now being in a false block is a bigger problem than not being connected to a db. [...] Ok. It could be nice to keep test cases that show what may happen? Restored. It looks weird now, but it fixes the unterminated quoted string. Typo "unterminted". I think I found an issue while testing in interactive: calvin=# \if false calvin@# \if false calvin@# \echo false-false command ignored, use \endif or Ctrl-C to exit current branch. calvin@# \endif calvin=# \echo in false in false The \if within the \if false branch is not tallied properly? Am I missing something? Maybe more test cases should be added to check that nesting checks do work properly? Maybe the documentation could add some kind of warning about that? I changed the paragraph to Lines within false branches are parsed normally, however, any completed queries are not sent to the server, and any completed commands other than conditionals (\if, \elif, \else, \endif) are ignored. I'm not sure about the ", however, " commas, but I'm sure that I do not know English punctuation rules:-) Maybe the sentence could be cut in shorter pieces. I think that the fact that "if" commands are checked for proper nesting could be kept in the explanation. There's no mention that psql variables AREN'T expanded, so the user has every expectation that they are. Ok. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > I'm not sure that '@' is the best choice, but this is just one char. > I noticed that it takes precedence over '!'. Why not. ISTM that orthogonal > features are not shown independently, but this is a preexisting state, and > it allows to have a shorter prompt, so why not. > My reasoning was this: if you're in a false block, and you're not connected to a db, the \c isn't going to work for you until you get out of the false block, so right now being in a false block is a bigger problem than not being connected to a db. I have no strong opinion about what should happen here, so I welcome suggestions for what tops what. > > Anyway, the '%R' documentation needs to be updated. Done. > It could be nice to keep test cases that show what may happen? > Restored. It looks weird now, but it fixes the unterminated quoted string. > The various simplifications required result in the feature being more > error prone for the user. Maybe the documentation could add some kind of > warning about that? I changed the paragraph to Lines within false branches are parsed normally, however, any completed queries are not sent to the server, and any completed commands other than conditionals (\if, \elif, \else, \endif) are ignored. There's no mention that psql variables AREN'T expanded, so the user has every expectation that they are. > > Add space after comma when calling send_query. > Done. > > I'm not sure why you removed the comments before \if in the doc example. > Maybe keep a one liner? > Didn't mean to, restored. > Why not reuse the pop loop trick to "destroy" the stack? Forgot about that, restored. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..625c9a8 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,81 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this 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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally, however, any completed +queries are not sent to the server, and any completed commands other +than conditionals (\if, \elif, +\else, \endif) are ignored. + + + \ir or \include_relative filename @@ -3703,7 +3778,8 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, +but ^ if in single-line mode, or +@ if the session is in a false conditional block, or ! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, v16 is everything v15 promised to be. My 0.02€: Patch applies, make check ok, psql make check ok as well. Welcome to v15, highlights: - all conditional data structure management moved to conditional.h and conditional.c Indeed. I cannot say that I find it better, but (1) Tom did required it and (2) it still works:-) If the stack stuff had stayed in "fe_utils", it would have been easy to reuse them in pgbench where they might be useful... But who cares? - conditional state lives in mainloop.c and is passed to HandleSlashCommands, exec_command and get_prompt as needed Same. - no more pset.active_branch, uses conditional_active(conditional_stack) instead Same. - PsqlScanState no longer has branching state Indeed. - Implements the %R '@' prompt on false branches. I'm not sure that '@' is the best choice, but this is just one char. I noticed that it takes precedence over '!'. Why not. ISTM that orthogonal features are not shown independently, but this is a preexisting state, and it allows to have a shorter prompt, so why not. Anyway, the '%R' documentation needs to be updated. - Variable expansion is never suppressed even in false blocks, regression test edited to reflect this. It could be nice to keep test cases that show what may happen? The various simplifications required result in the feature being more error prone for the user. Maybe the documentation could add some kind of warning about that? - ConditionalStack could morph into PsqlFileState without too much work. Probably. Code details: Add space after comma when calling send_query. I'm not sure why you removed the comments before \if in the doc example. Maybe keep a one liner? Why not reuse the pop loop trick to "destroy" the stack? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
2017-02-23 18:52 GMT+01:00 Fabien COELHO : > > Hello Daniel, > > Ah, I see why *that* wants to know about it ... I think. I suppose you're >>> arguing that variable expansion shouldn't be able to insert, say, an >>> \else >>> in a non-active branch? Maybe, but if it can insert an \else in an >>> active >>> branch, then why not non-active too? Seems a bit inconsistent. >>> >> >> Are we sold on the idea that conditionals should be implemented >> by meta-commands, rather than for example terminal symbols of >> a new grammar on top of the existing? >> > > I would say that this already exists server-side, and it is named > PL/pgSQL:-) > > I think that once psql has started with \xxx commands, then client-side > extensions must stick with it till the end of time. +1 we don't need strong client side scripting language - it should be just simple. Pavel > > > -- > 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: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Daniel, Ah, I see why *that* wants to know about it ... I think. I suppose you're arguing that variable expansion shouldn't be able to insert, say, an \else in a non-active branch? Maybe, but if it can insert an \else in an active branch, then why not non-active too? Seems a bit inconsistent. Are we sold on the idea that conditionals should be implemented by meta-commands, rather than for example terminal symbols of a new grammar on top of the existing? I would say that this already exists server-side, and it is named PL/pgSQL:-) I think that once psql has started with \xxx commands, then client-side extensions must stick with it till the end of time. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Tom Lane wrote: > Ah, I see why *that* wants to know about it ... I think. I suppose you're > arguing that variable expansion shouldn't be able to insert, say, an \else > in a non-active branch? Maybe, but if it can insert an \else in an active > branch, then why not non-active too? Seems a bit inconsistent. Are we sold on the idea that conditionals should be implemented by meta-commands, rather than for example terminal symbols of a new grammar on top of the existing? To recall the context, psql variables are really macros that may contain meta-commands, and when they do they're essentially injected and executed at the point of interpolation. That's more or less what started this thread: demo'ing how we could exit conditionally by injecting '\q' or nothing into a variable, and saying that even if doable it was pretty weird, and it would be better to have real conditional structures instead. But when conditional structures are implemented as meta-commands, there's the problem that this structure can be generated on the fly too, which in a way is no less weird. While I think that the introduction of conditionals in psql is great, I'm getting doubtful about that part. Are there popular script languages or preprocessors that accept variables/macros instead of symbols to structure the flow of instructions? I can't think of any myself. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > > Files "conditional.h" and "conditional.c" are missing from the patch. > > Also, is there a particular reason why tap test have been removed? That would be because I diffed against my last commit, not the master branch, sigh. v16 is everything v15 promised to be. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..dac8e37 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,79 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +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..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_help.o psqlscanslash.o \ @@ -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..6c15c01 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -38,6 +38,7 @@ #include "fe_utils/string_utils.h" #include "common.h" +#include "conditional.h" #include "copy.h" #include "crosstabview.h" #include "describe.h" @@ -62,6 +63,7 @@ typedef enum EditableObjectType /* functions for use in this file */ static backslashResult exec_command(const char *cmd, PsqlScanState scan_state, +ConditionalStack cstack, PQExpBuffer query_buf); static bool do_edit(const char *filename_arg, PQExpBuffer query_buf, int lineno, bool *edited); @@ -109,6 +111,7 @@ static void checkWin32Codepage(void); backslashResult
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Welcome to v15, highlights: Files "conditional.h" and "conditional.c" are missing from the patch. Also, is there a particular reason why tap test have been removed? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
On Wed, Feb 22, 2017 at 6:15 PM, Corey Huinker wrote: > On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane wrote: > >> Ah, I see why *that* wants to know about it ... I think. I suppose you're >> arguing that variable expansion shouldn't be able to insert, say, an \else >> in a non-active branch? Maybe, but if it can insert an \else in an active >> branch, then why not non-active too? Seems a bit inconsistent. >> > > The major reason was avoiding situations like what Daniel showed: where > value of a variable that is meaningless/undefined in the current > false-block context gets expanded anyway, and thus code inside a false > block has effects outside of that block. Granted, his example was > contrived. I'm open to removing that feature and seeing what breaks in the > test cases. > Welcome to v15, highlights: - all conditional data structure management moved to conditional.h and conditional.c - conditional state lives in mainloop.c and is passed to HandleSlashCommands, exec_command and get_prompt as needed - no more pset.active_branch, uses conditional_active(conditional_stack) instead - PsqlScanState no longer has branching state - Implements the %R '@' prompt on false branches. - Variable expansion is never suppressed even in false blocks, regression test edited to reflect this. - ConditionalStack could morph into PsqlFileState without too much work. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..dac8e37 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,79 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +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. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +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..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_help.o psqlscanslash.o \ @@ -61,8 +61,16 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -