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

1 files changed, 46 insertions, 36 deletions

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0c164a339c1..f17f6107175 100644
--- a/src/bin/psql/command.c
+++ b/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;
 	}