Re: remaining sql/json patches

Hi Alvaro,

On Fri, Jul 7, 2023 at 9:28 PM Alvaro Herrera <alvherre@alvh.no-ip.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