Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" <daniel@manitou-mail.org> writes:
>     Tom Lane wrote:
>> One possible compromise that would address your concern about display
>> is to modify the hook API some more so that variable hooks could actually
>> substitute new values.  Then for example the bool-variable hooks could
>> effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
>> "\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
>> of their values always produces sane results.

> Agreed, that looks like a good compromise.

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.

Maybe we should merge SetVariableSubstituteHook and SetVariableAssignHook
into one function?

            regards, tom lane

diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 0574b5b..985cfcb 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** 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)
  {
--- 777,804 ----


  /*
!  * 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)
  {
*************** EstablishVariableSpace(void)
*** 1002,1015 ****
--- 1019,1039 ----
  {
      pset.vars = CreateVariableSpace();

+     SetVariableSubstituteHook(pset.vars, "AUTOCOMMIT", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook);
+     SetVariableSubstituteHook(pset.vars, "ON_ERROR_STOP", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
+     SetVariableSubstituteHook(pset.vars, "QUIET", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "QUIET", quiet_hook);
+     SetVariableSubstituteHook(pset.vars, "SINGLELINE", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "SINGLELINE", singleline_hook);
+     SetVariableSubstituteHook(pset.vars, "SINGLESTEP", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "SINGLESTEP", singlestep_hook);
      SetVariableAssignHook(pset.vars, "FETCH_COUNT", fetch_count_hook);
      SetVariableAssignHook(pset.vars, "ECHO", echo_hook);
+     SetVariableSubstituteHook(pset.vars, "ECHO_HIDDEN", bool_substitute_hook);
      SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook);
+     SetVariableSubstituteHook(pset.vars, "ON_ERROR_ROLLBACK", bool_substitute_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);
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 91e4ae8..61c4ccc 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,288 ****
                  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;
  }

  /*
--- 274,358 ----
                  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 a substitute 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.  (But the hook
+  * might immediately change the NULL to something else.)
+  *
+  * The hook is immediately called on the variable's current value.
+  */
+ void
+ SetVariableSubstituteHook(VariableSpace space, const char *name,
+                           VariableSubstituteHook hook)
+ {
+     struct _variable *current,
+                *previous;
+
+     if (!space || !name)
+         return;
+
+     if (!valid_variable_name(name))
+         return;
+
+     for (previous = space, current = space->next;
+          current;
+          previous = current, current = current->next)
+     {
+         if (strcmp(current->name, name) == 0)
+         {
+             /* found entry, so update */
+             current->substitute_hook = hook;
+             current->value = (*hook) (current->value);
+             return;
+         }
+     }
+
      /* not present, make new entry */
      current = pg_malloc(sizeof *current);
      current->name = pg_strdup(name);
!     current->value = NULL;
!     current->substitute_hook = hook;
      current->assign_hook = NULL;
      current->next = NULL;
      previous->next = current;
!     current->value = (*hook) (current->value);
  }

  /*
*************** SetVariable(VariableSpace space, const c
*** 299,305 ****
   * get called before any user-supplied value is assigned.
   */
  void
! SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook)
  {
      struct _variable *current,
                 *previous;
--- 369,376 ----
   * get called before any user-supplied value is assigned.
   */
  void
! SetVariableAssignHook(VariableSpace space, const char *name,
!                       VariableAssignHook hook)
  {
      struct _variable *current,
                 *previous;
*************** SetVariableAssignHook(VariableSpace spac
*** 327,332 ****
--- 398,404 ----
      current = pg_malloc(sizeof *current);
      current->name = pg_strdup(name);
      current->value = NULL;
+     current->substitute_hook = NULL;
      current->assign_hook = hook;
      current->next = NULL;
      previous->next = current;
*************** 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;
  }

  /*
--- 423,429 ----
  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..f382bfd 100644
*** a/src/bin/psql/variables.h
--- b/src/bin/psql/variables.h
***************
*** 18,45 ****
   * 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.
   */
  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;
  };
--- 18,70 ----
   * 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 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.
   */
  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 SetVariableSubstituteHook(),
+  * it is applied to the variable's current value (typically NULL, if it wasn't
+  * set yet).  It's usually best to do that before installing the assign hook
+  * if there is one.
+  */
+ 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,70 ****
--- 90,96 ----
  void        PrintVariables(VariableSpace space);

  bool        SetVariable(VariableSpace space, const char *name, const char *value);
+ void        SetVariableSubstituteHook(VariableSpace space, const char *name, VariableSubstituteHook hook);
  void        SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook);
  bool        SetVariableBool(VariableSpace space, const char *name);
  bool        DeleteVariable(VariableSpace space, const char *name);
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