Re: Improvements in psql hooks for variables

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.bapat@enterprisedb.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@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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>