Обсуждение: Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" <daniel@manitou-mail.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@sss.pgh.pa.us> wrote:
> "Daniel Verite" <daniel@manitou-mail.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@postgresql.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