Re: Improvements in psql hooks for variables

Kindly consider following comments,

ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *valid)
 {
    size_t      len;

+   if (valid)
+       *valid = true;


          psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n",
+                      value, name);
+       if (valid)
+           *valid = false;


Why do we need this? IMO, valid should be always set to true if the value is parsed to be correct.
There should not be an option to the caller to not follow the behaviour of setting valid to either true/false.
As it is in the current patch, all callers of ParseVariableBool follow the behaviour of setting valid with either true/false.

In following examples, incorrect error message is begin displayed.
“ON_ERROR_ROLLBACK” is an enum and also
accepts value 'interactive' .  The error message says boolean expected.

postgres=# \set ON_ERROR_ROLLBACK eretere
unrecognized value "eretere" for "ON_ERROR_ROLLBACK": boolean expected
\set: error while setting variable

Similarly for ECHO_HIDDEN which is also an enum and accepts value 'no_exec'

  postgres=# \set ECHO_HIDDEN NULL
unrecognized value "NULL" for "ECHO_HIDDEN": boolean expected
\set: error while setting variable


+           bool    newval = ParseVariableBool(opt, "\\timing", &success);
+           if (success)
+               pset.timing = newval;
+       }
        else
            pset.timing = !pset.timing;
        if (!pset.quiet)
@@ -2660,7 +2669,14 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
        if (value && pg_strcasecmp(value, "auto") == 0)
            popt->topt.expanded = 2;
        else if (value)
-           popt->topt.expanded = ParseVariableBool(value, param);
+       {
+           bool    valid;
+           bool    newval = ParseVariableBool(value, param, &valid);
+           if (valid)

Should same variable names (success / valid) be used for consistency?