psql-var-hooks-v7.patch

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 9915731..042d277 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3213,9 +3213,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>, all lines read
+         in interactive mode are saved on the history list.
         </para>
         <note>
         <para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4139b77..091a138 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -253,26 +253,31 @@ exec_command(const char *cmd,
 		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;
-
-			free(opt1);
-			opt1 = read_connect_arg(scan_state);
+			bool on_off;
+			success = ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix, &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)			/* do not attempt to connect if reuse_previous argument 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 */
@@ -1548,7 +1553,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)
@@ -2660,7 +2665,16 @@ 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;
 	}
@@ -2669,7 +2683,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;
 	}
@@ -2724,7 +2738,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;
 	}
@@ -2756,10 +2770,13 @@ 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;
@@ -2778,7 +2795,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;
 	}
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 972bea4..3e3e97a 100644
--- a/src/bin/psql/input.c
+++ b/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;
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index bb306a4..dc25b4b 100644
--- a/src/bin/psql/mainloop.c
+++ b/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);
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 85aac4a..0b4f43e 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,71 @@ showVersion(void)
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
-static void
+
+static bool
 autocommit_hook(const char *newval)
 {
-	pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+	bool autocommit;
+	if (ParseVariableBool(newval, "AUTOCOMMIT", &autocommit))
+	{
+		/*
+		 * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
+		 * transaction, because it cannot be effective until the current
+		 * transaction is ended.
+		 */
+		if (autocommit && !pset.autocommit &&
+			pset.db && PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
+		{
+			psql_error("cannot set AUTOCOMMIT to %s when inside a transaction\n", newval);
+		}
+		else
+		{
+			pset.autocommit = autocommit;
+			return true;
+		}
+	}
+	return false;
 }
 
-static void
+static bool
 on_error_stop_hook(const char *newval)
 {
-	pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+	return ParseVariableBool(newval, "ON_ERROR_STOP", &pset.on_error_stop);
 }
 
-static void
+static bool
 quiet_hook(const char *newval)
 {
-	pset.quiet = ParseVariableBool(newval, "QUIET");
+	return ParseVariableBool(newval, "QUIET", &pset.quiet);
 }
 
-static void
+static bool
 singleline_hook(const char *newval)
 {
-	pset.singleline = ParseVariableBool(newval, "SINGLELINE");
+	return ParseVariableBool(newval, "SINGLELINE", &pset.singleline);
 }
 
-static void
+static bool
 singlestep_hook(const char *newval)
 {
-	pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
+	return ParseVariableBool(newval, "SINGLESTEP", &pset.singlestep);
 }
 
-static void
+static bool
 fetch_count_hook(const char *newval)
 {
-	pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
+	if (newval == NULL)
+		pset.fetch_count = -1;	/* default value */
+	else if (!ParseVariableNum(newval, &pset.fetch_count))
+	{
+		psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is expected\n",
+				   newval, "FETCH_COUNT");
+		return false;
+	}
+	return true;
 }
 
-static void
+static bool
 echo_hook(const char *newval)
 {
 	if (newval == NULL)
@@ -837,39 +865,55 @@ echo_hook(const char *newval)
 		pset.echo = PSQL_ECHO_NONE;
 	else
 	{
-		psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-				   newval, "ECHO", "none");
-		pset.echo = PSQL_ECHO_NONE;
+		PsqlVarEnumError("ECHO", newval, "none, errors, queries, all");
+		return false;
 	}
+	return true;
 }
 
-static void
+static bool
 echo_hidden_hook(const char *newval)
 {
 	if (newval == NULL)
 		pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
 	else if (pg_strcasecmp(newval, "noexec") == 0)
 		pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
-	else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
-		pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
-	else	/* ParseVariableBool printed msg if needed */
-		pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
+	else
+	{
+		bool on_off;
+		if (ParseVariableBool(newval, NULL, &on_off))
+			pset.echo_hidden = on_off ? PSQL_ECHO_HIDDEN_ON : PSQL_ECHO_HIDDEN_OFF;
+		else
+		{
+			PsqlVarEnumError("ECHO_HIDDEN", newval, "on, off, noexec");
+			return false;
+		}
+	}
+	return true;
 }
 
