From: | Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: jsonpath |
Date: | 2019-04-28 18:28:46 |
Message-ID: | CAPpHfdtgFgUuXKjpz0qEH2Q5+S-T2XJJ8_PodBNA0_vRabJiig@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Apr 25, 2019 at 10:29 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > I'm going to commit these adjustments if no objections.
>
> Sorry for not getting to this sooner. Looking quickly at the v2 patch,
> it seems like you didn't entirely take to heart the idea of preferring
> a useful primary error message over a boilerplate primary with errdetail.
> In particular, in places like
>
> - errmsg(ERRMSG_SINGLETON_JSON_ITEM_REQUIRED),
> - errdetail("expression should return a singleton boolean")));
> + errmsg("singleton SQL/JSON item required"),
> + errdetail("Singleton boolean result is expected.")));
>
> why bother with errdetail at all? It's not conveying any useful increment
> of information. In this example I think
>
> errmsg("expression should return a singleton boolean")
>
> is sufficient and well-phrased. Likewise, a bit further down,
>
> + errmsg("SQL/JSON member not found"),
> + errdetail("JSON object does not contain key \"%s\".",
>
> there is nothing being said here that wouldn't fit perfectly well into
> one errmsg.
Makes sense. Attached revision of patch gets rid of errdetail() where
it seems to be appropriate.
> > My question regarding jsonpath_yyerror() vs. bison errors is still
> > relevant. Should we bother making bison-based errdetail() a complete
> > sentence starting from uppercase character? If not, should we make
> > other yyerror() calls look the same? Or should we rather move bison
> > error from errdetail() to errmsg()?
>
> The latter I think. The core lexer just presents the yyerror message
> as primary:
>
> scanner_yyerror(const char *message, core_yyscan_t yyscanner)
> {
> ...
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s at end of input", _(message)),
> lexer_errposition()));
>
> and in a quick look at what jsonpath is doing, I'm not really seeing
> the point of it being different. You could do something like
>
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s in jsonpath input", _(message))
>
> to preserve the information that this is about jsonpath, but beyond
> that I don't see that splitting off an errdetail is helping much.
I've moved error message into errmsg().
> Or, perhaps, provide an errdetail giving the full jsonpath input string?
> That might or might not be redundant with other context information,
> so I'm not sure how useful it'd be.
I'm also not sure about this. Didn't do this for now.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment | Content-Type | Size |
---|---|---|
jsonpath-errors-improve-3.patch | application/octet-stream | 42.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2019-04-28 19:52:54 | Re: Data streaming between different databases |
Previous Message | Fabien COELHO | 2019-04-28 18:22:09 | Re: [PATCH v4] Add \warn to psql |