Improve psql's behavior for \set and \unset of its control variables.

This commit improves on the results of commit 511ae628 in two ways:

1. It restores the historical behavior that "\set FOO" is interpreted
as setting FOO to "on", if FOO is a boolean control variable.  We
already found one test script that was expecting that behavior, and
the psql documentation certainly does nothing to discourage people
from assuming that would work, since it often says just "if FOO is set"
when describing the effects of a boolean variable.  However, now this
case will result in actually setting FOO to "on", not an empty string.

2. It arranges for an "\unset" of a control variable to set the value
back to its default value, rather than becoming apparently undefined.
The control variables are also initialized that way at psql startup.

In combination, these things guarantee that a control variable always
has a displayable value that reflects what psql is actually doing.
That is a pretty substantial usability improvement.

The implementation involves adding a second type of variable hook function
that is able to replace a proposed new value (including NULL) with another
one.  We could alternatively have complicated the API of the assign hook,
but this way seems better since many variables can share the same
substitution hook function.

Also document the actual behavior of these variables more fully,
including covering assorted behaviors that were there before but
never documented.

This patch also includes some minor cleanup that should have been in
511ae628 but was missed.

Patch by me, but it owes a lot to discussions with Daniel Vérité.

Discussion: https://postgr.es/m/9572.1485821620@sss.pgh.pa.us

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

@@ -455,8 +455,8 @@ EOF
       any, by an equal sign on the command line. To unset a variable,
       leave off the equal sign. To set a variable with an empty value,
       use the equal sign but leave off the value. These assignments are
-      done during a very early stage of start-up, so variables reserved
-      for internal purposes might get overwritten later.
+      done during command line processing, so variables that reflect
+      connection state will get overwritten later.
       </para>
       </listitem>
     </varlistentry>
@@ -2692,7 +2692,7 @@ lo_import 152801
         class="parameter">name</replaceable> to <replaceable
         class="parameter">value</replaceable>, or if more than one value
         is given, to the concatenation of all of them. If only one
-        argument is given, the variable is set with an empty value. To
+        argument is given, the variable is set to an empty-string value. To
         unset a variable, use the <command>\unset</command> command.
         </para>
 
@@ -2709,9 +2709,11 @@ lo_import 152801
         </para>
 
         <para>
-        Although you are welcome to set any variable to anything you
-        want, <application>psql</application> treats several variables
-        as special. They are documented in the section about variables.
+        Certain variables are special, in that they
+        control <application>psql</application>'s behavior or are
+        automatically set to reflect connection state.  These variables are
+        documented in <xref linkend="APP-PSQL-variables"
+        endterm="APP-PSQL-variables-title">, below.
         </para>
 
         <note>
@@ -2835,6 +2837,14 @@ testdb=&gt; <userinput>\setenv LESS -imx4F</userinput>
         Unsets (deletes) the <application>psql</> variable <replaceable
         class="parameter">name</replaceable>.
         </para>
+
+        <para>
+        Most variables that control <application>psql</application>'s behavior
+        cannot be unset; instead, an <literal>\unset</> command is interpreted
+        as setting them to their default values.
+        See <xref linkend="APP-PSQL-variables"
+        endterm="APP-PSQL-variables-title">, below.
+        </para>
         </listitem>
       </varlistentry>
 
@@ -3053,7 +3063,7 @@ bar
 
     <para>
     If you call <command>\set</command> without a second argument, the
-    variable is set, with an empty string as value. To unset (i.e., delete)
+    variable is set to an empty-string value. To unset (i.e., delete)
     a variable, use the command <command>\unset</command>.  To show the
     values of all variables, call <command>\set</command> without any argument.
     </para>
@@ -3082,8 +3092,23 @@ bar
     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
-    treated variables follows.
+    using such variable names for your own purposes.
+   </para>
+
+   <para>
+    Variables that control <application>psql</application>'s behavior
+    generally cannot be unset or set to invalid values.  An <literal>\unset</>
+    command is allowed but is interpreted as setting the variable to its
+    default value.  A <literal>\set</> command without a second argument is
+    interpreted as setting the variable to <literal>on</>, for control
+    variables that accept that value, and is rejected for others.  Also,
+    control variables that accept the values <literal>on</>
+    and <literal>off</> will also accept other common spellings of Boolean
+    values, such as <literal>true</> and <literal>false</>.
+   </para>
+
+   <para>
+    The specially treated variables are:
    </para>
 
     <variablelist>
@@ -3153,7 +3178,7 @@ bar
         <para>
         The name of the database you are currently connected to. This is
         set every time you connect to a database (including program
-        start-up), but can be unset.
+        start-up), but can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3171,8 +3196,8 @@ bar
         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 behavior is <option>-b</option>. If unset, or if set to
-        <literal>none</literal>, then no queries are displayed.
+        for this behavior is <option>-b</option>. If set to
+        <literal>none</literal> (the default), then no queries are displayed.
         </para>
         </listitem>
       </varlistentry>
@@ -3187,8 +3212,9 @@ bar
         <productname>PostgreSQL</productname> internals and provide
         similar functionality in your own programs. (To select this behavior
         on program start-up, use the switch <option>-E</option>.)  If you set
-        the variable to the value <literal>noexec</literal>, the queries are
+        this variable to the value <literal>noexec</literal>, the queries are
         just shown but are not actually sent to the server and executed.
+        The default value is <literal>off</>.
         </para>
         </listitem>
       </varlistentry>
@@ -3200,7 +3226,7 @@ bar
         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.
+        with <literal>\encoding</>, but it can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3209,7 +3235,7 @@ bar
         <term><varname>FETCH_COUNT</varname></term>
         <listitem>
         <para>
-        If this variable is set to an integer value &gt; 0,
+        If this variable is set to an integer value greater than zero,
         the results of <command>SELECT</command> queries are fetched
         and displayed in groups of that many rows, rather than the
         default behavior of collecting the entire result set before
@@ -3220,6 +3246,13 @@ bar
         Keep in mind that when using this feature, a query might
         fail after having already displayed some rows.
         </para>
+
+        <para>
+        <varname>FETCH_COUNT</varname> is ignored if it is unset or does not
+        have a positive value.  It cannot be set to a value that is not
+        syntactically an integer.
+        </para>
+
         <tip>
         <para>
         Although you can use any output format with this feature,
@@ -3241,7 +3274,7 @@ 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> (the default), all lines
+         set to <literal>none</literal> (the default), all lines
          read in interactive mode are saved on the history list.
         </para>
         <note>
@@ -3257,8 +3290,12 @@ bar
         <term><varname>HISTFILE</varname></term>
         <listitem>
         <para>
-        The file name that will be used to store the history list. The default
-        value is <filename>~/.psql_history</filename>.  For example, putting:
+        The file name that will be used to store the history list.  If unset,
+        the file name is taken from the <envar>PSQL_HISTORY</envar>
+        environment variable.  If that is not set either, the default
+        is <filename>~/.psql_history</filename>,
+        or <filename>%APPDATA%\postgresql\psql_history</filename> on Windows.
+        For example, putting:
 <programlisting>
 \set HISTFILE ~/.psql_history- :DBNAME
 </programlisting>
@@ -3279,8 +3316,10 @@ bar
         <term><varname>HISTSIZE</varname></term>
         <listitem>
         <para>
-        The number of commands to store in the command history. The
-        default value is 500.
+        The maximum number of commands to store in the command history.
+        If unset, at most 500 commands are stored by default.
+        If set to a value that is negative or not an integer, no limit is
+        applied.
         </para>
         <note>
         <para>
@@ -3297,7 +3336,7 @@ bar
         <para>
         The database server host you are currently connected to. This is
         set every time you connect to a database (including program
-        start-up), but can be unset.
+        start-up), but can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3350,7 +3389,7 @@ bar
         generates an error, the error is ignored and the transaction
         continues. When set to <literal>interactive</>, such errors are only
         ignored in interactive sessions, and not when reading script
-        files. When unset or set to <literal>off</>, a statement in a
+        files. When set to <literal>off</> (the default), a statement in a
         transaction block that generates an error aborts the entire
         transaction. The error rollback mode works by issuing an
         implicit <command>SAVEPOINT</> for you, just before each command
@@ -3385,7 +3424,7 @@ bar
         <para>
         The database server port to which you are currently connected.
         This is set every time you connect to a database (including
-        program start-up), but can be unset.
+        program start-up), but can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3458,7 +3497,7 @@ bar
         <para>
         The database user you are currently connected as. This is set
         every time you connect to a database (including program
-        start-up), but can be unset.
+        start-up), but can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -3481,7 +3520,7 @@ bar
         <listitem>
         <para>
         This variable is set at program start-up to
-        reflect <application>psql</>'s version.  It can be unset or changed.
+        reflect <application>psql</>'s version.  It can be changed or unset.
         </para>
         </listitem>
       </varlistentry>
@@ -4015,6 +4054,7 @@ PSQL_EDITOR_LINENUMBER_ARG='--line '
     </para>
     <para>
      The location of the history file can be set explicitly via
+     the <varname>HISTFILE</varname> <application>psql</> variable or
      the <envar>PSQL_HISTORY</envar> environment variable.
     </para>
    </listitem>

src/bin/psql/startup.c

@@ -166,10 +166,8 @@ main(int argc, char *argv[])
 
 	SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
 
-	/* Default values for variables */
+	/* Default values for variables (that don't match the result of \unset) */
 	SetVariableBool(pset.vars, "AUTOCOMMIT");
-	SetVariable(pset.vars, "VERBOSITY", "default");
-	SetVariable(pset.vars, "SHOW_CONTEXT", "errors");
 	SetVariable(pset.vars, "PROMPT1", DEFAULT_PROMPT1);
 	SetVariable(pset.vars, "PROMPT2", DEFAULT_PROMPT2);
 	SetVariable(pset.vars, "PROMPT3", DEFAULT_PROMPT3);
@@ -578,17 +576,13 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
 					if (!equal_loc)
 					{
 						if (!DeleteVariable(pset.vars, value))
-						{
-							fprintf(stderr, _("%s: could not delete variable \"%s\"\n"),
-									pset.progname, value);
-							exit(EXIT_FAILURE);
-						}
+							exit(EXIT_FAILURE); /* error already printed */
 					}
 					else
 					{
 						*equal_loc = '\0';
 						if (!SetVariable(pset.vars, value, equal_loc + 1))
-							exit(EXIT_FAILURE);
+							exit(EXIT_FAILURE); /* error already printed */
 					}
 
 					free(value);
@@ -777,11 +771,28 @@ showVersion(void)
 
 
 /*
- * Assign hooks for psql variables.
+ * Substitute hooks and assign hooks for psql variables.
  *
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
+static char *
+bool_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+	{
+		/* "\unset FOO" becomes "\set FOO off" */
+		newval = pg_strdup("off");
+	}
+	else if (newval[0] == '\0')
+	{
+		/* "\set FOO" becomes "\set FOO on" */
+		pg_free(newval);
+		newval = pg_strdup("on");
+	}
+	return newval;
+}
+
 static bool
 autocommit_hook(const char *newval)
 {
@@ -822,12 +833,19 @@ fetch_count_hook(const char *newval)
 	return true;
 }
 
+static char *
+echo_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("none");
+	return newval;
+}
+
 static bool
 echo_hook(const char *newval)
 {
-	if (newval == NULL)
-		pset.echo = PSQL_ECHO_NONE;
-	else if (pg_strcasecmp(newval, "queries") == 0)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "queries") == 0)
 		pset.echo = PSQL_ECHO_QUERIES;
 	else if (pg_strcasecmp(newval, "errors") == 0)
 		pset.echo = PSQL_ECHO_ERRORS;
@@ -846,9 +864,8 @@ echo_hook(const char *newval)
 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)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "noexec") == 0)
 		pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
 	else
 	{
@@ -868,9 +885,8 @@ echo_hidden_hook(const char *newval)
 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)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "interactive") == 0)
 		pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
 	else
 	{
@@ -887,12 +903,19 @@ on_error_rollback_hook(const char *newval)
 	return true;
 }
 
+static char *
+comp_keyword_case_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("preserve-upper");
+	return newval;
+}
+
 static bool
 comp_keyword_case_hook(const char *newval)
 {
-	if (newval == NULL)
-		pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
-	else if (pg_strcasecmp(newval, "preserve-upper") == 0)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "preserve-upper") == 0)
 		pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
 	else if (pg_strcasecmp(newval, "preserve-lower") == 0)
 		pset.comp_case = PSQL_COMP_CASE_PRESERVE_LOWER;
@@ -909,12 +932,19 @@ comp_keyword_case_hook(const char *newval)
 	return true;
 }
 
