Re: jsonpath

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.

> 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.

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.

Anyway, my main criticism is still that this is way too eager to use
an errdetail message when it could perfectly well fit all the info
into the primary error.

            regards, tom lane