Re: Improvements in psql hooks for variables

  Hi,

I'm attaching v3 of the patch with error reporting for
FETCH_COUNT as mentioned upthread, and rebased
on the most recent master.

> I was suggesting change on the lines of extending generic_boolean_hook to
> include enum(part in enum_hooks which calls ParseVariableBool) and
> integer parsing as well.

Well, generic_boolean_hook() is meant to change this, for instance:

  static void
  on_error_stop_hook(const char *newval)
  {
     pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
  }

into that:

  static bool
  on_error_stop_hook(const char *newval)
  {
     return generic_boolean_hook(newval, "ON_ERROR_STOP",
       &pset.on_error_stop);
   }

with the goal that the assignment does not occur if "newval" is bogus.
The change is really minimal.

When we're dealing with enum-or-bool variables, such as for instance
ECHO_HIDDEN, the patch replaces this:

  static void
  echo_hidden_hook(const char *newval)
  {
    if (newval == NULL)
      pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
    else if (pg_strcasecmp(newval, "noexec") == 0)
      pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
    else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
      pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
    else  /* ParseVariableBool printed msg if needed */
      pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
  }

with that:

  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
    {
      bool isvalid;
      bool val = ParseVariableBool(newval, "ECHO_HIDDEN", &isvalid);
      if (!isvalid)
	return false; /* ParseVariableBool printed msg */
      pset.echo_hidden = val ? PSQL_ECHO_HIDDEN_ON : PSQL_ECHO_HIDDEN_OFF;
    }
    return true;
  }

The goal being again to reject a bogus assignment, as opposed to replacing
it with any hardwired value.
Code-wise, we can't call generic_boolean_hook() here because we need
to assign a non-boolean specific value after having parsed the ON/OFF
user-supplied string.

More generally, it turns out that the majority of hooks are concerned
by this patch, as they parse user-supplied values, but there
are 4 distinct categories of variables:

1- purely ON/OFF vars:
   AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP

2- ON/OFF mixed with enum values:
  ECHO_HIDDEN, ON_ERROR_ROLLBACK

3- purely enum values:
  COMP_KEYWORD_CASE, HISTCONTROL, ECHO, VERBOSITY, SHOW_CONTEXT

4- numeric values:
  FETCH_COUNT

If you suggest that the patch should refactor the implementation
of hooks for case #2, only two hooks are concerned and they consist
of non-mergeable enum-specific code interleaved with generic code,
so I don't foresee any gain in fusioning. I have the same opinion about
merging any of #1, #2, #3, #4 together.
But feel free to post code implementing your idea if you disagree,
maybe I just don't figure out what you have in mind.
For case #3, these hooks clearly follow a common pattern, but I also
don't see any benefit in an opportunistic rewrite given the nature of
the functions.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite