Re: [HACKERS] Improvements in psql hooks for variables

I wrote:
> Attached is a draft patch for that.  I chose to make a second hook rather
> than complicate the assign hook API, mainly because it allows more code
> sharing --- all the bool vars can share the same substitute hook, and
> so can the three-way vars as long as "on" and "off" are the appropriate
> substitutes.

> I only touched the behavior for bool vars here, but if people like this
> solution it could be fleshed out to apply to all the built-in variables.

Attached is a finished version that includes hooks for all the variables
for which they were clearly sensible.  (FETCH_COUNT doesn't seem to really
need one, and I didn't do anything with HISTSIZE or IGNOREEOF either.
It might be worth bringing the latter two into the hooks paradigm, but
that seems like fit material for a separate patch.)

I updated the documentation as well.  I think this is committable if
there are not objections.

            regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 4e51e90..b9c8fcc 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** EOF
*** 455,462 ****
        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.
        </para>
        </listitem>
      </varlistentry>
--- 455,462 ----
        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 command line processing, so variables that reflect
!       connection state will get overwritten later.
        </para>
        </listitem>
      </varlistentry>
*************** lo_import 152801
*** 2692,2698 ****
          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
          unset a variable, use the <command>\unset</command> command.
          </para>

--- 2692,2698 ----
          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 to an empty-string value. To
          unset a variable, use the <command>\unset</command> command.
          </para>

*************** lo_import 152801
*** 2709,2717 ****
          </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.
          </para>

          <note>
--- 2709,2719 ----
          </para>

          <para>
!         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>
*************** testdb=> <userinput>\setenv LESS -imx
*** 2835,2840 ****
--- 2837,2850 ----
          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>

*************** bar
*** 3053,3059 ****

      <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)
      a variable, use the command <command>\unset</command>.  To show the
      values of all variables, call <command>\set</command> without any argument.
      </para>
--- 3063,3069 ----

      <para>
      If you call <command>\set</command> without a second argument, the
!     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>
*************** bar
*** 3082,3089 ****
      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.
     </para>

      <variablelist>
--- 3092,3114 ----
      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.
!    </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>
*************** bar
*** 3153,3159 ****
          <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.
          </para>
          </listitem>
        </varlistentry>
--- 3178,3184 ----
          <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 changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3171,3178 ****
          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.
          </para>
          </listitem>
        </varlistentry>
--- 3196,3203 ----
          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 set to
!         <literal>none</literal> (the default), then no queries are displayed.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3187,3194 ****
          <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
          just shown but are not actually sent to the server and executed.
          </para>
          </listitem>
        </varlistentry>
--- 3212,3220 ----
          <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
!         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>
*************** bar
*** 3200,3206 ****
          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>
--- 3226,3232 ----
          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 changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3209,3215 ****
          <term><varname>FETCH_COUNT</varname></term>
          <listitem>
          <para>
!         If this variable is set to an integer value > 0,
          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
--- 3235,3241 ----
          <term><varname>FETCH_COUNT</varname></term>
          <listitem>
          <para>
!         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
*************** bar
*** 3220,3225 ****
--- 3246,3258 ----
          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,
*************** bar
*** 3241,3247 ****
           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
           read in interactive mode are saved on the history list.
          </para>
          <note>
--- 3274,3280 ----
           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
!          set to <literal>none</literal> (the default), all lines
           read in interactive mode are saved on the history list.
          </para>
          <note>
*************** bar
*** 3257,3264 ****
          <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:
  <programlisting>
  \set HISTFILE ~/.psql_history- :DBNAME
  </programlisting>
--- 3290,3301 ----
          <term><varname>HISTFILE</varname></term>
          <listitem>
          <para>
!         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>
*************** bar
*** 3279,3286 ****
          <term><varname>HISTSIZE</varname></term>
          <listitem>
          <para>
!         The number of commands to store in the command history. The
!         default value is 500.
          </para>
          <note>
          <para>
--- 3316,3325 ----
          <term><varname>HISTSIZE</varname></term>
          <listitem>
          <para>
