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;

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

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

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.

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,

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

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

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,

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

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

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

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

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

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)

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:

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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"

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

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

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 >

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

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

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

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

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 >

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

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

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

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

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

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

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

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

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

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

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 >

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

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

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

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

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

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;

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

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

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

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

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

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

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

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€

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

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

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

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

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

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

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

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)

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

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

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?

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

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:

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,

  1   2   >