-static void
+static bool
 on_error_rollback_hook(const char *newval)
 {
 	if (newval == NULL)
 		pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
 	else if (pg_strcasecmp(newval, "interactive") == 0)
 		pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
-	else if (ParseVariableBool(newval, "ON_ERROR_ROLLBACK"))
-		pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
-	else	/* ParseVariableBool printed msg if needed */
-		pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
+	else
+	{
+		bool on_off;
+		if (ParseVariableBool(newval, NULL, &on_off))
+			pset.on_error_rollback = on_off ? PSQL_ERROR_ROLLBACK_ON : PSQL_ERROR_ROLLBACK_OFF;
+		else
+		{
+			PsqlVarEnumError("ON_ERROR_ROLLBACK", newval, "on, off, interactive");
+			return false;
+		}
+	}
+	return true;
 }
 
-static void
+static bool
 comp_keyword_case_hook(const char *newval)
 {
 	if (newval == NULL)
@@ -884,13 +928,14 @@ comp_keyword_case_hook(const char *newval)
 		pset.comp_case = PSQL_COMP_CASE_LOWER;
 	else
 	{
-		psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-				   newval, "COMP_KEYWORD_CASE", "preserve-upper");
-		pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
+		PsqlVarEnumError("COMP_KEYWORD_CASE", newval,
+						 "lower, upper, preserve-lower, preserve-upper");
+		return false;
 	}
+	return true;
 }
 
-static void
+static bool
 histcontrol_hook(const char *newval)
 {
 	if (newval == NULL)
@@ -905,31 +950,35 @@ histcontrol_hook(const char *newval)
 		pset.histcontrol = hctl_none;
 	else
 	{
-		psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-				   newval, "HISTCONTROL", "none");
-		pset.histcontrol = hctl_none;
+		PsqlVarEnumError("HISTCONTROL", newval,
+						 "none, ignorespace, ignoredups, ignoreboth");
+		return false;
 	}
+	return true;
 }
 
-static void
+static bool
 prompt1_hook(const char *newval)
 {
 	pset.prompt1 = newval ? newval : "";
+	return true;
 }
 
-static void
+static bool
 prompt2_hook(const char *newval)
 {
 	pset.prompt2 = newval ? newval : "";
+	return true;
 }
 
-static void
+static bool
 prompt3_hook(const char *newval)
 {
 	pset.prompt3 = newval ? newval : "";
+	return true;
 }
 
-static void
+static bool
 verbosity_hook(const char *newval)
 {
 	if (newval == NULL)
@@ -942,16 +991,16 @@ verbosity_hook(const char *newval)
 		pset.verbosity = PQERRORS_VERBOSE;
 	else
 	{
-		psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-				   newval, "VERBOSITY", "default");
-		pset.verbosity = PQERRORS_DEFAULT;
+		PsqlVarEnumError("VERBOSITY", newval, "default, terse, verbose");
+		return false;
 	}
 
 	if (pset.db)
 		PQsetErrorVerbosity(pset.db, pset.verbosity);
+	return true;
 }
 
-static void
+static bool
 show_context_hook(const char *newval)
 {
 	if (newval == NULL)
@@ -964,13 +1013,13 @@ show_context_hook(const char *newval)
 		pset.show_context = PQSHOW_CONTEXT_ALWAYS;
 	else
 	{
-		psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-				   newval, "SHOW_CONTEXT", "errors");
-		pset.show_context = PQSHOW_CONTEXT_ERRORS;
+		PsqlVarEnumError("SHOW_CONTEXT", newval, "never, errors, always");
+		return false;
 	}
 
 	if (pset.db)
 		PQsetErrorContextVisibility(pset.db, pset.show_context);
