about EDITOR_LINENUMBER_SWITCH

Started by Peter Eisentrautover 14 years ago26 messages
#1Peter Eisentraut
peter_e@gmx.net

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.

This is perhaps sort of a Freudian slip. Since the editor itself is
configured using an environment variable, shouldn't any configuration
about the editor also be an environment variable, so people can
configure them together?

Another thought is that this whole thing could be done away with if we
just allowed people to pass through arbitrary options to the editor,
like

\edit file.sql +50 -a -b -c

For powerusers, this could have interesting possibilities.

#2Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#1)
Re: about EDITOR_LINENUMBER_SWITCH

On Sat, May 21, 2011 at 5:47 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.

This is perhaps sort of a Freudian slip.  Since the editor itself is
configured using an environment variable, shouldn't any configuration
about the editor also be an environment variable, so people can
configure them together?

It's probably the result of drift between the original patch and what
was eventually committed. IIRC, Pavel had it as an environment
variable originally, but Tom and I didn't feel the feature was
important enough to merit that treatment.

Another thought is that this whole thing could be done away with if we
just allowed people to pass through arbitrary options to the editor,
like

\edit file.sql +50 -a -b -c

For powerusers, this could have interesting possibilities.

That's an intriguing possibility. But part of the point of the
original feature was to be able to say:

\ef somefunc 10

...and end up on line 10 of somefunc, perhaps in response to an error
message complaining about that line. I don't think your proposal
would address that.

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

