Re: Surprising behaviour of \set AUTOCOMMIT ON

Thank you for comments.

>But there's a function in startup.c which might be the ideal location
>or the check, as it looks like the one and only place where the
>autocommit flag actually changes:

>static void
>autocommit_hook(const char *newval)

Thank you for pointing out this. This indeed is the only place where
autocommit setting changes in psql. However,
including the check here will require returning the status of command from
this hook. i.e if we throw error inside this
hook we will need to return false indicating the value has not changed.
This will change the existing definition of the hook
which returns void. This will also lead to changes in other implementations
of this hook apart from autocommit_hook.

>There are other values than ON: true/yes and theirs abbreviations,
>the value 1, and anything that doesn't resolve to OFF is taken as ON.
>ParseVariableBool() in commands.c already does the job of converting
>these to bool, the new code should probably just call that function
>instead of parsing the value itself.

I have included this in the attached patch.

Also I have included prior review comments by Rushabh Lathia and Amit
Langote in the attached patch

Regarding suggestion to move the code inside SetVariable(), I think it is
not a good idea because
SetVariable() and variable.c file in which it is present is about handling
of psql variables, checks for
correct variable names, values etc. This check is about correctness of the
command so it is better placed
in exec_command().

Thank you,
Rahila Syed

On Sat, Sep 3, 2016 at 4:39 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> Rushabh Lathia wrote:
>
> > It might happen that SetVariable() can be called from other places in
> > future,
> > so its good to have all the set variable related checks inside
> > SetVariable().
>
> There's already another way to skip the \set check:
> select 'on' as "AUTOCOMMIT" \gset
>
> But there's a function in startup.c which might be the ideal location
> for the check, as it looks like the one and only place where the
> autocommit flag actually changes:
>
> static void
> autocommit_hook(const char *newval)
> {
> pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
> }
>
>
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>