Обсуждение: Change error code for hstore syntax error
The hstore module uses elog() to default to ERRCODE_INTERNAL_ERROR (SQLSTATE XX000) when the error message reads "Syntax error near '%c' at position %d".
I propose to switch to ereport() to return ERRCODE_SYNTAX_ERROR (SQLSTATE 42601), on the grounds that it's more transparent. It took me longer to figure out what error code was being returned than to write both the patch and my originally intended function using the patch.
I also propose to raise the same error code for "Unexpected end of string" in hstore, since it serves the same purpose, at least in my mind: differentiating between valid and invalid hstore syntax.
I propose to switch to ereport() to return ERRCODE_SYNTAX_ERROR (SQLSTATE 42601), on the grounds that it's more transparent. It took me longer to figure out what error code was being returned than to write both the patch and my originally intended function using the patch.
I also propose to raise the same error code for "Unexpected end of string" in hstore, since it serves the same purpose, at least in my mind: differentiating between valid and invalid hstore syntax.
Any objections to my submitting the attached patch to the September commitfest?
Вложения
Sherrylyn Branchaw <sbranchaw@gmail.com> writes: > The hstore module uses elog() to default to ERRCODE_INTERNAL_ERROR > (SQLSTATE XX000) when the error message reads "Syntax error near '%c' at > position %d". Yeah, that is entirely bogus. No user-facing error report should ever return ERRCODE_INTERNAL_ERROR. > I propose to switch to ereport() to return ERRCODE_SYNTAX_ERROR (SQLSTATE > 42601), on the grounds that it's more transparent. Hm, class 42 is generally meant for SQL-level syntax errors. Are these errors not coming from subroutines of hstore_in()? I think our usual convention is to use ERRCODE_INVALID_TEXT_REPRESENTATION for complaints that a data value does not meet its type's conventions. In any case it seems closer to class 22 than 42. While you're at it, please make the error message texts conform to our style guidelines: http://www.postgresql.org/docs/devel/static/error-style-guide.html These seem to have multiple problems, starting with incorrect capitalization and extending to failure to quote the whole string being complained of. In short, though, +1 for improving this stuff. regards, tom lane
Finally returning to this...
Hm, class 42 is generally meant for SQL-level syntax errors. Are these
errors not coming from subroutines of hstore_in()? I think our usual
convention is to use ERRCODE_INVALID_TEXT_REPRESENTATION for complaints
that a data value does not meet its type's conventions. In any case
it seems closer to class 22 than 42.
I see, thanks for pointing that out. That seems like a useful distinction to preserve. I've updated it to ERRCODE_INVALID_TEXT_REPRESENTATION accordingly.
These seem to have multiple problems, starting with incorrect
capitalization and extending to failure to quote the whole string
being complained of.
I've modified the messages to comply with the guidelines, with a little help from a colleague who actually knows C.
I'm attaching a revised patch; please let me know if there are any other issues before I submit to the commitfest.
Best,
Sherrylyn
Вложения
On Mon, May 9, 2016 at 1:42 PM, Sherrylyn Branchaw <sbranchaw@gmail.com> wrote: > I'm attaching a revised patch; please let me know if there are any other > issues before I submit to the commitfest. Submitting to the CommitFest is how you make sure that someone looks at the patch to see if any issues exist. Obviously, if someone reviews before then, that's great, but you can't count on it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Submitting to the CommitFest is how you make sure that someone looks
at the patch to see if any issues exist. Obviously, if someone
reviews before then, that's great, but you can't count on it.
at the patch to see if any issues exist. Obviously, if someone
reviews before then, that's great, but you can't count on it.
On Thu, May 12, 2016 at 11:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, May 9, 2016 at 1:42 PM, Sherrylyn Branchaw <sbranchaw@gmail.com> wrote:
> I'm attaching a revised patch; please let me know if there are any other
> issues before I submit to the commitfest.
Submitting to the CommitFest is how you make sure that someone looks
at the patch to see if any issues exist. Obviously, if someone
reviews before then, that's great, but you can't count on it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, May 12, 2016 at 11:46 AM, Sherrylyn Branchaw <sbranchaw@gmail.com> wrote: > Understood. It's just that Tom had already replied, so I wanted to give him > first crack at it, if, say, I had grossly misinterpreted his suggestions. > The guide on submitting a patch advises taking silence as consent within > reason. Since there's been enough silence, I was planning on submitting to > the commitfest soon. No sweat. Just didn't want you to take silence as opposition. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Sherrylyn, On 2016-05-09 19:42, Sherrylyn Branchaw wrote: > I'm attaching a revised patch; please let me know if there are any other > issues before I submit to the commitfest. I think this is mostly good, but these two should be changed: errmsg("unexpected end of string: \"%s\"", state->begin) errmsg("syntax error at position %d: \"%s\"", ...) Right now, aside from the error code, these two look like they're reporting about an error in the SQL statement itself, and not in an input value for a type. I think they should look more like this: errmsg("invalid input syntax for type hstore: \"%s\"", string), errdetail("Unexpected end of input.") If possible, it might also make sense to provide more information than "unexpected end of string". For example: what character were you expecting to find, or what were you scanning? I didn't look too closely what exactly could be done here. I'll leave that part to you. .m
On Sun, Sep 4, 2016 at 7:15 PM, Marko Tiikkaja <marko@joh.to> wrote: > On 2016-05-09 19:42, Sherrylyn Branchaw wrote: >> >> I'm attaching a revised patch; please let me know if there are any other >> issues before I submit to the commitfest. > > I think this is mostly good, but these two should be changed: > > errmsg("unexpected end of string: \"%s\"", state->begin) > errmsg("syntax error at position %d: \"%s\"", ...) > > Right now, aside from the error code, these two look like they're reporting > about an error in the SQL statement itself, and not in an input value for a > type. I think they should look more like this: > > errmsg("invalid input syntax for type hstore: \"%s\"", string), > errdetail("Unexpected end of input.") > > If possible, it might also make sense to provide more information than > "unexpected end of string". For example: what character were you expecting > to find, or what were you scanning? I didn't look too closely what exactly > could be done here. I'll leave that part to you. Since no revised patch has been forthcoming and the CommitFest is due to end shortly, I've marked this "Returned with Feedback". Sherrylyn, please feel free to update the patch and resubmit to the next CommitFest. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback". Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.
to end shortly, I've marked this "Returned with Feedback". Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.
Sherrylyn