Feedback-ID: ia2694551:Fastmail
Date: Mon, 10 Jul 2023 16:47:12 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: remaining sql/json patches
Message-ID: <20230710144712.7xih2h5dry3uiydp@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+HiwqHh0MBugXj4v0t3j-5-eTuG3Q2fM83DKGGT1xiGax=8AQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230710144712.7xih2h5dry3uiydp%40alvherre.pgsql>
Precedence: bulk

On 2023-Jul-10, Amit Langote wrote:

> > I see that you add json_returning_clause_opt, but we already have
> > json_output_clause_opt.  Shouldn't these two be one and the same?
> > I think the new name is more sensible than the old one, since the
> > governing keyword is RETURNING; I suppose naming it "output" comes from
> > the fact that the standard calls this <JSON output clause>.
> 
> One difference between the two is that json_output_clause_opt allows
> specifying the FORMAT clause in addition to the RETURNING type name,
> while json_returning_clause_op only allows specifying the type name.
> 
> I'm inclined to keep only json_returning_clause_opt as you suggest and
> make parse_expr.c output an error if the FORMAT clause is specified in
> JSON() and JSON_SCALAR(), so turning the current syntax error on
> specifying RETURNING ... FORMAT for these functions into a parsing
> error.   Done that way in the attached updated patch and also updated
> the latter patch that adds JSON_EXISTS() and JSON_VALUE() to have
> similar behavior.

Yeah, that's reasonable.

> > I'm not in love with the fact that JSON and JSONB have pretty much
> > parallel type categorizing functionality. It seems entirely artificial.
> > Maybe this didn't matter when these were contained inside each .c file
> > and nobody else had to deal with that, but I think it's not good to make
> > this an exported concept.  Is it possible to do away with that?  I mean,
> > reduce both to a single categorization enum, and a single categorization
> > API.  Here you have to cast the enum value to int in order to make
> > ExecInitExprRec work, and that seems a bit lame; moreso when the
> > "is_jsonb" is determined separately (cf. ExecEvalJsonConstructor)
> 
> OK, I agree that a unified categorizing API might be better.  I'll
> look at making this better.  Btw, does src/include/common/jsonapi.h
> look like an appropriate place for that?

Hmm, that header is frontend-available, and the type-category appears to
be backend-only, so maybe no.  Perhaps jsonfuncs.h is more apropos?
execExpr.c is already dealing with array internals, so having to deal
with json internals doesn't seem completely out of place.


> > In the 2023 standard, JSON_SCALAR is just
> >
> > <JSON scalar> ::= JSON_SCALAR <left paren> <value expression> <right paren>
> >
> > but we seem to have added a <JSON output format> clause to it.  Should
> > we really?
> 
> Hmm, I am not seeing <JSON output format> in the rule for JSON_SCALAR,

Agh, yeah, I confused myself, sorry.

> Per what I wrote above, the grammar for JSON() and JSON_SCALAR() does
> not allow specifying the FORMAT clause.  Though considering what you
> wrote, the RETURNING clause does appear to be an extension to the
> standard's spec.

Hmm, I see that <JSON output clause> (which is RETURNING plus optional
FORMAT) appears included in JSON_OBJECT, JSON_ARRAY, JSON_QUERY,
JSON_SERIALIZE, JSON_OBJECTAGG, JSON_ARRAYAGG.  It's not necessarily a
bad thing to have it in other places, but we should consider it
carefully.  Do we really want/need it in JSON() and JSON_SCALAR()?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/