Re: [HACKERS] bug in json_to_record with arrays
On 11/28/2014 12:55 PM, Tom Lane wrote: * This would only really address Josh's complaint if we were to back-patch it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts? If we don't fix an error text in an RC, what would we fix in an RC? That's as minor as things get. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Josh Berkus j...@agliodbs.com writes: On 11/28/2014 12:55 PM, Tom Lane wrote: * This would only really address Josh's complaint if we were to back-patch it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts? If we don't fix an error text in an RC, what would we fix in an RC? That's as minor as things get. Code-wise, yeah, but it would put some pressure on the translators. What did you think of the new error texts in themselves? 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
Re: [HACKERS] bug in json_to_record with arrays
On 12/01/2014 12:28 PM, Josh Berkus wrote: On 11/28/2014 12:55 PM, Tom Lane wrote: * This would only really address Josh's complaint if we were to back-patch it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts? If we don't fix an error text in an RC, what would we fix in an RC? That's as minor as things get. I think the idea is that you only fix *major* things in an RC? That said, I agree that we should fix something so minor. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, @cmdpromptinc If we send our children to Caesar for their education, we should not be surprised when they come back as Romans. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
On 2014-12-01 15:30:06 -0500, Tom Lane wrote: Josh Berkus j...@agliodbs.com writes: On 11/28/2014 12:55 PM, Tom Lane wrote: * This would only really address Josh's complaint if we were to back-patch it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts? If we don't fix an error text in an RC, what would we fix in an RC? That's as minor as things get. Code-wise, yeah, but it would put some pressure on the translators. I think a untranslated, but on the point, error message is better than a translated confusing one. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
On 12/01/2014 12:30 PM, Tom Lane wrote: Josh Berkus j...@agliodbs.com writes: On 11/28/2014 12:55 PM, Tom Lane wrote: * This would only really address Josh's complaint if we were to back-patch it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts? If we don't fix an error text in an RC, what would we fix in an RC? That's as minor as things get. Code-wise, yeah, but it would put some pressure on the translators. What did you think of the new error texts in themselves? I would prefer invalid input syntax to malformed array literal, but I'll take anything we can backpatch. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Josh Berkus j...@agliodbs.com writes: On 12/01/2014 12:30 PM, Tom Lane wrote: Code-wise, yeah, but it would put some pressure on the translators. What did you think of the new error texts in themselves? I would prefer invalid input syntax to malformed array literal, but I'll take anything we can backpatch. I think if we're changing them at all, it's about the same cost no matter what we change them to exactly. I'd be good with going to invalid input syntax if we also change record_in to match. Anyone have a feeling pro or con about that? 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
Re: [HACKERS] bug in json_to_record with arrays
On Mon, Dec 1, 2014 at 3:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: Josh Berkus j...@agliodbs.com writes: On 12/01/2014 12:30 PM, Tom Lane wrote: Code-wise, yeah, but it would put some pressure on the translators. What did you think of the new error texts in themselves? I would prefer invalid input syntax to malformed array literal, but I'll take anything we can backpatch. I think if we're changing them at all, it's about the same cost no matter what we change them to exactly. I'd be good with going to invalid input syntax if we also change record_in to match. Anyone have a feeling pro or con about that? I don't know. malformed array literal is a mighty big clue that you have a bad postgres format array literal and will be well supported by googling -- this problem isn't new. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Merlin Moncure mmonc...@gmail.com writes: On Mon, Dec 1, 2014 at 3:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: I'd be good with going to invalid input syntax if we also change record_in to match. Anyone have a feeling pro or con about that? I don't know. malformed array literal is a mighty big clue that you have a bad postgres format array literal and will be well supported by googling -- this problem isn't new. Good point about googling, and after all we are already using malformed array literal for a subset of these cases. Let's stick with that wording. 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
Re: [HACKERS] bug in json_to_record with arrays
Josh Berkus j...@agliodbs.com writes:
On 11/26/2014 12:48 PM, Tom Lane wrote:
Wouldn't it be better if it said
ERROR: invalid input syntax for array: [potter,chef,programmer]
DETAIL: Dimension value is missing.
which is comparable to what you'd get out of most other input functions
that were feeling indigestion?
Yes, it most definitely would be better.
Here's a draft patch to improve array_in's error reports. With this
patch, what you'd get for this example is
# select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: malformed array literal: [potter,chef,programmer]
DETAIL: [ must introduce explicitly-specified array dimensions.
Some comments:
* Probably the main objection that could be leveled against this approach
is that it's reasonable to expect that array literal strings could be
pretty long, maybe longer than is reasonable to print all of in a primary
error message. However, the same could be said about record literal
strings; yet I've not heard any complaints about the fact that record_in
just prints the whole input string when it's unhappy. So that's what this
does too.
* The phrasing malformed array literal matches some already-existing
error texts, as well as record_in which uses malformed record literal.
I wonder though if we shouldn't change all of these to invalid input
syntax for array (resp. record), which seems more like our usual
phraseology in other datatype input functions. OTOH, that would make
the primary error message even longer.
* I changed every one of the ERRCODE_INVALID_TEXT_REPRESENTATION cases
to malformed array literal with an errdetail. I did not touch some
existing ereport's with different SQLSTATEs, notably upper bound cannot
be less than lower bound. Anyone have a different opinion about whether
that case needs to print the string contents?
* Some of the errdetails don't really seem all that helpful (the ones
triggered by the existing regression test cases are examples). Can anyone
think of better wording? Should we just leave those out?
* This would only really address Josh's complaint if we were to back-patch
it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts?
regards, tom lane
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 743351b..933c6b0 100644
*** a/src/backend/utils/adt/arrayfuncs.c
--- b/src/backend/utils/adt/arrayfuncs.c
*** array_in(PG_FUNCTION_ARGS)
*** 247,257
errmsg(number of array dimensions (%d) exceeds the maximum allowed (%d),
ndim + 1, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p)/* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(missing dimension value)));
if (*q == ':')
{
--- 247,259
errmsg(number of array dimensions (%d) exceeds the maximum allowed (%d),
ndim + 1, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++)
! /* skip */ ;
if (q == p)/* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(malformed array literal: \%s\, string),
! errdetail(\[\ must introduce explicitly-specified array dimensions.)));
if (*q == ':')
{
*** array_in(PG_FUNCTION_ARGS)
*** 259,269
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(missing dimension value)));
}
else
{
--- 261,273
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++)
! /* skip */ ;
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(malformed array literal: \%s\, string),
! errdetail(Missing array dimension value.)));
}
else
{
*** array_in(PG_FUNCTION_ARGS)
*** 273,279
if (*q != ']')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(missing \]\ in array dimensions)));
*q = '\0';
ub = atoi(p);
--- 277,285
if (*q != ']')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(malformed array literal: \%s\, string),
! errdetail(Missing \%s\ after array dimensions.,
! ])));
*q = '\0';
ub = atoi(p);
*** array_in(PG_FUNCTION_ARGS)
*** 293,299
if (*p != '{')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg(array value must start with \{\ or dimension information)));
ndim =
[HACKERS] bug in json_to_record with arrays
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
On 11/26/2014 11:54 AM, Josh Berkus wrote:
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
Lemme take that back, it didn't work. Just checked an old devel snapshot.
Looks like this is not intended to work, so the only bug is that we need
a less confusing error message.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Josh Berkus j...@agliodbs.com writes:
On 11/26/2014 11:54 AM, Josh Berkus wrote:
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
Lemme take that back, it didn't work. Just checked an old devel snapshot.
Looks like this is not intended to work, so the only bug is that we need
a less confusing error message.
What's happening is that this string:
[potter,chef,programmer]
is getting passed to array_in, which of course does not like it because
that's not the I/O syntax for Postgres arrays. Arguably,
populate_record_worker should be smart enough to convert somehow, but
it isn't today. Looks to me like it wouldn't succeed for the comparable
case of converting a sub-object to a Postgres composite type, either.
I'm satisfied with regarding those cases as missing features to be
added later.
As far as your request for a better error message is concerned, I'm a
bit inclined to lay the blame on array_in rather than the JSON code.
Wouldn't it be better if it said
ERROR: invalid input syntax for array: [potter,chef,programmer]
DETAIL: Dimension value is missing.
which is comparable to what you'd get out of most other input functions
that were feeling indigestion?
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
Re: [HACKERS] bug in json_to_record with arrays
On 11/26/2014 03:48 PM, Tom Lane wrote: Arguably, populate_record_worker should be smart enough to convert somehow, but it isn't today. Looks to me like it wouldn't succeed for the comparable case of converting a sub-object to a Postgres composite type, either. I'm satisfied with regarding those cases as missing features to be added later. Right. Before commit a749a23d7af4dba9b3468076ec561d2cbf69af09 it didn't try by default to treat the value as text, but instead errored out if the value was an array or object, with an appropriate message. Now we always try to treat it as text and pass that to the array or composite input functions, who now get the responsibility of telling us what's wrong. It might be possible to process such values recursively, but it would be far from trivial. As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. which is comparable to what you'd get out of most other input functions that were feeling indigestion? +1 cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
* Tom Lane (t...@sss.pgh.pa.us) wrote: As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. Sounds pretty reasonable to me, but I would just caution that we should check if that's considered 'leakproof' or not (or, if it is, if it'd ever possibly leak data it shouldn't or if it would only ever return information provided by the user). Otherwise, someone might be able to convince the planner to push it down below a security qual and expose data from rows which shouldn't be visible. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] bug in json_to_record with arrays
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. Sounds pretty reasonable to me, but I would just caution that we should check if that's considered 'leakproof' or not (or, if it is, if it'd ever possibly leak data it shouldn't or if it would only ever return information provided by the user). array_in could only be regarded as leakproof if every element-type input function it could ever call is also leakproof. Which ain't the case, so I sure hope it's not marked that way. (Likewise record_in, range_in, etc.) 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
