Re: Variable substitution in psql backtick expansion

Awhile back in the discussion about the \if feature for psql,
I'd pointed out that you shouldn't really need very much in
the way of boolean-expression evaluation smarts, because you
ought to be able to use a backtick shell escape:

\if `expr :foo \> :bar`
\echo :foo is greater than :bar
\endif

Now that the basic feature is in, I went to play around with this,
and was disappointed to find out that it doesn't work. The reason
is not far to seek: we do not do variable substitution within the
text between backticks. psqlscanslash.l already foresaw that some
day we'd want to do that:

/*
* backticked text: copy everything until next backquote, then evaluate.
*
* XXX Possible future behavioral change: substitute for :VARIABLE?
*/

I think today is that day, because it's going to make a material
difference to the usability of this feature.

I propose extending backtick processing so that

1. :VARIABLE is replaced by the contents of the variable

2. :'VARIABLE' is replaced by the contents of the variable,
single-quoted according to Unix shell conventions. (So the
processing would be a bit different from what it is for the
same notation in SQL contexts.)

This doesn't look like it would take very much new code to do.

Thoughts?

regards, tom lane

On Fri, Mar 31, 2017 at 2:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Awhile back in the discussion about the \if feature for psql,
> I'd pointed out that you shouldn't really need very much in
> the way of boolean-expression evaluation smarts, because you
> ought to be able to use a backtick shell escape:
>
> \if `expr :foo \> :bar`
> \echo :foo is greater than :bar
> \endif
>
> Now that the basic feature is in, I went to play around with this,
> and was disappointed to find out that it doesn't work. The reason
> is not far to seek: we do not do variable substitution within the
> text between backticks. psqlscanslash.l already foresaw that some
> day we'd want to do that:
>
> /*
> * backticked text: copy everything until next backquote, then evaluate.
> *
> * XXX Possible future behavioral change: substitute for :VARIABLE?
> */
>
> I think today is that day, because it's going to make a material
> difference to the usability of this feature.
>
> I propose extending backtick processing so that
>
> 1. :VARIABLE is replaced by the contents of the variable
>
> 2. :'VARIABLE' is replaced by the contents of the variable,
> single-quoted according to Unix shell conventions. (So the
> processing would be a bit different from what it is for the
> same notation in SQL contexts.)
>
> This doesn't look like it would take very much new code to do.
>
> Thoughts?

In short, +1.
--
Michael

On Thu, Mar 30, 2017 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> single-quoted according to Unix shell conventions. (So the
> processing would be a bit different from what it is for the
> same notation in SQL contexts.)
>

+1
Having been bit by format '%L' prepending an 'E' to any string that happens
to have a backslash in it, I'm in favor of this difference.

Any reason we wouldn't do :"VARIABLE" as well? People might expect it given
its use elsewhere, and it would make possible things like

SELECT '$HOME/lamentable application name dir/bin/myprog' as myprog \gset
`:"myprog" arg1 arg2`

both for expanding $HOME and keeping the lamentable dir path as one arg.

Tom Lane wrote:

> Thoughts?

ISTM that expr is too painful to use to be seen as the
idiomatic way of achieving comparison in psql.

Among its disadvantages, it won't work on windows, and its
interface is hard to work with due to the necessary
quoting of half its operators, and the mandatory spacing
between arguments.

Also the quoting rules and command line syntax
depend on the underlying shell.
Isn't it going to be tricky to produce code that works
across different families of shells, like bourne and csh?

I think that users would rather have the option to just put
an SQL expression behind \if. That implies a working connection
to evaluate, which expr doesn't, but that's no
different from the other backslash commands that read
the database.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

2017-03-31 15:00 GMT+02:00 Daniel Verite <daniel(at)manitou-mail(dot)org>:

> Tom Lane wrote:
>
> > Thoughts?
>
> ISTM that expr is too painful to use to be seen as the
> idiomatic way of achieving comparison in psql.
>
> Among its disadvantages, it won't work on windows, and its
> interface is hard to work with due to the necessary
> quoting of half its operators, and the mandatory spacing
> between arguments.
>
> Also the quoting rules and command line syntax
> depend on the underlying shell.
> Isn't it going to be tricky to produce code that works
> across different families of shells, like bourne and csh?
>
> I think that users would rather have the option to just put
> an SQL expression behind \if. That implies a working connection
> to evaluate, which expr doesn't, but that's no
> different from the other backslash commands that read
> the database.
>

some basic expression can be done on client side like defvar, serverver,
... but generic expression should be evaluated in server - I am not sure,
if it is what we would - when I have PLpgSQL.

In psql scripting I expecting really simple expressions - but it should to
work everywhere - using bash is not good enough.

Regards

Pavel

>
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> On Thu, Mar 30, 2017 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> single-quoted according to Unix shell conventions. (So the
>> processing would be a bit different from what it is for the
>> same notation in SQL contexts.)

> Any reason we wouldn't do :"VARIABLE" as well?

Well, what would that mean exactly? The charter of :'FOO', I think,
is to get the string value through shell parsing unscathed. I'm a
lot less clear on what :"FOO" ought to do. If it has any use then
I'd imagine that that includes allowing $shell_variable references in
the string to become expanded. But then you have a situation where some
shell special characters in the string are supposed to take effect but
others presumably still aren't. Figuring out the exact semantics would
take some specific use-cases, and more time than I really have available
right now.

In short, that's something I thought was best left as a later
refinement, rather than doing a rush job we might regret.

> People might expect it given
> its use elsewhere, and it would make possible things like

> SELECT '$HOME/lamentable application name dir/bin/myprog' as myprog \gset
> `:"myprog" arg1 arg2`

You could get about 80% of the way there with `":myprog" arg1 arg2`,
since backtick processing doesn't have any rule that would prevent
:myprog from being expanded inside shell double quotes.

regards, tom lane

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> ISTM that expr is too painful to use to be seen as the
> idiomatic way of achieving comparison in psql.

I'm not proposing it as the best permanent solution, just saying
that having this in v10 is a lot better than having nothing in v10.
And we certainly aren't going to get any more-capable boolean
expression syntax into v10 at this point.

> Among its disadvantages, it won't work on windows, and its
> interface is hard to work with due to the necessary
> quoting of half its operators, and the mandatory spacing
> between arguments.
> Also the quoting rules and command line syntax
> depend on the underlying shell.

All true, but that's true of just about any use of backtick
or \! commands. Shall we remove those features because they
are hard to use 100% portably?

> Isn't it going to be tricky to produce code that works
> across different families of shells, like bourne and csh?

Probably 95% of our users do not really care about that,
because they're only trying to produce scripts that will
work in their own environment.

> I think that users would rather have the option to just put
> an SQL expression behind \if. That implies a working connection
> to evaluate, which expr doesn't, but that's no
> different from the other backslash commands that read
> the database.

Well, it also implies being in a non-aborted transaction,
which seems like more of a problem to me for \if scripting.
Certainly there would be value in an option like that, but
it's not any more of a 100% solution than the `expr` one is.

Also, I didn't think I'd have to point it out, but expr(1)
is hardly the only command you can call from a backtick
substitution. There are *lots* of potential use-cases
here ... but not so many if you can't plug any variable
material into the shell command.

regards, tom lane

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> On Thu, Mar 30, 2017 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> single-quoted according to Unix shell conventions. (So the
>> processing would be a bit different from what it is for the
>> same notation in SQL contexts.)

> +1

Here's a proposed patch for this. I used the existing appendShellString()
logic, which already knows how to quote stuff safely on both Unix and
Windows. A small problem is that appendShellString() rejects LF and CR
characters. As written, it just printed an error and did exit(1), which
is more or less OK for existing callers but seemed like a pretty bad idea
for psql. I refactored it to get the behavior proposed in the patch,
which is that we print an error and decline to substitute the variable's
value, leading to executing the backtick command with the :'variable'
text still in place. This is more or less the same thing that happens
for encoding-check failures in the other variable-substitution cases,
so it didn't seem too unreasonable.

Perhaps it would be preferable to prevent execution of the backtick
command and/or fail the calling metacommand, but that seems to require
some very invasive refactoring (or else magic global variables), which
I didn't have the appetite for.

regards, tom lane

Bonjour Daniel,

> I think that users would rather have the option to just put
> an SQL expression behind \if.

Note that this is already available indirectly, as show in the
documentation.

SELECT some-boolean-expression AS okay \gset
\if :okay
\echo boolean expression was true
\else
\echo boolean expression was false
\endif

--
Fabien.

2017-04-02 8:53 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Bonjour Daniel,
>
> I think that users would rather have the option to just put
>> an SQL expression behind \if.
>>
>
> Note that this is already available indirectly, as show in the
> documentation.
>
> SELECT some-boolean-expression AS okay \gset
> \if :okay
> \echo boolean expression was true
> \else
> \echo boolean expression was false
> \endif
>
>
For this case can be nice to have function that returns server version as
number

some like version_num() .. 10000

comments?

Regards

Pavel

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

Hello Pavel,

> For this case can be nice to have function that returns server version
> as number some like version_num() .. 10000

The server side information can be queried:

SELECT current_setting(‘server_version_num’)
AS server_version_num \gset
-- 90602

However client side is not so clean:

\echo :VERSION
PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit

Probably some :VERSION_NUM would make some sense. See attached PoC patch.
Would it make sense?

--
Fabien.

> For this case can be nice to have function that returns server version as
> number
>
> some like version_num() .. 10000

Another possible trick to get out of a script which does not support \if,
relying on the fact that the unexpected command is simply ignored:

-- exit version below 10
\if false
\echo 'script requires version 10 or better'
\q
\endif

Also possible but less informative on errors:

\set ON_ERROR_STOP on
\if false \endif

--
Fabien.

2017-04-02 9:45 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> For this case can be nice to have function that returns server version as
>> number some like version_num() .. 10000
>>
>
> The server side information can be queried:
>
> SELECT current_setting(‘server_version_num’)
> AS server_version_num \gset
> -- 90602
>
> However client side is not so clean:
>
> \echo :VERSION
> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>
> Probably some :VERSION_NUM would make some sense. See attached PoC patch.
> Would it make sense?

It has sense

Pavel

>
>
> --
> Fabien.

2017-04-02 9:45 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> For this case can be nice to have function that returns server version as
>> number some like version_num() .. 10000
>>
>
> The server side information can be queried:
>
> SELECT current_setting(‘server_version_num’)
> AS server_version_num \gset
> -- 90602
>
> However client side is not so clean:
>
> \echo :VERSION
> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>
> Probably some :VERSION_NUM would make some sense. See attached PoC patch.
> Would it make sense?

Maybe better name for you CLIENT_VERSION_NUM

Can be SERVER_VERSION_NUM taken from connection info?

Regards

Pavel

>
>
> --
> Fabien.

Hello Pavel,

>> \echo :VERSION
>> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
>> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>>
>> Probably some :VERSION_NUM would make some sense. See attached PoC patch.
>> Would it make sense?
>
> Maybe better name for you CLIENT_VERSION_NUM

If it was starting from nothing I would tend to agree with you, but there
is already an existing :VERSION variable, so it seemed logical to keep on
and create variants with the same prefix.

> Can be SERVER_VERSION_NUM taken from connection info?

Probably it could. It seems a little less straightforward than defining a
client-side string at compile time. The information is displayed when the
connection is established, so the information is there somewhere.

psql (10devel, server 9.6.2)

--
Fabien.

> Can be SERVER_VERSION_NUM taken from connection info?

Here is a version with that, quite easy as well as the information was
already there for other checks.

I have not updated "help.c" because the initial VERSION was not presented
there in the first place.

--
Fabien.

>> Can be SERVER_VERSION_NUM taken from connection info?
>
> Here is a version with that, quite easy as well as the information was
> already there for other checks.
>
> I have not updated "help.c" because the initial VERSION was not presented
> there in the first place.

Here is a v3 to fix the documentation example. Sorry for the noise.

--
Fabien.

Fabien COELHO wrote:

> Note that this is already available indirectly, as show in the
> documentation.
>
> SELECT some-boolean-expression AS okay \gset
> \if :okay
> \echo boolean expression was true
> \else
> \echo boolean expression was false
> \endif

Yes, the question was whether we leave it as that for v10,
or if it's worth a last-minute improvement for usability,
assuming it's doable, similarly to what $subject does to backtick
expansion for external evaluation.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Hello Daniel,

>> SELECT some-boolean-expression AS okay \gset
>> \if :okay
>
> Yes, the question was whether we leave it as that for v10,
> or if it's worth a last-minute improvement for usability,
> assuming it's doable, similarly to what $subject does to backtick
> expansion for external evaluation.