+	return true;
 }
 
 
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 2669572..d0de198 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -79,92 +79,103 @@ GetVariable(VariableSpace space, const char *name)
 }
 
 /*
- * Try to interpret "value" as boolean value.
+ * Try to interpret "value" as boolean value, and if successful,
+ * put it in *result. Otherwise don't clobber *result.
  *
  * Valid values are: true, false, yes, no, on, off, 1, 0; as well as unique
  * prefixes thereof.
  *
  * "name" is the name of the variable we're assigning to, to use in error
  * report if any.  Pass name == NULL to suppress the error report.
+ *
+ * Return true when "value" is syntactically valid, false otherwise.
  */
 bool
-ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *result)
 {
 	size_t		len;
+	bool		valid = true;
 
 	if (value == NULL)
-		return false;			/* not set -> assume "off" */
+	{
+		*result = false;			/* not set -> assume "off" */
+		return valid;
+	}
 
 	len = strlen(value);
 
-	if (pg_strncasecmp(value, "true", len) == 0)
-		return true;
-	else if (pg_strncasecmp(value, "false", len) == 0)
-		return false;
-	else if (pg_strncasecmp(value, "yes", len) == 0)
-		return true;
-	else if (pg_strncasecmp(value, "no", len) == 0)
-		return false;
+	if (len > 0 && pg_strncasecmp(value, "true", len) == 0)
+		*result = true;
+	else if (len > 0 && pg_strncasecmp(value, "false", len) == 0)
+		*result = false;
+	else if (len > 0 && pg_strncasecmp(value, "yes", len) == 0)
+		*result = true;
+	else if (len > 0 && pg_strncasecmp(value, "no", len) == 0)
+		*result = false;
 	/* 'o' is not unique enough */
 	else if (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0)
-		return true;
+		*result = true;
 	else if (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0)
-		return false;
+		*result = false;
 	else if (pg_strcasecmp(value, "1") == 0)
-		return true;
+		*result = true;
 	else if (pg_strcasecmp(value, "0") == 0)
-		return false;
+		*result = false;
 	else
 	{
-		/* NULL is treated as false, so a non-matching value is 'true' */
+		/*
+		 * The string is not recognized. We don't clobber *result in this case.
+		 */
 		if (name)
-			psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-					   value, name, "on");
-		return true;
+			psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n",
+					   value, name);
+		valid = false;
 	}
+	return valid;
 }
 
 
 /*
- * Read numeric variable, or defaultval if it is not set, or faultval if its
- * value is not a valid numeric string.  If allowtrail is false, this will
- * include the case where there are trailing characters after the number.
+ * Parse a numeric variable from its string representation.
+ * If the syntax is valid, return true and set *result to the value.
+ * Otherwise return false and leave *result unchanged.
  */
-int
-ParseVariableNum(const char *val,
-				 int defaultval,
-				 int faultval,
-				 bool allowtrail)
+bool
+ParseVariableNum(const char *val, int *result)
 {
-	int			result;
+	char	*end;
+	int		numval;
 
-	if (!val)
-		result = defaultval;
-	else if (!val[0])
-		result = faultval;
-	else
+	if (!val || !val[0])
+		return false;
+	errno = 0;
+	numval = strtol(val, &end, 0);
+	if (errno == 0 && *end == '\0')
 	{
-		char	   *end;
-
-		result = strtol(val, &end, 0);
-		if (!allowtrail && *end)
-			result = faultval;
+		*result = numval;
+		return true;
 	}
-
-	return result;
+	return false;
 }
 
+
 int
 GetVariableNum(VariableSpace space,
 			   const char *name,
 			   int defaultval,
-			   int faultval,
-			   bool allowtrail)
+			   int faultval)
 {
 	const char *val;
+	int result;
 
 	val = GetVariable(space, name);
-	return ParseVariableNum(val, defaultval, faultval, allowtrail);
+	if (!val)
+		return defaultval;
+
+	if (ParseVariableNum(val, &result))
+		return result;
+	else
+		return faultval;
 }
 
 void
@@ -205,13 +216,26 @@ SetVariable(VariableSpace space, const char *name, const char *value)
 	{
 		if (strcmp(current->name, name) == 0)
 		{
-			/* found entry, so update */
-			if (current->value)
-				free(current->value);
-			current->value = pg_strdup(value);
-			if (current->assign_hook)
-				(*current->assign_hook) (current->value);
-			return true;
+			/*
+			 * Found entry, so update, unless a hook returns false.
+			 * The hook needs the value in a buffer with the same lifespan
+			 * as the variable, so allocate it right away, even if it needs
+			 * to be freed just after when the hook returns false.
+			 */
+			char *new_value = pg_strdup(value);
+
+			bool confirmed = current->assign_hook ?
+							(*current->assign_hook) (new_value) : true;
+
+			if (confirmed)
+			{
+				pg_free(current->value);
+				current->value = new_value;
+			}
+			else
+				pg_free(new_value); /* and current->value is left unchanged */
+
+			return confirmed;
 		}
 	}
 
@@ -248,8 +272,7 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
 		{
 			/* found entry, so update */
 			current->assign_hook = hook;
-			(*hook) (current->value);
-			return true;
+			return (*hook) (current->value);
 		}
 	}
 
@@ -260,8 +283,7 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
 	current->assign_hook = hook;
 	current->next = NULL;
 	previous->next = current;
-	(*hook) (NULL);
-	return true;
+	return (*hook) (NULL);
 }
 
 bool
@@ -303,3 +325,14 @@ DeleteVariable(VariableSpace space, const char *name)
 
 	return true;
 }
+
+/*
+ * Emit error with suggestions for variables or commands
+ * accepting enum-style arguments.
+ */
+void
+PsqlVarEnumError(const char* name, const char* value, const char* suggestions)
+{
+	psql_error("Unrecognized value \"%s\" for \"%s\"\nAvailable values: %s\n",
+			   value, name, suggestions);
+}
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index d235b17..c7d99d1 100644
--- a/src/bin/psql/variables.h
+++ b/src/bin/psql/variables.h
@@ -12,15 +12,19 @@
  * This implements a sort of variable repository. One could also think of it
  * as a cheap version of an associative array. In each one of these
  * datastructures you can store name/value pairs.  There can also be an
- * "assign hook" function that is called whenever the variable's value is
- * changed.
+ * "assign hook" function that is called before the variable's value is
+ * changed, and that returns a boolean indicating whether the assignment
+ * is confirmed or must be cancelled.
  *
- * An "unset" operation causes the hook to be called with newval == NULL.
+ * The hook is called with newval == NULL when a variable is created
+ * along with it by SetVariableAssignHook().
+ * An "unset" operation also causes the  hook to be called with newval == NULL,
+ * and is not cancellable even if the hook returns false.
  *
  * Note: if value == NULL then the variable is logically unset, but we are
  * keeping the struct around so as not to forget about its hook function.
  */
-typedef void (*VariableAssignHook) (const char *newval);
+typedef bool (*VariableAssignHook) (const char *newval);
 
 struct _variable
 {
@@ -35,16 +39,15 @@ typedef struct _variable *VariableSpace;
 VariableSpace CreateVariableSpace(void);
 const char *GetVariable(VariableSpace space, const char *name);
 
-bool		ParseVariableBool(const char *value, const char *name);
-int ParseVariableNum(const char *val,
-				 int defaultval,
-				 int faultval,
-				 bool allowtrail);
+bool		ParseVariableBool(const char *value, const char *name, bool *result);
+
+bool		ParseVariableNum(const char *val, int* result);
+
 int GetVariableNum(VariableSpace space,
 			   const char *name,
 			   int defaultval,
-			   int faultval,
-			   bool allowtrail);
+			   int faultval);
+
 
 void		PrintVariables(VariableSpace space);
 
@@ -53,4 +56,6 @@ bool		SetVariableAssignHook(VariableSpace space, const char *name, VariableAssig
 bool		SetVariableBool(VariableSpace space, const char *name);
 bool		DeleteVariable(VariableSpace space, const char *name);
 
+void		PsqlVarEnumError(const char* name, const char* value, const char* suggestions);
+
 #endif   /* VARIABLES_H */