pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: John Naylor <john.naylor@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: jsonpath
Date: Thu, 25 Apr 2019 15:28:52 -0400
Message-ID: <28019.1556220532@sss.pgh.pa.us> (raw)
In-Reply-To: <CAPpHfdu_fX=R1tPCAFeQJRhyapcvhBu19Z2Mk8ibP09CVOu3sw@mail.gmail.com>
References: <CAPpHfdvkMdV_nQR3gO41GWJS=2Gjdrm6QhhYH2DwXJ+fMa6gBw@mail.gmail.com>
	<23e82578-b648-ab57-a4af-791abccff344@2ndQuadrant.com>
	<dcff626f-d44d-ad96-ecc0-109601e2cfb9@2ndQuadrant.com>
	<4971.1553779874@sss.pgh.pa.us>
	<5656.1553781027@sss.pgh.pa.us>
	<b2e3d035-4f42-4670-97ad-e79727c40960@2ndQuadrant.com>
	<CAPpHfdu-MvmN9pJnTtTCyuzbcF4t2K4hsfMdEtTGjfx7ZbGVYw@mail.gmail.com>
	<CAPpHfdue-pzzEVK+zoY6sbqG4LYC3JvW7-j8J78EqT-TOLdNiw@mail.gmail.com>
	<19611.1554593792@sss.pgh.pa.us>
	<CAPpHfduP2ShBe3r80u0zsOGxRG1XRd4c7PTj-iPfmv4DzL2dyQ@mail.gmail.com>
	<20190409161557.xwc4xwqeaimxcb4d@development>
	<CAPpHfdvtb3MivKZA-8DUBJux=DwkR2Kx2qoHcw+_TKW7Dwv_Ew@mail.gmail.com>
	<1ccb6aa8-0ba5-a14c-d3a3-8c0b7f73244b@2ndQuadrant.com>
	<CACPNZCt5B8jDCCGQiFoSuqmg-za_NCy4QDioBTLaNRih9+-bXg@mail.gmail.com>
	<14890.1555523005@sss.pgh.pa.us>
	<CAPpHfdsKYKDvMXbVU-ojzams0NcMVzE0Fuyg-abxpQGr=0QRcg@mail.gmail.com>
	<CAPpHfds_A=nfJypMNyU3+HxAs3tzT++eGzzQn-QcUYofyjCFJQ@mail.gmail.com>
	<28819.1555886387@sss.pgh.pa.! us>
	<CAPpHfduxZqqnA1CU232+tbB6OrCdM8eHJUTGQO6G0BZ7tnXxxQ@mail.gmail.com>
	<CAPpHfdu_fX=R1tPCAFeQJRhyapcvhBu19Z2Mk8ibP09CVOu3sw@mail.gmail.com>

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





view thread (198+ messages)  latest in thread

Message-ID: <28019.1556220532@sss.pgh.pa.us>
Permalink:  ../28019.1556220532@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/28019.1556220532@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, a.korotkov@postgrespro.ru, john.naylor@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
  Subject: Re: jsonpath
  In-Reply-To: <28019.1556220532@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox