Re: jsonpath

On Thu, Apr 25, 2019 at 10:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <a.korotkov@postgrespro.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