Make psql reject attempts to set special variables to invalid values.

Previously, if the user set a special variable such as ECHO to an
unrecognized value, psql would bleat but store the new value anyway, and
then fall back to a default setting for the behavior controlled by the
variable.  This was agreed to be a not particularly good idea.  With
this patch, invalid values result in an error message and no change in
state.

(But this applies only to variables that affect psql's behavior; purely
informational variables such as ENCODING can still be set to random
values.)

To do this, modify the API for psql's assign-hook functions so that they
can return an OK/not OK result, and give them the responsibility for
printing error messages when they reject a value.  Adjust the APIs for
ParseVariableBool and ParseVariableNum to support the new behavior
conveniently.

In passing, document the variable VERSION, which had somehow escaped that.
And improve the quite-inadequate commenting in psql/variables.c.

Daniel Vérité, reviewed by Rahila Syed, some further tweaking by me

Discussion: https://postgr.es/m/7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm

Author: tglsfdc

doc/src/sgml/ref/psql-ref.sgml

@@ -3078,10 +3078,8 @@ bar
     by <application>psql</application>. They represent certain option
     settings that can be changed at run time by altering the value of
     the variable, or in some cases represent changeable state of
-    <application>psql</application>. Although
-    you can use these variables for other purposes, this is not
-    recommended, as the program behavior might grow really strange
-    really quickly. By convention, all specially treated variables' names
+    <application>psql</application>.
+    By convention, all specially treated variables' names
     consist of all upper-case ASCII letters (and possibly digits and
     underscores). To ensure maximum compatibility in the future, avoid
     using such variable names for your own purposes. A list of all specially
@@ -3170,12 +3168,11 @@ bar
         start-up, use the switch <option>-a</option>. If set to
         <literal>queries</literal>,
         <application>psql</application> prints each query to standard output
-        as it is sent to the server. The switch for this is
+        as it is sent to the server. The switch to select this behavior is
         <option>-e</option>. If set to <literal>errors</literal>, then only
         failed queries are displayed on standard error output. The switch
-        for this is <option>-b</option>. If unset, or if set to
-        <literal>none</literal> (or any other value than those above) then
-        no queries are displayed.
+        for this behavior is <option>-b</option>. If unset, or if set to
+        <literal>none</literal>, then no queries are displayed.
         </para>
         </listitem>
       </varlistentry>
@@ -3201,6 +3198,9 @@ bar
         <listitem>
         <para>
         The current client character set encoding.
+        This is set every time you connect to a database (including
+        program start-up), and when you change the encoding
+        with <literal>\encoding</>, but it can be unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3241,9 +3241,8 @@ bar
          list. If set to a value of <literal>ignoredups</literal>, lines
          matching the previous history line are not entered. A value of
          <literal>ignoreboth</literal> combines the two options. If
-         unset, or if set to <literal>none</literal> (or any other value
-         than those above), all lines read in interactive mode are
-         saved on the history list.
+         unset, or if set to <literal>none</literal> (the default), all lines
+         read in interactive mode are saved on the history list.
         </para>
         <note>
         <para>
@@ -3312,7 +3311,7 @@ bar
          to an interactive session of <application>psql</application>
          will terminate the application. If set to a numeric value,
          that many <acronym>EOF</> characters are ignored before the
-         application terminates.  If the variable is set but has no
+         application terminates.  If the variable is set but not to a
          numeric value, the default is 10.
         </para>
         <note>
@@ -3477,6 +3476,16 @@ bar
         </listitem>
       </varlistentry>
 
+      <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 unset or changed.
+        </para>
+        </listitem>
+      </varlistentry>
+
     </variablelist>
 
    </refsect3>

src/bin/psql/command.c

@@ -248,31 +248,37 @@ exec_command(const char *cmd,
 				   *opt2,
 				   *opt3,
 				   *opt4;
-		enum trivalue reuse_previous;
+		enum trivalue reuse_previous = TRI_DEFAULT;
 
 		opt1 = read_connect_arg(scan_state);
 		if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) == 0)
 		{
-			reuse_previous =
-				ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix) ?
-				TRI_YES : TRI_NO;
+			bool		on_off;
 
-			free(opt1);
-			opt1 = read_connect_arg(scan_state);
+			success = ParseVariableBool(opt1 + sizeof(prefix) - 1,
+										"-reuse-previous",
+										&on_off);
+			if (success)
+			{
+				reuse_previous = on_off ? TRI_YES : TRI_NO;
+				free(opt1);
+				opt1 = read_connect_arg(scan_state);
+			}
 		}
-		else
-			reuse_previous = TRI_DEFAULT;
 
-		opt2 = read_connect_arg(scan_state);
-		opt3 = read_connect_arg(scan_state);
-		opt4 = read_connect_arg(scan_state);
+		if (success)			/* give up if reuse_previous was invalid */
+		{
+			opt2 = read_connect_arg(scan_state);
+			opt3 = read_connect_arg(scan_state);
+			opt4 = read_connect_arg(scan_state);
 
-		success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+			success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
 
+			free(opt2);
+			free(opt3);
+			free(opt4);
+		}
 		free(opt1);
-		free(opt2);
-		free(opt3);
-		free(opt4);
 	}
 
 	/* \cd */