#3Pavel Stehule
pavel.stehule@gmail.com
In reply to: Peter Eisentraut (#1)
Re: about EDITOR_LINENUMBER_SWITCH

2011/5/21 Peter Eisentraut <peter_e@gmx.net>:

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.

This is perhaps sort of a Freudian slip.  Since the editor itself is
configured using an environment variable, shouldn't any configuration
about the editor also be an environment variable, so people can
configure them together?

Another thought is that this whole thing could be done away with if we
just allowed people to pass through arbitrary options to the editor,
like

\edit file.sql +50 -a -b -c

in original patch I had to do some magic operation with line number,
so I had to know, what is a line number. A idea with other options are
interesting. More usable can be store these option inside psql
variable (be consistent with current state). Maybe in EDITOR_OPTIONS ?
With possibility to overwrite this options from metacommand

\edit file.sql 10 -x -y -z

Regards

Pavel Stehule

Show quoted text

For powerusers, this could have interesting possibilities.

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

#4Peter Eisentraut
peter_e@gmx.net
In reply to: Robert Haas (#2)
Re: about EDITOR_LINENUMBER_SWITCH

On lör, 2011-05-21 at 20:39 -0400, Robert Haas wrote:

On Sat, May 21, 2011 at 5:47 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.

It's probably the result of drift between the original patch and what
was eventually committed. IIRC, Pavel had it as an environment
variable originally, but Tom and I didn't feel the feature was
important enough to merit that treatment.

I think it's not really a matter of "importance", it's a matter of
making things work correctly. I have a shell configuration that sets
different environment variables, including editor, depending on what
directory I'm in. Now I think that all the editors in question use the
+ syntax, but anyone else with something like that slightly out of the
ordinary would be stuck. The other problem is if I change the editor
here, I have to change this other piece there. Note that you cannot
even specify the editor itself in psqlrc.

Another thought is that this whole thing could be done away with if we
just allowed people to pass through arbitrary options to the editor,
like

\edit file.sql +50 -a -b -c

For powerusers, this could have interesting possibilities.

That's an intriguing possibility. But part of the point of the
original feature was to be able to say:

\ef somefunc 10

...and end up on line 10 of somefunc, perhaps in response to an error
message complaining about that line. I don't think your proposal
would address that.

Well, you'd write

\ef somefunc +10

instead. Or something else, depending on the editor, but then you'd
know what to write, since under the current theory you'd have to have
configured it previously. Using the "+10" syntax also looks a bit
clearer, in my mind.

#5Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#3)
Re: about EDITOR_LINENUMBER_SWITCH

On sön, 2011-05-22 at 06:30 +0200, Pavel Stehule wrote:

A idea with other options are
interesting. More usable can be store these option inside psql
variable (be consistent with current state). Maybe in
EDITOR_OPTIONS ?

There isn't really a need for that, since if you want to pass options to
your editor, you can stick them in the EDITOR variable. The idea would
be more to pass options per occasion.

#6Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#4)
Re: about EDITOR_LINENUMBER_SWITCH

On Tue, May 24, 2011 at 4:36 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

That's an intriguing possibility.  But part of the point of the
original feature was to be able to say:

\ef somefunc 10

...and end up on line 10 of somefunc, perhaps in response to an error
message complaining about that line.  I don't think your proposal
would address that.

Well, you'd write

\ef somefunc +10

instead.

But that would not put you on line 10 of the function.

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

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#6)
Re: about EDITOR_LINENUMBER_SWITCH

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, May 24, 2011 at 4:36 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

That's an intriguing possibility. But part of the point of the
original feature was to be able to say:

\ef somefunc 10

...and end up on line 10 of somefunc, perhaps in response to an error
message complaining about that line. I don't think your proposal
would address that.

Well, you'd write

\ef somefunc +10

instead.

But that would not put you on line 10 of the function.

Right. It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

regards, tom lane

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#2)
Re: about EDITOR_LINENUMBER_SWITCH

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, May 21, 2011 at 5:47 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.
This is perhaps sort of a Freudian slip.

It's probably the result of drift between the original patch and what
was eventually committed. IIRC, Pavel had it as an environment
variable originally, but Tom and I didn't feel the feature was
important enough to merit that treatment.

BTW, the above is merest historical revisionism: there was never a
version of the patch that did it that way. AFAICS the idea started
here:
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00089.php
to which you immediately asked whether it should be an environmental
variable, and I said no on what might be considered thin grounds:
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00182.php

I can't see any real objection other than complexity to having it look
for a psql variable and then an environment variable. Or we could drop
the psql variable part of that, if it seems too complicated.

Also, while we're on the subject, I'm not real sure why we don't allow
the code to provide a default value when EDITOR has a well-known value
like "vi" or "emacs". As long as there is a way to override that,
where's the harm in a default?

regards, tom lane

#9Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#8)
Re: about EDITOR_LINENUMBER_SWITCH

On Tue, May 24, 2011 at 5:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, May 21, 2011 at 5:47 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

I noticed the 9.1 release notes claim that the new
EDITOR_LINENUMBER_SWITCH thing is an environment variable, whereas it is
actually a psql variable.
This is perhaps sort of a Freudian slip.

It's probably the result of drift between the original patch and what
was eventually committed.  IIRC, Pavel had it as an environment
variable originally, but Tom and I didn't feel the feature was
important enough to merit that treatment.

BTW, the above is merest historical revisionism: there was never a
version of the patch that did it that way.

Even if you were correct, that's a snarky way to put it, and the point
is trivial anyway. But I don't think I'm imagining the getenv() call
in this version of the patch:

http://archives.postgresql.org/pgsql-hackers/2010-07/msg01253.php

Also, while we're on the subject, I'm not real sure why we don't allow
the code to provide a default value when EDITOR has a well-known value
like "vi" or "emacs".  As long as there is a way to override that,
where's the harm in a default?

Well, the question is how many people it'll help. Some people might
have a full pathname, others might called it vim...

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

#10Alvaro Herrera
alvherre@commandprompt.com
In reply to: Tom Lane (#7)
Re: about EDITOR_LINENUMBER_SWITCH

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right. It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such? Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#10)
Re: about EDITOR_LINENUMBER_SWITCH

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right. It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such? Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

1. I thought we already did have that.

2. In any case, EDITOR_LINENUMBER_SWITCH isn't a hazard for this,
because older versions will just think it's a variable without any
special meaning.

But the real question here is whether we want to change it to be also
(or instead?) an environment variable.

regards, tom lane

#12Alvaro Herrera
alvherre@commandprompt.com
In reply to: Tom Lane (#11)
Re: about EDITOR_LINENUMBER_SWITCH

Excerpts from Tom Lane's message of mié may 25 16:07:55 -0400 2011:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right. It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such? Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

1. I thought we already did have that.

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

2. In any case, EDITOR_LINENUMBER_SWITCH isn't a hazard for this,
because older versions will just think it's a variable without any
special meaning.

Good point.

But the real question here is whether we want to change it to be also
(or instead?) an environment variable.

I vote yes.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#13Peter Eisentraut
peter_e@gmx.net
In reply to: Robert Haas (#9)
1 attachment(s)
Re: about EDITOR_LINENUMBER_SWITCH

Here's a patch to fix what has been discussed:

* Change EDITOR_LINENUMBER_SWITCH to environment variable.
* I also changed "switch" to "arg" because "switch" is a bit of a
sloppy term.
* So the environment variable is called
PSQL_EDITOR_LINENUMBER_ARG.
* Set "+" as hardcoded default value on Unix (since "vi" is the
hardcoded default editor), so many users won't have to configure
this at all.
* I moved the documentation around a bit to centralize the editor
configuration under environment variables, rather than repeating
bits of it under every backslash command that invoked an editor.

Attachments:

psql-linenumber-arg.patchtext/x-patch; charset=UTF-8; name=psql-linenumber-arg.patchDownload
*** i/doc/src/sgml/ref/psql-ref.sgml
--- w/doc/src/sgml/ref/psql-ref.sgml
***************
*** 1440,1464 **** testdb=&gt;
          <literal>\r</> to cancel.
          </para>
  
-         <tip>
          <para>
!         <application>psql</application> checks the environment
!         variables <envar>PSQL_EDITOR</envar>, <envar>EDITOR</envar>, and
!         <envar>VISUAL</envar> (in that order) for an editor to use. If
!         all of them are unset, <filename>vi</filename> is used on Unix
!         systems, <filename>notepad.exe</filename> on Windows systems.
          </para>
-         </tip>
  
          <para>
!         If a line number is specified, <application>psql</application> will
!         position the cursor on the specified line of the file or query buffer.
!         This feature requires the <varname>EDITOR_LINENUMBER_SWITCH</varname>
!         variable to be set, so that <application>psql</application> knows how
!         to specify the line number to the editor.  Note that if a single
!         all-digits argument is given, <application>psql</application> assumes
!         it is a line number not a file name.
          </para>
          </listitem>
        </varlistentry>
  
--- 1440,1460 ----
          <literal>\r</> to cancel.
          </para>
  
          <para>
!         If a line number is specified, <application>psql</application> will
!         position the cursor on the specified line of the file or query buffer.
!         Note that if a single all-digits argument is given,
!         <application>psql</application> assumes it is a line number
!         not a file name.
          </para>
  
+         <tip>
          <para>
!         See under <xref linkend="app-psql-environment"
!         endterm="app-psql-environment-title"> for how to configure and
!         customize your editor.
          </para>
+         </tip>
          </listitem>
        </varlistentry>
  
***************
*** 1514,1526 **** Tue Oct 26 21:40:57 CEST 1999
  
          <para>
          If a line number is specified, <application>psql</application> will
!         position the cursor on the specified line of the function body
!         (note that the function body typically does not begin on the
!         first line of the file).
!         This feature requires the <varname>EDITOR_LINENUMBER_SWITCH</varname>
!         variable to be set, so that <application>psql</application> knows how
!         to specify the line number to the editor.
          </para>
          </listitem>
        </varlistentry>
  
--- 1510,1527 ----
  
          <para>
          If a line number is specified, <application>psql</application> will
!         position the cursor on the specified line of the function body.
!         (Note that the function body typically does not begin on the first
!         line of the file.)
!         </para>
! 
!         <tip>
!         <para>
!         See under <xref linkend="app-psql-environment"
!         endterm="app-psql-environment-title"> for how to configure and
!         customize your editor.
          </para>
+         </tip>
          </listitem>
        </varlistentry>
  
***************
*** 2599,2625 **** bar
        </varlistentry>
  
        <varlistentry>
-         <term><varname>EDITOR_LINENUMBER_SWITCH</varname></term>
-         <listitem>
-         <para>
-         When <command>\edit</command> or <command>\ef</command> is used with a
-         line number argument, this variable specifies the command-line switch
-         used to pass the line number to the user's editor.  For editors such
-         as <productname>emacs</> or <productname>vi</>, you can simply set
-         this variable to a plus sign.  Include a trailing space in the value
-         of the variable if there needs to be space between the switch name and
-         the line number.
-         Examples:
- 
- <programlisting>
- \set EDITOR_LINENUMBER_SWITCH +
- \set EDITOR_LINENUMBER_SWITCH '--line '
- </programlisting>
-         </para>
-         </listitem>
-       </varlistentry>
- 
-       <varlistentry>
          <term><varname>ENCODING</varname></term>
          <listitem>
          <para>
--- 2600,2605 ----
***************
*** 3167,3174 **** $endif
   </refsect1>
  
  
!  <refsect1>
!   <title>Environment</title>
  
    <variablelist>
  
--- 3147,3154 ----
   </refsect1>
  
  
!  <refsect1 id="app-psql-environment">
!   <title id="app-psql-environment-title">Environment</title>
  
    <variablelist>
  
***************
*** 3218,3225 **** $endif
  
      <listitem>
       <para>
!       Editor used by the <command>\e</command> command.  The variables
!       are examined in the order listed; the first that is set is used.
       </para>
      </listitem>
     </varlistentry>
--- 3198,3238 ----
  
      <listitem>
       <para>
!       Editor used by the <command>\e</command> and
!       <command>\ef</command> commands.  The variables are examined in
!       the order listed; the first that is set is used.
!      </para>
! 
!      <para>
!       The built-in default editors are <filename>vi</filename> on Unix
!       systems and <filename>notepad.exe</filename> on Windows systems.
!      </para>
!     </listitem>
!    </varlistentry>
! 
!    <varlistentry>
!     <term><envar>PSQL_EDITOR_LINENUMBER_ARG</envar></term>
! 
!     <listitem>
!      <para>
!       When <command>\e</command> or <command>\ef</command> is used
!       with a line number argument, this variable specifies the
!       command-line argument used to pass the starting line number to
!       the user's editor.  For editors such as <productname>Emacs</> or
!       <productname>vi</>, this is a plus sign.  Include a trailing
!       space in the value of the variable if there needs to be space
!       between the option name and the line number.  Examples:
! <programlisting>
! PSQL_EDITOR_LINENUMBER_ARG='+'
! PSQL_EDITOR_LINENUMBER_ARG='--line '
! </programlisting>
!      </para>
! 
!      <para>
!       The default is <literal>+</literal> on Unix systems
!       (corresponding to the default editor <filename>vi</filename>,
!       and useful for many other common editors); but there is no
!       default on Windows systems.
       </para>
      </listitem>
     </varlistentry>
*** i/doc/src/sgml/release-9.1.sgml
--- w/doc/src/sgml/release-9.1.sgml
***************
*** 1994,2000 ****
  
         <para>
          This is passed to the editor according to the
!         <envar>EDITOR_LINENUMBER_SWITCH</> psql variable.
         </para>
        </listitem>
  
--- 1994,2000 ----
  
         <para>
          This is passed to the editor according to the
!         <envar>PSQL_EDITOR_LINENUMBER_ARG</> environment variable.
         </para>
        </listitem>
  
*** i/src/bin/psql/command.c
--- w/src/bin/psql/command.c
***************
*** 1753,1759 **** static bool
  editFile(const char *fname, int lineno)
  {
  	const char *editorName;
! 	const char *editor_lineno_switch = NULL;
  	char	   *sys;
  	int			result;
  
--- 1753,1759 ----
  editFile(const char *fname, int lineno)
  {
  	const char *editorName;
! 	const char *editor_lineno_arg = NULL;
  	char	   *sys;
  	int			result;
  
***************
*** 1768,1781 **** editFile(const char *fname, int lineno)
  	if (!editorName)
  		editorName = DEFAULT_EDITOR;
  
! 	/* Get line number switch, if we need it. */
  	if (lineno > 0)
  	{
! 		editor_lineno_switch = GetVariable(pset.vars,
! 										   "EDITOR_LINENUMBER_SWITCH");
! 		if (editor_lineno_switch == NULL)
  		{
! 			psql_error("EDITOR_LINENUMBER_SWITCH variable must be set to specify a line number\n");
  			return false;
  		}
  	}
--- 1768,1784 ----
  	if (!editorName)
  		editorName = DEFAULT_EDITOR;
  
! 	/* Get line number argument, if we need it. */
  	if (lineno > 0)
  	{
! 		editor_lineno_arg = getenv("PSQL_EDITOR_LINENUMBER_ARG");
! #ifdef DEFAULT_EDITOR_LINENUMBER_ARG
! 		if (!editor_lineno_arg)
! 			editor_lineno_arg = DEFAULT_EDITOR_LINENUMBER_ARG;
! #endif
! 		if (!editor_lineno_arg)
  		{
! 			psql_error("environment variable PSQL_EDITOR_LINENUMBER_ARG must be set to specify a line number\n");
  			return false;
  		}
  	}
***************
*** 1783,1789 **** editFile(const char *fname, int lineno)
  	/* Allocate sufficient memory for command line. */
  	if (lineno > 0)
  		sys = pg_malloc(strlen(editorName)
! 						+ strlen(editor_lineno_switch) + 10		/* for integer */
  						+ 1 + strlen(fname) + 10 + 1);
  	else
  		sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
--- 1786,1792 ----
  	/* Allocate sufficient memory for command line. */
  	if (lineno > 0)
  		sys = pg_malloc(strlen(editorName)
! 						+ strlen(editor_lineno_arg) + 10		/* for integer */
  						+ 1 + strlen(fname) + 10 + 1);
  	else
  		sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
***************
*** 1798,1804 **** editFile(const char *fname, int lineno)
  #ifndef WIN32
  	if (lineno > 0)
  		sprintf(sys, "exec %s %s%d '%s'",
! 				editorName, editor_lineno_switch, lineno, fname);
  	else
  		sprintf(sys, "exec %s '%s'",
  				editorName, fname);
--- 1801,1807 ----
  #ifndef WIN32
  	if (lineno > 0)
  		sprintf(sys, "exec %s %s%d '%s'",
! 				editorName, editor_lineno_arg, lineno, fname);
  	else
  		sprintf(sys, "exec %s '%s'",
  				editorName, fname);
*** i/src/bin/psql/settings.h
--- w/src/bin/psql/settings.h
***************
*** 18,25 ****
--- 18,27 ----
  
  #if defined(WIN32) || defined(__CYGWIN__)
  #define DEFAULT_EDITOR	"notepad.exe"
+ /* no DEFAULT_EDITOR_LINENUMBER_ARG for Notepad */
  #else
  #define DEFAULT_EDITOR	"vi"
+ #define DEFAULT_EDITOR_LINENUMBER_ARG "+"
  #endif
  
  #define DEFAULT_PROMPT1 "%/%R%# "
#14Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#12)
1 attachment(s)
Re: about EDITOR_LINENUMBER_SWITCH

Alvaro Herrera wrote:

Excerpts from Tom Lane's message of mi�� may 25 16:07:55 -0400 2011:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right. It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such? Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

1. I thought we already did have that.

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

True. We don't add configuration changes in minor versions so having
minor-version granularity makes no sense.

The attached patch changes this to use the _major_ version number for
psql rc files. Does this have to be backward-compatible? Should I
check for minor and major matches? That is going to be confusing to
document.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Attachments:

/rtmp/psqltext/x-diffDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 662eab7..1ea728b
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** PSQL_EDITOR_LINENUMBER_ARG='--line '
*** 3332,3338 ****
       Both the system-wide <filename>psqlrc</filename> file and the user's
       <filename>~/.psqlrc</filename> file can be made version-specific
       by appending a dash and the <productname>PostgreSQL</productname>
!      release number, for example <filename>~/.psqlrc-&version;</filename>.
       A matching version-specific file will be read in preference to a
       non-version-specific file.
      </para>
--- 3332,3338 ----
       Both the system-wide <filename>psqlrc</filename> file and the user's
       <filename>~/.psqlrc</filename> file can be made version-specific
       by appending a dash and the <productname>PostgreSQL</productname>
!      major release number, for example <filename>~/.psqlrc-9.2</filename>.
       A matching version-specific file will be read in preference to a
       non-version-specific file.
      </para>
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 3c17eec..9670b7e
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** process_psqlrc_file(char *filename)
*** 600,607 ****
  #define R_OK 4
  #endif
  
! 	psqlrc = pg_malloc(strlen(filename) + 1 + strlen(PG_VERSION) + 1);
! 	sprintf(psqlrc, "%s-%s", filename, PG_VERSION);
  
  	if (access(psqlrc, R_OK) == 0)
  		(void) process_file(psqlrc, false, false);
--- 600,607 ----
  #define R_OK 4
  #endif
  
! 	psqlrc = pg_malloc(strlen(filename) + 1 + strlen(PG_MAJORVERSION) + 1);
! 	sprintf(psqlrc, "%s-%s", filename, PG_MAJORVERSION);
  
  	if (access(psqlrc, R_OK) == 0)
  		(void) process_file(psqlrc, false, false);
#15Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#14)
Re: about EDITOR_LINENUMBER_SWITCH

On Thu, Oct 13, 2011 at 11:31 AM, Bruce Momjian <bruce@momjian.us> wrote:

Alvaro Herrera wrote:

Excerpts from Tom Lane's message of mié may 25 16:07:55 -0400 2011:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right.  It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature.  However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable.  I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such?  Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

1. I thought we already did have that.

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

True.  We don't add configuration changes in minor versions so having
minor-version granularity makes no sense.

The attached patch changes this to use the _major_ version number for
psql rc files.  Does this have to be backward-compatible?  Should I
check for minor and major matches?  That is going to be confusing to
document.

Checking for a minor match and then a major match seems sensible.

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

#16Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#15)
1 attachment(s)
Re: about EDITOR_LINENUMBER_SWITCH

Robert Haas wrote:

On Thu, Oct 13, 2011 at 11:31 AM, Bruce Momjian <bruce@momjian.us> wrote:

Alvaro Herrera wrote:

Excerpts from Tom Lane's message of mi? may 25 16:07:55 -0400 2011:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mar may 24 17:11:17 -0400 2011:

Right. ?It would also increase the cognitive load on the user to have
to remember the command-line go-to-line-number switch for his editor.
So I don't particularly want to redesign this feature. ?However, I can
see the possible value of letting EDITOR_LINENUMBER_SWITCH be set from
the same place that you set EDITOR, which would suggest that we allow
the value to come from an environment variable. ?I'm not sure whether
there is merit in allowing both that source and ~/.psqlrc, though
possibly for Windows users it might be easier if ~/.psqlrc worked.

If we're going to increase the number of options in .psqlrc that do not
work with older psql versions, can I please have .psqlrc-MAJORVERSION or
some such? ?Having 8.3's psql complain all the time because it doesn't
understand "linestyle" is annoying.

1. I thought we already did have that.

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

True. ?We don't add configuration changes in minor versions so having
minor-version granularity makes no sense.

The attached patch changes this to use the _major_ version number for
psql rc files. ?Does this have to be backward-compatible? ?Should I
check for minor and major matches? ?That is going to be confusing to
document.

Checking for a minor match and then a major match seems sensible.

Done, and documented in the attached patch.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Attachments:

/rtmp/psqltext/x-diffDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 662eab7..4eefe3b
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** PSQL_EDITOR_LINENUMBER_ARG='--line '
*** 3332,3339 ****
       Both the system-wide <filename>psqlrc</filename> file and the user's
       <filename>~/.psqlrc</filename> file can be made version-specific
       by appending a dash and the <productname>PostgreSQL</productname>
!      release number, for example <filename>~/.psqlrc-&version;</filename>.
!      A matching version-specific file will be read in preference to a
       non-version-specific file.
      </para>
     </listitem>
--- 3332,3341 ----
       Both the system-wide <filename>psqlrc</filename> file and the user's
       <filename>~/.psqlrc</filename> file can be made version-specific
       by appending a dash and the <productname>PostgreSQL</productname>
!      major or minor release number, for example
!      <filename>~/.psqlrc-9.2</filename> or
!      <filename>~/.psqlrc-9.2.5</filename>.  The most specific
!      version-matching file will be read in preference to a
       non-version-specific file.
      </para>
     </listitem>
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 3c17eec..71829eb
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** process_psqlrc(char *argv0)
*** 594,613 ****
  static void
  process_psqlrc_file(char *filename)
  {
! 	char	   *psqlrc;
  
  #if defined(WIN32) && (!defined(__MINGW32__))
  #define R_OK 4
  #endif
  
! 	psqlrc = pg_malloc(strlen(filename) + 1 + strlen(PG_VERSION) + 1);
! 	sprintf(psqlrc, "%s-%s", filename, PG_VERSION);
  
! 	if (access(psqlrc, R_OK) == 0)
! 		(void) process_file(psqlrc, false, false);
  	else if (access(filename, R_OK) == 0)
  		(void) process_file(filename, false, false);
! 	free(psqlrc);
  }
  
  
--- 594,620 ----
  static void
  process_psqlrc_file(char *filename)
  {
! 	char	   *psqlrc_minor, *psqlrc_major;
  
  #if defined(WIN32) && (!defined(__MINGW32__))
  #define R_OK 4
  #endif
  
! 	psqlrc_minor = pg_malloc(strlen(filename) + 1 + strlen(PG_VERSION) + 1);
! 	sprintf(psqlrc_minor, "%s-%s", filename, PG_VERSION);
! 	psqlrc_major = pg_malloc(strlen(filename) + 1 + strlen(PG_MAJORVERSION) + 1);
! 	sprintf(psqlrc_major, "%s-%s", filename, PG_MAJORVERSION);
  
! 	/* check for minor version first, then major, then no version */
! 	if (access(psqlrc_minor, R_OK) == 0)
! 		(void) process_file(psqlrc_minor, false, false);
! 	else if (access(psqlrc_major, R_OK) == 0)
! 		(void) process_file(psqlrc_major, false, false);
  	else if (access(filename, R_OK) == 0)
  		(void) process_file(filename, false, false);
! 
! 	free(psqlrc_minor);
! 	free(psqlrc_major);
  }
  
  
#17Alvaro Herrera
alvherre@commandprompt.com
In reply to: Robert Haas (#15)
Re: about EDITOR_LINENUMBER_SWITCH

Excerpts from Robert Haas's message of vie oct 14 09:36:47 -0300 2011:

On Thu, Oct 13, 2011 at 11:31 AM, Bruce Momjian <bruce@momjian.us> wrote:

Alvaro Herrera wrote:

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

True.  We don't add configuration changes in minor versions so having
minor-version granularity makes no sense.

The attached patch changes this to use the _major_ version number for
psql rc files.  Does this have to be backward-compatible?  Should I
check for minor and major matches?  That is going to be confusing to
document.

Checking for a minor match and then a major match seems sensible.

And backwards compatible too! +1 to that. An idea that you can
describe in six words doesn't seem all that confusing.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#18Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#17)
Re: about EDITOR_LINENUMBER_SWITCH

Alvaro Herrera wrote:

Excerpts from Robert Haas's message of vie oct 14 09:36:47 -0300 2011:

On Thu, Oct 13, 2011 at 11:31 AM, Bruce Momjian <bruce@momjian.us> wrote:

Alvaro Herrera wrote:

Oh, true, we have that, though it's not very usable because you have to
rename the file from .psqlrc-9.0.3 to .psqlrc-9.0.4 when you upgrade,
which is kinda silly.

True. ��We don't add configuration changes in minor versions so having
minor-version granularity makes no sense.

The attached patch changes this to use the _major_ version number for
psql rc files. ��Does this have to be backward-compatible? ��Should I
check for minor and major matches? ��That is going to be confusing to
document.

Checking for a minor match and then a major match seems sensible.

And backwards compatible too! +1 to that. An idea that you can
describe in six words doesn't seem all that confusing.

Oops, I see a problem. Right now, our first major release has no zero,
e.g. 9.2, while our minors have a third digit, 9.2.5. The problem is
that with this patch it is confusing to have a psql config file that
matches 9.2.0, but not 9.2.5, because you can't write 9.2.0. A file
called .psql-9.2 matches 9.2.0, but also matches 9.2.X if there is no
matching minor release file. The bottom line is that with this patch,
.psql-9.2 is both a minor and possibly minor matcher. I can't blame the
patch, but rather our version numbering system.

Prior to the patch 9.2 always meant just 9.2.0. This patch adds an
additional confusion.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#19Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#18)
Re: about EDITOR_LINENUMBER_SWITCH

Bruce Momjian <bruce@momjian.us> writes:

Oops, I see a problem. Right now, our first major release has no zero,
e.g. 9.2, while our minors have a third digit, 9.2.5. The problem is
that with this patch it is confusing to have a psql config file that
matches 9.2.0, but not 9.2.5, because you can't write 9.2.0.

Uh, this seems like nonsense. We've been labeling major releases with
a dot-zero for some time, and that's embedded in process now (cf
version_stamp.pl) to the point that we're unlikely to forget to do so.

regards, tom lane

#20Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#19)
Re: about EDITOR_LINENUMBER_SWITCH

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Oops, I see a problem. Right now, our first major release has no zero,
e.g. 9.2, while our minors have a third digit, 9.2.5. The problem is
that with this patch it is confusing to have a psql config file that
matches 9.2.0, but not 9.2.5, because you can't write 9.2.0.

Uh, this seems like nonsense. We've been labeling major releases with
a dot-zero for some time, and that's embedded in process now (cf
version_stamp.pl) to the point that we're unlikely to forget to do so.

Ah, I see now. 9.1 has:

#define PG_VERSION "9.1.0"

I rarely see the non-dev trees. No problem then.

Patch applied.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#21Peter Eisentraut
peter_e@gmx.net
In reply to: Bruce Momjian (#14)
Re: about EDITOR_LINENUMBER_SWITCH

On tor, 2011-10-13 at 11:31 -0400, Bruce Momjian wrote:

The attached patch changes this to use the _major_ version number for
psql rc files. Does this have to be backward-compatible? Should I
check for minor and major matches? That is going to be confusing to
document.

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

#22Andrew Dunstan
andrew@dunslane.net
In reply to: Peter Eisentraut (#21)
Re: about EDITOR_LINENUMBER_SWITCH

On 10/15/2011 09:37 AM, Peter Eisentraut wrote:

On tor, 2011-10-13 at 11:31 -0400, Bruce Momjian wrote:

The attached patch changes this to use the _major_ version number for
psql rc files. Does this have to be backward-compatible? Should I
check for minor and major matches? That is going to be confusing to
document.

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

Wouldn't it be better to support some conditional syntax?

cheers

andrew

#23Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#21)
Re: about EDITOR_LINENUMBER_SWITCH

Peter Eisentraut wrote:

On tor, 2011-10-13 at 11:31 -0400, Bruce Momjian wrote:

The attached patch changes this to use the _major_ version number for
psql rc files. Does this have to be backward-compatible? Should I
check for minor and major matches? That is going to be confusing to
document.

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

Well, frankly, I think the fact we were matching on minor version number
was even worse. This is slightly better. I guess they could use
symlinks to keep a config file for multiple versions, but I agree it
isn't a long-term great solution.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#24Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#22)
Re: about EDITOR_LINENUMBER_SWITCH

Andrew Dunstan wrote:

On 10/15/2011 09:37 AM, Peter Eisentraut wrote:

On tor, 2011-10-13 at 11:31 -0400, Bruce Momjian wrote:

The attached patch changes this to use the _major_ version number for
psql rc files. Does this have to be backward-compatible? Should I
check for minor and major matches? That is going to be confusing to
document.

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

Wouldn't it be better to support some conditional syntax?

I suppose if we add that to psql we can remove this factility
completely.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#21)
.psqlrc version dependence (was Re: about EDITOR_LINENUMBER_SWITCH)

Peter Eisentraut <peter_e@gmx.net> writes:

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

Well, "in a few years" they won't need that conditionality any more at
all, so I'm not sure I believe the above argument. The problem seems
inherently self-limiting.

What struck me while looking at the patch is that it is conditional
based on *psql's* version. Not the version of the server you're
connected to. I'm not too sure what use-cases people have for version
dependence here, but I'd think that the server version would enter into
it sometimes.

(Of course, for server version to be used usefully, you'd need to
re-execute the rc file during \c, something we don't do now.)

regards, tom lane

#26Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#25)
Re: .psqlrc version dependence (was Re: about EDITOR_LINENUMBER_SWITCH)

Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Contrary to what the subject suggests, I think the main reason people
wanted this feature was to be able to set the linestyle to unicode
without getting a warning from older releases about unknown linestyle or
something. But in a few years, they'll have to
maintain .psqlrc-9.2, .psqlrc-9.3, .psqlrc-9.4, etc. That doesn't sound
like a useful long-term solution either.

Well, "in a few years" they won't need that conditionality any more at
all, so I'm not sure I believe the above argument. The problem seems
inherently self-limiting.

What struck me while looking at the patch is that it is conditional
based on *psql's* version. Not the version of the server you're
connected to. I'm not too sure what use-cases people have for version
dependence here, but I'd think that the server version would enter into
it sometimes.

The assumption is that the .psqlrc is controlling psql behavior. Not
sure what setting would be changed based on server version, maybe psql
variables.

I have updated the docs to indicate it is the psql version.

(Of course, for server version to be used usefully, you'd need to
re-execute the rc file during \c, something we don't do now.)

Yep, yuck.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +