remaining sql/json patches

On Mon, Jun 19, 2023 at 5:31 PM Amit Langote <amitlangote09@gmail.com> wrote:

So the following sql/json things still remain to be done:

* sql/json query functions:
json_exists()
json_query()
json_value()

* other sql/json functions:
json()
json_scalar()
json_serialize()

* finally:
json_table

Attached is the rebased patch for the 1st part.

...

I'll rebase and post the patches for "other sql/json functions" and
"json_table" shortly.

And here they are.

I realized that the patch for the "other sql/json functions" part is
relatively straightforward and has no dependence on the "sql/json
query functions" part getting done first. So I've made that one the
0001 patch. The patch I posted in the last email is now 0002, though
it only has changes related to changing the order of the patch, so I
decided not to change the patch version marker (v1).

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

On 21.06.23 10:25, Amit Langote wrote:

I realized that the patch for the "other sql/json functions" part is
relatively straightforward and has no dependence on the "sql/json
query functions" part getting done first. So I've made that one the
0001 patch. The patch I posted in the last email is now 0002, though
it only has changes related to changing the order of the patch, so I
decided not to change the patch version marker (v1).

(I suggest you change the version number anyway, next time. There are
plenty of numbers available.)

The 0001 patch contains a change to
doc/src/sgml/keywords/sql2016-02-reserved.txt, which seems
inappropriate. The additional keywords are already listed in the 2023
file, and they are not SQL:2016 keywords.

Another thing, I noticed that the SQL/JSON patches in PG16 introduced
some nonstandard indentation in gram.y. I would like to apply the
attached patch to straighten this out.

On Fri, Jul 7, 2023 at 8:31 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 21.06.23 10:25, Amit Langote wrote:

I realized that the patch for the "other sql/json functions" part is
relatively straightforward and has no dependence on the "sql/json
query functions" part getting done first. So I've made that one the
0001 patch. The patch I posted in the last email is now 0002, though
it only has changes related to changing the order of the patch, so I
decided not to change the patch version marker (v1).

(I suggest you change the version number anyway, next time. There are
plenty of numbers available.)

Will do. :)

The 0001 patch contains a change to
doc/src/sgml/keywords/sql2016-02-reserved.txt, which seems
inappropriate. The additional keywords are already listed in the 2023
file, and they are not SQL:2016 keywords.

Ah, indeed. Will remove.

Another thing, I noticed that the SQL/JSON patches in PG16 introduced
some nonstandard indentation in gram.y. I would like to apply the
attached patch to straighten this out.

Sounds fine to me.

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

On Fri, Jul 7, 2023 at 8:59 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 7, 2023 at 8:31 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 21.06.23 10:25, Amit Langote wrote:

I realized that the patch for the "other sql/json functions" part is
relatively straightforward and has no dependence on the "sql/json
query functions" part getting done first. So I've made that one the
0001 patch. The patch I posted in the last email is now 0002, though
it only has changes related to changing the order of the patch, so I
decided not to change the patch version marker (v1).

(I suggest you change the version number anyway, next time. There are
plenty of numbers available.)

Will do. :)

Here's v2.

0001 and 0002 are new patches for some improvements of the existing code.

In the main patches (0003~), I've mainly removed a few nonterminals in
favor of new rules in the remaining nonterminals, especially in the
JSON_TABLE patch.

I've also removed additions to sql2016-02-reserved.txt as Peter suggested.

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

Looking at 0001 now.

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.

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

typo "requeted"

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)

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?

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

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]/messages/by-id/cd0bb935-0158-78a7-08b5-904886deac4b@postgrespro.ru
saying that he added it back to the patch.

Here's v3 in the meantime.

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

[1]: /messages/by-id/cd0bb935-0158-78a7-08b5-904886deac4b@postgrespro.ru

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/

I forgot to add:

* 0001 looks an obvious improvement. You could just push it now, to
avoid carrying it forward anymore. I would just put the constructName
ahead of value expr in the argument list, though.

* 0002: I have no idea what this is (though I probably should). I would
also push it right away -- if anything, so that we figure out sooner
that it was actually needed in the first place. Or maybe you just need
the right test cases?

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

On Mon, Jul 10, 2023 at 11:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I forgot to add:

Thanks for the review of these.

* 0001 looks an obvious improvement. You could just push it now, to
avoid carrying it forward anymore. I would just put the constructName
ahead of value expr in the argument list, though.

Sure, that makes sense.

* 0002: I have no idea what this is (though I probably should). I would
also push it right away -- if anything, so that we figure out sooner
that it was actually needed in the first place. Or maybe you just need
the right test cases?

Hmm, I don't think having or not having CaseTestExpr makes a
difference to the result of evaluating JsonValueExpr.format_expr, so
there are no test cases to prove one way or the other.

After staring at this again for a while, I think I figured out why the
CaseTestExpr might have been put there in the first place. It seems
to have to do with the fact that JsonValueExpr.raw_expr is currently
evaluated independently of JsonValueExpr.formatted_expr and the
CaseTestExpr propagates the result of the former to the evaluation of
the latter. Actually, formatted_expr is effectively
formatting_function(<result-of-raw_expr>), so if we put raw_expr
itself into formatted_expr such that it is evaluated as part of
evaluating formatted_expr, then there is no need for the CaseTestExpr
as the propagator for raw_expr's result.

I've expanded the commit message to mention the details.

I'll push these tomorrow.

On Mon, Jul 10, 2023 at 11:47 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Jul-10, Amit Langote wrote:

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.

OK, attached 0003 does it like that. Essentially, I decided to only
keep JsonTypeCategory and json_categorize_type(), with some
modifications to accommodate the callers in jsonb.c.

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

I thought that removing that support breaks JSON_TABLE() or something
but it doesn't, so maybe we can do without the extension if there's no
particular reason it's there in the first place. Maybe Andrew (cc'd)
remembers why he decided in [1]/messages/by-id/1d44d832-4ea9-1ec9-81e9-bc6b2bd8cc43@dunslane.net to (re-) add the RETURNING clause to
JSON() and JSON_SCALAR()?

Updated patches, with 0003 being a new refactoring patch, are
attached. Patches 0004~ contain a few updates around JsonValueExpr.
Specifically, I removed the case for T_JsonValueExpr in
transformExprRecurse(), because I realized that JsonValueExpr
expressions never appear embedded in other expressions. That allowed
me to get rid of some needless refactoring around
transformJsonValueExpr() in the patch that adds JSON_VALUE() etc.

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

[1]: /messages/by-id/1d44d832-4ea9-1ec9-81e9-bc6b2bd8cc43@dunslane.net

On Wed, Jul 12, 2023 at 6:41 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 10, 2023 at 11:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I forgot to add:

Thanks for the review of these.

* 0001 looks an obvious improvement. You could just push it now, to
avoid carrying it forward anymore. I would just put the constructName
ahead of value expr in the argument list, though.

Sure, that makes sense.

* 0002: I have no idea what this is (though I probably should). I would
also push it right away -- if anything, so that we figure out sooner
that it was actually needed in the first place. Or maybe you just need
the right test cases?

Hmm, I don't think having or not having CaseTestExpr makes a
difference to the result of evaluating JsonValueExpr.format_expr, so
there are no test cases to prove one way or the other.

After staring at this again for a while, I think I figured out why the
CaseTestExpr might have been put there in the first place. It seems
to have to do with the fact that JsonValueExpr.raw_expr is currently
evaluated independently of JsonValueExpr.formatted_expr and the
CaseTestExpr propagates the result of the former to the evaluation of
the latter. Actually, formatted_expr is effectively
formatting_function(<result-of-raw_expr>), so if we put raw_expr
itself into formatted_expr such that it is evaluated as part of
evaluating formatted_expr, then there is no need for the CaseTestExpr
as the propagator for raw_expr's result.

I've expanded the commit message to mention the details.

I'll push these tomorrow.

I updated it to make the code in makeJsonConstructorExpr() that *does*
need to use a CaseTestExpr a bit more readable. Also, updated the
comment above CaseTestExpr to mention this instance of its usage.

On Mon, Jul 10, 2023 at 11:47 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Jul-10, Amit Langote wrote:

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.

