Re: bug in json_to_record with arrays

Josh Berkus <josh(at)agliodbs(dot)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