Re: Improvements in psql hooks for variables

Rahila Syed <rahilasyed90(at)gmail(dot)com> writes:
> Please consider following comments on the patch.

> In function ParseVariableNum,
>> if (!val || !val[0])
>> return false;
> Check for 'val == NULL' as in above condition is done even in callers of
> ParseVariableNum().
> There should be only one such check.

Meh ... I don't think it's unreasonable for ParseVariableNum to defend
itself against that. The callers might or might not be applying a check
--- for them, it would be a matter of do they need to detect presence
or absence of an option, but not about whether the option value is valid.

>> + psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
>> expected\n",

> Cant we have this error in ParseVariableNum() similar to
> ParseVariableBool() ?

Agreed, error messages should be stylistically similar. I'm not sure they
can be exactly alike though. Right now the patch has ParseVariableBool
saying

+ psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n",
+ value, name);

while callers that don't use that error use PsqlVarEnumError which has

+ psql_error("Unrecognized value \"%s\" for \"%s\"\nAvailable values: %s\n",
+ value, name, suggestions);

and then ParseVariableNum is as above. So first off we've got a
capitalization inconsistency. Project style for backend messages is
no initial cap; psql seems not to be on board with that entirely,
but I'm definitely inclined to go with it here. As for "invalid"
vs. "unrecognized", I'm not sure "unrecognized" really fits the
bill for "this isn't an integer". So I'm inclined to leave that
alone. I suggest we go with these:

"invalid value \"%s\" for \"%s\": integer expected\n"
"unrecognized value \"%s\" for \"%s\": boolean expected\n"
"unrecognized value \"%s\" for \"%s\"\nAvailable values are: %s\n."

where the last case is following the message style for hints.

>> + * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>> + * transaction, because it cannot be effective until the current
>> + * transaction is ended.

> The above change in autocommit behaviour needs to be documented.

Yeah, definitely.

I'll go ahead and push the patch with these fixes. Thanks for reviewing!

regards, tom lane