Re: Improvements in psql hooks for variables

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