Re: Improvements in psql hooks for variables

Hi,

Following the discussion on forbidding an AUTOCOMMIT off->on
switch mid-transaction [1], attached is a patch that let the hooks
return a boolean indicating whether a change is allowed.

Using the hooks, bogus assignments to built-in variables can
be dealt with more strictly.

For example, pre-patch behavior:

=# \set ECHO errors
=# \set ECHO on
unrecognized value "on" for "ECHO"; assuming "none"
=# \echo :ECHO
on

which has two problems:
- we have to assume a value, even though we can't know what the user meant.
- after assignment, the user-visible value of the variable diverges from its
internal counterpart (pset.echo in this case).

Post-patch:
=# \set ECHO errors
=# \set ECHO on
unrecognized value "on" for "ECHO"
\set: error while setting variable
=# \echo :ECHO
errors

Both the internal pset.* state and the user-visible value are kept unchanged
is the input value is incorrect.

Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
a switch when the conditions are not met.

Another user-visible effect of the patch is that, using a bogus value
for a built-in variable on the command-line becomes a fatal error
that prevents psql to continue.
This is not directly intended by the patch but is a consequence
of SetVariable() failing.

Example:
$ ./psql -vECHO=bogus
unrecognized value "bogus" for "ECHO"
psql: could not set variable "ECHO"
$ echo $?
1

The built-in vars concerned by the change are:

booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP

non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
HISTCONTROL, VERBOSITY, SHOW_CONTEXT

We could go further to close the gap between pset.* and the built-in
variables,
by changing how they're initialized and forbidding deletion as Tom
suggests in [2], but if there's negative feedback on the above changes,
I think we should hear it first.

[1]
/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
[2]
/message-id/4695.1473961140%40sss.pgh.pa.us

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

You may want to add this to the November commitfest
https://commitfest.postgresql.org/11/.

On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite <daniel(at)manitou-mail(dot)org> wrote:
> Hi,
>
> Following the discussion on forbidding an AUTOCOMMIT off->on
> switch mid-transaction [1], attached is a patch that let the hooks
> return a boolean indicating whether a change is allowed.
>
> Using the hooks, bogus assignments to built-in variables can
> be dealt with more strictly.
>
> For example, pre-patch behavior:
>
> =# \set ECHO errors
> =# \set ECHO on
> unrecognized value "on" for "ECHO"; assuming "none"
> =# \echo :ECHO
> on
>
> which has two problems:
> - we have to assume a value, even though we can't know what the user meant.
> - after assignment, the user-visible value of the variable diverges from its
> internal counterpart (pset.echo in this case).
>
>
> Post-patch:
> =# \set ECHO errors
> =# \set ECHO on
> unrecognized value "on" for "ECHO"
> \set: error while setting variable
> =# \echo :ECHO
> errors
>
> Both the internal pset.* state and the user-visible value are kept unchanged
> is the input value is incorrect.
>
> Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
> a switch when the conditions are not met.
>
>
> Another user-visible effect of the patch is that, using a bogus value
> for a built-in variable on the command-line becomes a fatal error
> that prevents psql to continue.
> This is not directly intended by the patch but is a consequence
> of SetVariable() failing.
>
> Example:
> $ ./psql -vECHO=bogus
> unrecognized value "bogus" for "ECHO"
> psql: could not set variable "ECHO"
> $ echo $?
> 1
>
> The built-in vars concerned by the change are:
>
> booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
>
> non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
> HISTCONTROL, VERBOSITY, SHOW_CONTEXT
>
> We could go further to close the gap between pset.* and the built-in
> variables,
> by changing how they're initialized and forbidding deletion as Tom
> suggests in [2], but if there's negative feedback on the above changes,
> I think we should hear it first.
>
> [1]
> /message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
> [2]
> /message-id/4695.1473961140%40sss.pgh.pa.us
>
>
> 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
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hello,

I am beginning to review this patch. Initial comment. I got following
compilation error when I applied the patch on latest sources and run make.

command.c: In function ‘exec_command’:
*command.c:257:5*: error: too few arguments to function ‘ParseVariableBool’
ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix) ?
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
*command.c:1551:4*: error: too few arguments to function ‘ParseVariableBool’
pset.timing = ParseVariableBool(opt, "\\timing");
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
command.c: In function ‘do_pset’:
*command.c:2663:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.expanded = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
*command.c:2672:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.numericLocale = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
*command.c:2727:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.tuples_only = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
*command.c:2759:4*: error: too few arguments to function ‘ParseVariableBool’
if (ParseVariableBool(value, param))
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^
*command.c:2781:4:* error: too few arguments to function ‘ParseVariableBool’
popt->topt.default_footer = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
from command.c:50:
variables.h:38:7: note: declared here
bool ParseVariableBool(const char *value, const char *name, bool *valid);
^

Thank you,
Rahila Syed

On Mon, Sep 19, 2016 at 11:21 AM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> You may want to add this to the November commitfest
> https://commitfest.postgresql.org/11/.
>
> On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
> wrote:
> > Hi,
> >
> > Following the discussion on forbidding an AUTOCOMMIT off->on
> > switch mid-transaction [1], attached is a patch that let the hooks
> > return a boolean indicating whether a change is allowed.
> >
> > Using the hooks, bogus assignments to built-in variables can
> > be dealt with more strictly.
> >
> > For example, pre-patch behavior:
> >
> > =# \set ECHO errors
> > =# \set ECHO on
> > unrecognized value "on" for "ECHO"; assuming "none"
> > =# \echo :ECHO
> > on
> >
> > which has two problems:
> > - we have to assume a value, even though we can't know what the user
> meant.
> > - after assignment, the user-visible value of the variable diverges from
> its
> > internal counterpart (pset.echo in this case).
> >
> >
> > Post-patch:
> > =# \set ECHO errors
> > =# \set ECHO on
> > unrecognized value "on" for "ECHO"
> > \set: error while setting variable
> > =# \echo :ECHO
> > errors
> >
> > Both the internal pset.* state and the user-visible value are kept
> unchanged
> > is the input value is incorrect.
> >
> > Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
> > a switch when the conditions are not met.
> >
> >
> > Another user-visible effect of the patch is that, using a bogus value
> > for a built-in variable on the command-line becomes a fatal error
> > that prevents psql to continue.
> > This is not directly intended by the patch but is a consequence
> > of SetVariable() failing.
> >
> > Example:
> > $ ./psql -vECHO=bogus
> > unrecognized value "bogus" for "ECHO"
> > psql: could not set variable "ECHO"
> > $ echo $?
> > 1
> >
> > The built-in vars concerned by the change are:
> >
> > booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
> >
> > non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
> > HISTCONTROL, VERBOSITY, SHOW_CONTEXT
> >
> > We could go further to close the gap between pset.* and the built-in
> > variables,
> > by changing how they're initialized and forbidding deletion as Tom
> > suggests in [2], but if there's negative feedback on the above changes,
> > I think we should hear it first.
> >
> > [1]
> > /message-id/f2cb5838-0ee9-4fe3-
> acc0-df77aeb7d4c7%40mm
> > [2]
> > /message-id/4695.1473961140%40sss.pgh.pa.us
> >
> >
> > 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
> >
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>
> --
> 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
>

Rahila Syed wrote:

> I am beginning to review this patch. Initial comment. I got following
> compilation error when I applied the patch on latest sources and run make.

Sorry about that, I forgot to make clean, here's an updated patch.

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

> Sorry about that, I forgot to make clean, here's an updated patch.
Ongoing CMake changes will help to avoid such things, "out of source build".

On Mon, Sep 19, 2016 at 6:20 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> Rahila Syed wrote:
>
>
> > I am beginning to review this patch. Initial comment. I got following
> > compilation error when I applied the patch on latest sources and run
> make.
>
> Sorry about that, I forgot to make clean, here's an updated patch.
>
> 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
>
>

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

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
>

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

>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

I agree if generic_boolean_hook() is used in its current form instead of
ParseVariableBool() inside
echo_hidden_hook or on_error_rollback_hook the code will not change much.

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.

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

I was suggesting something on the lines of having common parsing logic for
each implementation of hook. This is similar to what ParseVariableBool does
in
the existing code. ParseVariableBool is being called from different hooks
to
parse the boolean value of the variable. Thus representing common code in
various
implementations of hook.

>"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.
Thank you for explanation.

Thank you,
Rahila Syed

On Fri, Nov 11, 2016 at 2:57 AM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

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

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

Daniel,

* Daniel Verite (daniel(at)manitou-mail(dot)org) wrote:
> I'm attaching v3 of the patch with error reporting for
> FETCH_COUNT as mentioned upthread, and rebased
> on the most recent master.

Just fyi, there seems to be some issues with this patch because setting
my PROMPT1 and PROMPT2 variables result in rather odd behavior.

Here's what I use:

-------------
\set PROMPT1 '\033[33;1m%M(from '`hostname`').%/.%n.%> [%`date`]\033[0m\n=%x%# '
\set PROMPT2 '-%x%# '
-------------

In reviewing this patch, I also noticed that it's set up to assume a
'true' result when a variable can't be parsed by ParseVariableBool.

-----------
postgres=# \timing off
Timing is off.
postgres=# \timing asdsa
unrecognized value "asdsa" for "\timing": boolean expected
Timing is on.
-----------

That certainly doesn't feel right. I'm thinking that if we're going to
throw an error back to the user about a value being invalid then we
shouldn't change the current value.

My initial thought was that perhaps we should pass the current value to
ParseVariableBool() and let it return whatever the "right" answer is,
however, your use of ParseVariableBool() for enums that happen to accept
on/off means that won't really work.

Perhaps the right answer is to flip this around a bit and treat booleans
as a special case of enums and have a generic solution for enums.
Consider something like:

ParseVariableEnum(valid_enums, str_value, name, curr_value);

'valid_enums' would be a simple list of what the valid values are for a
given variable and their corresponding value, str_value the string the
user typed, name the name of the variable, and curr_value the current
value of the variable.

ParseVariableEnum() could then detect if the string passed in is valid
or not and report to the user if it's incorrect and leave the existing
value alone. This could also generically handle the question of if the
string passed in is a unique prefix of a correct value by comparing it
to all of the valid values and seeing if there's a unique match or not.

Thoughts?

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> In reviewing this patch, I also noticed that it's set up to assume a
> 'true' result when a variable can't be parsed by ParseVariableBool.

I suppose that's meant to be backwards-compatible with the current
behavior:

regression=# \timing foo
unrecognized value "foo" for "\timing"; assuming "on"
Timing is on.

but I agree that if we're changing things in this area, that would
be high on my list of things to change. I think what we want going
forward is to disallow setting "special" variables to invalid values,
and that should hold both for regular variables that have special
behaviors, and for the special-syntax cases like \timing.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > In reviewing this patch, I also noticed that it's set up to assume a
> > 'true' result when a variable can't be parsed by ParseVariableBool.
>
> I suppose that's meant to be backwards-compatible with the current
> behavior:

Ah, good point, however..

> but I agree that if we're changing things in this area, that would
> be high on my list of things to change. I think what we want going
> forward is to disallow setting "special" variables to invalid values,
> and that should hold both for regular variables that have special
> behaviors, and for the special-syntax cases like \timing.

I completely agree with you here. We shouldn't be assuming "invalid"
means "true".

Thanks!

Stephen

Tom Lane wrote:

> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > In reviewing this patch, I also noticed that it's set up to assume a
> > 'true' result when a variable can't be parsed by ParseVariableBool.
>
> I suppose that's meant to be backwards-compatible with the current
> behavior:
>
> regression=# \timing foo
> unrecognized value "foo" for "\timing"; assuming "on"
> Timing is on.

Exactly. The scope of the patch is limited to the effect
of \set assignments to built-in variables.

> but I agree that if we're changing things in this area, that would
> be high on my list of things to change. I think what we want going
> forward is to disallow setting "special" variables to invalid values,
> and that should hold both for regular variables that have special
> behaviors, and for the special-syntax cases like \timing.

+1

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

Daniel,

* Daniel Verite (daniel(at)manitou-mail(dot)org) wrote:
> Tom Lane wrote:
>
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > In reviewing this patch, I also noticed that it's set up to assume a
> > > 'true' result when a variable can't be parsed by ParseVariableBool.
> >
> > I suppose that's meant to be backwards-compatible with the current
> > behavior:
> >
> > regression=# \timing foo
> > unrecognized value "foo" for "\timing"; assuming "on"
> > Timing is on.
>
> Exactly. The scope of the patch is limited to the effect
> of \set assignments to built-in variables.
>
> > but I agree that if we're changing things in this area, that would
> > be high on my list of things to change. I think what we want going
> > forward is to disallow setting "special" variables to invalid values,
> > and that should hold both for regular variables that have special
> > behaviors, and for the special-syntax cases like \timing.
>
> +1

Not sure I follow your reply here. There seems to be broad agreement to
improve how we handle both \set and "special" variables and the code
paths are related and this patch is touching them, so it seems like the
correct next step here is to adjust the patch to update the code based
on that agreement.

Are you working to make those changes? I'd rather we make the changes
to this code once rather than push what you have now only to turn around
and change it significantly again.

Thanks!

Stephen

Stephen Frost wrote:

> Just fyi, there seems to be some issues with this patch because setting
> my PROMPT1 and PROMPT2 variables result in rather odd behavior.

Good catch! The issue is that the patch broke the assumption
of prompt hooks that their argument points to a long-lived buffer.
The attached v4 fixes the bug by passing to hooks a pointer to the final
strdup'ed value in VariableSpace rather than temp space, as commented
in SetVariable().