My 0.02 € about server-side expressions: ISTM that there is nothing
obvious/easy to do to include these:

- how would it work, both with \set ... and \if ...?

- should it be just simple expressions or may it allow complex
queries?

- how would error detection and handling work from a script?

- should it have some kind of continuation, as expressions are
likely to be longer than a constant?

- how would they interact with possible client-side expressions?

(on this point, I think that client-side is NOT needed for "psql".
It makes sense for "pgbench" in a benchmarking context where the
client must interact with the server in some special meaningful
way, but for simple scripting the performance requirement and
logic is not the same, so server-side could be enough).

Basically quite a few questions which would not find an instantaneous
answer and associated patch.

However I agree with you that there may be minimal usability things to add
before 10, similar to Tom's backtick variable substitution.

Having some access to the client version as suggested by Pavel looks like
a good idea for the kind of script which may rely on conditionals...

Maybe other things, not sure what, though. Maybe other client settings
could be exported as variables, but the version looks like the main which
is currently missing.

Maybe a way to know what is the client current status? eg in transaction,
transaction has aborted, things like that?

--
Fabien.

2017-04-02 13:13 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> \echo :VERSION
>>> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
>>> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>>>
>>> Probably some :VERSION_NUM would make some sense. See attached PoC patch.
>>> Would it make sense?
>>>
>>
>> Maybe better name for you CLIENT_VERSION_NUM
>>
>
> If it was starting from nothing I would tend to agree with you, but there
> is already an existing :VERSION variable, so it seemed logical to keep on
> and create variants with the same prefix.

you have true - so VERSION_NUM should be client side version

>
>
> Can be SERVER_VERSION_NUM taken from connection info?
>>
>
> Probably it could. It seems a little less straightforward than defining a
> client-side string at compile time. The information is displayed when the
> connection is established, so the information is there somewhere.
>

It is not too hard

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfce90..d1ae81646f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3320,16 +3320,21 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ char buffer[100];
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
pset.sversion = PQserverVersion(pset.db);

+ snprintf(buffer, 100, "%d", pset.sversion);
+
SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
SetVariable(pset.vars, "USER", PQuser(pset.db));
SetVariable(pset.vars, "HOST", PQhost(pset.db));
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ SetVariable(pset.vars, "SVERSION_NUM", buffer);

/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);

Regards

Pavel

>
> psql (10devel, server 9.6.2)
>
> --
> Fabien.
>

On Sun, Apr 2, 2017 at 11:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
>
> 2017-04-02 13:13 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:
>
>>
>> Hello Pavel,
>>
>> \echo :VERSION
>>>> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
>>>> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>>>>
>>>> Probably some :VERSION_NUM would make some sense. See attached PoC
>>>> patch.
>>>> Would it make sense?
>>>>
>>>
>>> Maybe better name for you CLIENT_VERSION_NUM
>>>
>>
>> If it was starting from nothing I would tend to agree with you, but there
>> is already an existing :VERSION variable, so it seemed logical to keep on
>> and create variants with the same prefix.
>
>
> you have true - so VERSION_NUM should be client side version
>
>
>>
>>
>> Can be SERVER_VERSION_NUM taken from connection info?
>>>
>>
>> Probably it could. It seems a little less straightforward than defining a
>> client-side string at compile time. The information is displayed when the
>> connection is established, so the information is there somewhere.
>>
>
> It is not too hard
>
> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
> index 94a3cfce90..d1ae81646f 100644
> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -3320,16 +3320,21 @@ checkWin32Codepage(void)
> void
> SyncVariables(void)
> {
> + char buffer[100];
> +
> /* get stuff from connection */
> pset.encoding = PQclientEncoding(pset.db);
> pset.popt.topt.encoding = pset.encoding;
> pset.sversion = PQserverVersion(pset.db);
>
> + snprintf(buffer, 100, "%d", pset.sversion);
> +
> SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
> SetVariable(pset.vars, "USER", PQuser(pset.db));
> SetVariable(pset.vars, "HOST", PQhost(pset.db));
> SetVariable(pset.vars, "PORT", PQport(pset.db));
> SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encod
> ing));
> + SetVariable(pset.vars, "SVERSION_NUM", buffer);
>
> /* send stuff to it, too */
> PQsetErrorVerbosity(pset.db, pset.verbosity);
>
> Regards
>
> Pavel
>
>
>>
>> psql (10devel, server 9.6.2)
>>
>> --
>> Fabien.
>>
>
>

I'm anxious to help with these patches, but they seem a bit of a moving
target. Happy to jump in and review as soon as we've settled on what should
be done.

Hello Corey,

> I'm anxious to help with these patches, but they seem a bit of a moving
> target. Happy to jump in and review as soon as we've settled on what should
> be done.

The "v3" I sent basically adds both client & server version numbers in
client-side variables, basically same code as suggested by Pavel for the
server version, and some documentation.

The questions are:

- which version should be provided (10.0 100000 ...)

- how should they be named?

In v3 there is VERSION{,_NAME,_NUM} for client and
SERVER_VERSION_{NUM,NAME} or SVERSION_NUM suggested
by Pavel for server.

- how desirable/useful is it to have this in 10?

Despite that this was not submitted in the relevant CF...
I created an entry in the July one, but this is for 11.

--
Fabien.

On Sun, Apr 2, 2017 at 12:29 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:

>
> Hello Corey,
>
> I'm anxious to help with these patches, but they seem a bit of a moving
>> target. Happy to jump in and review as soon as we've settled on what
>> should
>> be done.
>>
>
> The "v3" I sent basically adds both client & server version numbers in
> client-side variables, basically same code as suggested by Pavel for the
> server version, and some documentation.
>

patch applies via patch -p1

Works as advertised.
# \echo SERVER_VERSION_NAME
SERVER_VERSION_NAME
# \echo :SERVER_VERSION_NAME
10.0
# \echo :SERVER_VERSION_NUM
100000
# \echo :VERSION_NUM
100000

The new documentation is clear, and accurately reflects current name style.

Looking at #define STRINGIFY(), I got curious where else STRINGIFY was used:

$ git grep STRINGIFY
src/bin/psql/startup.c:#define STRINGIFY2(symbol) #symbol
src/bin/psql/startup.c:#define STRINGIFY(symbol) STRINGIFY2(symbol)
src/bin/psql/startup.c: SetVariable(pset.vars, "VERSION_NUM",
STRINGIFY(PG_VERSION_NUM));
src/tools/msvc/Solution.pm:s{PG_VERSION_STR "[^"]+"}{__STRINGIFY(x)
#x\n#define __STRINGIFY2(z) __STRINGIFY(z)\n#define PG_VERSION_STR
"PostgreSQL $self->{strver}$extraver, compiled by Visual C++ build "
__STRINGIFY2(_MSC_VER) ", $bits-bit"};

Without digging too deep, it seems like the redefinition wouldn't be
harmful, but it might make sense to not use the name STRINGIFY() if only to
avoid confusion with Solution.pm.

> The questions are:
>
> - which version should be provided (10.0 100000 ...)
>

A fixed length string without decimals seems best for the multitude of
tools that will want to manipulate that data.

> - how should they be named?
>
> In v3 there is VERSION{,_NAME,_NUM} for client and
> SERVER_VERSION_{NUM,NAME} or SVERSION_NUM suggested
> by Pavel for server.
>

SERVER_VERSION_* is good.
VERSION_* is ok. Would CLIENT_VERSION_* or PSQL_VERSION_* be better?

> - how desirable/useful is it to have this in 10?
>

Extensions and extension-ish packages will love the _NUM vars. The sooner
the better.

There's a lesser need for the _NAME vars.

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> Looking at #define STRINGIFY(), I got curious where else STRINGIFY was used:
> Without digging too deep, it seems like the redefinition wouldn't be
> harmful, but it might make sense to not use the name STRINGIFY() if only to
> avoid confusion with Solution.pm.

More to the point, we already have that. See c.h:

#define CppAsString(identifier) #identifier
#define CppAsString2(x) CppAsString(x)

regards, tom lane

> src/tools/msvc/Solution.pm:s{PG_VERSION_STR "[^"]+"}{__STRINGIFY(x)
> #x\n#define __STRINGIFY2(z) __STRINGIFY(z)\n#define PG_VERSION_STR
> "PostgreSQL $self->{strver}$extraver, compiled by Visual C++ build "
> __STRINGIFY2(_MSC_VER) ", $bits-bit"};

Well, this is the same hack.

> Without digging too deep, it seems like the redefinition wouldn't be
> harmful, but it might make sense to not use the name STRINGIFY() if only to
> avoid confusion with Solution.pm.

It is the usual name for these macro. What would you suggest?

>> - how desirable/useful is it to have this in 10?
>
> Extensions and extension-ish packages will love the _NUM vars.

Hmmmm. I'm afraid pg extension scripts (for CREATE EXTENSION) are not
executed through psql, but server side directly, so there is not much
backslash-command support.

> There's a lesser need for the _NAME vars.

I put them more for error reporting, eg:

\if :VERSION_NUM < 110000
\echo :VERSION_NAME is not supported, should be at least 11.0
\q
\endif

--
Fabien.

> More to the point, we already have that. See c.h:
>
> #define CppAsString(identifier) #identifier
> #define CppAsString2(x) CppAsString(x)

Thanks for the pointer. I grepped for them without success. Here is a v4.

--
Fabien

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
>>> - how desirable/useful is it to have this in 10?

>> Extensions and extension-ish packages will love the _NUM vars.

> Hmmmm. I'm afraid pg extension scripts (for CREATE EXTENSION) are not
> executed through psql, but server side directly, so there is not much
> backslash-command support.

Yeah.

>> There's a lesser need for the _NAME vars.

> I put them more for error reporting, eg:

> \if :VERSION_NUM < 110000
> \echo :VERSION_NAME is not supported, should be at least 11.0
> \q
> \endif

I kinda feel like we're getting ahead of ourselves here, in that
the above is not going to do what you want until we have some kind
of expression eval capability built into psql. You pointed out that
"\if false" can be used to reject pre-v10 psqls, but what about
rejecting v10? ISTM that if we leave this out until there's something
that can do something useful with it, then something along the lines of

\if false
-- pre-v10, complain and die
\endif
\if not defined VERSION_NUM
-- pre-v11, complain and die
\endif

would do the trick. Of course, there are probably other ways to
do that, but my point is that you haven't made a case why we need
to put this in now rather than later.

regards, tom lane

Hello Tom,

> about version numbers [...] Of course, there are probably other ways to
> do that, but my point is that you haven't made a case why we need to put
> this in now rather than later.

Indeed, I have not. What about having the ability to test for minor
versions?

\if false
-- pre 10.0
\q
\endif
SELECT :VERSION_NUM < 100002 AS minor_not_ok \gset
\if :minor_not_ok
\echo script requires at least pg 10.2
\q
\endif

Otherwise it will wait for next CF. Note that the patch is pretty minor
and straightforward, no need to spend much time on it.

--
Fabien.

Fabien COELHO wrote:

> My 0.02 € about server-side expressions: ISTM that there is nothing
> obvious/easy to do to include these:
>
> - how would it work, both with \set ... and \if ...?

The idea is that the need to have two command (a SELECT .. \gset
followed by an \if) and a temporary variable in-between would be
lifted by implementing a close equivalent in one command.
It would behave essentially the same as the two commands.

I don't see that \set must have to be involved in that improvement,
although it could be indirectly, depending on what exactly we
implement.

\set differs in that it already exists in released versions,
so we have the backward compatibility to consider.
With \if we are not bound by that, but what \if will be at feature
freeze needs to be as convenient as we can make it in this release,
and not block progress in v11 and later, as these future improvements
will have to be backward-compatible against v10.

> - should it be just simple expressions or may it allow complex
> queries?

Let's imagine that psql would support a syntax like this:
\if [select current_setting('server_version_num')::int < 110000]
or
\if [select 1 from pg_catalog.pg_extension where extname='pgcrypto']

where by convention [ and ] enclose an SQL query that's assumed to
return a single-row, single-column bool-ish value, and in which
psql variables would be expanded, like they are now in
backtick expressions.
Queries can be as complex as necessary, they just have to fit in one line.

> - how would error detection and handling work from a script?

The same as SELECT..\gset followed by \if, when the SELECT fails.

> - should it have some kind of continuation, as expressions are
> likely to be longer than a constant?

No, to me that falls into the issue of continuation of backslash
commands in general, which is discussed separately.

> - how would they interact with possible client-side expressions?

