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@manitou-mail.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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>