Thread: Re: [HACKERS] Variable substitution in psql backtick expansion

>> 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@cri.ensmp.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@gmail.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@sss.pgh.pa.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@gmail.com> writes:
> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane <tgl@sss.pgh.pa.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_NUM110000  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
upwardcompatible?
 

As always, the committer decides.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.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_NUM110000
 

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

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

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> 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

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..a693fb9 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** bar
*** 3688,3693 ****
--- 3688,3717 ----
        </varlistentry>

        <varlistentry>
+         <term><varname>SERVER_VERSION_NAME</varname></term>
+         <listitem>
+         <para>
+         The server's version number as a string, for
+         example <literal>9.6.2</>, <literal>10.1</>, or <literal>11beta1</>.
+         This is set every time you connect to a database (including
+         program start-up), but can be changed or unset.
+         </para>
+         </listitem>
+       </varlistentry>
+
+       <varlistentry>
+         <term><varname>SERVER_VERSION_NUM</varname></term>
+         <listitem>
+         <para>
+         The server's version number in numeric form, for
+         example <literal>90602</> or <literal>100001</>.
+         This is set every time you connect to a database (including
+         program start-up), but can be changed or unset.
+         </para>
+         </listitem>
+       </varlistentry>
+
+       <varlistentry>
          <term><varname>SINGLELINE</varname></term>
          <listitem>
          <para>
*************** bar
*** 3733,3742 ****

        <varlistentry>
          <term><varname>VERSION</varname></term>
          <listitem>
          <para>
!         This variable is set at program start-up to
!         reflect <application>psql</>'s version.  It can be changed or unset.
          </para>
          </listitem>
        </varlistentry>
--- 3757,3771 ----

        <varlistentry>
          <term><varname>VERSION</varname></term>
+         <term><varname>VERSION_NAME</varname></term>
+         <term><varname>VERSION_NUM</varname></term>
          <listitem>
          <para>
!         These variables are set at program start-up to reflect
!         <application>psql</>'s version, respectively as a verbose string,
!         a short string (e.g., <literal>9.6.2</>, <literal>10.1</>,
!         or <literal>11beta1</>), and a number (e.g., <literal>90602</>
!         or <literal>100001</>).  They can be changed or unset.
          </para>
          </listitem>
        </varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 96f3a40..4283bf3 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** checkWin32Codepage(void)
*** 3337,3342 ****
--- 3337,3345 ----
  void
  SyncVariables(void)
  {
+     char        vbuf[32];
+     const char *server_version;
+
      /* get stuff from connection */
      pset.encoding = PQclientEncoding(pset.db);
      pset.popt.topt.encoding = pset.encoding;
*************** SyncVariables(void)
*** 3348,3353 ****
--- 3351,3370 ----
      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 */
+     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);
*************** UnsyncVariables(void)
*** 3366,3371 ****
--- 3383,3390 ----
      SetVariable(pset.vars, "HOST", NULL);
      SetVariable(pset.vars, "PORT", NULL);
      SetVariable(pset.vars, "ENCODING", NULL);
+     SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+     SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
  }


diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index b3dbb59..ee612b0 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*************** helpVariables(unsigned short int pager)
*** 336,342 ****
       * Windows builds currently print one more line than non-Windows builds.
       * Using the larger number is fine.
       */
!     output = PageOutput(88, pager ? &(pset.popt.topt) : NULL);

      fprintf(output, _("List of specially treated variables\n\n"));

--- 336,342 ----
       * Windows builds currently print one more line than non-Windows builds.
       * Using the larger number is fine.
       */
!     output = PageOutput(93, pager ? &(pset.popt.topt) : NULL);

      fprintf(output, _("List of specially treated variables\n\n"));

*************** helpVariables(unsigned short int pager)
*** 344,378 ****
      fprintf(output, _("Usage:\n"));
      fprintf(output, _("  psql --set=NAME=VALUE\n  or \\set NAME VALUE inside psql\n\n"));