In no way at all,in the sense that, either you choose to use an SQL
evaluator, or a client-side evaluator (if it exists), or a backtick
expression.
They are mutually exclusive for a single \if invocation.

Client-side evaluation would have to be called with a syntax
that is unambiguously different. For example it could be
\if (:SQLSTATE = '23505')
\echo A record with the unique key :key_id already exists
rollback
\endif

AFAIK we don't have :SQLSTATE yet, but we should :)

Maybe the parentheses are not required, or we could require a different set
of brackets to enclose an expression to evaluate internally, like {}, or
whatever provided it's not ambiguous.

> (on this point, I think that client-side is NOT needed for "psql".
> It makes sense for "pgbench" in a benchmarking context where the
> client must interact with the server in some special meaningful
> way, but for simple scripting the performance requirement and
> logic is not the same, so server-side could be enough).

Client-side evaluation is useful for the example above, where
you expect that you might be in a failed transaction, or even
not connected, and you need to do quite simple tests.
We can do that already with backtick expansion and a shell evaluation
command, but it's a bit heavy/inelegant and creates a dependency on
external commands that is detrimental to portability.
I agree that we don't need a full-featured built-in evaluator, because
the cases where it's needed will be rare enough that it's reasonable
to have to defer to an external evaluator in those cases.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Let's imagine that psql would support a syntax like this:
> \if [select current_setting('server_version_num')::int < 110000]
> or
> \if [select 1 from pg_catalog.pg_extension where extname='pgcrypto']

I really dislike this syntax proposal. It would get us into having
to count nested brackets, and distinguish quoted from unquoted brackets,
and so on ... and for what? It's not really better than

\if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'

(ie, "\if sql ...text to send to server...").

If you're worried about shaving keystrokes, make the "select" be implicit.
That would be particularly convenient for the common case where you're
just trying to evaluate a boolean expression with no table reference.

regards, tom lane

Tom Lane wrote:

> I really dislike this syntax proposal. It would get us into having
> to count nested brackets, and distinguish quoted from unquoted brackets,
> and so on ... and for what? It's not really better than
>
> \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'
>
> (ie, "\if sql ...text to send to server...").

That's fine by me. The advantage of enclosing the query is to leave
open the possibility of accepting additional contents after the query,
like options (as \copy does), or other expression terms to combine
with the query's result. But we can live without that.

> If you're worried about shaving keystrokes, make the "select" be implicit.
> That would be particularly convenient for the common case where you're
> just trying to evaluate a boolean expression with no table reference.

These expressions look more natural without the SELECT keyword,
besides the size, but OTOH "\if sql 1 from table where expr"
looks awkward. Given an implicit select, I would prefer
"\if exists (select 1 from table where expr)" but now it's not shorter.

An advantage of prepending the SELECT automatically, is that it
would prevent people from abusing this syntax by putting
update/insert/delete or even DDL in there, imagining that this would
be a success/failure test for these operations.
Having these fail to execute in the first place, when called by \if,
seems like a sane failure mode that we would gain incidentally.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Hello Daniel,

>> - how would it work, both with \set ... and \if ...?
>
> The idea is that the need to have two command (a SELECT .. \gset
> followed by an \if) and a temporary variable in-between would be
> lifted by implementing a close equivalent in one command.
> It would behave essentially the same as the two commands.
>
> I don't see that \set must have to be involved in that improvement,
> although it could be indirectly, depending on what exactly we
> implement.

My point is that there was some idea expressed by Tom or Robert (?) at
some point that pgbench & psql should implement the same backslash
commands when appropriate.

Currently pgbench allows client-side expressions in \set, eg

\set d sqrt(1 + random(1000) * 17)

There is a patch pending to allow pgbench's \set a more developed
syntactic subset of pg SQL, including booleans, logical operator and such.
There is another pending patch which implements \gset and variant in
pgbench. If these patches get it, I would probably also implement \if
because it makes sense for benchmarking.

If psql's \if accepts expressions in psql, then it seems logical at some
level that this syntax would be more or less compatible with pgbench
expressions, somehow, and vice-versa. Hence my question. If I implement
\if in pgbench, I will trivially reuse the \set expression parser
developed by Robert, i.e. have "\if expression".

Now it could be decided that \set in psql stays simplistic because it is
not needed as much as it is with pgbench. Fine with me.

> \set differs in that it already exists in released versions,
> so we have the backward compatibility to consider.

Sure.

> With \if we are not bound by that, but what \if will be at feature
> freeze needs to be as convenient as we can make it in this release,
> and not block progress in v11 and later, as these future improvements
> will have to be backward-compatible against v10.

Sure.

>> - should it be just simple expressions or may it allow complex
>> queries?
>
> Let's imagine that psql would support a syntax like this:
> \if [select current_setting('server_version_num')::int < 110000]
> or
> \if [select 1 from pg_catalog.pg_extension where extname='pgcrypto']
>
> where by convention [ and ] enclose an SQL query that's assumed to
> return a single-row, single-column bool-ish value, and in which
> psql variables would be expanded, like they are now in
> backtick expressions.

Hmmm. Why not. or maybe a parenthesis? At least it looks less terrible
than a prefix thing like "\if sql".

> Queries can be as complex as necessary, they just have to fit in one line.

Hmmm. I'm not sure that the one-line constraint is desirable.

>> - how would error detection and handling work from a script?
>
> The same as SELECT..\gset followed by \if, when the SELECT fails.

There is a problem: AFAICS currently there is no way to test whether
something failed. When there was no \if, there was not way to test
anything, so no need to report issues. Now that there is a if, I think
that having some variable reporting would make sense, eg whether an error
occured, how many rows were affected, things like that.

>> - should it have some kind of continuation, as expressions are
>> likely to be longer than a constant?
>
> No, to me that falls into the issue of continuation of backslash
> commands in general, which is discussed separately.

Hmmm. If there is a begin/end syntactic marker, probably psql lexer could
handle waiting for the end of the expression.

>> - how would they interact with possible client-side expressions?
>
> In no way at all,in the sense that, either you choose to use an SQL
> evaluator, or a client-side evaluator (if it exists), or a backtick
> expression.

My strong desire is to avoid an explicit client vs server side evaluator
choice in the form of something like "\if sql ...". Maybe I could buy
brackets or parentheses, though.

> They are mutually exclusive for a single \if invocation.

Sure.

> Client-side evaluation would have to be called with a syntax
> that is unambiguously different. For example it could be
> \if (:SQLSTATE = '23505')
> \echo A record with the unique key :key_id already exists
> rollback
> \endif
>
> AFAIK we don't have :SQLSTATE yet, but we should :)

Yes, that is one of my points, error (and success) reporting through
variables become useful once you have a test available to do something
about it.

> Maybe the parentheses are not required, or we could require a different set
> of brackets to enclose an expression to evaluate internally, like {}, or
> whatever provided it's not ambiguous.

Hmmm. Yep, not ambiguous, and if possible transparent:-)

Another idea I'm toying with is that by default \if whatever... would
be an SQL server side expression, but for some client-side expressions
which could be filtered out by regex. It would be enough to catch
define and simple comparisons:

((not)? defined \w+|\d+ (=|<|>|<=|<>|!=|...) \d+)

That could be interpreted client side easily enough.

>> (on this point, I think that client-side is NOT needed for "psql".
>> It makes sense for "pgbench" in a benchmarking context where the
>> client must interact with the server in some special meaningful
>> way, but for simple scripting the performance requirement and
>> logic is not the same, so server-side could be enough).
>
> Client-side evaluation is useful for the example above, where
> you expect that you might be in a failed transaction, or even
> not connected, and you need to do quite simple tests.

Yep.

> We can do that already with backtick expansion and a shell evaluation
> command, but it's a bit heavy/inelegant and creates a dependency on
> external commands that is detrimental to portability.

Sure. I do not believe that backtick is a good solution.

> I agree that we don't need a full-featured built-in evaluator,

Yep.

> because the cases where it's needed will be rare enough that it's
> reasonable to have to defer to an external evaluator in those cases.

Hmmm.

--
Fabien.

Hello Tom,

>> \if [select current_setting('server_version_num')::int < 110000]
>
> I really dislike this syntax proposal.

> It would get us into having to count nested brackets, and distinguish
> quoted from unquoted brackets, and so on ... and for what? It's not
> really better than
>
> \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'

Hmmm. On the positive side, it looks more expression-like, and it allows
to think of handling a multi-line expression on day.

ISTM that the lexer already counts parentheses, so maybe using external
parentheses might work without much effort?

> (ie, "\if sql ...text to send to server...").
>
> If you're worried about shaving keystrokes, make the "select" be implicit.
> That would be particularly convenient for the common case where you're
> just trying to evaluate a boolean expression with no table reference.

I'm looking for a solution to avoid the suggested "sql" prefix, because "I
really dislike this proposal", as you put it.

--
Fabien.

> [...] but OTOH "\if sql 1 from table where expr" looks awkward. Given an
> implicit select, I would prefer "\if exists (select 1 from table where
> expr)" but now it's not shorter.

Possibly, but it is just an SQL expression, which looks good in the middle
of an sql script.

> An advantage of prepending the SELECT automatically, is that it
> would prevent people from abusing this syntax by putting
> update/insert/delete or even DDL in there, imagining that this would
> be a success/failure test for these operations.

> Having these fail to execute in the first place, when called by \if,
> seems like a sane failure mode that we would gain incidentally.

Yes, it should be avoided.

--
Fabien.

Fabien COELHO wrote:

> Now it could be decided that \set in psql stays simplistic because it is
> not needed as much as it is with pgbench. Fine with me.

It's not just that. It's that currently, if we do in psql:

\set d sqrt(1 + random(1000) * 17)

then we get that:

\echo :d
sqrt(1+random(1000)*17)

I assume we want to keep that pre-existing behavior of \set in
psql, that is, making a copy of that string and associating a
name to it, whereas I guess pgbench computes a value from it and
stores that value.

Certainly if we want the same sort of evaluator in pgbench and psql
we'd better share the code between them, but I don't think it will be
exposed by the same backslash commands in both programs,
if only for that backward compatibility concern.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

>
> I assume we want to keep that pre-existing behavior of \set in
> psql, that is, making a copy of that string and associating a
> name to it, whereas I guess pgbench computes a value from it and
> stores that value.
>

Maybe a \set-variant called \expr?

> \set d sqrt(1 + random(1000) * 17)
> \echo :d
> sqrt(1+random(1000)*17)
>
> I assume we want to keep that pre-existing behavior of \set in
> psql,

Ok. So no interpreted expression ever in psql's \set for backward
compatibility.

> that is, making a copy of that string and associating a name to it,
> whereas I guess pgbench computes a value from it and stores that value.

Actually it stores the parsed tree, ready to be executed.

--
Fabien.

On Mon, Apr 3, 2017 at 5:12 AM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> Queries can be as complex as necessary, they just have to fit in one line.

​Line continuation in general is missed though I thought something already
when in for 10.0 that improves upon this...​

> In no way at all,in the sense that, either you choose to use an SQL
> evaluator, or a client-side evaluator (if it exists), or a backtick
> expression.
> They are mutually exclusive for a single \if invocation.
>
> Client-side evaluation would have to be called with a syntax
> that is unambiguously different.

​Is that the universe: server, client, shell?

Shell already has backticks required
​Server, being the most common, ideally wouldn't need demarcation
Client thus would want its own symbol pairing to distinguish it from server.

Server doesn't need a leading marker but do we want to limit it to single
statements only and allow an optional trailing semi-colon (or backslash
command) which, if present, would end the "server" portion of the string
and allow for treatment of additional terms on the same line to be parsed
in a different context?

I'm conceptually for the implied "SELECT" idea. It overlaps with plpgsql
syntax.

David J.

2017-04-03 21:24 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Tom,
>
> \if [select current_setting('server_version_num')::int < 110000]
>>>
>>
>> I really dislike this syntax proposal.
>>
>
> It would get us into having to count nested brackets, and distinguish
>> quoted from unquoted brackets, and so on ... and for what? It's not really
>> better than
>>
>> \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'
>>
>
> Hmmm. On the positive side, it looks more expression-like, and it allows
> to think of handling a multi-line expression on day.
>
> ISTM that the lexer already counts parentheses, so maybe using external
> parentheses might work without much effort?
>
> (ie, "\if sql ...text to send to server...").
>>
>> If you're worried about shaving keystrokes, make the "select" be implicit.
>> That would be particularly convenient for the common case where you're
>> just trying to evaluate a boolean expression with no table reference.
>>
>
> I'm looking for a solution to avoid the suggested "sql" prefix, because "I
> really dislike this proposal", as you put it.
>
>
The expression evaluation is interesting question, but there is a
workaround - we can use \gset already.