Also I've changed something else in ParseVariableBool(). The code assumes
"false" when value==NULL, but when value is an empty string, the result
was true and considered valid, due to the following test being
positive when len==0 (both with HEAD or the v3 patch from this thread):

if (pg_strncasecmp(value, "true", len) == 0)
return true;
It happens that "" as a value yields the same result than "true" for this
test so it passes, but it seems rather unintentional.

The resulting problem from the POV of the user is that we can do that,
for instance:

test=> \set AUTOCOMMIT

without error message or feedback, and that leaves us without much
clue about autocommit being enabled:

test=> \echo :AUTOCOMMIT

test=>

So I've changed ParseVariableBool() in v4 to reject empty string as an
invalid boolean (but not NULL since the startup logic requires NULL
meaning false in the early initialization of these variables).

"make check" seems OK with that, I hope it doesn't cause any regression
elsewhere.

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

Stephen Frost wrote:

> Are you working to make those changes? I'd rather we make the changes
> to this code once rather than push what you have now only to turn around
> and change it significantly again.

If, as a maintainer, you prefer this together in one patch, I'm fine
with that. So I'll update it shortly with changes in \timing and
a few other callers of ParseVariableBool().

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

Daniel,

* Daniel Verite (daniel(at)manitou-mail(dot)org) wrote:
> "make check" seems OK with that, I hope it doesn't cause any regression
> elsewhere.

You can see what the code coverage of psql is in our current regression
tests by going here:

http://coverage.postgresql.org/src/bin/psql/index.html

It's not exactly a pretty sight and certainly not all callers of
ParseVariableBool() are covered.

I'd strongly suggest you either do sufficient manual testing, or add
regression tests, most likely using the tap test system (you can see an
example of that in src/bin/pg_dump/t and in other 't' directories).

You can generate that report after you make changes yourself using
'make coverage-html'.

Thanks!

Stephen

Daniel,

* Daniel Verite (daniel(at)manitou-mail(dot)org) wrote:
> Stephen Frost wrote:
>
> > Are you working to make those changes? I'd rather we make the changes
> > to this code once rather than push what you have now only to turn around
> > and change it significantly again.
>
> If, as a maintainer, you prefer this together in one patch, I'm fine
> with that. So I'll update it shortly with changes in \timing and
> a few other callers of ParseVariableBool().

Did you get a chance to review and consider the other comments from my
initial review about how we might use a different approach for bools, et
al?

Thanks!

Stephen

Stephen Frost wrote:

> Are you working to make those changes? I'd rather we make the changes
> to this code once rather than push what you have now only to turn around
> and change it significantly again.

So I went through the psql commands which don't fail on parse errors
for booleans. I'd like to attract attention on \c, because I see
several options.

\c [-reuse-previous=BOOL] ...other args..

Current: if we can't parse BOOL, assume it's ON, and go on with reconnecting.

Option1: if we can't parse BOOL, stop here, don't reconnect, set
the command result as "failed", also ignoring the other arguments.

Option2: maybe we want to create a difference between interactive
and non-interactive use, as there's already one in handling
the failure to connect through \c.
The manpage says:
If the connection attempt failed (wrong user name, access denied,
etc.), the previous connection will only be kept if psql is in
interactive mode. When executing a non-interactive script,
processing will immediately stop with an error.

Proposal: if interactive, same as Option1.
If not interactive, -reuse-previous=bogus has the same outcome
as a failure to connect. Which I think means two subcases:
if it's through \i then kill the connection but don't quit psql
if it's through -f then quit psql.

Option3: leave this command unchanged, avoiding trouble.

\timing BOOL

Current: non-parseable BOOL interpreted as TRUE. Empty BOOL toggles the
state.

Proposal: on non-parseable BOOL, command failure and pset.timing is
left unchanged.

\pset [x | expanded | vertical ] BOOL
\pset numericlocale BOOL
\pset [tuples_only | t] BOOL
\pset footer BOOL
\t BOOL (falls into pset_do("tuples_only", ...))
\pset pager BOOL

Current: non-parseable non-empty BOOL interpreted as TRUE. Empty BOOL
toggles the on/off state. In some cases, BOOL interpretation is attempted
only after specific built-in values have been ruled out first.

Proposal: non-parseable BOOL implies command failure and unchanged state.

About the empty string when a BOOL is expected. Only \c -reuse-previous
seems concerned:

#= \c -reuse-previous=
acts the same as
#= \c -reuse-previous=ON

Proposal: handle empty as when the value is bogus.

The other commands interpret this lack of value specifically to toggle
the state, so it's a non-issue for them.

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

Stephen Frost wrote:

> That certainly doesn't feel right. I'm thinking that if we're going to
> throw an error back to the user about a value being invalid then we
> shouldn't change the current value.
>
> My initial thought was that perhaps we should pass the current value to
> ParseVariableBool() and let it return whatever the "right" answer is,
> however, your use of ParseVariableBool() for enums that happen to accept
> on/off means that won't really work.

That's not needed once ParseVariableBool() informs the caller about
the validity of the boolean expression, which is what the patch already
does.

For instance I just implemented it for \timing and the diff consists of just
that:

if (opt)
- pset.timing = ParseVariableBool(opt, "\\timing",
NULL);
+ {
+ bool newval = ParseVariableBool(opt, "\\timing",
&success);
+ if (success)
+ pset.timing = newval;
+ }
else
pset.timing = !pset.timing;

That makes \timing foobar being rejected as a bad command with a
proper error message and no change of state, which is just what we want.

> Perhaps the right answer is to flip this around a bit and treat booleans
> as a special case of enums and have a generic solution for enums.
> Consider something like:
>
> ParseVariableEnum(valid_enums, str_value, name, curr_value);
>
> 'valid_enums' would be a simple list of what the valid values are for a
> given variable and their corresponding value, str_value the string the
> user typed, name the name of the variable, and curr_value the current
> value of the variable.

Firstly I'd like to insist that such a refactoring is not necessary
for this patch and I feel like it would be out of place in it.

That being said, if we wanted this, I think it would be successful
only if we'd first change our internal variables pset.* from a struct
of different types to a list of variables from some kind of common
abstract type and an abstraction layer to access them.
That would be an order of magnitude more sophisticated than what we
have.

Otherwise as I tried to explain in [1], I don't see how we could write
a ParseVariableEnum() that would return different types
and take variable inputs.
Or if we say that ParseVariableEnum should not return the value
but affect the variable directly, that would require refactoring
all call sites, and what's the benefit that would justify
such large changes?
Plus we have two different non-mergeable concepts of variables
that need this parser:
psql variables from VariableSpace stored as strings,
and C variables directly instantiated as native types.

Also, the argument that bools are just another type of enums
is legitimate in theory, but as in psql we accept any left-anchored
match of true/false/on/off/0/1, it means that the enumeration
of values is in fact:

0
1
t
tr
tru
true
f
fa
fal
fals
false
on
of
off