!     fprintf(output, _("  AUTOCOMMIT         if set, successful SQL commands are automatically committed\n"));
!     fprintf(output, _("  COMP_KEYWORD_CASE  determines the case used to complete SQL key words\n"
!                       "                     [lower, upper, preserve-lower, preserve-upper]\n"));
!     fprintf(output, _("  DBNAME             the currently connected database name\n"));
!     fprintf(output, _("  ECHO               controls what input is written to standard output\n"
!                       "                     [all, errors, none, queries]\n"));
!     fprintf(output, _("  ECHO_HIDDEN        if set, display internal queries executed by backslash commands;\n"
!                       "                     if set to \"noexec\", just show without execution\n"));
!     fprintf(output, _("  ENCODING           current client character set encoding\n"));
!     fprintf(output, _("  FETCH_COUNT        the number of result rows to fetch and display at a time\n"
!                       "                     (default: 0=unlimited)\n"));
!     fprintf(output, _("  HISTCONTROL        controls command history [ignorespace, ignoredups, ignoreboth]\n"));
!     fprintf(output, _("  HISTFILE           file name used to store the command history\n"));
!     fprintf(output, _("  HISTSIZE           max number of commands to store in the command history\n"));
!     fprintf(output, _("  HOST               the currently connected database server host\n"));
!     fprintf(output, _("  IGNOREEOF          number of EOFs needed to terminate an interactive session\n"));
!     fprintf(output, _("  LASTOID            value of the last affected OID\n"));
!     fprintf(output, _("  ON_ERROR_ROLLBACK  if set, an error doesn't stop a transaction (uses implicit
savepoints)\n"));
!     fprintf(output, _("  ON_ERROR_STOP      stop batch execution after error\n"));
!     fprintf(output, _("  PORT               server port of the current connection\n"));
!     fprintf(output, _("  PROMPT1            specifies the standard psql prompt\n"));
!     fprintf(output, _("  PROMPT2            specifies the prompt used when a statement continues from a previous
line\n"));
!     fprintf(output, _("  PROMPT3            specifies the prompt used during COPY ... FROM STDIN\n"));
!     fprintf(output, _("  QUIET              run quietly (same as -q option)\n"));
!     fprintf(output, _("  SHOW_CONTEXT       controls display of message context fields [never, errors, always]\n"));
!     fprintf(output, _("  SINGLELINE         end of line terminates SQL command mode (same as -S option)\n"));
!     fprintf(output, _("  SINGLESTEP         single-step mode (same as -s option)\n"));
!     fprintf(output, _("  USER               the currently connected database user\n"));
!     fprintf(output, _("  VERBOSITY          controls verbosity of error reports [default, verbose, terse]\n"));

      fprintf(output, _("\nDisplay settings:\n"));
      fprintf(output, _("Usage:\n"));
--- 344,383 ----
      fprintf(output, _("Usage:\n"));
      fprintf(output, _("  psql --set=NAME=VALUE\n  or \\set NAME VALUE inside psql\n\n"));

!     fprintf(output, _("  AUTOCOMMIT          if set, successful SQL commands are automatically committed\n"));
!     fprintf(output, _("  COMP_KEYWORD_CASE   determines the case used to complete SQL key words\n"
!                       "                      [lower, upper, preserve-lower, preserve-upper]\n"));
!     fprintf(output, _("  DBNAME              the currently connected database name\n"));
!     fprintf(output, _("  ECHO                controls what input is written to standard output\n"
!                       "                      [all, errors, none, queries]\n"));
!     fprintf(output, _("  ECHO_HIDDEN         if set, display internal queries executed by backslash commands;\n"
!                       "                      if set to \"noexec\", just show without execution\n"));
!     fprintf(output, _("  ENCODING            current client character set encoding\n"));
!     fprintf(output, _("  FETCH_COUNT         the number of result rows to fetch and display at a time\n"
!                       "                      (default: 0=unlimited)\n"));
!     fprintf(output, _("  HISTCONTROL         controls command history [ignorespace, ignoredups, ignoreboth]\n"));
!     fprintf(output, _("  HISTFILE            file name used to store the command history\n"));
!     fprintf(output, _("  HISTSIZE            max number of commands to store in the command history\n"));
!     fprintf(output, _("  HOST                the currently connected database server host\n"));
!     fprintf(output, _("  IGNOREEOF           number of EOFs needed to terminate an interactive session\n"));
!     fprintf(output, _("  LASTOID             value of the last affected OID\n"));
!     fprintf(output, _("  ON_ERROR_ROLLBACK   if set, an error doesn't stop a transaction (uses implicit
savepoints)\n"));
!     fprintf(output, _("  ON_ERROR_STOP       stop batch execution after error\n"));
!     fprintf(output, _("  PORT                server port of the current connection\n"));
!     fprintf(output, _("  PROMPT1             specifies the standard psql prompt\n"));
!     fprintf(output, _("  PROMPT2             specifies the prompt used when a statement continues from a previous
line\n"));
!     fprintf(output, _("  PROMPT3             specifies the prompt used during COPY ... FROM STDIN\n"));
!     fprintf(output, _("  QUIET               run quietly (same as -q option)\n"));
!     fprintf(output, _("  SERVER_VERSION_NAME server's version (short string)\n"));
!     fprintf(output, _("  SERVER_VERSION_NUM  server's version (numeric)\n"));
!     fprintf(output, _("  SHOW_CONTEXT        controls display of message context fields [never, errors, always]\n"));
!     fprintf(output, _("  SINGLELINE          end of line terminates SQL command mode (same as -S option)\n"));
!     fprintf(output, _("  SINGLESTEP          single-step mode (same as -s option)\n"));
!     fprintf(output, _("  USER                the currently connected database user\n"));
!     fprintf(output, _("  VERBOSITY           controls verbosity of error reports [default, verbose, terse]\n"));
!     fprintf(output, _("  VERSION             psql's version (verbose string)\n"));
!     fprintf(output, _("  VERSION_NAME        psql's version (short string)\n"));
!     fprintf(output, _("  VERSION_NUM         psql's version (numeric)\n"));

      fprintf(output, _("\nDisplay settings:\n"));
      fprintf(output, _("Usage:\n"));
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7f76797..1e48f4a 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** main(int argc, char *argv[])
*** 160,166 ****
--- 160,169 ----

      EstablishVariableSpace();

+     /* Create variables showing psql version number */
      SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+     SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+     SetVariable(pset.vars, "VERSION_NUM", CppAsString2(PG_VERSION_NUM));

      /* Default values for variables (that don't match the result of \unset) */
      SetVariableBool(pset.vars, "AUTOCOMMIT");

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

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

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2017-09-04 18:56 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.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

"Daniel Verite" <daniel@manitou-mail.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

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

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> 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@cri.ensmp.fr> writes:
> [ psql-version-num-8.patch ]

Pushed with some minor additional fiddling.
        regards, tom lane

I wrote:
> "Daniel Verite" <daniel@manitou-mail.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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@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);
 

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

Hello Gerdan,

This is an internal address that should not be exposed:

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

Pavel Stehule <pavel.stehule@gmail.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


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

On Mon, Nov 13, 2017 at 5:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Stehule <pavel.stehule@gmail.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


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

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