What is more important, because there is not any workaround, is detection
if some variable exists or not.

So possibilities

1. \if defined varname
2. \ifdefined or \ifdef varname
3. \if :?varname

I like first two, and I can live with @3 - although it is not intuitive

Regards

Pavel

Hello Pavel,

> The expression evaluation is interesting question, but there is a
> workaround - we can use \gset already.

Yes, that is a good point. It is a little bit inconvenient because it
requires a dummy variable name each time for testing.

SELECT whatever AS somename \gset
\if :somename

But this is an already functional solution to use server-side expressions,
so there is no hurry.

> What is more important, because there is not any workaround, is detection
> if some variable exists or not.
>
> So possibilities
>
> 1. \if defined varname

Yep, and as Tom pointed it should handle NOT as well.

My issue with this version is that Lane Tom convinced me some time ago
that client side expressions should look like SQL expressions, so that
everything in the script is somehow in the same language. I think that he
made a good point.

However "defined varname" is definitely not an SQL expression, so I do not
find that "intuitive", for a given subjective idea of intuitive.

Then there is the question of simple comparisons, which would also make
sense client-side, eg simple things like:

\if :VERSION_NUM >= 110000

Should not need to be executed on the server.

> 2. \ifdefined or \ifdef varname

I think that we want to avoid that if possible, but it is a cpp-like
possibility. This approach does not allow to support comparisons.

> 3. \if :?varname

Tom suggested that there is a special namespace problem with this option.
I did not understand what is the actual issue.

> I like first two, and I can live with @3 - although it is not intuitive

For me @3 is neither worth nor better than the already existing :'varname'
and :"varname" hacks, it is consistent with them, so it is just an
extension of the existing approach.

It seems easy to implement because the substitution would be handled by
the lexer, so there is no need for anything special like looking at the
first or second word, rewinding, whatever.

Basically I agree with everything Tom suggested (indeed, some client side
definition & comparison tests are really needed), but not with the
proposed prefix syntax because it does not look clean and SQL.

--
Fabien.

2017-04-04 9:53 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> The expression evaluation is interesting question, but there is a
>> workaround - we can use \gset already.
>>
>
> Yes, that is a good point. It is a little bit inconvenient because it
> requires a dummy variable name each time for testing.
>
> SELECT whatever AS somename \gset
> \if :somename
>
> But this is an already functional solution to use server-side expressions,
> so there is no hurry.
>
> What is more important, because there is not any workaround, is detection
>> if some variable exists or not.
>>
>> So possibilities
>>
>> 1. \if defined varname
>>
>
> Yep, and as Tom pointed it should handle NOT as well.
>
> My issue with this version is that Lane Tom convinced me some time ago
> that client side expressions should look like SQL expressions, so that
> everything in the script is somehow in the same language. I think that he
> made a good point.
>
> However "defined varname" is definitely not an SQL expression, so I do not
> find that "intuitive", for a given subjective idea of intuitive.
>
> Then there is the question of simple comparisons, which would also make
> sense client-side, eg simple things like:
>
> \if :VERSION_NUM >= 110000
>
> Should not need to be executed on the server.
>
> 2. \ifdefined or \ifdef varname
>>
>
> I think that we want to avoid that if possible, but it is a cpp-like
> possibility. This approach does not allow to support comparisons.
>
> 3. \if :?varname
>>
>
> Tom suggested that there is a special namespace problem with this option.
> I did not understand what is the actual issue.
>
> I like first two, and I can live with @3 - although it is not intuitive
>>
>
> For me @3 is neither worth nor better than the already existing :'varname'
> and :"varname" hacks, it is consistent with them, so it is just an
> extension of the existing approach.
>
> It seems easy to implement because the substitution would be handled by
> the lexer, so there is no need for anything special like looking at the
> first or second word, rewinding, whatever.
>
> Basically I agree with everything Tom suggested (indeed, some client side
> definition & comparison tests are really needed), but not with the proposed
> prefix syntax because it does not look clean and SQL.

I don't need a full SQL expression in \if commands ever. I prefer some
simple functional language here implemented only on client side - the code
from pgbench can be used maybe

\if fx( variable | constant [, ... ] )

the buildin functions can be only basic

defined, undefined, equal, greater, less

\if defined(varname)
\if geq(VERSION_NUM, 11000)

But this question is important - there is not a workaround

postgres=# select :xxx
postgres-# ;
ERROR: syntax error at or near ":"
LINE 1: select :xxx
^
postgres=# \if :xxx
unrecognized value ":xxx" for "\if expression": boolean expected
postgres(at)#

>
>
> --
> Fabien.
>

On 2 April 2017 at 07:53, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> Note that this is already available indirectly, as show in the
> documentation.
>
> SELECT some-boolean-expression AS okay \gset
> \if :okay
> \echo boolean expression was true
> \else
> \echo boolean expression was false
> \endif

Am I the only one who thinks that even if \if got the ability to
evaluate arbitrary SQL queries I would probably still always write
things as above? I think putting arbitrary SQL expressions (let alone
queries) would make scripts just a total mess and impossible for
humans to parse.

Whereas storing the results in psql variables and then using those
variables in \if makes even fairly complex queries and nested \if
structures straightforward. It would also make it far clearer in what
order the queries will be evaluated and under which set of conditions.

I don't think taking a simple command line execution environment like
psql and trying to embed a complete complex language parser in it is
going to result in a sensible programming environment. Having a simple
\if <single variable> is already pushing it. If someone wanted
anything more complex I would strongly recommend switching to perl or
python before trying to code up nesting arbitrary sql in nested
expressions.

--
greg

2017-04-10 13:07 GMT+02:00 Greg Stark <stark(at)mit(dot)edu>:

> On 2 April 2017 at 07:53, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> > Note that this is already available indirectly, as show in the
> > documentation.
> >
> > SELECT some-boolean-expression AS okay \gset
> > \if :okay
> > \echo boolean expression was true
> > \else
> > \echo boolean expression was false
> > \endif
>
>
> Am I the only one who thinks that even if \if got the ability to
> evaluate arbitrary SQL queries I would probably still always write
> things as above? I think putting arbitrary SQL expressions (let alone
> queries) would make scripts just a total mess and impossible for
> humans to parse.
>

Totally agree.

> Whereas storing the results in psql variables and then using those
> variables in \if makes even fairly complex queries and nested \if
> structures straightforward. It would also make it far clearer in what
> order the queries will be evaluated and under which set of conditions.
>
> I don't think taking a simple command line execution environment like
> psql and trying to embed a complete complex language parser in it is
> going to result in a sensible programming environment. Having a simple
> \if <single variable> is already pushing it. If someone wanted
> anything more complex I would strongly recommend switching to perl or
> python before trying to code up nesting arbitrary sql in nested
> expressions.
>

I think so some local expression evaluation could be - but it should not be
placed in \if statement

\expr issupported :VERSION_NUM >= 10000
\if :issuported

maybe \if can support the basic logic predicates NOT, OR, AND - but the
operands can be only evaluated variables

Regards

Pavel

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

Hello Greg,

>> SELECT some-boolean-expression AS okay \gset
>> \if :okay
>
> Am I the only one who thinks that even if \if got the ability to
> evaluate arbitrary SQL queries I would probably still always write
> things as above?

> I think putting arbitrary SQL expressions (let alone queries) would make
> scripts just a total mess and impossible for humans to parse.

No. Pavel does not like them. Tom wants them to be eventually possible...
However, fine with me if it is decided that there will never be
server-side expressions after \if. A good thing is that it potentially
simplifies minimal \if client-side expressions.

> Whereas storing the results in psql variables and then using those
> variables in \if makes even fairly complex queries and nested \if
> structures straightforward. It would also make it far clearer in what
> order the queries will be evaluated and under which set of conditions.

Hmmm. I'm not sure I get it. The penalty I see is that it adds a
dummy variable which must be given a sensible name, and for very short
expressions this is not a win. But this is a minor point.

--
Fabien.

2017-04-11 8:17 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Greg,
>
> SELECT some-boolean-expression AS okay \gset
>>> \if :okay
>>>
>>
>> Am I the only one who thinks that even if \if got the ability to
>> evaluate arbitrary SQL queries I would probably still always write
>> things as above?
>>
>
> I think putting arbitrary SQL expressions (let alone queries) would make
>> scripts just a total mess and impossible for humans to parse.
>>
>
> No. Pavel does not like them. Tom wants them to be eventually possible...
> However, fine with me if it is decided that there will never be server-side
> expressions after \if. A good thing is that it potentially simplifies
> minimal \if client-side expressions.

I think so implementation of simple expression evaluation should not be
hard - really just - we can expect so any variable will be replaced by
const in expression

Num (<|>|=|<=|>=) Num
Text (<|>|=|<=|>=) Text
not Bool
Bool (or|and) Bool

and special operator "defined"

It think so it is all what is necessary to calculate on client side (maybe
text operations are not necessary)

It can be good enough to write

\if not defined somevar
\quit "var is not defined"
\else
\if :somevar > 10000 and SERVER_NUM >= 100000
...
\end
\end

>
>
> Whereas storing the results in psql variables and then using those
>> variables in \if makes even fairly complex queries and nested \if
>> structures straightforward. It would also make it far clearer in what order
>> the queries will be evaluated and under which set of conditions.
>>
>
> Hmmm. I'm not sure I get it. The penalty I see is that it adds a dummy
> variable which must be given a sensible name, and for very short
> expressions this is not a win. But this is a minor point.

I know so it is not ideal - but the language with commands "\if", "\else"
... is not ideal language.

I am very happy so Corey did this work, but I have not and I had not idea
of using psql scripting like full functionality language - you know it well
- the hard barrier is interactivity of psql.

Sometimes I have a idea start new client - and maybe the generic usql
client written in go can be good possibility. This client can have
integrated some language like lua, that can be used for some client side
scripting, maybe for tab complete, ... But it is in my dream area :) - back
to ground :).

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

Hello Pavel,

> I think so some local expression evaluation could be - but it should not be
> placed in \if statement

Why?

> \expr issupported :VERSION_NUM >= 10000

Hmmm. Although I do not buy this, it could work as a replacement for \set
which it seems cannot be upgraded because some people may rely on it to
just store whatever comes after it in a variable.

Maybe \setexpr or \set_expr because it is setting a variable and there is
already a \set.

> \if :issuported
>
> maybe \if can support the basic logic predicates NOT, OR, AND -

ISTM that "NOT" is a minimal requirement, and the easy one.

Note that OR & AND imply a syntax tree, handling parentheses, not in the
same league.

> but the operands can be only evaluated variables.

Why?

If your idea was to be followed, it seems to suggest two parsers with
different constraints, one for the suggested "\expr" and one for the
existing "\if".

I think that if there is a client expression lexer/parser/executor, there
would be just one of them for one syntax. Two is one too many.

--
Fabien.

>
> \else
> \if :somevar > 10000 and SERVER_NUM >= 100000
>

should be
\if :somevar > 10000 and :SERVER_NUM >= 100000

> ...
> \end
>
>

2017-04-11 8:56 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> I think so some local expression evaluation could be - but it should not be
>> placed in \if statement
>>
>
> Why?
>
> \expr issupported :VERSION_NUM >= 10000
>>
>
> Hmmm. Although I do not buy this, it could work as a replacement for \set
> which it seems cannot be upgraded because some people may rely on it to
> just store whatever comes after it in a variable.
>
> Maybe \setexpr or \set_expr because it is setting a variable and there is
> already a \set.
>
> \if :issuported
>>
>> maybe \if can support the basic logic predicates NOT, OR, AND -
>>
>
> ISTM that "NOT" is a minimal requirement, and the easy one.
>
> Note that OR & AND imply a syntax tree, handling parentheses, not in the
> same league.
>
> but the operands can be only evaluated variables.
>>
>
> Why?
>
> If your idea was to be followed, it seems to suggest two parsers with
> different constraints, one for the suggested "\expr" and one for the
> existing "\if".
>
> I think that if there is a client expression lexer/parser/executor, there
> would be just one of them for one syntax. Two is one too many.

in this moment the I am thinking on concept level - \setexpr sounds better
- sure

Important idea is integrating some simple calculus (hard to mark it as
language) used for client side operations only. It can have own commands,
and maybe it can be used in \if command

Regards

Pavel

>
>
> --
> Fabien.
>

> I think so implementation of simple expression evaluation should not be
> hard

Indeed it is not hard, it is rather a matter of deciding what it should
do, and the syntax to do it.

