Thread: pgbench - allow backslash continuations in \set expressions

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: 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||$$:$$||sourcelineELSE 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

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

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.

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

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.

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.

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

> +1.  My vote is for backslash continuations.

I'm fine with that!

-- 
Fabien.

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

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.

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

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

    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

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.

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

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

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/

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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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/

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.

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/

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

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

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/

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

> 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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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