Re: Improvements in psql hooks for variables

	Tom Lane wrote:

> "Daniel Verite" <daniel@manitou-mail.org> writes:
> > [ psql-var-hooks-v6.patch ]
> 
> I took a quick look through this.  It seems to be going in generally
> the right direction, but here's a couple of thoughts:

Thanks for looking into this!
 
> I'm inclined to think that the best choice is for the function result
> to be the ok/not ok flag, and pass the variable-to-be-modified as an
> output parameter.  That fits better with the notion that the variable
> is not to be modified on failure.

Agreed, if never clobbering the variable, having the valid/invalid state
returned by ParseVariableBool() allows for simpler code. I'm changing it
that way.

> Also, why aren't you using ParseVariableBool's existing ability to report
> errors?

Its' because there are two cases:
- either only a boolean can be given to the command or variable,
in which we let ParseVariableBool() tell:
   unrecognized value "bogus" for "command": boolean expected

- or other parameters besides boolean are acceptable, in which case we
can't say "boolean expected", because in fact a boolean is no more or
less expected than the other valid values.

We could shave code by reducing ParseVariableBool()'s error message to:
  unrecognized value "bogus" for "name"
clearing the distinction between [only booleans are expected]
and [booleans or enum are expected].
Then almost all callers that have their own message could rely
on ParseVariableBool() instead, as they did previously.

Do we care about the "boolean expected" part of the error message?

>          else if (value)
> !         {
> !             bool    valid;
> !             bool    newval = ParseVariableBool(value, NULL, &valid);
> !             if (valid)
> !                 popt->topt.expanded = newval;
> !             else
> !             {
> !                 psql_error("unrecognized value \"%s\" for \"%s\"\n",
> !                            value, param);
> !                 return false;
> !             }
> !         }
> is really the hard way and you could have just done
> 
> -             popt->topt.expanded = ParseVariableBool(value, param);
> +             success = ParseVariableBool(value, param,
> &popt->topt.expanded);

I get the idea, except that in this example, the compiler is
unhappy because popt->topt.expanded is not a bool, and an
explicit cast here would be kludgy.

For the error printing part, it would go away with the above 
suggestion


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