> - really just - we can expect so any variable will be replaced by
> const in expression
>
> Num (<|>|=|<=|>=) Num

<> and != would seem handy as well.

> Text (<|>|=|<=|>=) Text

What would be the use case for handling TEXT?

> not Bool, Bool (or|and) Bool

Aka logical expressions.

> and special operator "defined"

I'm still not buying this suggestion at all because it does not look like
SQL and I think that client-side expressions should be a simple subset of
SQL expressions, which a "defined" operators is definitely not.

>> Hmmm. I'm not sure I get it. The penalty I see is that it adds a dummy
>> variable which must be given a sensible name, and for very short
>> expressions this is not a win. But this is a minor point.

> I know so it is not ideal - but the language with commands "\if", "\else"
> ... is not ideal language.

Sure.

--
Fabien.

2017-04-11 9:07 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> I think so implementation of simple expression evaluation should not be
>> hard
>>
>
> Indeed it is not hard, it is rather a matter of deciding what it should
> do, and the syntax to do it.
>
> - really just - we can expect so any variable will be replaced by
>> const in expression
>>
>> Num (<|>|=|<=|>=) Num
>>
>
> <> and != would seem handy as well.

sorry - I forgot

>
>
> Text (<|>|=|<=|>=) Text
>>
>
> What would be the use case for handling TEXT?
>
> not Bool, Bool (or|and) Bool
>>
>
> Aka logical expressions.
>
> and special operator "defined"
>>
>
> I'm still not buying this suggestion at all because it does not look like
> SQL and I think that client-side expressions should be a simple subset of
> SQL expressions, which a "defined" operators is definitely not.

The "defined" tests is not coming from SQL universe. It is coming from
scripting systems - In plain SQL I can use IS NULL. When I check any not
existing variable in plpgsql I expect syntax error. So SQL doesn't know any
similar to "defined" and it is ok. Currently In psql it is similar. When I
use undefined psql variable I got syntax error. When I expect so some
content of command line will come from command line or (possible) from some
interactive action I would to handle this situation to by my script more
user friendly - and I can write more user friendly error messages or I can
react on it - enforce user input.

I cannot do test on client side test on NULL - currently psql variables
doesn't support it - and I am think so it is not what I want - I am
interesting about some meta information from outside.

else

I need to check if I can use some psql variable. I have to do on client
side. In some languages is usual term defined - some other using some
special syntax or special environments.

The my proposal "defined variablename" should be simple on implementation,
but should not be one. It is just proposal.

The tests:

variable is defined, variable is null, ... is acceptable for me too -
although I have small problem with NULL, because NULL can got from server -
more psql variables doesn't support NULL, and support can enforce
incompatible change.

>
> Hmmm. I'm not sure I get it. The penalty I see is that it adds a dummy
>>> variable which must be given a sensible name, and for very short
>>> expressions this is not a win. But this is a minor point.
>>>
>>
> I know so it is not ideal - but the language with commands "\if", "\else"
>> ... is not ideal language.
>>
>
> Sure.
>
> --
> Fabien.
>

On Tue, Apr 11, 2017 at 2:56 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:

>
> Hello Pavel,
>
> I think so some local expression evaluation could be - but it should not be
>> placed in \if statement
>>
>
> Why?
>
> \expr issupported :VERSION_NUM >= 10000
>>
>
> Hmmm. Although I do not buy this, it could work as a replacement for \set
> which it seems cannot be upgraded because some people may rely on it to
> just store whatever comes after it in a variable.
>

I have no strong opinion on how expressive expressions should be, but
having a separate \expr (or \setexpr, etc) gives us a green field to
develop them.

>> Hmmm. Although I do not buy this, it could work as a replacement for \set
>> which it seems cannot be upgraded because some people may rely on it to
>> just store whatever comes after it in a variable.
>
> I have no strong opinion on how expressive expressions should be, but
> having a separate \expr (or \setexpr, etc) gives us a green field to
> develop them.

Yep.

One possible approach would be to reuse pgbench expression engine in order
to avoid redevelopping yet another lexer & parser & evaluator. This would
mean some abstraction work, but it looks like the simplest & most
effective approach right now. Currently it supports an SQL-expression
subset about int & float, and there is an ongoing submission to add
booleans and a few functions. If this is done this way, this suggests that
variable management should/could be merged as well, but there are some
differences (psql variables are not typed, it relies on a list, there is a
"namespace" thing I'm not sure I understood...).

Pavel also suggested some support for TEXT, although I would like to see a
use case. That could be another extension to the engine.

A drawback is that pgbench needs more powerfull client-side expressions
than psql, thus it adds some useless complexity to psql, but avoiding
another engine seems desirable.

--
Fabien.

Fabien COELHO wrote:

> Pavel also suggested some support for TEXT, although I would like to see a
> use case. That could be another extension to the engine.

SQLSTATE is text.

Also when issuing "psql -v someoption=value -f script", it's reasonable
to want to compare :someoptionvar to 'somevalue' in the script.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

2017-04-12 1:42 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hmmm. Although I do not buy this, it could work as a replacement for \set
>>> which it seems cannot be upgraded because some people may rely on it to
>>> just store whatever comes after it in a variable.
>>>
>>
>> I have no strong opinion on how expressive expressions should be, but
>> having a separate \expr (or \setexpr, etc) gives us a green field to
>> develop them.
>>
>
> Yep.
>
> One possible approach would be to reuse pgbench expression engine in order
> to avoid redevelopping yet another lexer & parser & evaluator. This would
> mean some abstraction work, but it looks like the simplest & most effective
> approach right now. Currently it supports an SQL-expression subset about
> int & float, and there is an ongoing submission to add booleans and a few
> functions. If this is done this way, this suggests that variable management
> should/could be merged as well, but there are some differences (psql
> variables are not typed, it relies on a list, there is a "namespace" thing
> I'm not sure I understood...).
>
> Pavel also suggested some support for TEXT, although I would like to see a
> use case. That could be another extension to the engine.
>

I checked the pgbench expr related code.

Now, there are not a boolean operations, and value compare operations. But
there are lot of functions for random numbers - it is nice for regress
tests.

The text support should be really minimalist - eq op - or can be removed,
if we will have special functions for SQLSTATE (but simple string eq
operation should be useful for writing some tests).

>
> A drawback is that pgbench needs more powerfull client-side expressions
> than psql, thus it adds some useless complexity to psql, but avoiding
> another engine seems desirable.

The pgbench expression language is perfect for us - there is not any new
dependency - it is working on all supported platforms.

Can be nice, if we can reuse pgbench expressions in psql - there are some
task that should be solved first (it is definitely topic for next release)

1. synchronise lexers - the psql lexer doesn't supports types, but
supports variable escaping
2. move pgbench expressions to separate module
3. teach pgbench expressions booleans and strings
4. because pgbench doesn't do early variable evaluation, implementation of
"defined" function is easy - we can introduce some new syntax for
implementation some bash patterns like "default value" or "own undefined
message"
5. we can introduce \setexpr in psql, and \if can use pgbench expr too (the
result of expression) must be boolean value like now
6. the psql builtin variables should be enhanced about server side and
client side numeric versions
7. the psql builtin variables should be enhanced about sqlstate - we are
able to handle errors due setting ON_ERROR_STOP already
8. the psql builtin variables can be enhanced about info about processed
rows

There is a benefit for pgbench - the code can be reduced after migration
expr related code to independent module.

The pgbench can take \if command and \setexpr command (although \setexpr
can be redundant there, there can be nice compatibility with psql)

Regards

Pavel

>
> --
> Fabien.
>

> I checked the pgbench expr related code.

> 2. move pgbench expressions to separate module

Probably already existing "fe_utils".

> 3. teach pgbench expressions booleans

See https://commitfest.postgresql.org/14/985/

--
Fabien.

2017-04-17 1:00 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> I checked the pgbench expr related code.
>>
>
> 2. move pgbench expressions to separate module
>>
>
> Probably already existing "fe_utils".
>
> 3. teach pgbench expressions booleans
>>
>
> See https://commitfest.postgresql.org/14/985/

so some work is done :)

Regards

Pavel

>
>
> --
> Fabien.
>

Hello Pavel,

A more detailed answer to your many points.

> The pgbench expression language is perfect for us - there is not any new
> dependency - it is working on all supported platforms.
>
> Can be nice, if we can reuse pgbench expressions in psql - there are some
> task that should be solved first (it is definitely topic for next release)
>
> 1. synchronise lexers - the psql lexer doesn't supports types, but
> supports variable escaping

Yep. Probably no big deal.

> 2. move pgbench expressions to separate module

Yep, that is needed, "fe_utils" looks like the place as I pointed out
earlier.

> 3. teach pgbench expressions booleans and strings

Boolean are in progress. For string, ISTM that = <> and maybe
|| would make sense.

> 4. because pgbench doesn't do early variable evaluation, implementation of
> "defined" function is easy - we can introduce some new syntax for
> implementation some bash patterns like "default value" or "own undefined
> message"

Maybe. ISTM that a :* syntax should be thought for so that it always work
where variable can be used, not only within client side expressions.

Consider:

\set port 5432

Then you can write:

SELECT :port ;
-- 5432

And it currently works as expected in SQL. Now I think that the same
behavior is desirable for variable definition testing, i.e. with a :*
syntax the substitution can be performed everywhere, eg with:

\if ...
\set port 5432
\endif

Then it would work both client side:

\let port_is_defined :?port

and also server side:

SELECT :?port AS port_is_defined \gset

However I do not think that this can be done cleanly with a "à la perl"
defined.

> 5. we can introduce \setexpr in psql, and \if can use pgbench expr too (the
> result of expression) must be boolean value like now

Yes.

> 6. the psql builtin variables should be enhanced about server side and
> client side numeric versions

Yes, add some typing where appropriate.

> 7. the psql builtin variables should be enhanced about sqlstate - we are
> able to handle errors due setting ON_ERROR_STOP already

Probably.

> 8. the psql builtin variables can be enhanced about info about processed
> rows

Yep. I've already submitted something about ROW_COUNT and such, see:

https://commitfest.postgresql.org/14/1103/

> The pgbench can take \if command and \setexpr command (although \setexpr
> can be redundant there, there can be nice compatibility with psql)

I now believe that "\let" is the nicest sounding close to set short
option, and indeed it should be made to work for pgbench as well to keep
things consistent, for some definition of consistent.

--
Fabien.

>
>
> 4. because pgbench doesn't do early variable evaluation, implementation of
>> "defined" function is easy - we can introduce some new syntax for
>> implementation some bash patterns like "default value" or "own undefined
>> message"
>>
>
> Maybe. ISTM that a :* syntax should be thought for so that it always work
> where variable can be used, not only within client side expressions.
>

has sense

>
> Consider:
>
> \set port 5432
>
> Then you can write:
>
> SELECT :port ;
> -- 5432
>
> And it currently works as expected in SQL. Now I think that the same
> behavior is desirable for variable definition testing, i.e. with a :*
> syntax the substitution can be performed everywhere, eg with:
>
> \if ...
> \set port 5432
> \endif
>
> Then it would work both client side:
>
> \let port_is_defined :?port
>
> and also server side:
>
> SELECT :?port AS port_is_defined \gset
>
> However I do not think that this can be done cleanly with a "à la perl"
> defined.

The syntax is minor problem in this case - I can live with any syntax
there. I prefer a verbose syntax against not well known symbols. If I can
choose between some solutions, then my preferences are 1. some verbose
simple solution with usual syntax, 2. some syntax from another well known
product, 3. some special new PostgreSQL syntax. I don't think so :?xxx is
good idea, because for me :xxx is related to content, not to metadata and
Robert's tip of using bash syntax is more logical for me (to have syntax
for default and custom message). I understand well so it is subjective -
and more, don't think so this point is significant. We should to have this
functionality. That is all.

>
> 5. we can introduce \setexpr in psql, and \if can use pgbench expr too (the
>> result of expression) must be boolean value like now
>>
>
> Yes.
>
> 6. the psql builtin variables should be enhanced about server side and
>> client side numeric versions
>>
>
> Yes, add some typing where appropriate.
>
> 7. the psql builtin variables should be enhanced about sqlstate - we are
>> able to handle errors due setting ON_ERROR_STOP already
>>
>
> Probably.
>
> 8. the psql builtin variables can be enhanced about info about processed
>> rows
>>
>
> Yep. I've already submitted something about ROW_COUNT and such, see:
>
> https://commitfest.postgresql.org/14/1103/
>
> The pgbench can take \if command and \setexpr command (although \setexpr
>> can be redundant there, there can be nice compatibility with psql)
>>
>
> I now believe that "\let" is the nicest sounding close to set short
> option, and indeed it should be made to work for pgbench as well to keep
> things consistent, for some definition of consistent.