@@ -1208,10 +1214,7 @@ exec_command(const char *cmd,
 
 			if (result &&
 				!SetVariable(pset.vars, opt, result))
-			{
-				psql_error("\\%s: error while setting variable\n", cmd);
 				success = false;
-			}
 
 			if (result)
 				free(result);
@@ -1325,10 +1328,8 @@ exec_command(const char *cmd,
 			}
 
 			if (!SetVariable(pset.vars, opt0, newval))
-			{
-				psql_error("\\%s: error while setting variable\n", cmd);
 				success = false;
-			}
+
 			free(newval);
 		}
 		free(opt0);
@@ -1564,7 +1565,7 @@ exec_command(const char *cmd,
 												 OT_NORMAL, NULL, false);
 
 		if (opt)
-			pset.timing = ParseVariableBool(opt, "\\timing");
+			success = ParseVariableBool(opt, "\\timing", &pset.timing);
 		else
 			pset.timing = !pset.timing;
 		if (!pset.quiet)
@@ -1589,10 +1590,8 @@ exec_command(const char *cmd,
 			success = false;
 		}
 		else if (!SetVariable(pset.vars, opt, NULL))
-		{
-			psql_error("\\%s: error while setting variable\n", cmd);
 			success = false;
-		}
+
 		free(opt);
 	}
 
@@ -2593,7 +2592,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			psql_error("\\pset: allowed formats are unaligned, aligned, wrapped, html, asciidoc, latex, latex-longtable, troff-ms\n");
 			return false;
 		}
-
 	}
 
 	/* set table line style */
@@ -2612,7 +2610,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			psql_error("\\pset: allowed line styles are ascii, old-ascii, unicode\n");
 			return false;
 		}
-
 	}
 
 	/* set unicode border line style */
@@ -2665,7 +2662,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	{
 		if (value)
 			popt->topt.border = atoi(value);
-
 	}
 
 	/* set expanded/vertical mode */
@@ -2676,7 +2672,17 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 		if (value && pg_strcasecmp(value, "auto") == 0)
 			popt->topt.expanded = 2;
 		else if (value)
-			popt->topt.expanded = ParseVariableBool(value, param);
+		{
+			bool		on_off;
+
+			if (ParseVariableBool(value, NULL, &on_off))
+				popt->topt.expanded = on_off ? 1 : 0;
+			else
+			{
+				PsqlVarEnumError(param, value, "on, off, auto");
+				return false;
+			}
+		}
 		else
 			popt->topt.expanded = !popt->topt.expanded;
 	}
@@ -2685,7 +2691,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	else if (strcmp(param, "numericlocale") == 0)
 	{
 		if (value)
-			popt->topt.numericLocale = ParseVariableBool(value, param);
+			return ParseVariableBool(value, param, &popt->topt.numericLocale);
 		else
 			popt->topt.numericLocale = !popt->topt.numericLocale;
 	}
@@ -2740,7 +2746,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
 	{
 		if (value)
-			popt->topt.tuples_only = ParseVariableBool(value, param);
+			return ParseVariableBool(value, param, &popt->topt.tuples_only);
 		else
 			popt->topt.tuples_only = !popt->topt.tuples_only;
 	}
@@ -2772,10 +2778,14 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.pager = 2;
 		else if (value)
 		{
-			if (ParseVariableBool(value, param))
-				popt->topt.pager = 1;
-			else
-				popt->topt.pager = 0;
+			bool		on_off;
+
+			if (!ParseVariableBool(value, NULL, &on_off))
+			{
+				PsqlVarEnumError(param, value, "on, off, always");
+				return false;
+			}
+			popt->topt.pager = on_off ? 1 : 0;
 		}
 		else if (popt->topt.pager == 1)
 			popt->topt.pager = 0;
@@ -2794,7 +2804,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	else if (strcmp(param, "footer") == 0)
 	{
 		if (value)
-			popt->topt.default_footer = ParseVariableBool(value, param);
+			return ParseVariableBool(value, param, &popt->topt.default_footer);
 		else
 			popt->topt.default_footer = !popt->topt.default_footer;
 	}

src/bin/psql/common.c

@@ -841,7 +841,6 @@ StoreQueryTuple(const PGresult *result)
 
 			if (!SetVariable(pset.vars, varname, value))
 			{
-				psql_error("could not set variable \"%s\"\n", varname);
 				free(varname);
 				success = false;
 				break;

src/bin/psql/input.c

@@ -541,7 +541,7 @@ finishInput(void)
 	{
 		int			hist_size;
 
-		hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1, true);
+		hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1);
 		(void) saveHistory(psql_history, hist_size);
 		free(psql_history);
 		psql_history = NULL;

src/bin/psql/mainloop.c

@@ -162,7 +162,7 @@ MainLoop(FILE *source)
 				/* This tries to mimic bash's IGNOREEOF feature. */
 				count_eof++;
 
-				if (count_eof < GetVariableNum(pset.vars, "IGNOREEOF", 0, 10, false))
+				if (count_eof < GetVariableNum(pset.vars, "IGNOREEOF", 0, 10))
 				{
 					if (!pset.quiet)
 						printf(_("Use \"\\q\" to leave %s.\n"), pset.progname);