!         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>
*************** bar
*** 3297,3303 ****
          <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.
          </para>
          </listitem>
        </varlistentry>
--- 3336,3342 ----
          <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 changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3350,3356 ****
          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
          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
--- 3389,3395 ----
          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 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
*************** bar
*** 3385,3391 ****
          <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.
          </para>
          </listitem>
        </varlistentry>
--- 3424,3430 ----
          <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 changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3458,3464 ****
          <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.
          </para>
          </listitem>
        </varlistentry>
--- 3497,3503 ----
          <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 changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** bar
*** 3481,3487 ****
          <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>
--- 3520,3526 ----
          <listitem>
          <para>
          This variable is set at program start-up to
!         reflect <application>psql</>'s version.  It can be changed or unset.
          </para>
          </listitem>
        </varlistentry>
*************** PSQL_EDITOR_LINENUMBER_ARG='--line '
*** 4015,4020 ****
--- 4054,4060 ----
      </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>
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 0574b5b..a3654e6 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** main(int argc, char *argv[])
*** 166,175 ****

      SetVariable(pset.vars, "VERSION", PG_VERSION_STR);

!     /* Default values for variables */
      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);
--- 166,173 ----

      SetVariable(pset.vars, "VERSION", PG_VERSION_STR);

!     /* Default values for variables (that don't match the result of \unset) */
      SetVariableBool(pset.vars, "AUTOCOMMIT");
      SetVariable(pset.vars, "PROMPT1", DEFAULT_PROMPT1);
      SetVariable(pset.vars, "PROMPT2", DEFAULT_PROMPT2);
      SetVariable(pset.vars, "PROMPT3", DEFAULT_PROMPT3);
