Re: [COMMITTERS] pgsql: Mark JSON error detail messages for translation.

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgresql(dot)org, Robert Haas <rhaas(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Mark JSON error detail messages for translation.
Date: 2012-06-12 18:40:52
Message-ID: 20494.1339526452@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

Robert Haas <rhaas(at)postgresql(dot)org> writes:
> Mark JSON error detail messages for translation.
> Per gripe from Tom Lane.

OK, that was one generically bad thing about json.c's error messages.
The other thing that was bothering me was the style of the errdetail
strings, eg

ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for type json"),
errdetail("line %d: Invalid escape \"\\%s\".",
lex->line_number, extract_mb_char(s))));

I'm not thrilled with the "line %d:" prefixes. That seems to me to
violate the letter and spirit of the guideline about making errdetail
messages be complete sentences. Furthermore, the line number is not
the most important part of the detail message, if indeed it has any
usefulness at all which is debatable. (It's certainly not going to
be tremendously useful when the error report doesn't show any of the
specific input string being complained of.)

Also, a minority of the errors report the whole input string:

ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for type json: \"%s\"",
lex->input),
errdetail("The input string ended unexpectedly.")));

And then there are some that provide the whole input string AND a line
number, just in case you thought there was any intentional design
decision there.

A minimum change IMO would be to recast the detail messages as complete
sentences, say "The escape sequence \"\\%s\" in line %d is invalid."
But I'd like a better-thought-out solution to identifying the location
of the error.

I notice that there's an unfinished attempt to maintain a line_start
pointer; if that were carried through, we could imagine printing the
current line up to the point of an error, which might provide a
reasonable balance between verbosity and insufficient context.
As an example, instead of

regression=# select '{"x":1,
z "y":"txt1"}'::json;
ERROR: invalid input syntax for type json
LINE 1: select '{"x":1,
^
DETAIL: line 2: Token "z" is invalid.

maybe we could print

regression=# select '{"x":1,
z "y":"txt1"}'::json;
ERROR: invalid input syntax for type json
LINE 1: select '{"x":1,
^
DETAIL: Token "z" is invalid in line 2: "z".

or perhaps better let it run to the end of the line:

regression=# select '{"x":1,
z "y":"txt1"}'::json;
ERROR: invalid input syntax for type json
LINE 1: select '{"x":1,
^
DETAIL: Token "z" is invalid in line 2: "z "y":"txt1"}".

Thoughts?

regards, tom lane

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Robert Haas 2012-06-12 18:49:04 Re: [COMMITTERS] pgsql: Mark JSON error detail messages for translation.
Previous Message Robert Haas 2012-06-12 18:40:09 Re: Re: [COMMITTERS] pgsql: Run pgindent on 9.2 source tree in preparation for first 9.3

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2012-06-12 18:42:49 Re: WIP patch for Todo Item : Provide fallback_application_name in contrib/pgbench, oid2name, and dblink
Previous Message Robert Haas 2012-06-12 18:40:09 Re: Re: [COMMITTERS] pgsql: Run pgindent on 9.2 source tree in preparation for first 9.3