Re: Improvements in psql hooks for variables

I applied and tested the patch on latest master branch.

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?

Thank you,
Rahila Syed

On Wed, Nov 23, 2016 at 5:47 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> I wrote:
>
> > So I went through the psql commands which don't fail on parse errors
> > for booleans
> > [...]
>
> Here's a v5 patch implementing the suggestions mentioned upthread:
> all meta-commands calling ParseVariableBool() now fail
> when the boolean argument can't be parsed successfully.
>
> Also includes a minor change to SetVariableAssignHook() that now
> returns the result of the hook it calls after installing it.
> It doesn't make any difference in psql behavior since callers
> of SetVariableAssignHook() ignore its return value, but it's
> more consistent with SetVariable().
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>