Обсуждение: array_in sub function ReadArrayDimensions error message

while reviewing the json query doc,
I found out the following error message was not quite right.

select '[1,2]'::int[];
ERROR:  malformed array literal: "[1,2]"
LINE 1: select '[1,2]'::int[];
               ^
DETAIL:  Missing "]" after array dimensions.
should it be:
"Missing delimiter ":" while specifying array dimensions."
?
Because here, the first problem is the comma should be colon.


also
"DETAIL:  Missing "]" after array dimensions."
should be
DETAIL:  Missing "]" while specifying array dimensions.
?

jian he <jian.universality@gmail.com> writes:
> while reviewing the json query doc,
> I found out the following error message was not quite right.

> select '[1,2]'::int[];
> ERROR:  malformed array literal: "[1,2]"
> LINE 1: select '[1,2]'::int[];
>                ^
> DETAIL:  Missing "]" after array dimensions.

> should it be:
> "Missing delimiter ":" while specifying array dimensions."

That's presuming quite a lot about the nature of the error.
All the code knows is that what follows the "1" should be
either ":" or "]", and when it sees "," instead it throws
this error.  I agree the existing message isn't great, but
trying to be more specific could confuse people even more
if the more-specific message doesn't apply either.

One possibility could be

         if (*p != ']')
             ereturn(escontext, false,
                     (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
                      errmsg("malformed array literal: \"%s\"", origStr),
+                     (strchr(p, ']') != NULL) ?
+                     errdetail("Array dimensions have invalid syntax.") :
                      errdetail("Missing \"%s\" after array dimensions.",
                                "]")));

that is, only say "Missing "]"" if there's no ']' anywhere, and
otherwise just say the dimensions are wrong.  This could be fooled
by a ']' that's part of some string in the data, but even then the
errdetail isn't wrong.

Or we could just say "Array dimensions have invalid syntax."
unconditionally.

            regards, tom lane

On Mon, Jul 8, 2024 at 10:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > while reviewing the json query doc,
> > I found out the following error message was not quite right.
>
> > select '[1,2]'::int[];
> > ERROR:  malformed array literal: "[1,2]"
> > LINE 1: select '[1,2]'::int[];
> >                ^
> > DETAIL:  Missing "]" after array dimensions.
>
> > should it be:
> > "Missing delimiter ":" while specifying array dimensions."
>
> That's presuming quite a lot about the nature of the error.
> All the code knows is that what follows the "1" should be
> either ":" or "]", and when it sees "," instead it throws
> this error.  I agree the existing message isn't great, but
> trying to be more specific could confuse people even more
> if the more-specific message doesn't apply either.
>
> One possibility could be
>
>          if (*p != ']')
>              ereturn(escontext, false,
>                      (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>                       errmsg("malformed array literal: \"%s\"", origStr),
> +                     (strchr(p, ']') != NULL) ?
> +                     errdetail("Array dimensions have invalid syntax.") :
>                       errdetail("Missing \"%s\" after array dimensions.",
>                                 "]")));
>
> that is, only say "Missing "]"" if there's no ']' anywhere, and
> otherwise just say the dimensions are wrong.  This could be fooled
> by a ']' that's part of some string in the data, but even then the
> errdetail isn't wrong.
>
> Or we could just say "Array dimensions have invalid syntax."
> unconditionally.
>
>                         regards, tom lane


we can
if (*p == ':')
{
....

if (*p != ']')
ereturn(escontext, false,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", origStr),
errdetail("Missing \"%s\" after array dimensions.",
"]")));
}
else
{
if (*p != ']')
ereturn(escontext, false,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", origStr),
errdetail("Missing delimiter \"%s\" while specifying array dimensions.",
":")));
}


different error message in IF ELSE blocks.
please check attached.

jian he <jian.universality@gmail.com> writes:
> On Mon, Jul 8, 2024 at 10:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> One possibility could be
>> ...
>> that is, only say "Missing "]"" if there's no ']' anywhere, and
>> otherwise just say the dimensions are wrong.  This could be fooled
>> by a ']' that's part of some string in the data, but even then the
>> errdetail isn't wrong.
>> 
>> Or we could just say "Array dimensions have invalid syntax."
>> unconditionally.

> please check attached.

Meh.  This just replaces one possibly-off-point error message
with a different one that will be off-point in a different set
of cases.  I think we should simply reduce the specificity of
the message.

BTW, I think we have pretty much the same issue with respect
to the check for "=" later on.  For instance, if someone
is confused about whether to insert commas:

=# select '[1:1],[1:2]={{3,4}}'::int[];
ERROR:  malformed array literal: "[1:1],[1:2]={{3,4}}"
LINE 1: select  '[1:1],[1:2]={{3,4}}'::int[];
                ^