+static char *
+histcontrol_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("none");
+	return newval;
+}
+
 static bool
 histcontrol_hook(const char *newval)
 {
-	if (newval == NULL)
-		pset.histcontrol = hctl_none;
-	else if (pg_strcasecmp(newval, "ignorespace") == 0)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "ignorespace") == 0)
 		pset.histcontrol = hctl_ignorespace;
 	else if (pg_strcasecmp(newval, "ignoredups") == 0)
 		pset.histcontrol = hctl_ignoredups;
@@ -952,12 +982,19 @@ prompt3_hook(const char *newval)
 	return true;
 }
 
+static char *
+verbosity_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("default");
+	return newval;
+}
+
 static bool
 verbosity_hook(const char *newval)
 {
-	if (newval == NULL)
-		pset.verbosity = PQERRORS_DEFAULT;
-	else if (pg_strcasecmp(newval, "default") == 0)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "default") == 0)
 		pset.verbosity = PQERRORS_DEFAULT;
 	else if (pg_strcasecmp(newval, "terse") == 0)
 		pset.verbosity = PQERRORS_TERSE;
@@ -974,12 +1011,19 @@ verbosity_hook(const char *newval)
 	return true;
 }
 
+static char *
+show_context_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("errors");
+	return newval;
+}
+
 static bool
 show_context_hook(const char *newval)
 {
-	if (newval == NULL)
-		pset.show_context = PQSHOW_CONTEXT_ERRORS;
-	else if (pg_strcasecmp(newval, "never") == 0)
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "never") == 0)
 		pset.show_context = PQSHOW_CONTEXT_NEVER;
 	else if (pg_strcasecmp(newval, "errors") == 0)
 		pset.show_context = PQSHOW_CONTEXT_ERRORS;
@@ -1002,20 +1046,52 @@ EstablishVariableSpace(void)
 {
 	pset.vars = CreateVariableSpace();
 
-	SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook);
-	SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
-	SetVariableAssignHook(pset.vars, "QUIET", quiet_hook);
-	SetVariableAssignHook(pset.vars, "SINGLELINE", singleline_hook);
-	SetVariableAssignHook(pset.vars, "SINGLESTEP", singlestep_hook);
-	SetVariableAssignHook(pset.vars, "FETCH_COUNT", fetch_count_hook);
-	SetVariableAssignHook(pset.vars, "ECHO", echo_hook);
-	SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook);
-	SetVariableAssignHook(pset.vars, "ON_ERROR_ROLLBACK", on_error_rollback_hook);
-	SetVariableAssignHook(pset.vars, "COMP_KEYWORD_CASE", comp_keyword_case_hook);
-	SetVariableAssignHook(pset.vars, "HISTCONTROL", histcontrol_hook);
-	SetVariableAssignHook(pset.vars, "PROMPT1", prompt1_hook);
-	SetVariableAssignHook(pset.vars, "PROMPT2", prompt2_hook);
-	SetVariableAssignHook(pset.vars, "PROMPT3", prompt3_hook);
-	SetVariableAssignHook(pset.vars, "VERBOSITY", verbosity_hook);
-	SetVariableAssignHook(pset.vars, "SHOW_CONTEXT", show_context_hook);
+	SetVariableHooks(pset.vars, "AUTOCOMMIT",
+					 bool_substitute_hook,
+					 autocommit_hook);
+	SetVariableHooks(pset.vars, "ON_ERROR_STOP",
+					 bool_substitute_hook,
+					 on_error_stop_hook);
+	SetVariableHooks(pset.vars, "QUIET",
+					 bool_substitute_hook,
+					 quiet_hook);
+	SetVariableHooks(pset.vars, "SINGLELINE",
+					 bool_substitute_hook,
+					 singleline_hook);
+	SetVariableHooks(pset.vars, "SINGLESTEP",
+					 bool_substitute_hook,
+					 singlestep_hook);
+	SetVariableHooks(pset.vars, "FETCH_COUNT",
+					 NULL,
+					 fetch_count_hook);
+	SetVariableHooks(pset.vars, "ECHO",
+					 echo_substitute_hook,
+					 echo_hook);
+	SetVariableHooks(pset.vars, "ECHO_HIDDEN",
+					 bool_substitute_hook,
+					 echo_hidden_hook);
+	SetVariableHooks(pset.vars, "ON_ERROR_ROLLBACK",
+					 bool_substitute_hook,
+					 on_error_rollback_hook);
+	SetVariableHooks(pset.vars, "COMP_KEYWORD_CASE",
+					 comp_keyword_case_substitute_hook,
+					 comp_keyword_case_hook);
+	SetVariableHooks(pset.vars, "HISTCONTROL",
+					 histcontrol_substitute_hook,
+					 histcontrol_hook);
+	SetVariableHooks(pset.vars, "PROMPT1",
+					 NULL,
+					 prompt1_hook);
+	SetVariableHooks(pset.vars, "PROMPT2",
+					 NULL,
+					 prompt2_hook);
+	SetVariableHooks(pset.vars, "PROMPT3",
+					 NULL,
+					 prompt3_hook);
+	SetVariableHooks(pset.vars, "VERBOSITY",
+					 verbosity_substitute_hook,
+					 verbosity_hook);
+	SetVariableHooks(pset.vars, "SHOW_CONTEXT",
+					 show_context_substitute_hook,
+					 show_context_hook);
 }

