pgbench - allow backslash continuations in \set expressions

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

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.

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

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

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/

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.

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

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.

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/

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.

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

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

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

+1. My vote is for backslash continuations.

I'm fine with that!

--
Fabien.

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

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/

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

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

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.

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

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

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

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.

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

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

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.

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

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

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