Re: [HACKERS] Change error code for hstore syntax error
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. Will do, Robert, and many thanks to Marko for the feedback. I apologize for the delay; I had surgery two days ago and will get back to this as soon as possible. Sherrylyn
Re: [HACKERS] Change error code for hstore syntax error
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. 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. On Thu, May 12, 2016 at 11:35 AM, Robert Haas wrote: > On Mon, May 9, 2016 at 1:42 PM, 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. > > 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 >
Re: [HACKERS] Change error code for hstore syntax error
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
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 0c1d99a..4ee3d05 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -81,7 +81,10 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
}
else if (*(state->ptr) == '=' && !ignoreeq)
{
-elog(ERROR, "Syntax error near '%c' at position %d", *(state->ptr), (int32) (state->ptr - state->begin));
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("syntax error at position %d: \"%s\"",
+ (int32) (state->ptr - state->begin), state->begin;
}
else if (*(state->ptr) == '\\')
{
@@ -138,7 +141,9 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
}
else if (*(state->ptr) == '\0')
{
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
}
else
{
@@ -150,7 +155,9 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
else if (st == GV_WAITESCIN)
{
if (*(state->ptr) == '\0')
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
RESIZEPRSBUF;
*(state->cur) = *(state->ptr);
state->cur++;
@@ -159,14 +166,16 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
else if (st == GV_WAITESCESCIN)
{
if (*(state->ptr) == '\0')
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
RESIZEPRSBUF;
*(state->cur) = *(state->ptr);
state->cur++;
st = GV_INESCVAL;
}
else
- elog(ERROR, "Unknown state %d at position line %d in file '%s'", st, __LINE__, __FILE__);
+ elog(ERROR, "unknown state %d at position line %d in file \"%s\"", st, __LINE__, __FILE__);
state->ptr++;
}
@@ -216,11 +225,16 @@ parse_hstore(HSParser *state)
}
else if (*(state->ptr) == '\0')
{
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
}
else if (!isspace((unsigned char) *(state->ptr)))
{
-elog(ERROR, "Syntax error near '%c' at position %d", *(state->ptr), (int32) (state->ptr - state->begin));
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("syntax error at position %d: \"%s\"",
+ (int32) (state->ptr - state->begin), state->begin;
}
}
else if (st == WGT)
@@ -231,17 +245,24 @@ parse_hstore(HSParser *state)
}
else if (*(state->ptr) == '\0')
{
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
}
else
{
-elog(ERROR, "Syntax error near '%c' at position %d", *(state->ptr), (int32) (state->ptr - state->begin));
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("syntax error at position %d: \"%s\"",
+ (int32) (state->ptr - state->begin), state->begin;
}
}
else if (st == WVAL)
{
if (!get_val(state, true, &escaped))
-elog(ERROR, "Unexpected end of string");
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ (errmsg("unexpected end of string: \"%s\"", state->begin;
state->pairs[state->pcur].val = state->word;
state->pairs[state->pcur].vallen = hstoreCheckValLen(state->cur - state->word);
state->pairs[state->pcur].isnull = false;
@@ -268,11 +289,14 @@ parse_hstore(HSParser *state)
}
else if (!isspace((unsigned char) *(state->ptr)))
{
-elog(ERROR, "Syntax error near '%c' at position %d", *(state->ptr), (int32) (state->ptr - state->begin));
+ereport(ERROR,
+ (errcode(ERRCODE_
[HACKERS] 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.
Any objections to my submitting the attached patch to the September
commitfest?
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 0c1d99a..0cbc865 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -81,7 +81,10 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
}
else if (*(state->ptr) == '=' && !ignoreeq)
{
- elog(ERROR, "Syntax error near '%c' at position
%d", *(state->ptr), (int32) (state->ptr - state->begin));
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Syntax error near
'%c' at position %d",
+ *(state->ptr), (int32) (state->ptr -
state->begin);
}
else if (*(state->ptr) == '\\')
{
@@ -138,7 +141,9 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
}
else if (*(state->ptr) == '\0')
{
- elog(ERROR, "Unexpected end of string");
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Unexpected end of
string";
}
else
{
@@ -150,7 +155,9 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
else if (st == GV_WAITESCIN)
{
if (*(state->ptr) == '\0')
- elog(ERROR, "Unexpected end of string");
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Unexpected end of
string";
RESIZEPRSBUF;
*(state->cur) = *(state->ptr);
state->cur++;
@@ -159,7 +166,9 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
else if (st == GV_WAITESCESCIN)
{
if (*(state->ptr) == '\0')
- elog(ERROR, "Unexpected end of string");
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Unexpected end of
string";
RESIZEPRSBUF;
*(state->cur) = *(state->ptr);
state->cur++;
@@ -216,11 +225,16 @@ parse_hstore(HSParser *state)
}
else if (*(state->ptr) == '\0')
{
- elog(ERROR, "Unexpected end of string");
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Unexpected end of
string";
}
else if (!isspace((unsigned char) *(state->ptr)))
{
- elog(ERROR, "Syntax error near '%c' at position
%d", *(state->ptr), (int32) (state->ptr - state->begin));
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Syntax error near
'%c' at position %d",
+ *(state->ptr), (int32) (state->ptr -
state->begin);
}
}
else if (st == WGT)
@@ -231,17 +245,24 @@ parse_hstore(HSParser *state)
}
else if (*(state->ptr) == '\0')
{
- elog(ERROR, "Unexpected end of string");
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+(errmsg("Unexpected end of
string";