*************** parse_psql_options(int argc, char *argv[
*** 578,594 ****
                      if (!equal_loc)
                      {
                          if (!DeleteVariable(pset.vars, value))
!                         {
!                             fprintf(stderr, _("%s: could not delete variable \"%s\"\n"),
!                                     pset.progname, value);
!                             exit(EXIT_FAILURE);
!                         }
                      }
                      else
                      {
                          *equal_loc = '\0';
                          if (!SetVariable(pset.vars, value, equal_loc + 1))
!                             exit(EXIT_FAILURE);
                      }

                      free(value);
--- 576,588 ----
                      if (!equal_loc)
                      {
                          if (!DeleteVariable(pset.vars, value))
!                             exit(EXIT_FAILURE); /* error already printed */
                      }
                      else
                      {
                          *equal_loc = '\0';
                          if (!SetVariable(pset.vars, value, equal_loc + 1))
!                             exit(EXIT_FAILURE); /* error already printed */
                      }

                      free(value);
*************** showVersion(void)
*** 777,787 ****


  /*
!  * Assign hooks for psql variables.
   *
   * This isn't an amazingly good place for them, but neither is anywhere else.
   */

  static bool
  autocommit_hook(const char *newval)
  {
--- 771,798 ----


  /*
!  * 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)
  {
*************** fetch_count_hook(const char *newval)
*** 822,833 ****
      return true;
  }

  static bool
  echo_hook(const char *newval)
  {
!     if (newval == NULL)
!         pset.echo = PSQL_ECHO_NONE;
!     else if (pg_strcasecmp(newval, "queries") == 0)
          pset.echo = PSQL_ECHO_QUERIES;
      else if (pg_strcasecmp(newval, "errors") == 0)
          pset.echo = PSQL_ECHO_ERRORS;
--- 833,851 ----
      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)
  {
!     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;
*************** echo_hook(const char *newval)
*** 846,854 ****
  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
      {
--- 864,871 ----
  static bool
  echo_hidden_hook(const char *newval)
  {
!     Assert(newval != NULL);        /* else substitute hook messed up */
!     if (pg_strcasecmp(newval, "noexec") == 0)
          pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
      else
      {
*************** echo_hidden_hook(const char *newval)
*** 868,876 ****
  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
      {
--- 885,892 ----
  static bool
  on_error_rollback_hook(const char *newval)
  {
!     Assert(newval != NULL);        /* else substitute hook messed up */
!     if (pg_strcasecmp(newval, "interactive") == 0)
          pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
      else
      {
*************** on_error_rollback_hook(const char *newva
*** 887,898 ****
      return true;
  }

  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)
          pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
      else if (pg_strcasecmp(newval, "preserve-lower") == 0)
          pset.comp_case = PSQL_COMP_CASE_PRESERVE_LOWER;
--- 903,921 ----
      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)
  {
!     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;
*************** comp_keyword_case_hook(const char *newva
*** 909,920 ****
      return true;
  }

  static bool
  histcontrol_hook(const char *newval)
  {
!     if (newval == NULL)
!         pset.histcontrol = hctl_none;
!     else if (pg_strcasecmp(newval, "ignorespace") == 0)
          pset.histcontrol = hctl_ignorespace;
      else if (pg_strcasecmp(newval, "ignoredups") == 0)
          pset.histcontrol = hctl_ignoredups;
--- 932,950 ----
      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)
  {
!     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;
*************** prompt3_hook(const char *newval)
*** 952,963 ****
      return true;
  }

  static bool
  verbosity_hook(const char *newval)
  {
!     if (newval == NULL)
!         pset.verbosity = PQERRORS_DEFAULT;
!     else if (pg_strcasecmp(newval, "default") == 0)
          pset.verbosity = PQERRORS_DEFAULT;
      else if (pg_strcasecmp(newval, "terse") == 0)
          pset.verbosity = PQERRORS_TERSE;
--- 982,1000 ----
      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)
  {
!     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;
*************** verbosity_hook(const char *newval)
*** 974,985 ****
      return true;
  }

  static bool
  show_context_hook(const char *newval)
  {
!     if (newval == NULL)
!         pset.show_context = PQSHOW_CONTEXT_ERRORS;
!     else 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;
--- 1011,1029 ----
      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)
  {
!     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;
*************** EstablishVariableSpace(void)
*** 1002,1021 ****
  {
      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);
  }
--- 1046,1097 ----
  {
      pset.vars = CreateVariableSpace();

!     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);
  }
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 91e4ae8..b9b8fcb 100644
*** a/src/bin/psql/variables.c
--- b/src/bin/psql/variables.c
*************** CreateVariableSpace(void)
*** 52,57 ****
--- 52,58 ----
      ptr = pg_malloc(sizeof *ptr);
      ptr->name = NULL;
      ptr->value = NULL;
+     ptr->substitute_hook = NULL;
      ptr->assign_hook = NULL;
      ptr->next = NULL;

*************** ParseVariableBool(const char *value, con
*** 101,111 ****
      size_t        len;
      bool        valid = true;

      if (value == NULL)
!     {
!         *result = false;        /* not set -> assume "off" */
!         return valid;
!     }

      len = strlen(value);

--- 102,110 ----
      size_t        len;
      bool        valid = true;

+     /* Treat "unset" as an empty string, which will lead to error below */
      if (value == NULL)
!         value = "";

      len = strlen(value);

*************** ParseVariableNum(const char *value, cons
*** 152,159 ****
      char       *end;
      long        numval;

      if (value == NULL)
!         return false;
      errno = 0;
      numval = strtol(value, &end, 0);
      if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
--- 151,160 ----
      char       *end;
      long        numval;

+     /* Treat "unset" as an empty string, which will lead to error below */
      if (value == NULL)
!         value = "";
!
      errno = 0;
      numval = strtol(value, &end, 0);
      if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
*************** SetVariable(VariableSpace space, const c
*** 235,247 ****

      if (!valid_variable_name(name))
      {
          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)
--- 236,248 ----

      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;
      }

      for (previous = space, current = space->next;
           current;
           previous = current, current = current->next)
*************** SetVariable(VariableSpace space, const c
*** 249,262 ****
          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.
               */
!             char       *new_value = pg_strdup(value);
              bool        confirmed;

              if (current->assign_hook)
                  confirmed = (*current->assign_hook) (new_value);
              else
--- 250,269 ----
          if (strcmp(current->name, name) == 0)
          {
              /*
!              * 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 = 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
*************** SetVariable(VariableSpace space, const c
*** 267,305 ****
                  if (current->value)
                      pg_free(current->value);
                  current->value = new_value;
              }
!             else
                  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;
      return true;
  }

  /*
!  * Attach an assign hook function to the named variable.
   *
!  * 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.)
   *
!  * 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.
   */
  void
! SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook)
  {
      struct _variable *current,
                 *previous;
--- 274,334 ----
                  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 if (new_value)
                  pg_free(new_value);        /* current->value is left unchanged */

              return confirmed;
          }
      }

!     /* 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 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(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 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
! SetVariableHooks(VariableSpace space, const char *name,
!                  VariableSubstituteHook shook,
!                  VariableAssignHook ahook)
  {
      struct _variable *current,
                 *previous;
*************** SetVariableAssignHook(VariableSpace spac
*** 317,324 ****
          if (strcmp(current->name, name) == 0)
          {
              /* found entry, so update */
!             current->assign_hook = hook;
!             (void) (*hook) (current->value);
              return;
          }
      }
--- 346,357 ----
          if (strcmp(current->name, name) == 0)
          {
              /* found entry, so update */
!             current->substitute_hook = shook;
!             current->assign_hook = ahook;
!             if (shook)
!                 current->value = (*shook) (current->value);
!             if (ahook)
!                 (void) (*ahook) (current->value);
              return;
          }
      }
*************** SetVariableAssignHook(VariableSpace spac
*** 327,336 ****
      current = pg_malloc(sizeof *current);
      current->name = pg_strdup(name);
      current->value = NULL;
!     current->assign_hook = hook;
      current->next = NULL;
      previous->next = current;
!     (void) (*hook) (NULL);
  }

  /*
--- 360,373 ----
      current = pg_malloc(sizeof *current);
      current->name = pg_strdup(name);
      current->value = NULL;
!     current->substitute_hook = shook;
!     current->assign_hook = ahook;
      current->next = NULL;
      previous->next = current;
!     if (shook)
!         current->value = (*shook) (current->value);
!     if (ahook)
!         (void) (*ahook) (current->value);
  }

  /*
*************** SetVariableBool(VariableSpace space, con
*** 351,392 ****
  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;
  }

  /*
--- 388,394 ----
  bool
  DeleteVariable(VariableSpace space, const char *name)
  {
!     return SetVariable(space, name, NULL);
  }

  /*
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index 274b4af..84be780 100644
*** a/src/bin/psql/variables.h
--- b/src/bin/psql/variables.h
***************
*** 18,29 ****
   * 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
   * 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
   * 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.
--- 18,29 ----
   * prevent invalid values from being assigned, and can update internal C
   * variables to keep them in sync with the variable's current value.
   *
!  * 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 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.
***************
*** 31,45 ****
  typedef bool (*VariableAssignHook) (const 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.
   */
  struct _variable
  {
      char       *name;
      char       *value;
      VariableAssignHook assign_hook;
      struct _variable *next;
  };
--- 31,69 ----
  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(s).
   */
  struct _variable
  {
      char       *name;
      char       *value;
+     VariableSubstituteHook substitute_hook;
      VariableAssignHook assign_hook;
      struct _variable *next;
  };
*************** int GetVariableNum(VariableSpace space,
*** 65,74 ****
  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        PsqlVarEnumError(const char *name, const char *value, const char *suggestions);

  #endif   /* VARIABLES_H */
--- 89,101 ----
  void        PrintVariables(VariableSpace space);

  bool        SetVariable(VariableSpace space, const char *name, const char *value);
  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 */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 420825a..026a4f0 100644
*** a/src/test/regress/expected/psql.out
--- b/src/test/regress/expected/psql.out
*************** invalid variable name: "invalid/name"
*** 11,16 ****
--- 11,33 ----
  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
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 79624b9..d823d11 100644
*** a/src/test/regress/sql/psql.sql
--- b/src/test/regress/sql/psql.sql
***************
*** 10,15 ****
--- 10,25 ----
  -- 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


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