From: Tom Lane <tgl@sss.pgh.pa.us>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Daniel Verite <daniel@manitou-mail.org>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improvements in psql hooks for variables
Date: Mon, 30 Jan 2017 12:47:44 -0500
Message-ID: <17593.1485798464@sss.pgh.pa.us> (raw)
In-Reply-To: <CAH2L28ugrq+N_=i8f6RvWL0NPUhT84CAp8SrDg1RoSc0T7+qBg@mail.gmail.com>
References: <fa2685de-5262-460d-9eca-f027a5de0d7d@manitou-mail.org>
<11456.1485288184@sss.pgh.pa.us>
<CAH2L28uYE6mRr1rdjgL=MBH-xg1_-viBvjnKR7T59pwZuPFnsw@mail.gmail.com>
<CAH2L28ugrq+N_=i8f6RvWL0NPUhT84CAp8SrDg1RoSc0T7+qBg@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Rahila Syed <rahilasyed90@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, rahilasyed90@gmail.com, daniel@manitou-mail.org, sfrost@snowman.net, ashutosh.bapat@enterprisedb.com
Subject: Re: Improvements in psql hooks for variables
In-Reply-To: <17593.1485798464@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox