Re: Improvements in psql hooks for variables

Rahila Syed wrote:

> I have applied this patch on latest HEAD and have done basic testing which
> works fine.

Thanks for reviewing this patch!

> > if (current->assign_hook)
> >- (*current->assign_hook) (current->value);
> >- return true;
> >+ {
> >+ confirmed = (*current->assign_hook) (value);
> >+ }
> >+ if (confirmed)
>
> Spurious brackets

OK.

> >static bool
> >+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
> >+{
>
> Contrary to what name suggests this function does not seem to have other
> implementations as in a hook.
> Also this takes care of rejecting a syntactically wrong value only for
> boolean variable hooks like autocommit_hook,
> on_error_stop_hook. However, there are other variable hooks which call
> ParseVariableBool.
> For instance, echo_hidden_hook which is handled separately in the patch.
> Thus there is some duplication of code between generic_boolean_hook and
> echo_hidden_hook.
> Similarly for on_error_rollback_hook.

The purpose of generic_boolean_hook() is to handle the case of a
boolean variable that only accepts ON or OFF, and has its pset.varname
declared as bool. I thought of this case as "generic" because that's
the base case and several variables need no more than that.

ECHO_HIDDEN differs from the generic boolean case because it also
accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When
considering refactoring echo_hidden_hook() to call
generic_boolean_hook() instead of ParseVariableBool() after
having established that the value is not "noexec", I don't see
any advantage in clarity or code size, so I'm not in favor of that change.

The same applies to on_error_rollback_hook(), which has to deal
with a specific enum as well.

> >-static void
> >+static bool
> > fetch_count_hook(const char *newval)
> > {
> > pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
> >+ return true;
> > }
>
> Shouldn't invalid numeric string assignment for numeric variables be
> handled too?

Agreed. Assignments like "\set FETCH_COUNT bogus" don't provoke any
user feedback currently, which is not ideal. I'll add this in a
v3 of the patch tomorrow.

> Instead of generic_boolean_hook cant we have something like follows which
> like generic_boolean_hook can be called from specific variable assignment
> hooks,
>
> static bool
> ParseVariable(newval, VariableName, &pset.var)
> {
> if (VariableName == ‘AUTOCOMMIT’ || ECHO_HIDDEN || other variable with
> hooks which call ParseVariableBool )
> <logic here same as generic_boolean_hook in patch
> <additional lines as there in the patch for ECHO_HIDDEN,
> ON_ERROR_ROLLBACK>
> else if (VariableName == ‘FETCH_COUNT’)
> ParseVariableNum();
> }

It's not possible because pset.var corresponds to different fields from
struct _psqlSettings that have different types: bool, int and some
enum types.
Besides, I don't think it would go well with hooks. If we wanted one
big function that knows all about parsing all built-in variables, we
could just as well dispense with hooks, since their current purpose in
psql is to achieve this parsing, but in a decentralized way.
Or if we keep them, our various built-in variables would be
essentially tied to the same one-big-hook-that-does-all, but isn't
that an antipattern for hooks?

> >@@ -260,7 +276,7 @@ SetVariableAssignHook(VariableSpace space, const char
> *name, VariableAssignHook
> > current->assign_hook = hook;
> > current->next = NULL;
> > previous->next = current;
> >- (*hook) (NULL);
> >+ (void)(*hook) (NULL); /* ignore return value */
>
> Sorry for my lack of understanding, can you explain why is above change
> needed?

"hook" is changed by the patch from [pointer to function returning
void], to [pointer to function returning bool]. The cast to void is
not essential, it just indicates that we deliberately want to
ignore the return value here. I expect some compilers might
complain under a high level of warnings without this cast, although
TBH if you ask me, I wouldn't know which compiler with which flags
exactly.

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