Re: [HACKERS] Improvements in psql hooks for variables - Mailing list pgsql-hackers

I wrote:
> 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 got more excited about doing that after noticing that not only would
it clean up the behavior of those particular variables, but we could get
rid of some code.  First, we'd not need the rather warty GetVariableNum()
anymore, and second, we'd then be almost at the point where every control
variable has a hook, and therefore we could drop tab-complete.c's private
list of known variable names.  That was only ever needed to cover the
possibility of important variables not being present in the variables
list.

So the attached proposed patch does these things:

1. Modify FETCH_COUNT to always have a defined value, like other control
variables, mainly so it will always appear in "\set" output.

2. Add hooks to force HISTSIZE to be defined and require it to have an
integer value.  (I don't see any point in allowing it to be set to
non-integral values.)

3. Add hooks to force IGNOREEOF to be defined and require it to have an
integer value.  Unlike the other cases, here we're trying to be
bug-compatible with a rather bogus externally-defined behavior, so I think
we need to continue to allow "\set IGNOREEOF whatever".  What I propose is
that the substitution hook silently replace non-numeric values with "10",
so that the stored value always reflects what we're really doing.

4. Add a dummy assign hook for HISTFILE, just so it's always in
variables.c's list.  We can't require it to be defined always, because
that would break the interaction with the PSQL_HISTORY environment
variable, so there isn't any change in visible behavior here.

5. Remove tab-complete.c's private list of known variable names.  Given
the other changes, there are no control variables it won't show anyway.
This does mean that if for some reason you've unset one of the status
variables (DBNAME, HOST, etc), that variable would not appear in tab
completion for \set.  But I think that's fine, for at least two reasons:
we shouldn't be encouraging people to use those variables as regular
variables, and if someone does do so anyway, why shouldn't it act just
like a regular variable?

6. Remove no-longer-used-anywhere GetVariableNum().

Any objections?

            regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b9c8fcc..ae58708 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** bar
*** 3247,3258 ****
          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,
--- 3247,3252 ----
*************** bar
*** 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>
--- 3310,3317 ----
          <term><varname>HISTSIZE</varname></term>
          <listitem>
          <para>
!         The maximum number of commands to store in the command history
!         (default 500).  If set to a negative value, no limit is applied.
          </para>
          <note>
          <para>
*************** bar
*** 3345,3357 ****
          <term><varname>IGNOREEOF</varname></term>
          <listitem>
          <para>
!          If unset, sending an <acronym>EOF</> character (usually
           <keycombo action="simul"><keycap>Control</><keycap>D</></>)
           to an interactive session of <application>psql</application>
!          will terminate the application. If set to a numeric value,
!          that many <acronym>EOF</> characters are ignored before the
!          application terminates.  If the variable is set but not to a
!          numeric value, the default is 10.
          </para>
          <note>
          <para>
--- 3337,3349 ----
          <term><varname>IGNOREEOF</varname></term>
          <listitem>
          <para>
!          If set to 1 or less, sending an <acronym>EOF</> character (usually
           <keycombo action="simul"><keycap>Control</><keycap>D</></>)
           to an interactive session of <application>psql</application>
!          will terminate the application.  If set to a larger numeric value,
!          that many consecutive <acronym>EOF</> characters must be typed to
!          make an interactive session terminate.  If the variable is set to a
!          non-numeric value, it is interpreted as 10.
          </para>
          <note>
          <para>
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 5365629..3e3cab4 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*************** helpVariables(unsigned short int pager)
*** 348,356 ****
                        "                     (default: 0=unlimited)\n"));
      fprintf(output, _("  HISTCONTROL        controls command history [ignorespace, ignoredups, ignoreboth]\n"));
      fprintf(output, _("  HISTFILE           file name used to store the command history\n"));
!     fprintf(output, _("  HISTSIZE           the number of commands to store in the command history\n"));
      fprintf(output, _("  HOST               the currently connected database server host\n"));
!     fprintf(output, _("  IGNOREEOF          if unset, sending an EOF to interactive session terminates
application\n"));
      fprintf(output, _("  LASTOID            value of the last affected OID\n"));
      fprintf(output, _("  ON_ERROR_ROLLBACK  if set, an error doesn't stop a transaction (uses implicit
savepoints)\n"));
      fprintf(output, _("  ON_ERROR_STOP      stop batch execution after error\n"));
--- 348,356 ----
                        "                     (default: 0=unlimited)\n"));
      fprintf(output, _("  HISTCONTROL        controls command history [ignorespace, ignoredups, ignoreboth]\n"));
      fprintf(output, _("  HISTFILE           file name used to store the command history\n"));
!     fprintf(output, _("  HISTSIZE           max number of commands to store in the command history\n"));
      fprintf(output, _("  HOST               the currently connected database server host\n"));
!     fprintf(output, _("  IGNOREEOF          number of EOFs needed to terminate an interactive session\n"));
      fprintf(output, _("  LASTOID            value of the last affected OID\n"));
      fprintf(output, _("  ON_ERROR_ROLLBACK  if set, an error doesn't stop a transaction (uses implicit
savepoints)\n"));
      fprintf(output, _("  ON_ERROR_STOP      stop batch execution after error\n"));
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 3e3e97a..b8c9a00 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
*************** finishInput(void)
*** 539,548 ****
  #ifdef USE_READLINE
      if (useHistory && psql_history)
      {
!         int            hist_size;
!
!         hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1);
!         (void) saveHistory(psql_history, hist_size);
          free(psql_history);
          psql_history = NULL;
      }
--- 539,545 ----
  #ifdef USE_READLINE
      if (useHistory && psql_history)
      {
!         (void) saveHistory(psql_history, pset.histsize);
          free(psql_history);
          psql_history = NULL;
      }
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index dc25b4b..6e358e2 100644
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*************** MainLoop(FILE *source)
*** 162,168 ****
                  /* This tries to mimic bash's IGNOREEOF feature. */
                  count_eof++;

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

!                 if (count_eof < pset.ignoreeof)
                  {
                      if (!pset.quiet)
                          printf(_("Use \"\\q\" to leave %s.\n"), pset.progname);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 4c7c3b1..195f5a1 100644
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*************** typedef struct _psqlSettings
*** 125,130 ****
--- 125,132 ----
      bool        singleline;
      bool        singlestep;
      int            fetch_count;
+     int            histsize;
+     int            ignoreeof;
      PSQL_ECHO    echo;
      PSQL_ECHO_HIDDEN echo_hidden;
      PSQL_ERROR_ROLLBACK on_error_rollback;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index a3654e6..88d686a 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** showVersion(void)
*** 774,779 ****
--- 774,784 ----
   * Substitute hooks and assign hooks for psql variables.
   *
   * This isn't an amazingly good place for them, but neither is anywhere else.
+  *
+  * By policy, every special variable that controls any psql behavior should
+  * have one or both hooks, even if they're just no-ops.  This ensures that
+  * the variable will remain present in variables.c's list even when unset,
+  * which ensures that it's known to tab completion.
   */

  static char *
*************** singlestep_hook(const char *newval)
*** 823,839 ****
      return ParseVariableBool(newval, "SINGLESTEP", &pset.singlestep);
  }

  static bool
  fetch_count_hook(const char *newval)
  {
!     if (newval == NULL)
!         pset.fetch_count = -1;    /* default value */
!     else if (!ParseVariableNum(newval, "FETCH_COUNT", &pset.fetch_count))
!         return false;
      return true;
  }

  static char *
  echo_substitute_hook(char *newval)
  {
      if (newval == NULL)
--- 828,899 ----
      return ParseVariableBool(newval, "SINGLESTEP", &pset.singlestep);
  }

+ static char *
+ fetch_count_substitute_hook(char *newval)
+ {
+     if (newval == NULL)
+         newval = pg_strdup("0");
+     return newval;
+ }
+
  static bool
  fetch_count_hook(const char *newval)
  {
!     return ParseVariableNum(newval, "FETCH_COUNT", &pset.fetch_count);
! }
!
! static bool
! histfile_hook(const char *newval)
! {
!     /*
!      * Someday we might try to validate the filename, but for now, this is
!      * just a placeholder to ensure HISTFILE is known to tab completion.
!      */
      return true;
  }

  static char *
+ histsize_substitute_hook(char *newval)
+ {
+     if (newval == NULL)
+         newval = pg_strdup("500");
+     return newval;
+ }
+
+ static bool
+ histsize_hook(const char *newval)
+ {
+     return ParseVariableNum(newval, "HISTSIZE", &pset.histsize);
+ }
+
+ static char *
+ ignoreeof_substitute_hook(char *newval)
+ {
+     int            dummy;
+
+     /*
+      * This tries to mimic the behavior of bash, to wit "If set, the value is
+      * the number of consecutive EOF characters which must be typed as the
+      * first characters on an input line before bash exits.  If the variable
+      * exists but does not have a numeric value, or has no value, the default
+      * value is 10.  If it does not exist, EOF signifies the end of input to
+      * the shell."  Unlike bash, however, we insist on the stored value
+      * actually being a valid integer.
+      */
+     if (newval == NULL)
+         newval = pg_strdup("0");
+     else if (!ParseVariableNum(newval, NULL, &dummy))
+         newval = pg_strdup("10");
+     return newval;
+ }
+
+ static bool
+ ignoreeof_hook(const char *newval)
+ {
+     return ParseVariableNum(newval, "IGNOREEOF", &pset.ignoreeof);
+ }
+
+ static char *
  echo_substitute_hook(char *newval)
  {
      if (newval == NULL)
*************** EstablishVariableSpace(void)
*** 1062,1069 ****
                       bool_substitute_hook,
                       singlestep_hook);
      SetVariableHooks(pset.vars, "FETCH_COUNT",
!                      NULL,
                       fetch_count_hook);
      SetVariableHooks(pset.vars, "ECHO",
                       echo_substitute_hook,
                       echo_hook);
--- 1122,1138 ----
                       bool_substitute_hook,
                       singlestep_hook);
      SetVariableHooks(pset.vars, "FETCH_COUNT",
!                      fetch_count_substitute_hook,
                       fetch_count_hook);