src/bin/psql/variables.c

@@ -52,6 +52,7 @@ CreateVariableSpace(void)
 	ptr = pg_malloc(sizeof *ptr);
 	ptr->name = NULL;
 	ptr->value = NULL;
+	ptr->substitute_hook = NULL;
 	ptr->assign_hook = NULL;
 	ptr->next = NULL;
 
@@ -101,11 +102,9 @@ ParseVariableBool(const char *value, const char *name, bool *result)
 	size_t		len;
 	bool		valid = true;
 
+	/* Treat "unset" as an empty string, which will lead to error below */
 	if (value == NULL)
-	{
-		*result = false;		/* not set -> assume "off" */
-		return valid;
-	}
+		value = "";
 
 	len = strlen(value);
 
@@ -152,8 +151,10 @@ ParseVariableNum(const char *value, const char *name, int *result)
 	char	   *end;
 	long		numval;
 
+	/* Treat "unset" as an empty string, which will lead to error below */
 	if (value == NULL)
-		return false;
+		value = "";
+
 	errno = 0;
 	numval = strtol(value, &end, 0);
 	if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
@@ -235,13 +236,13 @@ SetVariable(VariableSpace space, const char *name, const char *value)
 
 	if (!valid_variable_name(name))
 	{
+		/* Deletion of non-existent variable is not an error */
+		if (!value)
+			return true;
 		psql_error("invalid variable name: \"%s\"\n", name);
 		return false;
 	}
 
-	if (!value)
-		return DeleteVariable(space, name);
-
 	for (previous = space, current = space->next;
 		 current;
 		 previous = current, current = current->next)
@@ -249,14 +250,20 @@ SetVariable(VariableSpace space, const char *name, const char *value)
 		if (strcmp(current->name, name) == 0)
 		{
 			/*
-			 * Found entry, so update, unless hook returns false.  The hook
-			 * may need the passed value to have the same lifespan as the
-			 * variable, so allocate it right away, even though we'll have to
-			 * free it again if the hook returns false.
+			 * Found entry, so update, unless assign hook returns false.
+			 *
+			 * We must duplicate the passed value to start with.  This
+			 * simplifies the API for substitute hooks.  Moreover, some assign
+			 * hooks assume that the passed value has the same lifespan as the
+			 * variable.  Having to free the string again on failure is a
+			 * small price to pay for keeping these APIs simple.
 			 */
-			char	   *new_value = pg_strdup(value);
+			char	   *new_value = value ? pg_strdup(value) : NULL;
 			bool		confirmed;
 
+			if (current->substitute_hook)
+				new_value = (*current->substitute_hook) (new_value);
+
 			if (current->assign_hook)
 				confirmed = (*current->assign_hook) (new_value);
 			else
@@ -267,39 +274,61 @@ SetVariable(VariableSpace space, const char *name, const char *value)
 				if (current->value)
 					pg_free(current->value);
 				current->value = new_value;
+
+				/*
+				 * If we deleted the value, and there are no hooks to
+				 * remember, we can discard the variable altogether.
+				 */
+				if (new_value == NULL &&
+					current->substitute_hook == NULL &&
+					current->assign_hook == NULL)
+				{
+					previous->next = current->next;
+					free(current->name);
+					free(current);
+				}
 			}
-			else
+			else if (new_value)
 				pg_free(new_value);		/* current->value is left unchanged */
 
 			return confirmed;
 		}
 	}
 
-	/* not present, make new entry */
-	current = pg_malloc(sizeof *current);
-	current->name = pg_strdup(name);
-	current->value = pg_strdup(value);
-	current->assign_hook = NULL;
-	current->next = NULL;
-	previous->next = current;
+	/* not present, make new entry ... unless we were asked to delete */
+	if (value)
+	{
+		current = pg_malloc(sizeof *current);
+		current->name = pg_strdup(name);
+		current->value = pg_strdup(value);
+		current->substitute_hook = NULL;
+		current->assign_hook = NULL;
+		current->next = NULL;
+		previous->next = current;
+	}
 	return true;
 }
 
 /*
- * Attach an assign hook function to the named variable.
+ * Attach substitute and/or assign hook functions to the named variable.
+ * If you need only one hook, pass NULL for the other.
  *
- * If the variable doesn't already exist, create it with value NULL,
- * just so we have a place to store the hook function.  (Externally,
- * this isn't different from it not being defined.)
+ * If the variable doesn't already exist, create it with value NULL, just so
+ * we have a place to store the hook function(s).  (The substitute hook might
+ * immediately change the NULL to something else; if not, this state is
+ * externally the same as the variable not being defined.)
  *
- * The hook is immediately called on the variable's current value.  This is
- * meant to let it update any derived psql state.  If the hook doesn't like
- * the current value, it will print a message to that effect, but we'll ignore
- * it.  Generally we do not expect any such failure here, because this should
- * get called before any user-supplied value is assigned.
+ * The substitute hook, if given, is immediately called on the variable's
+ * value.  Then the assign hook, if given, is called on the variable's value.
+ * This is meant to let it update any derived psql state.  If the assign hook
+ * doesn't like the current value, it will print a message to that effect,
+ * but we'll ignore it.  Generally we do not expect any such failure here,
+ * because this should get called before any user-supplied value is assigned.
  */
 void
-SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook)
+SetVariableHooks(VariableSpace space, const char *name,
+				 VariableSubstituteHook shook,
+				 VariableAssignHook ahook)
 {
 	struct _variable *current,
 			   *previous;
@@ -317,8 +346,12 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
 		if (strcmp(current->name, name) == 0)
 		{
 			/* found entry, so update */
-			current->assign_hook = hook;
-			(void) (*hook) (current->value);
+			current->substitute_hook = shook;
+			current->assign_hook = ahook;
+			if (shook)
+				current->value = (*shook) (current->value);
+			if (ahook)
+				(void) (*ahook) (current->value);
 			return;
 		}
 	}
@@ -327,10 +360,14 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
 	current = pg_malloc(sizeof *current);
 	current->name = pg_strdup(name);
 	current->value = NULL;
-	current->assign_hook = hook;
+	current->substitute_hook = shook;
+	current->assign_hook = ahook;
 	current->next = NULL;
 	previous->next = current;
-	(void) (*hook) (NULL);
+	if (shook)
+		current->value = (*shook) (current->value);
+	if (ahook)
+		(void) (*ahook) (current->value);
 }
 
 /*
@@ -351,42 +388,7 @@ SetVariableBool(VariableSpace space, const char *name)
 bool
 DeleteVariable(VariableSpace space, const char *name)
 {
-	struct _variable *current,
-			   *previous;
-
-	if (!space)
-		return true;
-
-	for (previous = space, current = space->next;
-		 current;
-		 previous = current, current = current->next)
-	{
-		if (strcmp(current->name, name) == 0)
-		{
-			if (current->assign_hook)
-			{
-				/* Allow deletion only if hook is okay with NULL value */
-				if (!(*current->assign_hook) (NULL))
-					return false;		/* message printed by hook */
-				if (current->value)
-					free(current->value);
-				current->value = NULL;
-				/* Don't delete entry, or we'd forget the hook function */
-			}
-			else
-			{
-				/* We can delete the entry as well as its value */
-				if (current->value)
-					free(current->value);
-				previous->next = current->next;
-				free(current->name);
-				free(current);
-			}
-			return true;
-		}
-	}
-
-	return true;
+	return SetVariable(space, name, NULL);
 }
 
 /*

src/bin/psql/variables.h

@@ -18,28 +18,52 @@
  * prevent invalid values from being assigned, and can update internal C
  * variables to keep them in sync with the variable's current value.
  *
- * A hook function is called before any attempted assignment, with the
+ * An assign hook function is called before any attempted assignment, with the
  * proposed new value of the variable (or with NULL, if an \unset is being
  * attempted).  If it returns false, the assignment doesn't occur --- it
  * should print an error message with psql_error() to tell the user why.
  *
- * When a hook function is installed with SetVariableAssignHook(), it is
+ * When an assign hook function is installed with SetVariableHooks(), it is
  * called with the variable's current value (or with NULL, if it wasn't set
  * yet).  But its return value is ignored in this case.  The hook should be
  * set before any possibly-invalid value can be assigned.
  */
 typedef bool (*VariableAssignHook) (const char *newval);
 
+/*
+ * Variables can also be given "substitute hook" functions.  The substitute
+ * hook can replace values (including NULL) with other values, allowing
+ * normalization of variable contents.  For example, for a boolean variable,
+ * we wish to interpret "\unset FOO" as "\set FOO off", and we can do that
+ * by installing a substitute hook.  (We can use the same substitute hook
+ * for all bool or nearly-bool variables, which is why this responsibility
+ * isn't part of the assign hook.)
+ *
+ * The substitute hook is called before any attempted assignment, and before
+ * the assign hook if any, passing the proposed new value of the variable as a
+ * malloc'd string (or NULL, if an \unset is being attempted).  It can return
+ * the same value, or a different malloc'd string, or modify the string
+ * in-place.  It should free the passed-in value if it's not returning it.
+ * The substitute hook generally should not complain about erroneous values;
+ * that's a job for the assign hook.
+ *
+ * When a substitute hook is installed with SetVariableHooks(), it is applied
+ * to the variable's current value (typically NULL, if it wasn't set yet).
+ * That also happens before applying the assign hook.
+ */
+typedef char *(*VariableSubstituteHook) (char *newval);
+
 /*
  * Data structure representing one variable.
  *
  * 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.
+ * keeping the struct around so as not to forget about its hook function(s).
  */
 struct _variable
 {
 	char	   *name;
 	char	   *value;
+	VariableSubstituteHook substitute_hook;
 	VariableAssignHook assign_hook;
 	struct _variable *next;
 };
@@ -65,10 +89,13 @@ int GetVariableNum(VariableSpace space,
 void		PrintVariables(VariableSpace space);
 
 bool		SetVariable(VariableSpace space, const char *name, const char *value);
-void		SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook);
 bool		SetVariableBool(VariableSpace space, const char *name);
 bool		DeleteVariable(VariableSpace space, const char *name);
 
+void SetVariableHooks(VariableSpace space, const char *name,
+				 VariableSubstituteHook shook,
+				 VariableAssignHook ahook);
+
 void		PsqlVarEnumError(const char *name, const char *value, const char *suggestions);
 
 #endif   /* VARIABLES_H */

src/test/regress/expected/psql.out

@@ -11,6 +11,23 @@ invalid variable name: "invalid/name"
 unrecognized value "foo" for "AUTOCOMMIT": boolean expected
 \set FETCH_COUNT foo
 invalid value "foo" for "FETCH_COUNT": integer expected
+-- check handling of built-in boolean variable
+\echo :ON_ERROR_ROLLBACK
+off
+\set ON_ERROR_ROLLBACK
+\echo :ON_ERROR_ROLLBACK
+on
+\set ON_ERROR_ROLLBACK foo
+unrecognized value "foo" for "ON_ERROR_ROLLBACK"
+Available values are: on, off, interactive.
+\echo :ON_ERROR_ROLLBACK
+on
+\set ON_ERROR_ROLLBACK on
+\echo :ON_ERROR_ROLLBACK
+on
+\unset ON_ERROR_ROLLBACK
+\echo :ON_ERROR_ROLLBACK
+off
 -- \gset
 select 10 as test01, 20 as test02, 'Hello' as test03 \gset pref01_
 \echo :pref01_test01 :pref01_test02 :pref01_test03

src/test/regress/sql/psql.sql

@@ -10,6 +10,16 @@
 -- fail: invalid value for special variable
 \set AUTOCOMMIT foo
 \set FETCH_COUNT foo
+-- check handling of built-in boolean variable
+\echo :ON_ERROR_ROLLBACK
+\set ON_ERROR_ROLLBACK
+\echo :ON_ERROR_ROLLBACK
+\set ON_ERROR_ROLLBACK foo
+\echo :ON_ERROR_ROLLBACK
+\set ON_ERROR_ROLLBACK on
+\echo :ON_ERROR_ROLLBACK
+\unset ON_ERROR_ROLLBACK
+\echo :ON_ERROR_ROLLBACK
 
 -- \gset