Re: jsonpath

On 2019-Apr-22, Alexander Korotkov wrote:

> Hi!
> 
> Thank you for your review!
> 
> On Mon, Apr 22, 2019 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> >                 RETURN_ERROR(ereport(ERROR,
> >                                      (errcode(ERRCODE_JSON_ARRAY_NOT_FOUND),
> >                                       errmsg(ERRMSG_JSON_ARRAY_NOT_FOUND),
> >                                       errdetail("Jsonpath wildcard array accessor "
> >
> > In the first place, I'm not certain that this will result in the error
> > message being translatable --- do the gettext tools know how to expand
> > macros?
> >
> > In the second place, the actual strings are just restatements of their
> > ERRMSG macro names, which IMO is not conformant to our message style,
> > but it's too hard to see that from source code like this.  Also this
> > style is pretty unworkable/unfriendly if the message needs to contain
> > any %-markers, so I suspect that having a coding style like this may be
> > discouraging you from providing values in places where it'd be helpful to
> > do so.  What I actually see happening as a consequence of this approach is
> > that you're pushing the useful information off to an errdetail, which is
> > not really helpful and it's not per project style either.  The idea is to
> > make the primary message as helpful as possible without being long, not
> > to make it a simple restatement of the SQLSTATE that nobody can understand
> > without also looking at the errdetail.
> >
> > In the third place, this makes it hard for people to grep for occurrences
> > of an error string in our source code.
> >
> > And in the fourth place, we don't do this elsewhere; it does not help
> > anybody for jsonpath to invent its own coding conventions that are unlike
> > the rest of Postgres.
> 
> Just to clarify things.  Do you propose to get rid of RETURN_ERROR()
> macro by expanding it at every occurrence?  Or do you have other ideas
> in the mind?

I think he's not talking about the RETURN_ERROR macro, but about the
ERRMSG_JSON_ARRAY_NOT_FOUND macro.  The PG convention is to repeat the
message literal in every place instead of defining a macro with the
literal.  But at the same time, using the same errmsg() and only vary
the errdetail() is unhelpful, so we want most detail in the errmsg
instead; I think it'd be something like this:

ereport(ERROR,
    (errcode(ERRCODE_JSON_ARRAY_NOT_FOUND),
     errmsg("%s wildcard array accessor not found", "jsonpath")));

note I put the type name "jsonpath" in a separate literal, so that only
the interesting part is seen by translators.

I don't think Tom said anything about the RETURN_ERROR macro.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services