DETAIL:  Missing "=" after array dimensions.

Here again, the problem is not a missing "=", it's invalid
syntax somewhere before that.

Another thing we could consider doing here (and similarly
for your original case) is

DETAIL:  Expected "=" not "," after array dimensions.

The advantage of this is that it provides a little more
clarity as to exactly where things went wrong.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> One possibility all this ignores is that what we are calling
> array-dimensions are in reality most likely a user using json array syntax
> in our SQL arrays.  That seems eminently more likely than someone
> mis-typing this niche incantation of building an array literal nowadays.

Yeah, that's a good point.

> I'd add a hint if the first symbol is [ and we fail to get to the point of
> actually seeing the equal sign or the first subsequent unquoted symbol is a
> comma instead of a colon.

That seems closely related to my suggestion of applying strchr() to
see if the character we are expecting to see actually appears
anywhere.  Or did you have something else in mind?  Please be
specific, both about the test you are thinking of and how the
message should be worded.

            regards, tom lane

On Tue, Jul 9, 2024 at 11:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > On Mon, Jul 8, 2024 at 10:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> One possibility could be
> >> ...
> >> that is, only say "Missing "]"" if there's no ']' anywhere, and
> >> otherwise just say the dimensions are wrong.  This could be fooled
> >> by a ']' that's part of some string in the data, but even then the
> >> errdetail isn't wrong.
> >>
> >> Or we could just say "Array dimensions have invalid syntax."
> >> unconditionally.
>
> > please check attached.
>
> Meh.  This just replaces one possibly-off-point error message
> with a different one that will be off-point in a different set
> of cases.  I think we should simply reduce the specificity of
> the message.

After playing around, I agree with you that reducing the specificity of
the message here is better.


> BTW, I think we have pretty much the same issue with respect
> to the check for "=" later on.  For instance, if someone
> is confused about whether to insert commas:
>
> =# select '[1:1],[1:2]={{3,4}}'::int[];
> ERROR:  malformed array literal: "[1:1],[1:2]={{3,4}}"
> LINE 1: select  '[1:1],[1:2]={{3,4}}'::int[];
>                 ^
> DETAIL:  Missing "=" after array dimensions.
>
> Here again, the problem is not a missing "=", it's invalid
> syntax somewhere before that.
>
> Another thing we could consider doing here (and similarly
> for your original case) is
>
> DETAIL:  Expected "=" not "," after array dimensions.
>
> The advantage of this is that it provides a little more
> clarity as to exactly where things went wrong.
>
this looks good to me.


I also found other issues, like.
select '[-2:2147483644]={3}'::int[];
ERROR:  malformed array literal: "[-2:2147483644]={3}"
LINE 1: select '[-2:2147483644]={3}'::int[];
               ^
DETAIL:  Specified array dimensions do not match array contents.

select '[-2:2147483645]={3}'::int[];
ERROR:  array size exceeds the maximum allowed (134217727)
LINE 1: select '[-2:2147483645]={3}'::int[];
               ^

Both cases exceed the maximum allowed (134217727), but error is different.
also
"ERROR:  array size exceeds the maximum allowed (134217727)"
is weird. we are still validating the dimension info function, we
don't even know the actual size of the array.
maybe
"ERROR:  array dimensions specified array size exceeds the maximum
allowed (134217727)"


This customized dimension info is kind of obscure, even the
documentation of it is buried
in the third paragraph of
https://www.postgresql.org/docs/current/arrays.html#ARRAYS-IO


to address David G. Johnston concern,
in ReadArrayDimensions, some error message can unconditionally mention
errhint like:
errhint("array dimension template is \"[lower_bound:upper_bound]\", we
can optional have 6 of this"

I also found other issues, like.
select '[-2:2147483644]={3}'::int[];
ERROR:  malformed array literal: "[-2:2147483644]={3}"
LINE 1: select '[-2:2147483644]={3}'::int[];
               ^
DETAIL:  Specified array dimensions do not match array contents.

select '[-2:2147483645]={3}'::int[];
ERROR:  array size exceeds the maximum allowed (134217727)
LINE 1: select '[-2:2147483645]={3}'::int[];
               ^

Both cases exceed the maximum allowed (134217727), but error is different.
also

"ERROR:  array size exceeds the maximum allowed (134217727)"
is weird. we are still validating the dimension info function, we
don't even know the actual size of the array. 

maybe
"ERROR:  array dimensions specified array size exceeds the maximum
allowed (134217727)"

to address David G. Johnston concern,
in ReadArrayDimensions, some error message can unconditionally mention
errhint like:
errhint("array dimension template is \"[lower_bound:upper_bound]\", we
can optional have 6 of this"