Thread
Commits
-
Allow backslash line continuations in pgbench's meta commands.
- cdc2a70470bd 10.0 landed
-
pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-10-03T08:57:44Z
Attached patch does what is described in the title, hopefully. Continuations in other pgbench backslash-commands should be dealt with elsewhere... Also attached is a small test script. While looking at the code, I noticed that newline is \n. Maybe it should be (\r|\n|\r\n) to allow for MacOS & Windows. I have not changed that for now. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Christoph Berg <myon@debian.org> — 2016-10-03T10:43:44Z
Re: Fabien COELHO 2016-10-03 <alpine.DEB.2.20.1610031049310.19411@lancre> > > Attached patch does what is described in the title, hopefully. Continuations > in other pgbench backslash-commands should be dealt with elsewhere... Would (a similar version of) that patch also apply to .psqlrc? I "\set" a bunch of lengthy SQL commands in there, e.g. \set config 'SELECT name, current_setting(name), CASE source WHEN $$configuration file$$ THEN sourcefile||$$:$$||sourceline ELSE source END FROM pg_settings WHERE source <> $$default$$;' Being able to split that over several lines would greatly improve maintainability. (Though I do realize this would also require a notion for splitting/continuing strings.) Christoph
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-10-03T11:58:04Z
Hello Christoph, >> Attached patch does what is described in the title, hopefully. Continuations >> in other pgbench backslash-commands should be dealt with elsewhere... > > Would (a similar version of) that patch also apply to .psqlrc? Pgbench has its own lexer & parser for \set expressions, so the continuation is handled there. > I "\set" a bunch of lengthy SQL commands in there, e.g. I agree that this looks like a desirable feature, however I would tend to see that as material for another independent patch. I think that .pgsqrc is really just a "psql" script so the handling would be somewhere there... I'll have a look. > \set config 'SELECT name, current_setting(name), CASE source WHEN > $$configuration file$$ THEN sourcefile||$$:$$||sourceline ELSE source > END FROM pg_settings WHERE source <> $$default$$;' Hmmm. I'm not sure how this is parsed. If this is considered a string '...', then maybe \set should wait for the end of the string instead of the end of the line, i.e. no continuation would be needed... \set config ' SELECT name, ... CASE ... END FROM pg_settings WHERE ...;' > Being able to split that over several lines would greatly improve > maintainability. (Though I do realize this would also require a notion > for splitting/continuing strings.) Yep. I'm not sure of the actual feature which is needed. -- Fabien. -
Re: pgbench - allow backslash continuations in \set expressions
Christoph Berg <myon@debian.org> — 2016-10-03T12:46:46Z
Re: Fabien COELHO 2016-10-03 <alpine.DEB.2.20.1610031259400.19411@lancre> > > I "\set" a bunch of lengthy SQL commands in there, e.g. > > I agree that this looks like a desirable feature, however I would tend to > see that as material for another independent patch. Sure, my question was by no means intending to stop your pgbench patch from going forward by adding extra requirements. > Hmmm. I'm not sure how this is parsed. If this is considered a string '...', > then maybe \set should wait for the end of the string instead of the end of > the line, i.e. no continuation would be needed... > > \set config ' > SELECT name, ... > CASE ... END > FROM pg_settings > WHERE ...;' I guess that would be the sane solution here, yes. Not adding any \ chars at the end of the line would also mean cut-and-paste of the RHS content would work. Thanks for the feedback! Christoph
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2016-11-01T07:19:34Z
Hie Fabien, Seems like an interesting addition to pgbench interface, but not sure where it's required, it'll be good if you may provide some cases where it's utility can be witnessed. Something like where you absolutely need continuations in expression. While applying it is giving some trailing whitespace errors, please correct them. As an additional comment you may consider reformatting following snippet > {continuation} { /* ignore */ } > > as > {continuation} { > /* ignore */ > } Thanks. On Mon, Oct 3, 2016 at 6:16 PM, Christoph Berg <myon@debian.org> wrote: > Re: Fabien COELHO 2016-10-03 <alpine.DEB.2.20.1610031259400.19411@lancre> > > > I "\set" a bunch of lengthy SQL commands in there, e.g. > > > > I agree that this looks like a desirable feature, however I would tend to > > see that as material for another independent patch. > > Sure, my question was by no means intending to stop your pgbench patch > from going forward by adding extra requirements. > > > Hmmm. I'm not sure how this is parsed. If this is considered a string > '...', > > then maybe \set should wait for the end of the string instead of the end > of > > the line, i.e. no continuation would be needed... > > > > \set config ' > > SELECT name, ... > > CASE ... END > > FROM pg_settings > > WHERE ...;' > > I guess that would be the sane solution here, yes. Not adding any \ > chars at the end of the line would also mean cut-and-paste of the RHS > content would work. > > Thanks for the feedback! > > Christoph > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-01T08:07:56Z
Hello Rafia, > Seems like an interesting addition to pgbench interface, but not sure where > it's required, it'll be good if you may provide some cases where it's > utility can be witnessed. Something like where you absolutely need > continuations in expression. It is never "absolutely needed", you can always put the expression on one longer line. As an long line example, supposing some other currently submitted patches are committed to pgbench, for implementing tpc-b according to the specification one may write the following: \set bid CASE WHEN random(0, 99) < 85 THEN :tbid ELSE :abid + (:abid >= :tbid) END or \set bid \ CASE WHEN random(0, 99) < 85 \ THEN :tbid \ ELSE :abid + (:abid >= :tbid) \ END I find the second version more readable, but it is a really matter of taste, obviously. > While applying it is giving some trailing whitespace errors, please > correct them. sh> git checkout -b tmp sh> git apply ~/pgbench-set-continuation-1.patch sh> git commit -a sh> I do not get any errors, and I have not found any trailing spaces in the patch. Could you give me the precise error message you got? > As an additional comment you may consider reformatting > following snippet > >> {continuation} { /* ignore */ } >> >> as > >> {continuation} { >> /* ignore */ >> } Hmmm... Other lex rules to ignore spaces use the one line syntax, I would prefer to keep it the same as those. -- Fabien. -
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-01T10:53:12Z
> Attached patch does what is described in the title, hopefully. Continuations > in other pgbench backslash-commands should be dealt with elsewhere... > > Also attached is a small test script. Here is another approach, with "infered" continuations: no backslash is needed, the parsing is pursued if the last token of the line cannot end an expression (eg an operator) or if there is an unclosed parenthesis. I think that backslashes are less surprising for the classically minded user, but this one is more fun:-) Also, this version changes a little more the scanner because on each token the next state (continued or not) must be decided. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2016-11-01T11:44:41Z
Well with this new approach, the example you gave previously for better readability: > \set bid > CASE WHEN random(0, 99) < 85 > THEN :tbid > ELSE :abid + (:abid >= :tbid) > END will give error at the first line. In general, this new approach is likely to create confusions in such cases. As an end-user one needs to be real careful to check what portions have to split between lines. Keeping this in mind, I'd prefer the previous approach. On Tue, Nov 1, 2016 at 4:23 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Attached patch does what is described in the title, hopefully. >> Continuations in other pgbench backslash-commands should be dealt with >> elsewhere... >> >> Also attached is a small test script. >> > > Here is another approach, with "infered" continuations: no backslash is > needed, the parsing is pursued if the last token of the line cannot end an > expression (eg an operator) or if there is an unclosed parenthesis. > > I think that backslashes are less surprising for the classically minded > user, but this one is more fun:-) Also, this version changes a little more > the scanner because on each token the next state (continued or not) must be > decided. > > -- > Fabien. > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-01T12:26:14Z
Hello Rafia, > Well with this new approach, the example you gave previously for better > readability: > >> \set bid >> CASE WHEN random(0, 99) < 85 >> THEN :tbid >> ELSE :abid + (:abid >= :tbid) >> END > > will give error at the first line. Indeed you are right for the patch I sent, but it is ok if the initial state is COEX, i.e. it does not allow an empty expression. > In general, this new approach is likely to create confusions in such > cases. See attached version. > As an end-user one needs to be real careful to check what portions have > to split between lines. Keeping this in mind, I'd prefer the previous > approach. I will not fight over this one. I like it in "scala", though, and I find it rather elegant, especially as backslashes are quite ugly. Another reason not to take it is that it would be much harder to have that in psql as well. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-08T10:33:36Z
Hello Rafia, Please keep copying to the list. > Though I find the first version of this patch acceptable in terms that it > would be helpful on enhancing the readability of expressions as you > mentioned. However, the second version is not clear as I mentioned before > also, there has to be detailed documentation regarding this, still as an > end-user one needs to be too careful to decide which statements to split > lines based on the patch, for example following > \set bid > CASE WHEN random(0, 99) < 85 > THEN :tbid > ELSE :abid + (:abid >= :tbid) > END > > should be written as > > \set bid CASE WHEN random(0, 99) < 85 > THEN :tbid > ELSE :abid + (:abid >= :tbid) > END I do not understand the "should". It is a matter of style, and both cases would work with the second version of the patch. > (and a few more variants of splitting lines just after an operator or > open parenthesis) which does not look like much enhancement in > readability to me. As I said, I will not fight over this one. I like inferred continuations in scala, but I guess it would be surprising for such an old school script like pgbench, and it would be hard to do that consistently for pgsql because the backslash command syntax may depends on the command itself (eg does it have to respect strings and parenthesis, or some other structures...). > So this patch needs to add the support to read the next line even when > "\set a" and other such similar expressions are encountered. The scala convention is that if the line is not finished the user must either use explicit parenthesis are use an unambiguous operator which expects an operand, so that the lexer knows it has to go on. I do not think that having both inferred continuations and explicit backslash continuations is desirable, it should be one or the other. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Robert Haas <robertmhaas@gmail.com> — 2016-11-08T22:28:51Z
On Tue, Nov 8, 2016 at 5:33 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > I do not think that having both inferred continuations and explicit > backslash continuations is desirable, it should be one or the other. +1. My vote is for backslash continuations. Inferred continuations require you to end your expressions in places where they can't legally stop, so you can't do \set x 2 +3 will not do the same thing as \set x 2+ 3 I don't want to get into a situation where every future bit of pgbench syntax we want to introduce has to worry about what the interaction with inferred continuations might be. Backslash continuations are kind of ugly, but it's a simple rule and virtually everybody who is likely to be writing pgbench scripts will understand it immediately. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-09T09:58:58Z
> +1. My vote is for backslash continuations. I'm fine with that! -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2016-11-09T10:50:12Z
On Wed, Nov 9, 2016 at 3:28 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > +1. My vote is for backslash continuations. >> > > I'm fine with that! > > -- > Fabien. > Looks good to me also. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
-
Re: pgbench - allow backslash continuations in \set expressions
Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-30T19:20:25Z
Rafia Sabih <rafia.sabih@enterprisedb.com> writes: > On Wed, Nov 9, 2016 at 3:28 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: >>>> +1. My vote is for backslash continuations. >> I'm fine with that! > Looks good to me also. I concur that we don't want implicit continuation; that creates too many special cases and will certainly fail to generalize to other backslash commands. My gripe with pgbench-set-continuation-1.patch is actually the latter: I do not like the idea that this works only for \set and not other backslash commands. I think we should have uniform behavior across all backslash commands, which probably means implementing this in psqlscan.l not exprscan.l, which probably means that it would apply in psql as well as pgbench. But I argue that's a Good Thing. I certainly don't see that psql needs this less ... try a \copy command with a complicated SELECT source sometime. In short, I want to mark this RWF for today and ask for a version that applies globally to all backslash commands in psql and pgbench. regards, tom lane
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-11-30T23:04:43Z
Hello Tom, > In short, I want to mark this RWF for today and ask for a version that > applies globally to all backslash commands in psql and pgbench. Hmmm. The modus operandi of backslash command scanning is to switch to possibly another scanner just after scanning the backslash command, so I did the simple thing which is to handle this in the expression scanner. A simple feature achieved with a simple code. Factoring out the behavior means handling continuations from the master scanner, probably by using some other intermediate buffer which is then rescanned... This implies that all backslash commands share some kind of minimal lexical convention that \ followed by a (\r|\n|\r\n) is a continuation... But then what if the backslash command accept quoted strings, should a continuation still be a continuation inside quotes? If not, how do I know? Are all currently existing backslash command compatible with a common set of lexical convention? Or do some commands should accept backslash continuations and others not, and have to be treated differently and knowingly by the lexer? There is also the issue of locating error messages if the continuation is in another buffer, probably someone will complain. Basically, many issues arise, all of them somehow solvable, but I'm afraid with many more lines than my essentially one line patch:-) I'm not sure such a simple feature deserves so much energy. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-30T23:38:21Z
Fabien COELHO <coelho@cri.ensmp.fr> writes: >> In short, I want to mark this RWF for today and ask for a version that >> applies globally to all backslash commands in psql and pgbench. > I'm not sure such a simple feature deserves so much energy. It's not a "simple feature". As you have it, it's a non-orthogonal wart. It's like telling users that the return key only works in \set commands and elsewhere you have to type shift-return to end a line. I'm fine with setting up a global policy that backslash-newline works to continue backslash commands across lines, but I'm not fine with saying that it only works in \set. That's just weird, and the fact that you can do it with a trivial patch doesn't make it less weird. You're proposing to put a new cognitive load on users because you can't be bothered to write a patch that's not half-baked. We don't do things like that around here. FWIW, I looked a bit further and concluded that probably psqlscan.l doesn't need to be modified; so likely you could do this across all of pgbench's commands without touching psql. That might be an acceptable compromise for now, though I still think that as soon as we have this for pgbench, users will start wanting it in psql. regards, tom lane
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-12-01T08:08:24Z
Hello Tom, > [...] I definitely agree that having homogeneous continuations on every backslash commands is better, but this has a cost in code lines and possible backward compatibilities. This is the kind of thing better addressed in the initial design rather than long afterwards... > FWIW, I looked a bit further and concluded that probably psqlscan.l > doesn't need to be modified; so likely you could do this across all of > pgbench's commands without touching psql. Hmmm, I'm a little bit lost, you just asked for that: >>> In short, I want to [...] ask for a version that applies globally to >>> all backslash commands in psql and pgbench. I'm pointing out that this requirements implies that all such commands share some minimal common lexical structure, higher that the current "pass every characters up to the next new line". - what would be the common acceptable requirements? at least \<nl> = continuation - maybe \\<nl> is just \<nl> if need be? or not?? - what about simple/double quoted strings (eg in \copy ...)? or can we say that the continuation preprocessing does not look into that, and is pretty rough, and that is fine? - if not, are possible corner case backward incompatibilities introduced by such feature ok? > That might be an acceptable compromise for now, though I still think > that as soon as we have this for pgbench, users will start wanting it in > psql. ISTM that you put the case for having them everywhere or nowhere, so I'm trying to measure the effort implied by the former before deciding what I'll do. -- Fabien. -
Re: pgbench - allow backslash continuations in \set expressions
Daniel Verite <daniel@manitou-mail.org> — 2016-12-01T09:16:26Z
Fabien COELHO wrote: > - if not, are possible corner case backward incompatibilities introduced > by such feature ok? In psql, if backslash followed by [CR]LF is interpreted as a continuation symbol, commands like these seem problematic on Windows since backslash is the directory separator: \cd \ \cd c:\somepath\ Shell invocations also come to mind: \! dir \ Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-12-01T14:15:43Z
Hello Daniel, >> - if not, are possible corner case backward incompatibilities introduced >> by such feature ok? > > In psql, if backslash followed by [CR]LF is interpreted as a > continuation symbol, commands like these seem problematic > on Windows since backslash is the directory separator: > > \cd \ > \cd c:\somepath\ > > Shell invocations also come to mind: > \! dir \ Thanks for pointing out these particular cases. I was afraid of such potential issues, hence my questions... Would requiring a space after the \ to accept these be ok, even if it is somehow a regression? By the way, how does changing directory to a directory with a space in the name works? I.e. does "\<space>" already means something for some commands? Given these examples, I'm not sure I see a way out to having continuations on all backslash commands without really breaking some stuff... Or maybe having continuations for some commands only, but this is exactly what is rejected by Tom about my patch... -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Tom Lane <tgl@sss.pgh.pa.us> — 2016-12-01T19:13:44Z
Fabien COELHO <coelho@cri.ensmp.fr> writes: >> In psql, if backslash followed by [CR]LF is interpreted as a >> continuation symbol, commands like these seem problematic >> on Windows since backslash is the directory separator: >> >> \cd \ >> \cd c:\somepath\ >> >> Shell invocations also come to mind: >> \! dir \ > Thanks for pointing out these particular cases. I was afraid of such > potential issues, hence my questions... Those look like nasty counterexamples, but I think they are not, because they don't work today. What you get is regression=# \cd \ Invalid command \. Try \? for help. AFAICT you would need to write it regression=# \cd '\\' \cd: could not change directory to "\": No such file or directory (That's on Unix of course, on Windows I'd expect it to succeed.) The reason for this is that psql already has a policy that an unquoted backslash begins a new backslash command on the same line. Since there is no command named backslash-return, this is available syntax that can be repurposed in the direction we want. I believe that we need to do basically the same thing in pgbench, and I'm fine with that because I think we should have an overall policy of synchronizing the psql and pgbench metacommand syntaxes as best we can. This will at some point necessitate invention of a quoting rule for pgbench metacommand arguments, so that you can write a backslash as part of an argument when you need to. We might not need to do that today (as I'm not sure there are any use-cases for one), but maybe it'd be best to just bite the bullet and put it in. regards, tom lane
-
Re: pgbench - allow backslash continuations in \set expressions
Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-12-02T06:36:34Z
On Fri, Dec 2, 2016 at 6:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Fabien COELHO <coelho@cri.ensmp.fr> writes: > >> In psql, if backslash followed by [CR]LF is interpreted as a > >> continuation symbol, commands like these seem problematic > >> on Windows since backslash is the directory separator: > >> > >> \cd \ > >> \cd c:\somepath\ > >> > >> Shell invocations also come to mind: > >> \! dir \ > > > Thanks for pointing out these particular cases. I was afraid of such > > potential issues, hence my questions... > > Those look like nasty counterexamples, but I think they are not, because > they don't work today. What you get is > > regression=# \cd \ > Invalid command \. Try \? for help. > > AFAICT you would need to write it > > regression=# \cd '\\' > \cd: could not change directory to "\": No such file or directory > > (That's on Unix of course, on Windows I'd expect it to succeed.) > > The reason for this is that psql already has a policy that an unquoted > backslash begins a new backslash command on the same line. Since > there is no command named backslash-return, this is available syntax > that can be repurposed in the direction we want. > > I believe that we need to do basically the same thing in pgbench, and > I'm fine with that because I think we should have an overall policy of > synchronizing the psql and pgbench metacommand syntaxes as best we can. > This will at some point necessitate invention of a quoting rule for > pgbench metacommand arguments, so that you can write a backslash as part > of an argument when you need to. We might not need to do that today > (as I'm not sure there are any use-cases for one), but maybe it'd be best > to just bite the bullet and put it in. > > The review from the committer is arrived at the end of commitfest, so moved the patch into next CF with "waiting on author" status. Regards, Hari Babu Fujitsu Australia
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2016-12-03T08:22:39Z
> FWIW, I looked a bit further and concluded that probably psqlscan.l > doesn't need to be modified; so likely you could do this across all of > pgbench's commands without touching psql. That might be an acceptable > compromise for now, though I still think that as soon as we have this > for pgbench, users will start wanting it in psql. The attached patch adds backslash-return (well newline really) continuations to all pgbench backslash-commands. The attached test uses continuations on all such commands (sleep set setshell and shell). I think that adding continuations to psql should be a distinct patch. -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2017-01-11T04:18:57Z
On Sat, Dec 3, 2016 at 1:52 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > >> FWIW, I looked a bit further and concluded that probably psqlscan.l >> doesn't need to be modified; so likely you could do this across all of >> pgbench's commands without touching psql. That might be an acceptable >> compromise for now, though I still think that as soon as we have this >> for pgbench, users will start wanting it in psql. > > > The attached patch adds backslash-return (well newline really) continuations > to all pgbench backslash-commands. > > The attached test uses continuations on all such commands (sleep set > setshell and shell). > > I think that adding continuations to psql should be a distinct patch. > > -- > Fabien. Hello Fabien, The patch does not apply on the latest head, I guess this requires rebasing since yours is posted in December. Again, it is giving trailing whitespace errors (as I reported for the earlier version), plus it does not apply with git apply, hopefully that would be fixed once rebased. Other than that, I observed that if after backslash space is there, then the command fails. I think it should be something like if after backslash some spaces are there, followed by end-of-line then it should ignore these spaces and read next line, atleast with this new meaning of backslash. Otherwise, it should be mentioned in the docs that backslash should not be followed by space. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
-
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-01-11T12:09:17Z
Hello, >> The attached patch adds backslash-return (well newline really) continuations >> to all pgbench backslash-commands. >> >> The attached test uses continuations on all such commands (sleep set >> setshell and shell). >> >> I think that adding continuations to psql should be a distinct patch. > The patch does not apply on the latest head, I guess this requires > rebasing since yours is posted in December. Strange. Here is a new version and a test for all known backslash-commands in pgbench. sh> git br test master sh> git apply ~/pgbench-continuation-3.patch # ok sh> git diff # ok sh> cd src/bin/pgbench sh> make sh> ./pgbench -t 1 -f SQL/cont.sql starting vacuum...end. debug(script=0,command=1): int 0 debug(script=0,command=2): int 1 debug(script=0,command=4): int 2 3 debug(script=0,command=8): int 4 transaction type: SQL/cont.sql > Again, it is giving trailing whitespace errors (as I reported for the > earlier version), plus it does not apply with git apply, It does above with the attached version. > hopefully that would be fixed once rebased. Other than that, I observed > that if after backslash space is there, then the command fails. Yes, this is expected. > I think it should be something like if after backslash some spaces are > there, followed by end-of-line then it should ignore these spaces and > read next line, atleast with this new meaning of backslash. Hmmm. This is not the behavior of backslash continuation in bash or python, I do not think that this is desirable to have a different behavior. > Otherwise, it should be mentioned in the docs that backslash should not > be followed by space. I'm not sure. Doc says that continuation is "backslash-return", it cannot be more explicit. If it must say what it is not, where should it stop? -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2017-01-13T10:00:44Z
On Wed, Jan 11, 2017 at 5:39 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Hello, > >>> The attached patch adds backslash-return (well newline really) >>> continuations >>> to all pgbench backslash-commands. >>> >>> The attached test uses continuations on all such commands (sleep set >>> setshell and shell). >>> >>> I think that adding continuations to psql should be a distinct patch. > > >> The patch does not apply on the latest head, I guess this requires >> rebasing since yours is posted in December. > > > Strange. Here is a new version and a test for all known backslash-commands > in pgbench. > > sh> git br test master > sh> git apply ~/pgbench-continuation-3.patch > # ok > sh> git diff > # ok > sh> cd src/bin/pgbench > sh> make > sh> ./pgbench -t 1 -f SQL/cont.sql > starting vacuum...end. > debug(script=0,command=1): int 0 > debug(script=0,command=2): int 1 > debug(script=0,command=4): int 2 > 3 > debug(script=0,command=8): int 4 > transaction type: SQL/cont.sql > >> Again, it is giving trailing whitespace errors (as I reported for the >> earlier version), plus it does not apply with git apply, > > > It does above with the attached version. It still gives me whitespace errors with git apply, /Users/edb/Downloads/pgbench-continuation-3.patch:31: trailing whitespace. continuation \\{newline} /Users/edb/Downloads/pgbench-continuation-3.patch:39: trailing whitespace. {continuation} { /* ignore */ } /Users/edb/Downloads/pgbench-continuation-3.patch:40: trailing whitespace. /Users/edb/Downloads/pgbench-continuation-3.patch:48: trailing whitespace. {continuation} { /* ignore */ } /Users/edb/Downloads/pgbench-continuation-3.patch:49: trailing whitespace. error: patch failed: doc/src/sgml/ref/pgbench.sgml:810 error: doc/src/sgml/ref/pgbench.sgml: patch does not apply error: patch failed: src/bin/pgbench/exprscan.l:65 error: src/bin/pgbench/exprscan.l: patch does not apply Looks like an editor issue, I used awk '{ sub("\r$", ""); print }' patch1 > patch2 to clean it and it was applying then. >> hopefully that would be fixed once rebased. Other than that, I observed >> that if after backslash space is there, then the command fails. > > > Yes, this is expected. > >> I think it should be something like if after backslash some spaces are >> there, followed by end-of-line then it should ignore these spaces and read >> next line, atleast with this new meaning of backslash. > > > Hmmm. This is not the behavior of backslash continuation in bash or python, > I do not think that this is desirable to have a different behavior. > Okay, seems sensible. >> Otherwise, it should be mentioned in the docs that backslash should not be >> followed by space. > > > I'm not sure. Doc says that continuation is "backslash-return", it cannot be > more explicit. If it must say what it is not, where should it stop? > > -- > Fabien. Please post the clean version and I'll mark it as ready for committer then. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-01-13T11:27:57Z
Hello Rafia, >> sh> git apply ~/pgbench-continuation-3.patch >> # ok > > It still gives me whitespace errors with git apply, Strange. > /Users/edb/Downloads/pgbench-continuation-3.patch:31: trailing whitespace. > continuation \\{newline} > Looks like an editor issue, I used awk '{ sub("\r$", ""); print }' > patch1 > patch2 to clean it and it was applying then. Doing that does not change the file for me. I see no \r in the patch file according to "od", I just see "nl" (0x0a). sha1sum: 97fe805a89707565210699694467f9256ca02dab pgbench-continuation-3.patch Could it be related to transformations operated when the file is downloaded and saved, because it is a text file? -- Fabien. -
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2017-01-13T15:30:31Z
On Fri, Jan 13, 2017 at 4:57 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Hello Rafia, > >>> sh> git apply ~/pgbench-continuation-3.patch >>> # ok >> >> >> It still gives me whitespace errors with git apply, > > > Strange. > Yes, quite strange. >> /Users/edb/Downloads/pgbench-continuation-3.patch:31: trailing whitespace. >> continuation \\{newline} > > >> Looks like an editor issue, I used awk '{ sub("\r$", ""); print }' >> patch1 > patch2 to clean it and it was applying then. > > > Doing that does not change the file for me. > > I see no \r in the patch file according to "od", I just see "nl" (0x0a). > > sha1sum: 97fe805a89707565210699694467f9256ca02dab > pgbench-continuation-3.patch > > Could it be related to transformations operated when the file is downloaded > and saved, because it is a text file? > I think this is delaying the patch unnecessarily, I have attached a version, please see if you can apply it successfully, we can proceed with that safely then... -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-01-13T15:45:36Z
>> Could it be related to transformations operated when the file is downloaded >> and saved, because it is a text file? > > I think this is delaying the patch unnecessarily, I have attached a > version, please see if you can apply it successfully, we can proceed > with that safely then... This is the same file I sent: sh> sha1sum pgbench-continuation-4.patch pgbench-continuation-3.patch 97fe805a89707565210699694467f9256ca02dab pgbench-continuation-4.patch 97fe805a89707565210699694467f9256ca02dab pgbench-continuation-3.patch The difference is that your version is mime-typed with a generic Application/OCTET-STREAM While the one I sent was mime-typed as text/x-diff as defined by the system in /etc/mime.types on my xenial laptop. My guess is that with the later your mail client changes the file when saving it, because it sees "text". -- Fabien.
-
Re: pgbench - allow backslash continuations in \set expressions
Rafia Sabih <rafia.sabih@enterprisedb.com> — 2017-01-17T07:38:01Z
On Fri, Jan 13, 2017 at 9:15 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote: > >>> Could it be related to transformations operated when the file is >>> downloaded >>> and saved, because it is a text file? >> >> >> I think this is delaying the patch unnecessarily, I have attached a >> version, please see if you can apply it successfully, we can proceed >> with that safely then... > > > This is the same file I sent: > > sh> sha1sum pgbench-continuation-4.patch pgbench-continuation-3.patch > 97fe805a89707565210699694467f9256ca02dab pgbench-continuation-4.patch > 97fe805a89707565210699694467f9256ca02dab pgbench-continuation-3.patch > > The difference is that your version is mime-typed with a generic > > Application/OCTET-STREAM > > While the one I sent was mime-typed as > > text/x-diff > > as defined by the system in /etc/mime.types on my xenial laptop. > > My guess is that with the later your mail client changes the file when > saving it, because it sees "text". > Okay, I am marking it as 'Ready for committer' with the following note to committer, I am getting whitespace errors in v3 of patch, which I corrected in v4, however, Fabien is of the opinion that v3 is clean and is showing whitespace errors because of downloader, etc. issues in my setup. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
-
Re: pgbench - allow backslash continuations in \set expressions
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-19T20:59:30Z
Rafia Sabih <rafia.sabih@enterprisedb.com> writes: > Okay, I am marking it as 'Ready for committer' with the following note > to committer, > I am getting whitespace errors in v3 of patch, which I corrected in > v4, however, Fabien is of the opinion that v3 is clean and is showing > whitespace errors because of downloader, etc. issues in my setup. FWIW, what I see is that v3 saves out with Windows-style newlines (\r\n) while v4 saves out with Unix newlines. It wouldn't really matter, because "patch" converts Windows newlines (at least mine does), but Unix newlines are our project style. As for the substance of the patch ... the fix for continuations in the INITIAL state is not this simple. You have rules for {nonspace}+ {space}+ {newline} {continuation} Remember that flex always takes the rule that produces the longest match starting at the current point. {space}+ and {newline} don't conflict with continuations, but {nonspace}+ does: suppose that the next four characters are "a" "b" "\" newline. flex will decide that the token is "ab\", rather than taking the token as being "ab" and letting the "\" wait till next time when it can be parsed as {continuation}. In other words, it works to write backslash-newline immediately after a token in the EXPR state (mainly because backslash isn't a legal part of any token EXPR state currently allows), but not to write it immediately after a token in INITIAL state. I think this is surprising and inconsistent. Probably the easiest fix is to add a rule that explicitly matches this situation: {nonspace}+{continuation} { ... throw back 2 chars and return the rest ... } The "throw back" part is easily done with "yyless(yyleng - 2)"; compare some rules in the core lexer. I have not actually tested this, but I think it will work, because when it applies it would always be the longest possible match. regards, tom lane -
Re: pgbench - allow backslash continuations in \set expressions
Fabien COELHO <coelho@cri.ensmp.fr> — 2017-01-20T13:00:51Z
> You have rules for > > {nonspace}+ [...] > {continuation} > > Remember that flex always takes the rule that produces the longest match > starting at the current point. {space}+ and {newline} don't conflict with > continuations, but {nonspace}+ does: Indeed, I totally overlooked "{nonspace}+". > [...] I think this is surprising and inconsistent. Sure. > Probably the easiest fix is to add a rule that explicitly matches this > situation: > > {nonspace}+{continuation} { ... throw back 2 chars and return the rest ... } Well, as the continuation characters must be ignored, so there is no need to throw them back, just adding the special case is enough? Attached a patch which adds the rule and just sends the found word, plus a test script which also exercises this particular case. Note anyway that it is not necessarily what people may intend when using a continuation: foo\ bla might mean "foobla" rather than "foo" then "bla". For instance with bash: sh>ec\ > ho 1 1 But the same trick in python gives a syntax error: py> print\ ... (1) 1 # ok... py> pri\ ... nt(1) File "<stdin>", line 2 nt(1) ^ SyntaxError: invalid syntax I think it fine if pgbench behaves as python. -- Fabien. -
Re: pgbench - allow backslash continuations in \set expressions
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-20T16:14:17Z
Fabien COELHO <coelho@cri.ensmp.fr> writes: >> Probably the easiest fix is to add a rule that explicitly matches this >> situation: >> {nonspace}+{continuation} { ... throw back 2 chars and return the rest ... } > Well, as the continuation characters must be ignored, so there is no need > to throw them back, just adding the special case is enough? True, for the moment. If we ever put an action into the continuation rule (e.g. to count line numbers in the script) it might be better the other way, but for now this is fine. > Note anyway that it is not necessarily what people may intend when using a > continuation: Yeah, but as you say this varies from one language to another. I'm fine with treating the continuation marker like whitespace. Pushed with cosmetic adjustments (mostly, adding comments). regards, tom lane