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