I don't see that it would help if the code treated the above like just a
vanilla list of values, comparable to the other qualifiers like "auto",
"expanded", "vertical", an so on, notwithstanding the fact
that they don't share the same types.

I think that the current code with ParseVariableBool() that only
handles booleans is better in terms of separation of concerns
and readability.

[1]
/message-id/fc879967-da93-43b6-aa5a-92f2d825e786@mm

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

I wrote:

> So I went through the psql commands which don't fail on parse errors
> for booleans
> [...]

Here's a v5 patch implementing the suggestions mentioned upthread:
all meta-commands calling ParseVariableBool() now fail
when the boolean argument can't be parsed successfully.

Also includes a minor change to SetVariableAssignHook() that now
returns the result of the hook it calls after installing it.
It doesn't make any difference in psql behavior since callers
of SetVariableAssignHook() ignore its return value, but it's
more consistent with SetVariable().

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

On Wed, Nov 23, 2016 at 11:17 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> I wrote:
>
> > So I went through the psql commands which don't fail on parse errors
> > for booleans
> > [...]
>
> Here's a v5 patch implementing the suggestions mentioned upthread:
> all meta-commands calling ParseVariableBool() now fail
> when the boolean argument can't be parsed successfully.
>
> Also includes a minor change to SetVariableAssignHook() that now
> returns the result of the hook it calls after installing it.
> It doesn't make any difference in psql behavior since callers
> of SetVariableAssignHook() ignore its return value, but it's
> more consistent with SetVariable().

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

I applied and tested the patch on latest master branch.

Kindly consider following comments,

ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *valid)
{
size_t len;

+ if (valid)
+ *valid = true;

psql_error("unrecognized value \"%s\" for \"%s\": boolean
expected\n",
+ value, name);
+ if (valid)
+ *valid = false;

Why do we need this? IMO, valid should be always set to true if the value
is parsed to be correct.
There should not be an option to the caller to not follow the behaviour of
setting valid to either true/false.
As it is in the current patch, all callers of ParseVariableBool follow the
behaviour of setting valid with either true/false.

In following examples, incorrect error message is begin displayed.
“ON_ERROR_ROLLBACK” is an enum and also
accepts value 'interactive' . The error message says boolean expected.

postgres=# \set ON_ERROR_ROLLBACK eretere
unrecognized value "eretere" for "ON_ERROR_ROLLBACK": boolean expected
\set: error while setting variable

Similarly for ECHO_HIDDEN which is also an enum and accepts value 'no_exec'

postgres=# \set ECHO_HIDDEN NULL
unrecognized value "NULL" for "ECHO_HIDDEN": boolean expected
\set: error while setting variable

+ bool newval = ParseVariableBool(opt, "\\timing", &success);
+ if (success)
+ pset.timing = newval;
+ }
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2669,14 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
- popt->topt.expanded = ParseVariableBool(value, param);
+ {
+ bool valid;
+ bool newval = ParseVariableBool(value, param, &valid);
+ if (valid)

Should same variable names (success / valid) be used for consistency?

Thank you,
Rahila Syed

On Wed, Nov 23, 2016 at 5:47 PM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> I wrote:
>
> > So I went through the psql commands which don't fail on parse errors
> > for booleans
> > [...]
>
> Here's a v5 patch implementing the suggestions mentioned upthread:
> all meta-commands calling ParseVariableBool() now fail
> when the boolean argument can't be parsed successfully.
>
> Also includes a minor change to SetVariableAssignHook() that now
> returns the result of the hook it calls after installing it.
> It doesn't make any difference in psql behavior since callers
> of SetVariableAssignHook() ignore its return value, but it's
> more consistent with SetVariable().
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>

Rahila Syed wrote:

> Kindly consider following comments,

Sorry for taking so long to post an update.

> There should not be an option to the caller to not follow the behaviour of
> setting valid to either true/false.

OK, fixed.

> In following examples, incorrect error message is begin displayed.
> “ON_ERROR_ROLLBACK” is an enum and also
> accepts value 'interactive' . The error message says boolean expected.

Indeed. Fixed for all callers of ParseVariableBool() than can accept
non-boolean arguments too.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> [ psql-var-hooks-v6.patch ]

I took a quick look through this. It seems to be going in generally
the right direction, but here's a couple of thoughts:

* It seems like you're making life hard for yourself, and confusing for
readers, by having opposite API conventions at different levels. You've
got ParseVariableBool returning the parsed value as function result with
validity flag going into an output parameter, but the boolean variable
hooks do it the other way.

I'm inclined to think that the best choice is for the function result
to be the ok/not ok flag, and pass the variable-to-be-modified as an
output parameter. That fits better with the notion that the variable
is not to be modified on failure. You're having to change every caller
of ParseVariableBool anyway, so changing them a bit more doesn't seem
like a problem. I think actually you don't need generic_boolean_hook()
at all if you do that; it appears to do nothing except fix this impedance
mismatch.

Also, why aren't you using ParseVariableBool's existing ability to report
errors? It seems like this:

else if (value)
! {
! bool valid;
! bool newval = ParseVariableBool(value, NULL, &valid);
! if (valid)
! popt->topt.expanded = newval;
! else
! {
! psql_error("unrecognized value \"%s\" for \"%s\"\n",
! value, param);
! return false;
! }
! }

is really the hard way and you could have just done

- popt->topt.expanded = ParseVariableBool(value, param);
+ success = ParseVariableBool(value, param, &popt->topt.expanded);

I'd do it the same way for ParseCheckVariableNum. Also, is there a reason
why that's adding new code rather than changing ParseVariableNum?
I think if we're going to have a policy that bool variables must be valid
bools, there's no reason not to insist similarly for integers.

* More attention is needed to comments. Most glaringly, you've changed
the API for VariableAssignHook without any attention to the API spec
above that typedef. (That comment block is a bit confused anyway, since
half of it is an overall explanation of what the module does and the
other half is an API spec for the hooks. I think I'd move the overall
explanation into the file header comment.) Also, I don't like this way
of explaining an output parameter:

+ * "valid" points to a bool reporting whether the value was valid.

because it's not really clear that the function is setting that value
rather than expecting it to be set to something by the caller.
Assuming you take my advice in the previous point, you could document
ParseVariableBool and ParseVariableNum along the lines of

* Returns true if string contents represent a valid value, false if not.
* If the string is valid, the value is stored into *value, else *value
* remains unchanged.

regards, tom lane

Tom Lane wrote:

> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > [ psql-var-hooks-v6.patch ]
>
> I took a quick look through this. It seems to be going in generally
> the right direction, but here's a couple of thoughts:

Thanks for looking into this!

> I'm inclined to think that the best choice is for the function result
> to be the ok/not ok flag, and pass the variable-to-be-modified as an
> output parameter. That fits better with the notion that the variable
> is not to be modified on failure.

Agreed, if never clobbering the variable, having the valid/invalid state
returned by ParseVariableBool() allows for simpler code. I'm changing it
that way.

> Also, why aren't you using ParseVariableBool's existing ability to report
> errors?

Its' because there are two cases:
- either only a boolean can be given to the command or variable,
in which we let ParseVariableBool() tell:
unrecognized value "bogus" for "command": boolean expected

- or other parameters besides boolean are acceptable, in which case we
can't say "boolean expected", because in fact a boolean is no more or
less expected than the other valid values.

We could shave code by reducing ParseVariableBool()'s error message to:
unrecognized value "bogus" for "name"
clearing the distinction between [only booleans are expected]
and [booleans or enum are expected].
Then almost all callers that have their own message could rely
on ParseVariableBool() instead, as they did previously.

Do we care about the "boolean expected" part of the error message?

> else if (value)
> ! {
> ! bool valid;
> ! bool newval = ParseVariableBool(value, NULL, &valid);
> ! if (valid)
> ! popt->topt.expanded = newval;
> ! else
> ! {
> ! psql_error("unrecognized value \"%s\" for \"%s\"\n",
> ! value, param);
> ! return false;
> ! }
> ! }
> is really the hard way and you could have just done
>
> - popt->topt.expanded = ParseVariableBool(value, param);
> + success = ParseVariableBool(value, param,
> &popt->topt.expanded);

I get the idea, except that in this example, the compiler is
unhappy because popt->topt.expanded is not a bool, and an
explicit cast here would be kludgy.

For the error printing part, it would go away with the above
suggestion

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Tom Lane wrote:
>> Also, why aren't you using ParseVariableBool's existing ability to report
>> errors?

> Its' because there are two cases:
> - either only a boolean can be given to the command or variable,
> in which we let ParseVariableBool() tell:
> unrecognized value "bogus" for "command": boolean expected
> - or other parameters besides boolean are acceptable, in which case we
> can't say "boolean expected", because in fact a boolean is no more or
> less expected than the other valid values.

Ah. Maybe it's time for a ParseVariableEnum, or some other way of
dealing with those cases in a more unified fashion.

> Do we care about the "boolean expected" part of the error message?

I'm not especially in love with that particular wording, but I'm doubtful
that we should give up all indication of what valid values are, especially
in the cases where there are more than just bool-equivalent values.
I think the right thing to do here is to fix it so that the input routine
has full information about all the valid values. On the backend side,
we've gone to considerable lengths to make sure that error messages are
helpful for such cases, eg:

regression=# set backslash_quote to foo;
ERROR: invalid value for parameter "backslash_quote": "foo"
HINT: Available values: safe_encoding, on, off.

and I think it may be worth working equally hard here.

> I get the idea, except that in this example, the compiler is
> unhappy because popt->topt.expanded is not a bool, and an
> explicit cast here would be kludgy.

Yeah, you'll need an intermediate variable if you're trying to use
ParseVariableBool for such a case.

regards, tom lane

Hi,

I had a quick look into this patch and it seems to me like it takes
care of all the built-in variables except ENCODING type. I think we
need to apply such rule for ENCODING variable too.

postgres=# \echo :ENCODING
UTF8
postgres=# \set ENCODING xyz
postgres=# \echo :ENCODING
xyz

I think currently we are not even showing what are the different valid
encoding names to the end users like we show it for other built-in
variables
VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab
command it does show me the valid values for VERBOSITY but not for
ENCODING.

postgres=# \set VERBOSITY
default terse verbose

Moreover, I feel we are just passing the error message to end users
for any bogus assignments but not the hint message showing the correct
set of values that are accepted.

postgres=# \set ECHO on
unrecognized value "on" for "ECHO"
\set: error while setting variable

Above error message should also have some expected values with it.

Please note that I haven't gone through the entire mail chain so not
sure if above thoughts have already been raised by others. Sorry about
that.

With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 16, 2017 at 10:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
>> Tom Lane wrote:
>>> Also, why aren't you using ParseVariableBool's existing ability to report
>>> errors?
>
>> Its' because there are two cases:
>> - either only a boolean can be given to the command or variable,
>> in which we let ParseVariableBool() tell:
>> unrecognized value "bogus" for "command": boolean expected
>> - or other parameters besides boolean are acceptable, in which case we
>> can't say "boolean expected", because in fact a boolean is no more or
>> less expected than the other valid values.
>
> Ah. Maybe it's time for a ParseVariableEnum, or some other way of
> dealing with those cases in a more unified fashion.
>
>> Do we care about the "boolean expected" part of the error message?
>
> I'm not especially in love with that particular wording, but I'm doubtful
> that we should give up all indication of what valid values are, especially
> in the cases where there are more than just bool-equivalent values.
> I think the right thing to do here is to fix it so that the input routine
> has full information about all the valid values. On the backend side,
> we've gone to considerable lengths to make sure that error messages are
> helpful for such cases, eg:
>
> regression=# set backslash_quote to foo;
> ERROR: invalid value for parameter "backslash_quote": "foo"
> HINT: Available values: safe_encoding, on, off.
>
> and I think it may be worth working equally hard here.
>
>> I get the idea, except that in this example, the compiler is
>> unhappy because popt->topt.expanded is not a bool, and an
>> explicit cast here would be kludgy.
>
> Yeah, you'll need an intermediate variable if you're trying to use
> ParseVariableBool for such a case.
>
> regards, tom lane
>
>
> --
> 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

Ashutosh Sharma wrote:

> postgres=# \echo :ENCODING
> UTF8
> postgres=# \set ENCODING xyz
> postgres=# \echo :ENCODING
> xyz
>
> I think currently we are not even showing what are the different valid
> encoding names to the end users like we show it for other built-in
> variables
> VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab
> command it does show me the valid values for VERBOSITY but not for
> ENCODING.

Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT.
In a way, it's a read-only variable that's here to inform the user,
not as a means to change the encoding (\encoding does that and
has proper support for tab completion)

What we could do as of this patch is emit an error when we try
to change ENCODING, with a hook returning false and
a proper error message hinting to \encoding.

I'm working on adding such messages to other variables.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT.
> In a way, it's a read-only variable that's here to inform the user,
> not as a means to change the encoding (\encoding does that and
> has proper support for tab completion)

Right.

> What we could do as of this patch is emit an error when we try
> to change ENCODING, with a hook returning false and
> a proper error message hinting to \encoding.

I think that the current behavior is intentional: it avoids making
those variables reserved. That is, if you're unaware that psql
sets them and try to use them for your own purposes, it will work.

However, it only really works if psql never overwrites the values
after startup, whereas I believe all of these are overwritten by
a \c command.

So maybe it's more user-friendly to make these variables fully
reserved, even at the risk of breaking existing scripts. But
I don't think it's exactly an open-and-shut question.

regards, tom lane

Tom Lane wrote:

> However, it only really works if psql never overwrites the values
> after startup, whereas I believe all of these are overwritten by
> a \c command.

Yes, there are reset to reflect the properties of the new connection.

> So maybe it's more user-friendly to make these variables fully
> reserved, even at the risk of breaking existing scripts. But
> I don't think it's exactly an open-and-shut question.

You mean if we make that fail: \set ENCODING UTF8
it's going to make that fail too:
SELECT something AS "ENCODING"[,...] \gset
and I agree it's not obvious that this trade-off has to be
made. Personally I'm fine with the status quo and will
not add that hook into the patch unless pressed to.

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

Tom Lane wrote:

> I took a quick look through this. It seems to be going in generally
> the right direction, but here's a couple of thoughts:

Here's an update with these changes:

per Tom's suggestions upthread:

- change ParseVariableBool() signature to return validity as bool.

- remove ParseCheckVariableNum() in favor of using tightened up
ParseVariableNum() and GetVariableNum().

- updated header comments in variables.h

other changes:

- autocommit_hook rejects transitions from OFF to ON when inside a
transaction, per suggestion of Rahila Syed (which was the original
motivation for the set of changes of this patch).

- slight doc update for HISTCONTROL (values outside of enum not longer
allowed)

- add enum-style suggestions on invalid input for \pset x, \pset pager,
and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
HISTCONTROL, VERBOSITY, SHOW_CONTEXT, \x, \pager

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

I just wrote:

> - add enum-style suggestions on invalid input for \pset x, \pset pager,
> and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
> HISTCONTROL, VERBOSITY, SHOW_CONTEXT, \x, \pager

There's no such thing as \pager, I meant to write:

\pset x, \pset pager,
and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
HISTCONTROL, VERBOSITY, SHOW_CONTEXT

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Here's an update with these changes:

I scanned this patch very quickly and think it addresses my previous
stylistic objections. Rahila is the reviewer of record though, so
I'll wait for his comments before going further.

regards, tom lane

Hello,

The patch works fine on applying on latest master branch and testing it for
various variables as listed in PsqlSettings struct.
I will post further comments on patch soon.

Thank you,
Rahila Syed

On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > Here's an update with these changes:
>
> I scanned this patch very quickly and think it addresses my previous
> stylistic objections. Rahila is the reviewer of record though, so
> I'll wait for his comments before going further.
>
> regards, tom lane
>

Hello,

Please consider following comments on the patch.

In function ParseVariableNum,

> if (!val || !val[0])
> return false;

Check for 'val == NULL' as in above condition is done even in callers of
ParseVariableNum().
There should be only one such check.

>+ psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
expected\n",
Cant we have this error in ParseVariableNum() similar to
ParseVariableBool() ?

>+ /*
>+ * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>+ * transaction, because it cannot be effective until the current
>+ * transaction is ended.
>+ */
>+ if (autocommit && !pset.autocommit &&
>+ pset.db && PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
>+ {
>+ psql_error("cannot set AUTOCOMMIT to %s when inside a
transaction\n", newval);
>+ }
The above change in autocommit behaviour needs to be documented.

Thank you,
Rahila Syed

On Wed, Jan 25, 2017 at 5:48 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

> Hello,
>
> The patch works fine on applying on latest master branch and testing it
> for various variables as listed in PsqlSettings struct.
> I will post further comments on patch soon.
>
> Thank you,
> Rahila Syed
>
> On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
>> > Here's an update with these changes:
>>
>> I scanned this patch very quickly and think it addresses my previous
>> stylistic objections. Rahila is the reviewer of record though, so
>> I'll wait for his comments before going further.
>>
>> regards, tom lane
>>
>
>

Rahila Syed <rahilasyed90(at)gmail(dot)com> writes:
> Please consider following comments on the patch.

> In function ParseVariableNum,
>> if (!val || !val[0])
>> return false;
> Check for 'val == NULL' as in above condition is done even in callers of
> ParseVariableNum().
> There should be only one such check.

Meh ... I don't think it's unreasonable for ParseVariableNum to defend
itself against that. The callers might or might not be applying a check
--- for them, it would be a matter of do they need to detect presence
or absence of an option, but not about whether the option value is valid.

>> + psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
>> expected\n",

> Cant we have this error in ParseVariableNum() similar to
> ParseVariableBool() ?

Agreed, error messages should be stylistically similar. I'm not sure they
can be exactly alike though. Right now the patch has ParseVariableBool
saying

+ psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n",
+ value, name);

while callers that don't use that error use PsqlVarEnumError which has

+ psql_error("Unrecognized value \"%s\" for \"%s\"\nAvailable values: %s\n",
+ value, name, suggestions);

and then ParseVariableNum is as above. So first off we've got a
capitalization inconsistency. Project style for backend messages is
no initial cap; psql seems not to be on board with that entirely,
but I'm definitely inclined to go with it here. As for "invalid"
vs. "unrecognized", I'm not sure "unrecognized" really fits the
bill for "this isn't an integer". So I'm inclined to leave that
alone. I suggest we go with these:

"invalid value \"%s\" for \"%s\": integer expected\n"
"unrecognized value \"%s\" for \"%s\": boolean expected\n"
"unrecognized value \"%s\" for \"%s\"\nAvailable values are: %s\n."

where the last case is following the message style for hints.

>> + * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>> + * transaction, because it cannot be effective until the current
>> + * transaction is ended.

> The above change in autocommit behaviour needs to be documented.

Yeah, definitely.

I'll go ahead and push the patch with these fixes. Thanks for reviewing!

regards, tom lane

BTW, while testing this patch I noticed that the error reports are
a tad redundant:

regression=# \set AUTOCOMMIT foo
unrecognized value "foo" for "AUTOCOMMIT": boolean expected
\set: error while setting variable
regression=#

The "error while setting variable" message seems entirely content-free.
I think we should drop that and instead establish a rule that SetVariable
should print a message for itself about any failure. (There are a lot
of call sites that don't check for or print a message, but that's only
because they aren't expecting failure. If one were to happen, printing
a message doesn't seem unreasonable.) That would in turn propagate into
an API requirement that var hooks that return FALSE are responsible for
printing a message about the reason, which is why it would be appropriate
to make that change as part of this patch.

Barring objections PDQ, I'll make this change.

regards, tom lane

I wrote:
> Rahila Syed <rahilasyed90(at)gmail(dot)com> writes:
>>> + * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>>> + * transaction, because it cannot be effective until the current
>>> + * transaction is ended.

>> The above change in autocommit behaviour needs to be documented.

> Yeah, definitely.

Actually ... while trying to write some documentation for that, I found
myself wondering why we need such a prohibition at all. If you are inside
a transaction, then autocommit has no effect until after you get out of
the transaction, and the documentation about it seems clear enough on the
point. Also, if you want to argue that allowing it to change intra-
transaction is too confusing, why would we only forbid this direction
of change and not both directions?

I'm afraid we might be breaking some peoples' scripts to no particularly
good end, so I'm going to leave this out of the committed patch. If you
think this really is a valid change to make, we can commit it separately,
but let's discuss it on its own merits.

regards, tom lane

So I pushed this, and the buildfarm members that are testing RedisFDW
immediately fell over:

*** /home/andrew/bf/root/HEAD/redis_fdw.build/test/expected/redis_fdw.out 2017-01-30 18:20:27.440677318 -0500
--- /home/andrew/bf/root/HEAD/redis_fdw.build/test/results/redis_fdw.out 2017-01-30 18:32:33.404677320 -0500
***************
*** 26,31 ****
--- 26,32 ----
options (database '15', tabletype 'zset');
-- make sure they are all empty - if any are not stop the script right now
\set ON_ERROR_STOP
+ unrecognized value "" for "ON_ERROR_STOP": boolean expected
do $$
declare
rows bigint;

======================================================================

Evidently, this test script is relying on the preceding behavior that
setting a bool variable to an empty string was equivalent to setting
it to "true". If it's just that script I would be okay with saying
"well, it's a bug in that script" ... but I'm a bit worried that this
may be the tip of the iceberg, ie maybe a lot of people have done
things like this. Should we reconsider the decision to reject empty
strings in ParseVariableBool?

regards, tom lane

Tom Lane wrote:

> Also, if you want to argue that allowing it to change intra-
> transaction is too confusing, why would we only forbid this direction
> of change and not both directions?

The thread "Surprising behaviour of \set AUTOCOMMIT ON" at:
/message-id/CAH2L28sTP-9dio3X1AaZRyWb0-ANAx6BDBi37TGmvw1hBiu0oA%40mail.gmail.com
seemed to converge towards the conclusion implemented in the autocommit_hook
proposed in the patch.

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

Tom Lane wrote:

> Evidently, this test script is relying on the preceding behavior that
> setting a bool variable to an empty string was equivalent to setting
> it to "true". If it's just that script I would be okay with saying
> "well, it's a bug in that script" ... but I'm a bit worried that this
> may be the tip of the iceberg, ie maybe a lot of people have done
> things like this. Should we reconsider the decision to reject empty
> strings in ParseVariableBool?

Sigh. It was considered upthread, I'm requoting the relevant bit:

<quote>
if (pg_strncasecmp(value, "true", len) == 0)
return true;
It happens that "" as a value yields the same result than "true" for this
test so it passes, but it seems rather unintentional.

The resulting problem from the POV of the user is that we can do that,
for instance:

test=> \set AUTOCOMMIT

without error message or feedback, and that leaves us without much
clue about autocommit being enabled:

test=> \echo :AUTOCOMMIT

test=>

So I've changed ParseVariableBool() in v4 to reject empty string as an
invalid boolean (but not NULL since the startup logic requires NULL
meaning false in the early initialization of these variables).

"make check" seems OK with that, I hope it doesn't cause any regression
elsewhere.
</quote>

So it does cause regressions. I don't know if we should reaccept
empty strings immediately to fix that, but in the long run, I think
that the above situation with the empty :AUTOCOMMIT is not really
sustainable: when we extend what we do with variables
(/if /endif and so on), it will become even more of a problem.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Tom Lane wrote:
>> Evidently, this test script is relying on the preceding behavior that
>> setting a bool variable to an empty string was equivalent to setting
>> it to "true". If it's just that script I would be okay with saying
>> "well, it's a bug in that script" ... but I'm a bit worried that this
>> may be the tip of the iceberg, ie maybe a lot of people have done
>> things like this. Should we reconsider the decision to reject empty
>> strings in ParseVariableBool?
> ...
> So it does cause regressions. I don't know if we should reaccept
> empty strings immediately to fix that, but in the long run, I think
> that the above situation with the empty :AUTOCOMMIT is not really
> sustainable: when we extend what we do with variables
> (/if /endif and so on), it will become even more of a problem.

Yeah, in a green field we'd certainly not allow this, but the problem
I'm having is that in all previous versions you could do, eg,

\set ON_ERROR_STOP
... stuff expecting ON_ERROR_STOP to be on
\unset ON_ERROR_STOP
... stuff expecting ON_ERROR_STOP to be off

and it works, and looks perfectly natural, and people may well be relying
on that. Especially since the docs aren't very clear that this shouldn't
work --- psql-ref.sgml repeatedly uses phrases like "FOO is set" to
indicate that the boolean variable FOO is considered to be "on".

Moreover, the committed patch is inconsistent in that it forbids
only one of the above. Why is it okay to treat unset as "off",
but not okay to treat the default empty-string value as "on"?

Maybe it's worth breaking backwards compatibility on this point, but
I'm feeling unconvinced. This seems rather different from rejecting
clearly-wrongly-spelled values.

One possible compromise that would address your concern about display
is to modify the hook API some more so that variable hooks could actually
substitute new values. Then for example the bool-variable hooks could
effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
"\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
of their values always produces sane results.

regards, tom lane

Tom Lane wrote:

> Moreover, the committed patch is inconsistent in that it forbids
> only one of the above. Why is it okay to treat unset as "off",
> but not okay to treat the default empty-string value as "on"?

Treating unset (NULL in the value) as "off" comes from the fact
that except AUTOCOMMIT, our built-in variables are not initialized
with a default value. For instance we call this in EstablishVariableSpace();
SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
then on_error_stop_hook is called with NULL as the value
then ParseVariableBool(NULL, "ON_ERROR_STOP", &pset.on_error_stop)
is what initializes pset.on_error_stop to false.

The same happens if/when the variable is unset. Again the hook is called
with NULL, and it sets back the pset.* variable in a hardcoded default state,
which is false for all booleans.

Incidentally I want to suggest to change that, so that all variables
should be initialized with a non-NULL value right from the start, and the
value would possibly come to NULL only if it's unset.
This would allow the hook to distinguish between initialization and
unsetting, which in turn will allow it to deny the \unset in the
cases when it doesn't make any sense conceptually (like AUTOCOMMIT).

Forcing values for our built-in variables would also avoid the following:

=# \echo :ON_ERROR_STOP
:ON_ERROR_STOP

Even if the variable ON_ERROR_STOP does exist in the VariableSpace
and has a hook and there's an initialized corresponding pset.on_error_stop,
from the user's viewpoint, it's as if the variable doesn't exist
until they intialize it explicitly.
I suggest that it doesn't make much sense and it would be better
to have that instead right from the start:

=# \echo :ON_ERROR_STOP
off

> One possible compromise that would address your concern about display
> is to modify the hook API some more so that variable hooks could actually
> substitute new values. Then for example the bool-variable hooks could
> effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
> "\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
> of their values always produces sane results.

Agreed, that looks like a good compromise.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> Tom Lane wrote:
>> One possible compromise that would address your concern about display
>> is to modify the hook API some more so that variable hooks could actually
>> substitute new values. Then for example the bool-variable hooks could
>> effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
>> "\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
>> of their values always produces sane results.

> Agreed, that looks like a good compromise.

Attached is a draft patch for that. I chose to make a second hook rather
than complicate the assign hook API, mainly because it allows more code
sharing --- all the bool vars can share the same substitute hook, and
so can the three-way vars as long as "on" and "off" are the appropriate
substitutes.

I only touched the behavior for bool vars here, but if people like this
solution it could be fleshed out to apply to all the built-in variables.

Maybe we should merge SetVariableSubstituteHook and SetVariableAssignHook
into one function?

regards, tom lane

I wrote:

> This would allow the hook to distinguish between initialization and
> unsetting, which in turn will allow it to deny the \unset in the
> cases when it doesn't make any sense conceptually (like AUTOCOMMIT).

I notice that in the commited patch, you added the ability
for DeleteVariable() to reject the deletion if the hook
disagrees.
+ {
+ /* Allow deletion only if hook is okay with
NULL value */
+ if (!(*current->assign_hook) (NULL))
+ return false; /* message
printed by hook */

But this can't happen in practice because as mentioned just upthread
the hook called with NULL doesn't know if the variable is getting unset
or initialized, so rejecting on NULL is not an option.

Attached is a proposed patch to add initial values to
SetVariableAssignHook() to solve this problem, and an example for
\unset AUTOCOMMIT being denied by the hook.

As a side effect, we see all buit-in variables when issuing \set
rather than just a few.

Does it make sense?

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> I notice that in the commited patch, you added the ability
> for DeleteVariable() to reject the deletion if the hook
> disagrees.

Right.

> But this can't happen in practice because as mentioned just upthread
> the hook called with NULL doesn't know if the variable is getting unset
> or initialized, so rejecting on NULL is not an option.

It would have required the caller to set a value before installing the
hook, which would require some reshuffling of responsibility. In the
draft patch I sent a little bit ago, this is handled by installing a
"substitution hook" first, and that hook transmogrifies NULL into an
allowed setting. That gets to the same place in a slightly different
way, but it also covers allowing "\unset FOO", which inserting initial
values wouldn't.

> Attached is a proposed patch to add initial values to
> SetVariableAssignHook() to solve this problem, and an example for
> \unset AUTOCOMMIT being denied by the hook.

I think disallowing \unset is a nonstarter on compatibility grounds.
We should allow \unset but treat it like setting to "off" (or whatever
the default value is).

regards, tom lane

I wrote:
> Attached is a draft patch for that. I chose to make a second hook rather
> than complicate the assign hook API, mainly because it allows more code
> sharing --- all the bool vars can share the same substitute hook, and
> so can the three-way vars as long as "on" and "off" are the appropriate
> substitutes.

> I only touched the behavior for bool vars here, but if people like this
> solution it could be fleshed out to apply to all the built-in variables.

Attached is a finished version that includes hooks for all the variables
for which they were clearly sensible. (FETCH_COUNT doesn't seem to really
need one, and I didn't do anything with HISTSIZE or IGNOREEOF either.
It might be worth bringing the latter two into the hooks paradigm, but
that seems like fit material for a separate patch.)

I updated the documentation as well. I think this is committable if
there are not objections.

regards, tom lane

BTW ... while I've been fooling with this issue, I've gotten a bit
annoyed at the fact that "\set" prints the variables in, essentially,
creation order. That makes the list ugly and hard to find things in,
and it exposes some psql implementation details to users. I propose
the attached simple patch to keep the list in name order instead.

(This is on top of my previous patch, but it'd be pretty trivial
to modify to apply against HEAD.)

regards, tom lane

Tom Lane wrote:

> I updated the documentation as well. I think this is committable if
> there are not objections.

That works for me. I tested and read it and did not find anything
unexpected or worth objecting.
"\unset var" acting as "\set var off" makes sense considering
that its opposite "\set var" is now an accepted
synonym of "\set var on" for the boolean built-ins.

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

"Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> That works for me. I tested and read it and did not find anything
> unexpected or worth objecting.
> "\unset var" acting as "\set var off" makes sense considering
> that its opposite "\set var" is now an accepted
> synonym of "\set var on" for the boolean built-ins.

Thanks for reviewing! I've pushed this now --- Andrew, you should
be able to revert the RedisFDW test script to the way it was.

regards, tom lane

I wrote:
> Attached is a finished version that includes hooks for all the variables
> for which they were clearly sensible. (FETCH_COUNT doesn't seem to really
> need one, and I didn't do anything with HISTSIZE or IGNOREEOF either.
> It might be worth bringing the latter two into the hooks paradigm, but
> that seems like fit material for a separate patch.)

I got more excited about doing that after noticing that not only would
it clean up the behavior of those particular variables, but we could get
rid of some code. First, we'd not need the rather warty GetVariableNum()
anymore, and second, we'd then be almost at the point where every control
variable has a hook, and therefore we could drop tab-complete.c's private
list of known variable names. That was only ever needed to cover the
possibility of important variables not being present in the variables
list.

So the attached proposed patch does these things:

1. Modify FETCH_COUNT to always have a defined value, like other control
variables, mainly so it will always appear in "\set" output.

2. Add hooks to force HISTSIZE to be defined and require it to have an
integer value. (I don't see any point in allowing it to be set to
non-integral values.)

3. Add hooks to force IGNOREEOF to be defined and require it to have an
integer value. Unlike the other cases, here we're trying to be
bug-compatible with a rather bogus externally-defined behavior, so I think
we need to continue to allow "\set IGNOREEOF whatever". What I propose is
that the substitution hook silently replace non-numeric values with "10",
so that the stored value always reflects what we're really doing.

4. Add a dummy assign hook for HISTFILE, just so it's always in
variables.c's list. We can't require it to be defined always, because
that would break the interaction with the PSQL_HISTORY environment
variable, so there isn't any change in visible behavior here.

5. Remove tab-complete.c's private list of known variable names. Given
the other changes, there are no control variables it won't show anyway.
This does mean that if for some reason you've unset one of the status
variables (DBNAME, HOST, etc), that variable would not appear in tab
completion for \set. But I think that's fine, for at least two reasons:
we shouldn't be encouraging people to use those variables as regular
variables, and if someone does do so anyway, why shouldn't it act just
like a regular variable?

6. Remove no-longer-used-anywhere GetVariableNum().

Any objections?

regards, tom lane

On 02/01/2017 11:26 AM, Tom Lane wrote:
> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
>> That works for me. I tested and read it and did not find anything
>> unexpected or worth objecting.
>> "\unset var" acting as "\set var off" makes sense considering
>> that its opposite "\set var" is now an accepted
>> synonym of "\set var on" for the boolean built-ins.
> Thanks for reviewing! I've pushed this now --- Andrew, you should
> be able to revert the RedisFDW test script to the way it was.
>
>

Done.

cheers

andrew