Re: Improvements in psql hooks for variables

Hello,

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

Some comments,

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

Spurious brackets

>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.

>-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?

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();
}
This will help merge the logic which is to check for valid syntax before
assigning and returning false on error, in one place.

>@@ -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?

Thank you,
Rahila Syed

On Tue, Sep 20, 2016 at 11:16 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> Ashutosh Bapat wrote:
>
> > You may want to add this to the November commitfest
> > https://commitfest.postgresql.org/11/.
>
> Done. It's at https://commitfest.postgresql.org/11/799/
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>