OK, attached 0003 does it like that. Essentially, I decided to only
keep JsonTypeCategory and json_categorize_type(), with some
modifications to accommodate the callers in jsonb.c.

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

I thought that removing that support breaks JSON_TABLE() or something
but it doesn't, so maybe we can do without the extension if there's no
particular reason it's there in the first place. Maybe Andrew (cc'd)
remembers why he decided in [1] to (re-) add the RETURNING clause to
JSON() and JSON_SCALAR()?

Updated patches, with 0003 being a new refactoring patch, are
attached. Patches 0004~ contain a few updates around JsonValueExpr.
Specifically, I removed the case for T_JsonValueExpr in
transformExprRecurse(), because I realized that JsonValueExpr
expressions never appear embedded in other expressions. That allowed
me to get rid of some needless refactoring around
transformJsonValueExpr() in the patch that adds JSON_VALUE() etc.

I noticed that 0003 was giving some warnings, which is fixed in the
attached updated set of patches.

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

On Wed, Jul 12, 2023 at 10:23 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Jul 12, 2023 at 6:41 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 10, 2023 at 11:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I forgot to add:

Thanks for the review of these.

* 0001 looks an obvious improvement. You could just push it now, to
avoid carrying it forward anymore. I would just put the constructName
ahead of value expr in the argument list, though.

Sure, that makes sense.

* 0002: I have no idea what this is (though I probably should). I would
also push it right away -- if anything, so that we figure out sooner
that it was actually needed in the first place. Or maybe you just need
the right test cases?

Hmm, I don't think having or not having CaseTestExpr makes a
difference to the result of evaluating JsonValueExpr.format_expr, so
there are no test cases to prove one way or the other.

After staring at this again for a while, I think I figured out why the
CaseTestExpr might have been put there in the first place. It seems
to have to do with the fact that JsonValueExpr.raw_expr is currently
evaluated independently of JsonValueExpr.formatted_expr and the
CaseTestExpr propagates the result of the former to the evaluation of
the latter. Actually, formatted_expr is effectively
formatting_function(<result-of-raw_expr>), so if we put raw_expr
itself into formatted_expr such that it is evaluated as part of
evaluating formatted_expr, then there is no need for the CaseTestExpr
as the propagator for raw_expr's result.

I've expanded the commit message to mention the details.

I'll push these tomorrow.

I updated it to make the code in makeJsonConstructorExpr() that *does*
need to use a CaseTestExpr a bit more readable. Also, updated the
comment above CaseTestExpr to mention this instance of its usage.

Pushed these two just now.

On Mon, Jul 10, 2023 at 11:47 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Jul-10, Amit Langote wrote:

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.

OK, attached 0003 does it like that. Essentially, I decided to only
keep JsonTypeCategory and json_categorize_type(), with some
modifications to accommodate the callers in jsonb.c.

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

I thought that removing that support breaks JSON_TABLE() or something
but it doesn't, so maybe we can do without the extension if there's no
particular reason it's there in the first place. Maybe Andrew (cc'd)
remembers why he decided in [1] to (re-) add the RETURNING clause to
JSON() and JSON_SCALAR()?

Updated patches, with 0003 being a new refactoring patch, are
attached. Patches 0004~ contain a few updates around JsonValueExpr.
Specifically, I removed the case for T_JsonValueExpr in
transformExprRecurse(), because I realized that JsonValueExpr
expressions never appear embedded in other expressions. That allowed
me to get rid of some needless refactoring around
transformJsonValueExpr() in the patch that adds JSON_VALUE() etc.

I noticed that 0003 was giving some warnings, which is fixed in the
attached updated set of patches.

Here are the remaining patches, rebased. I'll remove the RETURNING
clause from JSON() and JSON_SCALAR() in the next version that I will
post tomorrow unless I hear objections.

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

I looked at your 0001. My 0001 are some trivial comment cleanups to
that.

I scrolled through all of jsonfuncs.c to see if there was a better place
for the new function than the end of the file. Man, is that one ugly
file. There are almost no comments! I almost wish you would create a
new file so that you don't have to put this new function in such bad
company. But maybe it'll improve someday, so ... whatever.

In the original code, the functions here being (re)moved do not need to
return a type output function in a few cases. This works okay when the
functions are each contained in a single file (because each function
knows that the respective datum_to_json/datum_to_jsonb user of the
returned values won't need the function OID in those other cases); but
as an exported function, that strange API doesn't seem great. (It only
works for 0002 because the only thing that the executor does with these
cached values is call datum_to_json/b). That seems easy to solve, since
we can return the hardcoded output function OID in those cases anyway.
A possible complaint about this is that the OID so returned would be
untested code, so they might be wrong and we'd never know. However,
ISTM it's better to make a promise about always returning a function OID
and later fixing any bogus function OID if we ever discover that we
return one, rather than having to document in the function's comment
that "we only return function OIDs in such and such cases". So I made
this change my 0002.

A similar complain can be made about which casts we look for. Right
now, only an explicit cast to JSON is useful, so that's the only thing
we do. But maybe one day a cast to JSONB would become useful if there's
no cast to JSON for some datatype (in the is_jsonb case only?); and
maybe another type of cast would be useful. However, that seems like
going too much into uncharted territory with no useful use case, so
let's just not go there for now. Maybe in the future we can improve
this aspect of it, if need arises.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Hi Alvaro,

On Fri, Jul 14, 2023 at 1:54 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I looked at your 0001. My 0001 are some trivial comment cleanups to
that.

Thanks.

I scrolled through all of jsonfuncs.c to see if there was a better place
for the new function than the end of the file. Man, is that one ugly
file. There are almost no comments! I almost wish you would create a
new file so that you don't have to put this new function in such bad
company. But maybe it'll improve someday, so ... whatever.

I tried to put it somewhere that is not the end of the file, though
anywhere would have looked arbitrary anyway for the reasons you
mention, so I didn't after all.

In the original code, the functions here being (re)moved do not need to
return a type output function in a few cases. This works okay when the
functions are each contained in a single file (because each function
knows that the respective datum_to_json/datum_to_jsonb user of the
returned values won't need the function OID in those other cases); but
as an exported function, that strange API doesn't seem great. (It only
works for 0002 because the only thing that the executor does with these
cached values is call datum_to_json/b).

Agreed about not tying the new API too closely to datum_to_json[b]'s needs.

That seems easy to solve, since
we can return the hardcoded output function OID in those cases anyway.
A possible complaint about this is that the OID so returned would be
untested code, so they might be wrong and we'd never know. However,
ISTM it's better to make a promise about always returning a function OID
and later fixing any bogus function OID if we ever discover that we
return one, rather than having to document in the function's comment
that "we only return function OIDs in such and such cases". So I made
this change my 0002.

+1

A similar complaint can be made about which casts we look for. Right
now, only an explicit cast to JSON is useful, so that's the only thing
we do. But maybe one day a cast to JSONB would become useful if there's
no cast to JSON for some datatype (in the is_jsonb case only?); and
maybe another type of cast would be useful. However, that seems like
going too much into uncharted territory with no useful use case, so
let's just not go there for now. Maybe in the future we can improve
this aspect of it, if need arises.

Hmm, yes, the note in the nearby comment stresses "to json (not to
jsonb)", though the (historical) reason why is not so clear to me.
I'm inclined to leave that as-is.

I've merged your deltas in the attached 0001 and rebased the other
patches. In 0002, I have now removed RETURNING support for JSON() and
JSON_SCALAR().

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

hi.
seems there is no explanation about, json_api_common_syntax in
functions-json.html

I can get json_query full synopsis from functions-json.html as follows:
json_query ( context_item, path_expression [ PASSING { value AS
varname } [, ...]] [ RETURNING data_type [ FORMAT JSON [ ENCODING UTF8
] ] ] [ { WITHOUT | WITH { CONDITIONAL | [UNCONDITIONAL] } } [ ARRAY ]
WRAPPER ] [ { KEEP | OMIT } QUOTES [ ON SCALAR STRING ] ] [ { ERROR |
NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])

seems doesn't have a full synopsis for json_table? only partial one
by one explanation.

Op 7/17/23 om 07:00 schreef jian he:

hi.
seems there is no explanation about, json_api_common_syntax in
functions-json.html

I can get json_query full synopsis from functions-json.html as follows:
json_query ( context_item, path_expression [ PASSING { value AS
varname } [, ...]] [ RETURNING data_type [ FORMAT JSON [ ENCODING UTF8
] ] ] [ { WITHOUT | WITH { CONDITIONAL | [UNCONDITIONAL] } } [ ARRAY ]
WRAPPER ] [ { KEEP | OMIT } QUOTES [ ON SCALAR STRING ] ] [ { ERROR |
NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])

seems doesn't have a full synopsis for json_table? only partial one
by one explanation.

FWIW, Re: json_api_common_syntax

An (old) pdf that I have (ISO/IEC TR 19075-6 First edition 2017-03)
contains the below specification. It's probably the source of the
particular term. It's easy to see how it maps onto the current v7
SQL/JSON implementation. (I don't know if it has changed in later
incarnations.)

------ 8< ------------
5.2 JSON API common syntax

The SQL/JSON query functions all need a path specification, the JSON
value to be input to that path specification for querying and
processing, and optional parameter values passed to the path
specification. They use a common syntax:

<JSON API common syntax> ::=
<JSON context item> <comma> <JSON path specification>
[ AS <JSON table path name> ]
[ <JSON passing clause> ]

<JSON context item> ::=
<JSON value expression>

<JSON path specification> ::=
<character string literal>

<JSON passing clause> ::=
PASSING <JSON argument> [ { <comma> <JSON argument> } ]

<JSON argument> ::=
<JSON value expression> AS <identifier>

------ 8< ------------

And yes, we might need a readable translation of that in the docs
although it might be easier to just get get rid of the term
'json_api_common_syntax'.

HTH,

Erik Rijkers

Hi,

On Mon, Jul 17, 2023 at 4:14 PM Erik Rijkers <er@xs4all.nl> wrote:

Op 7/17/23 om 07:00 schreef jian he:

hi.
seems there is no explanation about, json_api_common_syntax in
functions-json.html

I can get json_query full synopsis from functions-json.html as follows:
json_query ( context_item, path_expression [ PASSING { value AS
varname } [, ...]] [ RETURNING data_type [ FORMAT JSON [ ENCODING UTF8
] ] ] [ { WITHOUT | WITH { CONDITIONAL | [UNCONDITIONAL] } } [ ARRAY ]
WRAPPER ] [ { KEEP | OMIT } QUOTES [ ON SCALAR STRING ] ] [ { ERROR |
NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])

seems doesn't have a full synopsis for json_table? only partial one
by one explanation.

I looked through the history of the docs portion of the patch and it
looks like the synopsis for JSON_TABLE(...) used to be there but was
taken out during one of the doc reworks [1]/messages/by-id/044204fa-738d-d89a-0e81-1c04696ba676@dunslane.net.

I've added it back in the patch as I agree that it would help to have
it. Though, I am not totally sure where I've put it is the right
place for it. JSON_TABLE() is a beast that won't fit into the table
that JSON_QUERY() et al are in, so maybe that's how it will have to
be? I have no better idea.

FWIW, Re: json_api_common_syntax

...

An (old) pdf that I have (ISO/IEC TR 19075-6 First edition 2017-03)
contains the below specification. It's probably the source of the
particular term. It's easy to see how it maps onto the current v7
SQL/JSON implementation. (I don't know if it has changed in later
incarnations.)

------ 8< ------------
5.2 JSON API common syntax

The SQL/JSON query functions all need a path specification, the JSON
value to be input to that path specification for querying and
processing, and optional parameter values passed to the path
specification. They use a common syntax:

<JSON API common syntax> ::=
<JSON context item> <comma> <JSON path specification>
[ AS <JSON table path name> ]
[ <JSON passing clause> ]

<JSON context item> ::=
<JSON value expression>

<JSON path specification> ::=
<character string literal>

<JSON passing clause> ::=
PASSING <JSON argument> [ { <comma> <JSON argument> } ]

<JSON argument> ::=
<JSON value expression> AS <identifier>

------ 8< ------------

And yes, we might need a readable translation of that in the docs
although it might be easier to just get get rid of the term
'json_api_common_syntax'.

I found a patch proposed by Andrew Dunstan in the v15 dev cycle to get
rid of the term in the JSON_TABLE docs that Erik seemed to agree with
[2]: /messages/by-id/10c997db-9270-bdd5-04d5-0ffc1eefcdb7@dunslane.net

Attached updated patches. In 0002, I removed the mention of the
RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I
had forgotten to do in the last version which removed its support in
code.

I think 0001 looks ready to go. Alvaro?

Also, I've been wondering if it isn't too late to apply the following
to v16 too, so as to make the code look similar in both branches:

b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr
785480c953 Pass constructName to transformJsonValueExpr()

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

[1]: /messages/by-id/044204fa-738d-d89a-0e81-1c04696ba676@dunslane.net
[2]: /messages/by-id/10c997db-9270-bdd5-04d5-0ffc1eefcdb7@dunslane.net

On 2023-Jul-18, Amit Langote wrote:

Attached updated patches. In 0002, I removed the mention of the
RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I
had forgotten to do in the last version which removed its support in
code.

I think 0001 looks ready to go. Alvaro?

It looks reasonable to me.

Also, I've been wondering if it isn't too late to apply the following
to v16 too, so as to make the code look similar in both branches:

Hmm.

785480c953 Pass constructName to transformJsonValueExpr()

I think 785480c953 can easily be considered a bugfix on 7081ac46ace8, so
I agree it's better to apply it to 16.

b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr

I feel a bit uneasy about this one. It seems to assume that
formatted_expr is always set, but at the same time it's not obvious that
it is. (Maybe this aspect just needs some more commentary). I agree
that it would be better to make both branches identical, because if
there's a problem, we are better equipped to get a fix done to both.

As for the removal of makeCaseTestExpr(), I agree -- of the six callers
of makeNode(CastTestExpr), only two of them would be able to use the new
function, so it doesn't look of general enough usefulness.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Jul-18, Amit Langote wrote:

Attached updated patches. In 0002, I removed the mention of the
RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I
had forgotten to do in the last version which removed its support in
code.

I think 0001 looks ready to go. Alvaro?

It looks reasonable to me.

Thanks for taking another look.

I will push this tomorrow.

Also, I've been wondering if it isn't too late to apply the following
to v16 too, so as to make the code look similar in both branches:

Hmm.

785480c953 Pass constructName to transformJsonValueExpr()

I think 785480c953 can easily be considered a bugfix on 7081ac46ace8, so
I agree it's better to apply it to 16.

OK.

b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr

I feel a bit uneasy about this one. It seems to assume that
formatted_expr is always set, but at the same time it's not obvious that
it is. (Maybe this aspect just needs some more commentary).

Hmm, I agree that the comments about formatted_expr could be improved
further, for which I propose the attached. Actually, staring some
more at this, I'm inclined to change makeJsonValueExpr() to allow
callers to pass it the finished 'formatted_expr' rather than set it by
themselves.

I agree
that it would be better to make both branches identical, because if
there's a problem, we are better equipped to get a fix done to both.

As for the removal of makeCaseTestExpr(), I agree -- of the six callers
of makeNode(CastTestExpr), only two of them would be able to use the new
function, so it doesn't look of general enough usefulness.

OK, so you agree with back-patching this one too, though perhaps only
after applying something like the aforementioned patch. Just to be
sure, would the good practice in this case be to squash the fixup
patch into b6e1157e7d before back-patching?

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

On Wed, Jul 19, 2023 at 5:17 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Jul-18, Amit Langote wrote:

b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr

I feel a bit uneasy about this one. It seems to assume that
formatted_expr is always set, but at the same time it's not obvious that
it is. (Maybe this aspect just needs some more commentary).

Hmm, I agree that the comments about formatted_expr could be improved
further, for which I propose the attached. Actually, staring some
more at this, I'm inclined to change makeJsonValueExpr() to allow
callers to pass it the finished 'formatted_expr' rather than set it by
themselves.

Hmm, after looking some more, it may not be entirely right that
formatted_expr is always set in the code paths that call
transformJsonValueExpr(). Will look at this some more tomorrow.

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