sounds well

Regards

Pavel

>
>
> --
> Fabien.

> I don't think so :?xxx is good idea, because for me :xxx is related to
> content, not to metadata

Hmmm. Indeed it is not. I do not see it as an issue, but it is a good
point.

Consider perl "defined $x" or "exists $f{k}". $x/$f{k} should be contents,
but it is not, the dereferencing is suspended by "defined/exists" Yuk,
but simple and effective.

Also with CPP: "#define x 1, #ifdef x", somehow "x" should be the value,
not the name, but yet again it is not dereferenced.

Now consider python: "if 'varname' in locals():" at least it is
consistent, but I cannot say it looks better in the end:-)

So playing around with a value vs metadata is a frequent trick to keep the
syntax simple, even if the logic is not all there as you point out.

> and Robert's tip of using bash syntax is more logical for me (to have
> syntax for default and custom message).

There is no way to simply test for definition in bash, which is exactly
what is needed...

A second issue with sh-like proposal is that it needs a boundary thing,
i.e. bash uses braces ${name<operator>value}. If it was the beginning of
psql I would suggest to consider ${name} stuff, but now I'm not sure that
such a thing can be introduced like ":{xxx}" ? Maybe that can be done.

However it does not change the issue that sh does not allow to test
whether a variable is defined, which is the thought for feature. Providing
a default value or erroring out is not the same thing.

Another question to address: how do you handle ' and " escaping? Pg
:'name' and :"name" solutions are somewhat horrible, but they are there
which show that it was needed. I'm not sure how to translate that with
braces in pg. Maybe :{'name'} and :{"name"}? Hmmm...
Or ":{name}", but then what happens if I write ':{n} x :{m}', should the
lexer interpolate and escape them inside the strings? That is the sh
solution, but I'm not sure it should be done now in psql.

> I understand well so it is subjective - and more, don't think so this
> point is significant.

Well, depending on the syntax things can be done or not, eg test the
variable definition server-side, not only client side. Hence the
discussion:-)

> We should to have this functionality.

Yes.

--
Fabien.

2017-04-17 10:58 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> I don't think so :?xxx is good idea, because for me :xxx is related to
>> content, not to metadata
>>
>
> Hmmm. Indeed it is not. I do not see it as an issue, but it is a good
> point.
>
> Consider perl "defined $x" or "exists $f{k}". $x/$f{k} should be contents,
> but it is not, the dereferencing is suspended by "defined/exists" Yuk, but
> simple and effective.
>
> Also with CPP: "#define x 1, #ifdef x", somehow "x" should be the value,
> not the name, but yet again it is not dereferenced.
>
> Now consider python: "if 'varname' in locals():" at least it is
> consistent, but I cannot say it looks better in the end:-)
>
> So playing around with a value vs metadata is a frequent trick to keep the
> syntax simple, even if the logic is not all there as you point out.
>
> and Robert's tip of using bash syntax is more logical for me (to have
>> syntax for default and custom message).
>>
>
> There is no way to simply test for definition in bash, which is exactly
> what is needed...
>
> A second issue with sh-like proposal is that it needs a boundary thing,
> i.e. bash uses braces ${name<operator>value}. If it was the beginning of
> psql I would suggest to consider ${name} stuff, but now I'm not sure that
> such a thing can be introduced like ":{xxx}" ? Maybe that can be done.
>
> However it does not change the issue that sh does not allow to test
> whether a variable is defined, which is the thought for feature. Providing
> a default value or erroring out is not the same thing.
>
> Another question to address: how do you handle ' and " escaping? Pg
> :'name' and :"name" solutions are somewhat horrible, but they are there
> which show that it was needed. I'm not sure how to translate that with
> braces in pg. Maybe :{'name'} and :{"name"}? Hmmm...
> Or ":{name}", but then what happens if I write ':{n} x :{m}', should the
> lexer interpolate and escape them inside the strings? That is the sh
> solution, but I'm not sure it should be done now in psql.

I have same thinks. We can disallow nesting - it can be acceptable limit.
The :{xxx:operator} can be used for more things - default, check, user
input, ...

necessary escaping can be done in next line

>
>
> I understand well so it is subjective - and more, don't think so this
>> point is significant.
>>
>
> Well, depending on the syntax things can be done or not, eg test the
> variable definition server-side, not only client side. Hence the
> discussion:-)

It depends if variables are declared or defined by value. In psql there are
defined by value. So some tests if var is defined or not is necessary.

>
>
> We should to have this functionality.
>>
>
> Yes.
>
> --
> Fabien.
>

Hi

2017-04-02 21:57 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> More to the point, we already have that. See c.h:
>>
>> #define CppAsString(identifier) #identifier
>> #define CppAsString2(x) CppAsString(x)
>>
>
> Thanks for the pointer. I grepped for them without success. Here is a v4.

I am sending a review of this patch.

This patch has trivial implementation - and there are not any objection to
used new variable names.

1. there was not any problem with patching, compiling
2. all regress tests passed
3. no problems with doc build

I'll mark this patch as ready for commiters

Regards

Pavel

>
> --
> Fabien

>> Thanks for the pointer. I grepped for them without success. Here is a v4.
>
> I am sending a review of this patch.
>
> This patch has trivial implementation - and there are not any objection to
> used new variable names.
>
> 1. there was not any problem with patching, compiling
> 2. all regress tests passed
> 3. no problems with doc build
>
> I'll mark this patch as ready for commiters

Thanks for the review.

--
Fabien.

On Sun, May 21, 2017 at 11:37 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>> This patch has trivial implementation - and there are not any objection to
>> used new variable names.
>>
>> 1. there was not any problem with patching, compiling
>> 2. all regress tests passed
>> 3. no problems with doc build
>>
>> I'll mark this patch as ready for commiters
>
> Thanks for the review.

I am attempting to understand the status of this patch. It looks like
the patch that was the original subject of this thread was committed
as f833c847b8fa4782efab45c8371d3cee64292d9b on April 1st by Tom, who
was its author. Subsequently, a new patch not obviously related to the
subject line was proposed by Fabien Coelho, and that patch was
subsequently marked Ready for Committer by Pavel Stehule. Meanwhile,
objections were raised by Tom, who seems to think that we should make
\if accept an expression language before we consider this change.
AFAICT, there's no patch for that.

Personally, I have mixed feelings about Tom's objection. On the one
hand, it seems a bit petty to reject this patch on the grounds that
the marginally-related feature of having \if accept an expression
doesn't exist yet. It's hard enough to get things into PostgreSQL
already; we shouldn't make it harder without a very fine reason. On
the other hand, given that there is no client-side expression language
yet, the only way to use this seems to be to send a query to the
server, and if you're going to do that, you may as well use select
current_setting('server_version_num') instead of referencing a psql
variable containing the same value. Maybe the client version number
has some use, though; I think that's not otherwise available right
now.

Some comments on the patch itself:

- Documentation needs attention from a native English speaker.
- Is it a good idea/practical to prevent the new variables from being
modified by the user?
- I think Pavel's upthread suggestion of prefixing the client
variables with "client" to match the way the server variables are
named is a good idea.

I guess the bottom line is that I'm willing to fix this up and commit
it if we've got consensus on it, but I read this thread as Fabien and
Pavel voting +1 on this patch, Tom as voting -1, and a bunch of other
people (including me) not taking a strong position. In my view,
that's not enough consensus to proceed. Anybody else want to vote, or
change their vote?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I am attempting to understand the status of this patch. It looks like
> the patch that was the original subject of this thread was committed
> as f833c847b8fa4782efab45c8371d3cee64292d9b on April 1st by Tom, who
> was its author. Subsequently, a new patch not obviously related to the
> subject line was proposed by Fabien Coelho, and that patch was
> subsequently marked Ready for Committer by Pavel Stehule. Meanwhile,
> objections were raised by Tom, who seems to think that we should make
> \if accept an expression language before we consider this change.

My question was more about how much of a use-case there is for these
values if there's no expression language yet. On reflection though,
you can use either expr-in-backticks or a server query to make
comparisons, so there's at least some use-case for the numeric
versions today. I'm still not sure that there's any use case for the
string versions ("9.6.4" etc).

> - Is it a good idea/practical to prevent the new variables from being
> modified by the user?

We haven't done that for existing informational variables, only control
variables that affect psql's behavior. I think we should continue that
policy for new informational variables. If we make them read-only, we
risk breaking scripts that are using those names for their own purposes.
If we don't make them read-only, we risk breaking scripts that are using
those names for their own purposes AND expecting them to provide the
built-in values. The latter risk seems strictly smaller, probably much
smaller.

> - I think Pavel's upthread suggestion of prefixing the client
> variables with "client" to match the way the server variables are
> named is a good idea.

Well, the issue is the precedent of VERSION for the long-form string
spelling of psql's version. But I agree that's not a very nice
precedent. No strong opinion here.

regards, tom lane

On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> My question was more about how much of a use-case there is for these
> values if there's no expression language yet. On reflection though,
> you can use either expr-in-backticks or a server query to make
> comparisons, so there's at least some use-case for the numeric
> versions today. I'm still not sure that there's any use case for the
> string versions ("9.6.4" etc).

If somebody's doing comparisons, they probably want the numeric
version, but somebody might want to print the string version in an
error message e.g. \if <test involving VERSION_NUM> \echo this thing
doesn't work on :VERSION_NAME \quit \endif

>> - Is it a good idea/practical to prevent the new variables from being
>> modified by the user?
>
> We haven't done that for existing informational variables, only control
> variables that affect psql's behavior. I think we should continue that
> policy for new informational variables. If we make them read-only, we
> risk breaking scripts that are using those names for their own purposes.
> If we don't make them read-only, we risk breaking scripts that are using
> those names for their own purposes AND expecting them to provide the
> built-in values. The latter risk seems strictly smaller, probably much
> smaller.

OK, got it.

>> - I think Pavel's upthread suggestion of prefixing the client
>> variables with "client" to match the way the server variables are
>> named is a good idea.
>
> Well, the issue is the precedent of VERSION for the long-form string
> spelling of psql's version. But I agree that's not a very nice
> precedent. No strong opinion here.

Hmm, well I think that's probably a pretty good reason to stick with
the names in the proposed patch. VERSION seems like it was
shortsighted, but I think we're probably best off being consistent
with the precedent at this point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ... I'm still not sure that there's any use case for the
>> string versions ("9.6.4" etc).

> If somebody's doing comparisons, they probably want the numeric
> version, but somebody might want to print the string version in an
> error message e.g. \if <test involving VERSION_NUM> \echo this thing
> doesn't work on :VERSION_NAME \quit \endif

OK, but if human-friendly display is the use-case then it ought to
duplicate what psql itself would print in, eg, the startup message about
server version mismatch. The v4 patch does not, in particular it neglects
PQparameterStatus(pset.db, "server_version"). This would result in
printing, eg, "11.0" when the user would likely rather see "11devel".

regards, tom lane

On Fri, Aug 25, 2017 at 6:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> ... I'm still not sure that there's any use case for the
>>> string versions ("9.6.4" etc).
>
>> If somebody's doing comparisons, they probably want the numeric
>> version, but somebody might want to print the string version in an
>> error message e.g. \if <test involving VERSION_NUM> \echo this thing
>> doesn't work on :VERSION_NAME \quit \endif
>
> OK, but if human-friendly display is the use-case then it ought to
> duplicate what psql itself would print in, eg, the startup message about
> server version mismatch. The v4 patch does not, in particular it neglects
> PQparameterStatus(pset.db, "server_version"). This would result in
> printing, eg, "11.0" when the user would likely rather see "11devel".

Oh. Well, that seems suboptimal.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello Tom,

>>> ... I'm still not sure that there's any use case for the
>>> string versions ("9.6.4" etc).
>
>> If somebody's doing comparisons, they probably want the numeric
>> version, but somebody might want to print the string version in an
>> error message e.g. \if <test involving VERSION_NUM> \echo this thing
>> doesn't work on :VERSION_NAME \quit \endif
>
> OK, but if human-friendly display is the use-case then it ought to
> duplicate what psql itself would print in, eg, the startup message about
> server version mismatch. The v4 patch does not, in particular it neglects
> PQparameterStatus(pset.db, "server_version"). This would result in
> printing, eg, "11.0" when the user would likely rather see "11devel".

I understand that you would prefer VERSION_NAME to show something like

"11devel, server 9.6.4"

Instead of the current "11devel" when there is a client/server mismatch? I
do not like it much. Note that the server version is already available as
:SERVER_NAME/NUM.

Moreover I would like to point out that pre-existing :VERSION does not do
such a thing. I was just extending it to have something more convenient
and simple, hence the names.

Now they can be named :CLIENT_VERSION_NAME/NUM instead, as suggested by
Robert, but that would be a little bit inconsistent with the existing
VERSION...

Or maybe we could rename it CLIENT_VERSION as well, and make the ambiguous
VERSION be the "11devel, server 9.6.4" thing?

In summary, my prefered option is to have:
CLIENT_VERSION "PostgreSQL 11devel on ..."
CLIENT_VERSION_NAME "11devel"
CLIENT_VERSION_NUM 110000
SERVER_VERSION_NAME "9.6.4"
SERVER_VERSION_NUM 090604
maybe SERVER_VERSION for the long string?
and VERSION as "11devel server 9.6.4" is no match, or just the short
string if match, so that it is nearly upward compatible?

As always, the committer decides.

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
>> OK, but if human-friendly display is the use-case then it ought to
>> duplicate what psql itself would print in, eg, the startup message about
>> server version mismatch. The v4 patch does not, in particular it neglects
>> PQparameterStatus(pset.db, "server_version"). This would result in
>> printing, eg, "11.0" when the user would likely rather see "11devel".

> I understand that you would prefer VERSION_NAME to show something like
> "11devel, server 9.6.4"

No, that's not what I said. I'm just complaining that as the patch stands
it will set SERVER_NAME to "11.0", where I think it should say "11devel"
(as of today).

> In summary, my prefered option is to have:
> CLIENT_VERSION "PostgreSQL 11devel on ..."
> CLIENT_VERSION_NAME "11devel"
> CLIENT_VERSION_NUM 110000

I don't think we want to drop :VERSION; that would accomplish little
beyond breaking existing scripts. Plausible choices include duplicating
it, like:

VERSION "PostgreSQL 11devel on ..."
CLIENT_VERSION "PostgreSQL 11devel on ..."
CLIENT_VERSION_NAME "11devel"
CLIENT_VERSION_NUM 110000

or just ignoring the discrepancy:

VERSION "PostgreSQL 11devel on ..."
CLIENT_VERSION_NAME "11devel"
CLIENT_VERSION_NUM 110000

or just leaving "CLIENT" implicit for all of these variables:

VERSION "PostgreSQL 11devel on ..."
VERSION_NAME "11devel"
VERSION_NUM 110000

Robert seems to prefer the last of those, and that'd be fine with me.
(Note that CLIENT is ambiguous anyway: does it mean psql itself, or
libpq?)

> SERVER_VERSION_NAME "9.6.4"
> SERVER_VERSION_NUM 090604

I'm on board with this, except I don't think we should have any leading
zero in the numeric form. There are contexts where somebody might think
that means octal.

regards, tom lane

Hello Tom,

>> I understand that you would prefer VERSION_NAME to show something like
>> "11devel, server 9.6.4"

> No, that's not what I said. I'm just complaining that as the patch stands
> it will set SERVER_NAME to "11.0", where I think it should say "11devel"
> (as of today).

Ok.

> [...]
> VERSION "PostgreSQL 11devel on ..."
> CLIENT_VERSION_NAME "11devel"
> CLIENT_VERSION_NUM 110000

This kind of inconsistencies is hard for human memory:-(

> or just leaving "CLIENT" implicit for all of these variables:
>
> VERSION "PostgreSQL 11devel on ..."
> VERSION_NAME "11devel"
> VERSION_NUM 110000

That is already what the patch does, because of the VERSION precedent.

> Robert seems to prefer the last of those, and that'd be fine with me.
> (Note that CLIENT is ambiguous anyway: does it mean psql itself, or
> libpq?)

Hmmm. Indeed.

>> SERVER_VERSION_NAME "9.6.4"
>> SERVER_VERSION_NUM 090604
>
> I'm on board with this, except I don't think we should have any leading
> zero in the numeric form. There are contexts where somebody might think
> that means octal.

Indeed. The implementation already does this, I just typed it without
checking.

So basically the only thing needed from Robert & you seems to change
"11.0" to "11devel", which is fine with me.

The attached v5 does that.

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> So basically the only thing needed from Robert & you seems to change
> "11.0" to "11devel", which is fine with me.
> The attached v5 does that.

I think you are taking unreasonable shortcuts here:

+ SetVariable(pset.vars, "SERVER_VERSION_NAME", PQparameterStatus(pset.db, "server_version"));

The existing code in connection_warnings() does this:

const char *server_version;

/* Try to get full text form, might include "devel" etc */
server_version = PQparameterStatus(pset.db, "server_version");
/* Otherwise fall back on pset.sversion */
if (!server_version)
{
formatPGVersionNumber(pset.sversion, true,
sverbuf, sizeof(sverbuf));
server_version = sverbuf;
}

and I think you should duplicate that logic verbatim. Now admittedly,
server_version has been available for a long time, so that this might
never matter in practice. But we shouldn't be doing this one way
in one place and differently somewhere else.

regards, tom lane

Hello Tom,

> I think you are taking unreasonable shortcuts here:
>
> + SetVariable(pset.vars, "SERVER_VERSION_NAME", PQparameterStatus(pset.db, "server_version"));
>
> The existing code in connection_warnings() does this:
>
> const char *server_version;
>
> /* Try to get full text form, might include "devel" etc */
> server_version = PQparameterStatus(pset.db, "server_version");
> /* Otherwise fall back on pset.sversion */
> if (!server_version)
> {
> formatPGVersionNumber(pset.sversion, true,
> sverbuf, sizeof(sverbuf));
> server_version = sverbuf;
> }
>
> and I think you should duplicate that logic verbatim. Now admittedly,
> server_version has been available for a long time, so that this might
> never matter in practice. But we shouldn't be doing this one way
> in one place and differently somewhere else.

Hmmm. I think this code may have been justified around version 6/7. This
code could probably be removed: according to the online documentation,
"server_version" seems supported at least back to 7.4. Greping old sources
suggest that it is not implemented in 7.3, though.

Spending developer time to write code for the hypothetical someone running
a psql version 11 linked to a libpq < 7.4, if it can even link, does not
look like a very good investment... Anyway, here is required the update.

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> Spending developer time to write code for the hypothetical someone running
> a psql version 11 linked to a libpq < 7.4, if it can even link, does not
> look like a very good investment... Anyway, here is required the update.

The question is the server's version, not libpq. Modern psql does still
talk to ancient servers (I tried 11devel against 7.2 just now, to be
sure). The stuff in describe.c may not work well, but basic functionality
is there and I don't want to break it.

regards, tom lane

>> Spending developer time to write code for the hypothetical someone running
>> a psql version 11 linked to a libpq < 7.4, if it can even link, does not
>> look like a very good investment... Anyway, here is required the update.
>
> The question is the server's version, not libpq.

Ok.

> Modern psql does still talk to ancient servers (I tried 11devel against
> 7.2 just now, to be sure).

Wow:-)

> The stuff in describe.c may not work well, but basic functionality
> is there and I don't want to break it.

Ok. Then the updated patch should work, although I do not have a setup to
test that easily.

--
Fabien.

So I thought we were done bikeshedding the variable names for this
feature, but as I was reviewing the patch with intent to commit,
I noticed you hadn't updated helpVariables() to mention them.
Possibly you missed this because it doesn't mention VERSION either,
but that doesn't seem very defensible.

I inserted text to describe all five variables --- but
"SERVER_VERSION_NAME" is too long to fit in the available column space.
In the attached updated patch, I moved all the descriptive text over one
column, and really should have moved it over two columns; but adding even
one space makes a couple of the lines longer than 80 columns when they
were not before. Since we've blown past 80 columns on some of the other
output, maybe that doesn't matter. Or maybe we should shorten this
variable name so it doesn't force reformatting of all this text.

Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
"SERVER_VERSION_STR". (The last saves only one character, whereas
we really need to save two if we're trying not to be wider than any
other documented variable.)

Thoughts?

Attached updated patch changes helpVariables() as we'd need to do if
not renaming, and does some minor doc/comment wordsmithing elsewhere.

regards, tom lane

I wrote:
> ... Or maybe we should shorten this
> variable name so it doesn't force reformatting of all this text.
> Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
> "SERVER_VERSION_STR". (The last saves only one character, whereas
> we really need to save two if we're trying not to be wider than any
> other documented variable.)

Just had another idea: maybe make the new variable names

SERVER_VERSION_S
SERVER_VERSION_N
VERSION_S
VERSION_N

"_S" could usefully be read as either "string" or "short", and probably
we should document it as meaning "short". This way avoids creating any
weird inconsistencies with the existing precedent of the VERSION variable.

regards, tom lane

Hi

2017-09-04 18:31 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> So I thought we were done bikeshedding the variable names for this
> feature, but as I was reviewing the patch with intent to commit,
> I noticed you hadn't updated helpVariables() to mention them.
> Possibly you missed this because it doesn't mention VERSION either,
> but that doesn't seem very defensible.
>
> I inserted text to describe all five variables --- but
> "SERVER_VERSION_NAME" is too long to fit in the available column space.
> In the attached updated patch, I moved all the descriptive text over one
> column, and really should have moved it over two columns; but adding even
> one space makes a couple of the lines longer than 80 columns when they
> were not before. Since we've blown past 80 columns on some of the other
> output, maybe that doesn't matter. Or maybe we should shorten this
> variable name so it doesn't force reformatting of all this text.
>
> Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
> "SERVER_VERSION_STR". (The last saves only one character, whereas
> we really need to save two if we're trying not to be wider than any
> other documented variable.)
>
> Thoughts?
>

I prefer SERVER_VERSION_NAME - although it touch 80 columns limit - it is
consistent with VERSION_NAME.

Or maybe break a column line and don't impact other rows.

Regards

Pavel

> Attached updated patch changes helpVariables() as we'd need to do if
> not renaming, and does some minor doc/comment wordsmithing elsewhere.
>
> regards, tom lane
>
>

2017-09-04 18:56 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> I wrote:
> > ... Or maybe we should shorten this
> > variable name so it doesn't force reformatting of all this text.
> > Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
> > "SERVER_VERSION_STR". (The last saves only one character, whereas
> > we really need to save two if we're trying not to be wider than any
> > other documented variable.)
>
> Just had another idea: maybe make the new variable names
>
> SERVER_VERSION_S
> SERVER_VERSION_N
> VERSION_S
> VERSION_N
>
> "_S" could usefully be read as either "string" or "short", and probably
> we should document it as meaning "short". This way avoids creating any
> weird inconsistencies with the existing precedent of the VERSION variable.
>

-1

With respect, it doesn't look well and intuitive.

SERVER_VERSION_STR looks better than this.

I can live very well with SERVER_VERSION_STR and SERVER_VERSION_NUM

Regards

Pavel

>
> regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2017-09-04 18:56 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Just had another idea: maybe make the new variable names
>> SERVER_VERSION_S
>> SERVER_VERSION_N
>> VERSION_S
>> VERSION_N

> SERVER_VERSION_STR looks better than this.

I dunno, I'm not very pleased with the "STR" idea because the verbose
version string is also a string. Documenting "S" as meaning "short"
would dodge that objection.

regards, tom lane

Tom Lane wrote:

> Since we've blown past 80 columns on some of the other
> output, maybe that doesn't matter. Or maybe we should shorten this
> variable name so it doesn't force reformatting of all this text.

The two-space left margin on the entire block does not add that
much to readability, IMV, so maybe we could reclaim these
two characters.

Another idea: since there are already several variables for which
the text + spacing + presentation don't fit anyway,
we could forget about the tabular presentation and grow
vertically.

That would look like the following, for example, with a 3-space margin
for the description:

AUTOCOMMIT
If set, successful SQL commands are automatically committed
COMP_KEYWORD_CASE
Determines the case used to complete SQL key words
[lower, upper, preserve-lower, preserve-upper]
DBNAME
The currently connected database name
ECHO
Controls what input is written to standard output
[all, errors, none, queries]
ECHO_HIDDEN
If set, display internal queries executed by backslash commands;
if set to "noexec", just show without execution
ENCODING
Current client character set encoding
FETCH_COUNT
The number of result rows to fetch and display at a time
(default: 0=unlimited)
etc..

To me that looks like a good trade-off: it eases the size constraints
for both the description and the name of the variable, at the cost
of consuming one more line per variable, but that's why the pager
is for.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

2017-09-04 19:08 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > 2017-09-04 18:56 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> >> Just had another idea: maybe make the new variable names
> >> SERVER_VERSION_S
> >> SERVER_VERSION_N
> >> VERSION_S
> >> VERSION_N
>
> > SERVER_VERSION_STR looks better than this.
>
> I dunno, I'm not very pleased with the "STR" idea because the verbose
> version string is also a string. Documenting "S" as meaning "short"
> would dodge that objection.
>
>
You have to necessary read doc to understand this, and I don't think so it
is good idea.

So SERVER_VERSION_NAME looks like best solution to me.

> regards, tom lane
>

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> The two-space left margin on the entire block does not add that
> much to readability, IMV, so maybe we could reclaim these
> two characters.

Well, it's a sub-list of the entire output of helpVariables(), so
I think some indentation is a good idea.

> That would look like the following, for example, with a 3-space margin
> for the description:

> AUTOCOMMIT
> If set, successful SQL commands are automatically committed

But we could do something close to that, say two-space indent for the
variable names and four-space for the descriptions.

> To me that looks like a good trade-off: it eases the size constraints
> for both the description and the name of the variable, at the cost
> of consuming one more line per variable, but that's why the pager
> is for.

Yeah, we're already past the point where it's likely that
helpVariables()'s output would fit on one screen for anybody, so
maybe this is the best way.

regards, tom lane

2017-09-04 19:35 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > The two-space left margin on the entire block does not add that
> > much to readability, IMV, so maybe we could reclaim these
> > two characters.
>
> Well, it's a sub-list of the entire output of helpVariables(), so
> I think some indentation is a good idea.
>
> > That would look like the following, for example, with a 3-space margin
> > for the description:
>
> > AUTOCOMMIT
> > If set, successful SQL commands are automatically committed
>
> But we could do something close to that, say two-space indent for the
> variable names and four-space for the descriptions.
>
> > To me that looks like a good trade-off: it eases the size constraints
> > for both the description and the name of the variable, at the cost
> > of consuming one more line per variable, but that's why the pager
> > is for.
>
> Yeah, we're already past the point where it's likely that
> helpVariables()'s output would fit on one screen for anybody, so
> maybe this is the best way.
>

the "less" pager supports horizontal scrolling very well.

regards

Pavel

>
> regards, tom lane
>

Hello Tom,

> So I thought we were done bikeshedding the variable names for this
> feature, but as I was reviewing the patch with intent to commit,
> I noticed you hadn't updated helpVariables() to mention them.

Indeed.

> Possibly you missed this because it doesn't mention VERSION either,
> but that doesn't seem very defensible.

Long time ago. Maybe I greped for it to check where it was appearing and
did not find what does not exist...

> I inserted text to describe all five variables --- but
> "SERVER_VERSION_NAME" is too long to fit in the available column space.

Indeed.

> In the attached updated patch, I moved all the descriptive text over one
> column, and really should have moved it over two columns; but adding even
> one space makes a couple of the lines longer than 80 columns when they
> were not before. Since we've blown past 80 columns on some of the other
> output, maybe that doesn't matter. Or maybe we should shorten this
> variable name so it doesn't force reformatting of all this text.

It seems that PSQL_EDITOR_LINENUMBER_ARG (25 characters) has been accepted
before for an environment variable.

> Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
> "SERVER_VERSION_STR". (The last saves only one character, whereas
> we really need to save two if we're trying not to be wider than any
> other documented variable.)
>
> Thoughts?

Like Pavel, I must admit that I do not like these options much, nor the
other ones down thread: I hate "hungarian" naming, ISTM that mixing abbrev
and full words is better avoided. These are really minor aethetical
preferences that I may break occasionally, eg with _NUM for the nice
similarity with _NAME.

ISTM that it needs to be consistent with the pre-existing VERSION, which
rules out "VER".

Now if this is a bloker, I think that anything is more useful than no
variable as it is useful to have them for simple scripting test through
server side expressions.

I also like Daniel's idea to update formatting rules, eg following what is
done for environment variables and accepting that it won't fit in one page
anyway.

SERVER_VERSION NAME
bla bla bla

> Attached updated patch changes helpVariables() as we'd need to do if
> not renaming, and does some minor doc/comment wordsmithing elsewhere.

Given my broken English, I'm fine with wordsmithing.

I like trying to keep the 80 (or 88 it seems) columns limit if possible,
because my getting older eyes water on long lines.

In the documentation, I do not think that both SERVER_VERSION_NAME and
_NUM (or whatever their chosen name) deserve two independent explanations
with heavy repeats just one after the other, and being treated differently
from VERSION_*.

The same together-ness approach can be used for helpVariables(), see v8
attached for instance.

Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
sure of the better way to get it. I tried with "SELECT VERSION() AS
SERVER_VERSION \gset" but varnames are lowerized.

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> I also like Daniel's idea to update formatting rules, eg following what is
> done for environment variables and accepting that it won't fit in one page
> anyway.

Yeah. When you look at all three portions of the helpVariables output,
it's clear that we have faced this issue multiple times before and not
been too consistent about how we dealt with it. There are places that
go over 80 columns; there are places that break the description into
multiple lines (and some of those *also* exceed 80 columns); there are
places that just shove the description onto the next line.

I think we should go with Daniel's idea for all three parts.

> I like trying to keep the 80 (or 88 it seems) columns limit if possible,
> because my getting older eyes water on long lines.

Me too :-(. Also, it seems like we should not assume that we have
indefinite amounts of space in both dimensions. We've already accepted
the need to page vertically, so let's run with that and try to hold
the line on horizontal space.

> In the documentation, I do not think that both SERVER_VERSION_NAME and
> _NUM (or whatever their chosen name) deserve two independent explanations
> with heavy repeats just one after the other, and being treated differently
> from VERSION_*.

I started with it that way, but it seemed pretty unreadable with the
parenthetical examples added. And I think we need the examples,
particularly the one pointing out that you might get something like "beta".

> Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
> sure of the better way to get it. I tried with "SELECT VERSION() AS
> SERVER_VERSION \gset" but varnames are lowerized.

The problem there is you can't get version() without an extra round trip
to the server --- and an extra logged query --- which people are going to
complain about.

regards, tom lane

> I think we should go with Daniel's idea for all three parts.

I'm okay with that, although probably it should be an independent patch.

>> In the documentation, I do not think that both SERVER_VERSION_NAME and
>> _NUM (or whatever their chosen name) deserve two independent explanations
>> with heavy repeats just one after the other, and being treated differently
>> from VERSION_*.
>
> I started with it that way, but it seemed pretty unreadable with the
> parenthetical examples added. And I think we need the examples,
> particularly the one pointing out that you might get something like "beta".

Yes for "beta" which is also in the v8 patch I sent. One shared doc with
different examples does not look too bad to me, and having things repeated
so closely do not look good.

>> Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
>> sure of the better way to get it. I tried with "SELECT VERSION() AS
>> SERVER_VERSION \gset" but varnames are lowerized.
>
> The problem there is you can't get version() without an extra round trip
> to the server --- and an extra logged query --- which people are going to
> complain about.

Yep.

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> [ psql-version-num-8.patch ]

Pushed with some minor additional fiddling.

regards, tom lane

I wrote:
> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
>> That would look like the following, for example, with a 3-space margin
>> for the description:

>> AUTOCOMMIT
>> If set, successful SQL commands are automatically committed

> But we could do something close to that, say two-space indent for the
> variable names and four-space for the descriptions.

Done like that. I forgot to credit you with the idea in the commit log;
sorry about that.

regards, tom lane

>> [ psql-version-num-8.patch ]
>
> Pushed with some minor additional fiddling.

Ok, Thanks. I also noticed the reformating.

--
Fabien.

>> Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
>> sure of the better way to get it. I tried with "SELECT VERSION() AS
>> SERVER_VERSION \gset" but varnames are lowerized.
>
> The problem there is you can't get version() without an extra round trip
> to the server --- and an extra logged query --- which people are going to
> complain about.

Here is a PoC that does it through a guc, just like "server_version" (the
short version) is transmitted, with a fallback if it is not there.

Whether it is worth it is debatable, but I like the symmetry of having
the same informations accessible the same way for client and server sides.

--
Fabien.

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, passed

When i try apply this patch he failed with a following messenger:

File to patch: /src/postgresql/src/bin/psql/command.c
patching file /src/postgresql/src/bin/psql/command.c
Reversed (or previously applied) patch detected! Assume -R? [n] y
Hunk #1 succeeded at 3209 (offset -128 lines).
Hunk #2 FAILED at 3348.
Hunk #3 succeeded at 3252 (offset -128 lines).
1 out of 3 hunks FAILED -- saving rejects to file /src/postgresql/src/bin/psql/command.c.rej
(Stripping trailing CRs from patch; use --binary to disable.)
can't find file to patch at input line 91
Perhaps you should have used the -p or --strip option?
The text leading up to this was:

postgres(at)pgdev:/src/postgresql/src/bin/psql$ cat /src/postgresql/src/bin/psql/command.c.rej
--- command.c
+++ command.c
@@ -3348,20 +3345,6 @@ SyncVariables(void)
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));

- /* this bit should match connection_warnings(): */
- /* Try to get full text form of version, might include "devel" etc */
- server_version = PQparameterStatus(pset.db, "server_version");
- /* Otherwise fall back on pset.sversion for servers prior 7.4 */
- if (!server_version)
- {
- formatPGVersionNumber(pset.sversion, true, vbuf, sizeof(vbuf));
- server_version = vbuf;
- }
- SetVariable(pset.vars, "SERVER_VERSION_NAME", server_version);
-
- snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
- SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
-
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);

Hello Gerdan,

This is an internal address that should not be exposed:

postgresql(dot)org(at)coelho(dot)net

I'm unsure why it gets substituted by the "commitfest application"...

> When i try apply this patch he failed with a following messenger:

It just worked for me on head with

git checkout -b test
git apply ~/psql-server-version-1.patch

My guess is that your mailer or navigator mangles the file contents
because its mime type is "text/x-diff" and that it considers that it is
okay to change NL to CR or CRNL... which is a BAD IDEA (tm).

I re-attached the file compressed so that it uses another mime-type and
should not change its contents.

--
Fabien.

Hi

I am sending a review of last patch psql-server-version-1.patch.gz

This patch is trivial - the most big problem is choosing correct name for
GUC. I am thinking so server_version_raw is acceptable.

I had to fix doc - see attached updated patch

All tests passed.

I'll mark this patch as ready for commiters

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ psql-server-version-2.patch ]

I think this patch should be rejected. It adds no new functionality;
you can get the string in question with "select version()". Moreover,
you've been able to do that for lo these many years. Any application
that tried to depend on this new way of getting the string would fail
when working with an older server or older psql. That does not seem
like a good property for a version check. Also, because the string
isn't especially machine-friendly, it's not very clear to me what the
use-case is for an application to use it at all, rather than the other
version formats we already provide.

regards, tom lane

On Mon, Nov 13, 2017 at 5:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> [ psql-server-version-2.patch ]
>
> I think this patch should be rejected. It adds no new functionality;
> you can get the string in question with "select version()". Moreover,
> you've been able to do that for lo these many years. Any application
> that tried to depend on this new way of getting the string would fail
> when working with an older server or older psql. That does not seem
> like a good property for a version check. Also, because the string
> isn't especially machine-friendly, it's not very clear to me what the
> use-case is for an application to use it at all, rather than the other
> version formats we already provide.

+1 for rejection as version() returns PG_VERSION_STR already. It is
also already possible to define a VERSION variable psqlrc simply with
that:
\set VERSION 'version();'

+-- check consistency of SERVER_VERSION
+-- which is transmitted as GUC "server_version_raw"
+SELECT :'SERVER_VERSION' = VERSION()
+ AND :'SERVER_VERSION' = current_setting('server_version_raw')
+ AND :'SERVER_VERSION' = :'VERSION'
+ AS "SERVER_VERSION is consistent";
Not much enthusiastic with this test when thinking about cross-upgrades.
--
Michael

Hello Tom & Michaël,

>> I think this patch should be rejected.
> +1 for rejection [...]

The noes have it!

Note that the motivation was really symmetric completion:

fabien=# \echo :VERSION_NAME
11devel
fabien=# \echo :VERSION_NUM
110000
fabien=# \echo :VERSION
PostgreSQL 11devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
fabien=# \echo :SERVER_VERSION_NAME
10.1
fabien=# \echo :SERVER_VERSION_NUM
100001

But

fabien=# \echo :SERVER_VERSION
:SERVER_VERSION

To get it into a variable the work around is really:

fabien=# SELECT version() AS "SERVER_VERSION" \gset
fabien=# \echo :SERVER_VERSION
PostgreSQL 10.1 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit

--
Fabien