+     SetVariableHooks(pset.vars, "HISTFILE",
+                      NULL,
+                      histfile_hook);
+     SetVariableHooks(pset.vars, "HISTSIZE",
+                      histsize_substitute_hook,
+                      histsize_hook);
+     SetVariableHooks(pset.vars, "IGNOREEOF",
+                      ignoreeof_substitute_hook,
+                      ignoreeof_hook);
      SetVariableHooks(pset.vars, "ECHO",
                       echo_substitute_hook,
                       echo_hook);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d6fffcf..6e759d0 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** append_variable_names(char ***varnames,
*** 3775,3782 ****
  /*
   * This function supports completion with the name of a psql variable.
   * The variable names can be prefixed and suffixed with additional text
!  * to support quoting usages. If need_value is true, only the variables
!  * that have the set values are picked up.
   */
  static char **
  complete_from_variables(const char *text, const char *prefix, const char *suffix,
--- 3775,3783 ----
  /*
   * This function supports completion with the name of a psql variable.
   * The variable names can be prefixed and suffixed with additional text
!  * to support quoting usages. If need_value is true, only variables
!  * that are currently set are included; otherwise, special variables
!  * (those that have hooks) are included even if currently unset.
   */
  static char **
  complete_from_variables(const char *text, const char *prefix, const char *suffix,
*************** complete_from_variables(const char *text
*** 3789,3821 ****
      int            i;
      struct _variable *ptr;

-     static const char *const known_varnames[] = {
-         "AUTOCOMMIT", "COMP_KEYWORD_CASE", "DBNAME", "ECHO", "ECHO_HIDDEN",
-         "ENCODING", "FETCH_COUNT", "HISTCONTROL", "HISTFILE", "HISTSIZE",
-         "HOST", "IGNOREEOF", "LASTOID", "ON_ERROR_ROLLBACK", "ON_ERROR_STOP",
-         "PORT", "PROMPT1", "PROMPT2", "PROMPT3", "QUIET",
-         "SHOW_CONTEXT", "SINGLELINE", "SINGLESTEP",
-         "USER", "VERBOSITY", NULL
-     };
-
      varnames = (char **) pg_malloc((maxvars + 1) * sizeof(char *));

-     if (!need_value)
-     {
-         for (i = 0; known_varnames[i] && nvars < maxvars; i++)
-             append_variable_names(&varnames, &nvars, &maxvars,
-                                   known_varnames[i], prefix, suffix);
-     }
-
      for (ptr = pset.vars->next; ptr; ptr = ptr->next)
      {
          if (need_value && !(ptr->value))
              continue;
-         for (i = 0; known_varnames[i]; i++)        /* remove duplicate entry */
-         {
-             if (strcmp(ptr->name, known_varnames[i]) == 0)
-                 continue;
-         }
          append_variable_names(&varnames, &nvars, &maxvars, ptr->name,
                                prefix, suffix);
      }
--- 3790,3801 ----
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 9ca1000..d9d0763 100644
*** a/src/bin/psql/variables.c
--- b/src/bin/psql/variables.c
*************** ParseVariableNum(const char *value, cons
*** 180,210 ****
  }

  /*
-  * Read integer value of the numeric variable named "name".
-  *
-  * Return defaultval if it is not set, or faultval if its value is not a
-  * valid integer.  (No error message is issued.)
-  */
- int
- GetVariableNum(VariableSpace space,
-                const char *name,
-                int defaultval,
-                int faultval)
- {
-     const char *val;
-     int            result;
-
-     val = GetVariable(space, name);
-     if (!val)
-         return defaultval;
-
-     if (ParseVariableNum(val, NULL, &result))
-         return result;
-     else
-         return faultval;
- }
-
- /*
   * Print values of all variables.
   */
  void
--- 180,185 ----
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index 84be780..1925793 100644
*** a/src/bin/psql/variables.h
--- b/src/bin/psql/variables.h
*************** bool ParseVariableBool(const char *value
*** 81,91 ****
  bool ParseVariableNum(const char *value, const char *name,
                   int *result);

- int GetVariableNum(VariableSpace space,
-                const char *name,
-                int defaultval,
-                int faultval);
-
  void        PrintVariables(VariableSpace space);

  bool        SetVariable(VariableSpace space, const char *name, const char *value);
--- 81,86 ----

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