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 <t...@sss.pgh.pa.us> wrote: > "Daniel Verite" <dan...@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 (firstname.lastname@example.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers