Re: remaining sql/json patches

Hi Alvaro,

On Fri, Jul 7, 2023 at 9:28 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Looking at 0001 now.

Thanks.

> I noticed that it adds JSON, JSON_SCALAR and JSON_SERIALIZE as reserved
> keywords to doc/src/sgml/keywords/sql2016-02-reserved.txt; but those
> keywords do not appear in the 2016 standard as reserved. I see that
> those keywords appear as reserved in sql2023-02-reserved.txt, so I
> suppose you're covered as far as that goes; you don't need to patch
> sql2016, and indeed that's the wrong thing to do.

Yeah, fixed that after Peter pointed it out.

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

> typo "requeted"

Fixed.

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

> 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,
which looks like this in the current grammar:

func_expr_common_subexpr:
...
| JSON_SCALAR '(' a_expr json_returning_clause_opt ')'
{
JsonScalarExpr *n = makeNode(JsonScalarExpr);

n->expr = (Expr *) $3;
n->output = (JsonOutput *) $4;
n->location = @1;
$$ = (Node *) n;
}
...
json_returning_clause_opt:
RETURNING Typename
{
JsonOutput *n = makeNode(JsonOutput);

n->typeName = $2;
n->returning = makeNode(JsonReturning);
n->returning->format =
makeJsonFormat(JS_FORMAT_DEFAULT, JS_ENC_DEFAULT, @2);
$$ = (Node *) n;
}
| /* EMPTY */ { $$ = NULL; }
;

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. I can't find any reasoning in the original
discussion as to how that came about, except an email from Andrew [1]
saying that he added it back to the patch.

Here's v3 in the meantime.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/flat/cd0bb935-0158-78a7-08b5-904886deac4b%40postgrespro.ru