Thread: 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]
https://www.postgresql.org/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
[2]
https://www.postgresql.org/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@manitou-mail.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]
> https://www.postgresql.org/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
> [2]
> https://www.postgresql.org/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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



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

    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

    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

    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@manitou-mail.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@snowman.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@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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@snowman.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@manitou-mail.org) wrote:
>     Tom Lane wrote:
>
> > Stephen Frost <sfrost@snowman.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@manitou-mail.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@manitou-mail.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
willonly be kept if psql is in  interactive mode. When executing a non-interactive script,  processing will immediately
stopwith 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]
https://www.postgresql.org/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

    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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers