Re: jsonpath

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