Обсуждение: SQL/JSON query functions context_item doc entry and type requirement

hi
based on gram.y and function transformJsonValueExpr.

gram.y:
| JSON_QUERY '('
json_value_expr ',' a_expr json_passing_clause_opt
json_returning_clause_opt
json_wrapper_behavior
json_quotes_clause_opt
json_behavior_clause_opt
')'

| JSON_EXISTS '('
json_value_expr ',' a_expr json_passing_clause_opt
json_on_error_clause_opt
')'

| JSON_VALUE '('
json_value_expr ',' a_expr json_passing_clause_opt
json_returning_clause_opt
json_behavior_clause_opt
')'

json_format_clause_opt contains:
| FORMAT_LA JSON
{
$$ = (Node *) makeJsonFormat(JS_FORMAT_JSON, JS_ENC_DEFAULT, @1);
}


That means, all the context_item can specify "FORMAT JSON" options,
in the meantime, do we need to update these functions
synopsis/signature in the doc?

some examples:
create table a(b jsonb);
create table a1(b int4range);
select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a;
select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a1;
select json_value(text '"1"' format json, 'strict $[*]' DEFAULT 9 ON ERROR);

------------------------------------------------
transformJsonValueExpr

/* Try to coerce to the target type. */
coerced = coerce_to_target_type(pstate, expr, exprtype,
targettype, -1,
COERCION_EXPLICIT,
COERCE_EXPLICIT_CAST,
location);

based on the function transformJsonValueExpr and subfunction
coerce_to_target_type,
for SQL/JSON query functions (JSON_EXISTS, JSON_QUERY, and JSON_VALUE)
the context_item requirement is any data type that not error out while
explicitly casting to jsonb in coerce_to_target_type.

I played around with it, I think these types can be used in context_item.
{char,text,bpchar,character varying } and these types of associated domains.
bytea data type too, but need specify "ENCODING UTF8".
e.g.
select json_value(bytea '"1"' format json ENCODING UTF8, 'strict $[*]'
DEFAULT 9 ON ERROR);


Maybe we can add some brief explanation in this para to explain more
about "context_item"
{
SQL/JSON functions JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE()
described in Table 9.52 can be used to query JSON documents. Each of
these functions apply a path_expression (the query) to a context_item
(the document); see Section 9.16.2 for more details on what
path_expression can contain.
}

Hi,

On Tue, Jun 4, 2024 at 12:11 AM jian he <jian.universality@gmail.com> wrote:
>
> hi
> based on gram.y and function transformJsonValueExpr.
>
> gram.y:
> | JSON_QUERY '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_returning_clause_opt
> json_wrapper_behavior
> json_quotes_clause_opt
> json_behavior_clause_opt
> ')'
>
> | JSON_EXISTS '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_on_error_clause_opt
> ')'
>
> | JSON_VALUE '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_returning_clause_opt
> json_behavior_clause_opt
> ')'
>
> json_format_clause_opt contains:
> | FORMAT_LA JSON
> {
> $$ = (Node *) makeJsonFormat(JS_FORMAT_JSON, JS_ENC_DEFAULT, @1);
> }
>
>
> That means, all the context_item can specify "FORMAT JSON" options,
> in the meantime, do we need to update these functions
> synopsis/signature in the doc?
>
> some examples:
> create table a(b jsonb);
> create table a1(b int4range);
> select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a;
> select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a1;
> select json_value(text '"1"' format json, 'strict $[*]' DEFAULT 9 ON ERROR);
>
> ------------------------------------------------
> transformJsonValueExpr
>
> /* Try to coerce to the target type. */
> coerced = coerce_to_target_type(pstate, expr, exprtype,
> targettype, -1,
> COERCION_EXPLICIT,
> COERCE_EXPLICIT_CAST,
> location);
>
> based on the function transformJsonValueExpr and subfunction
> coerce_to_target_type,
> for SQL/JSON query functions (JSON_EXISTS, JSON_QUERY, and JSON_VALUE)
> the context_item requirement is any data type that not error out while
> explicitly casting to jsonb in coerce_to_target_type.
>
> I played around with it, I think these types can be used in context_item.
> {char,text,bpchar,character varying } and these types of associated domains.
> bytea data type too, but need specify "ENCODING UTF8".
> e.g.
> select json_value(bytea '"1"' format json ENCODING UTF8, 'strict $[*]'
> DEFAULT 9 ON ERROR);
>
>
> Maybe we can add some brief explanation in this para to explain more
> about "context_item"
> {
> SQL/JSON functions JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE()
> described in Table 9.52 can be used to query JSON documents. Each of
> these functions apply a path_expression (the query) to a context_item
> (the document); see Section 9.16.2 for more details on what
> path_expression can contain.
> }

If I understand correctly, you're suggesting that we add a line to the
above paragraph to mention which types are appropriate for
context_item.  How about we add the following:

<replaceable>context_item</replaceable> expression can be a value of
any type that can be cast to <type>jsonb</type>. This includes types
such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
<type>character varying</type>, and <type>bytea</type> (with
<code>ENCODING UTF8</code>), as well as any domains over these types.

--
Thanks, Amit Langote

On Mon, Jun 17, 2024 at 2:43 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi,
>
> On Tue, Jun 4, 2024 at 12:11 AM jian he <jian.universality@gmail.com> wrote:
> >
> > hi
> > based on gram.y and function transformJsonValueExpr.
> >
> > gram.y:
> > | JSON_QUERY '('
> > json_value_expr ',' a_expr json_passing_clause_opt
> > json_returning_clause_opt
> > json_wrapper_behavior
> > json_quotes_clause_opt
> > json_behavior_clause_opt
> > ')'
> >
> > | JSON_EXISTS '('
> > json_value_expr ',' a_expr json_passing_clause_opt
> > json_on_error_clause_opt
> > ')'
> >
> > | JSON_VALUE '('
> > json_value_expr ',' a_expr json_passing_clause_opt
> > json_returning_clause_opt
> > json_behavior_clause_opt
> > ')'
> >
> > json_format_clause_opt contains:
> > | FORMAT_LA JSON
> > {
> > $$ = (Node *) makeJsonFormat(JS_FORMAT_JSON, JS_ENC_DEFAULT, @1);
> > }
> >
> >
> > That means, all the context_item can specify "FORMAT JSON" options,
> > in the meantime, do we need to update these functions
> > synopsis/signature in the doc?
> >

>
> If I understand correctly, you're suggesting that we add a line to the
> above paragraph to mention which types are appropriate for
> context_item.  How about we add the following:
>
> <replaceable>context_item</replaceable> expression can be a value of
> any type that can be cast to <type>jsonb</type>. This includes types
> such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
> <type>character varying</type>, and <type>bytea</type> (with
> <code>ENCODING UTF8</code>), as well as any domains over these types.

your wording looks ok to me. I want to add two sentences. so it becomes:

+   The <replaceable>context_item</replaceable> expression can be a value of
+    any type that can be cast to <type>jsonb</type>. This includes types
+   such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
+    <type>character varying</type>, and <type>bytea</type> (with
+    <code>ENCODING UTF8</code>), as well as any domains over these types.
+    The <replaceable>context_item</replaceable> expression can also
be followed with
+    <literal>FORMAT JSON</literal>, <literal>ENCODING UTF8</literal>.
+    These two options currently don't have actual meaning.
+    <literal>ENCODING UTF8</literal> can only be specified when
<replaceable>context_item</replaceable> type is <type>bytea</type>.

imho, "These two options currently don't have actual meaning." is accurate,
but still does not explain why we allow "FORMAT JSON ENCODING UTF8".
I think we may need an explanation for  "FORMAT JSON ENCODING UTF8".
because json_array, json_object, json_serialize, json all didn't
mention the meaning of "[ FORMAT JSON [ ENCODING UTF8 ] ] ".


I added "[ FORMAT JSON [ ENCODING UTF8 ] ] " to the function
signature/synopsis of json_exists, json_query, json_value.

Hi,

On 06/17/24 02:43, Amit Langote wrote:
> <replaceable>context_item</replaceable> expression can be a value of
> any type that can be cast to <type>jsonb</type>. This includes types
> such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
> <type>character varying</type>, and <type>bytea</type> (with
> <code>ENCODING UTF8</code>), as well as any domains over these types.

Reading this message in conjunction with [0] makes me think that we are
really talking about a function that takes a first parameter of type jsonb,
and behaves exactly that way (so any cast required is applied by the system
ahead of the call). Under those conditions, this seems like an unusual
sentence to add in the docs, at least until we have also documented that
tan's argument can be of any type that can be cast to double precision.

On the other hand, if the behavior of the functions were to be changed
(perhaps using prosupport rewriting as suggested in [1]?) so that it was
not purely describable as a function accepting exactly jsonb with a
possible system-applied cast in front, then in that case such an added
explanation in the docs might be very fitting.

Regards,
-Chap


[0]
https://www.postgresql.org/message-id/CA%2BHiwqGuqLfAEP-FwW3QHByfQOoUpyj6YZG6R6bScpQswvNYDA%40mail.gmail.com
[1] https://www.postgresql.org/message-id/66703054.6040109%40acm.org

On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack <jcflack@acm.org> wrote:
>
> Hi,
>
> On 06/17/24 02:43, Amit Langote wrote:
> > <replaceable>context_item</replaceable> expression can be a value of
> > any type that can be cast to <type>jsonb</type>. This includes types
> > such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
> > <type>character varying</type>, and <type>bytea</type> (with
> > <code>ENCODING UTF8</code>), as well as any domains over these types.
>
> Reading this message in conjunction with [0] makes me think that we are
> really talking about a function that takes a first parameter of type jsonb,
> and behaves exactly that way (so any cast required is applied by the system
> ahead of the call). Under those conditions, this seems like an unusual
> sentence to add in the docs, at least until we have also documented that
> tan's argument can be of any type that can be cast to double precision.
>

I guess it would be fine to add an unusual sentence to the docs.

imagine a function: array_avg(anyarray) returns anyelement.
array_avg calculate an array's elements's avg. like
array('{1,2,3}'::int[]) returns 2.
but array_avg won't make sense if the input argument is a date array.
so mentioning in the doc: array_avg can accept anyarray, but anyarray
cannot date array.
seems ok.


> On the other hand, if the behavior of the functions were to be changed
> (perhaps using prosupport rewriting as suggested in [1]?) so that it was
> not purely describable as a function accepting exactly jsonb with a
> possible system-applied cast in front, then in that case such an added
> explanation in the docs might be very fitting.
>

prosupport won't work, I think.
because json_exists, json_value, json_query, json_table don't have
pg_proc entries.
These are more like expressions.

On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> On Wed, Jun 19, 2024 at 8:29 AM jian he <jian.universality@gmail.com> wrote:
>>
>> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack <jcflack@acm.org> wrote:
>> >
>> > Hi,
>> >
>> > On 06/17/24 02:43, Amit Langote wrote:
>> > > <replaceable>context_item</replaceable> expression can be a value of
>> > > any type that can be cast to <type>jsonb</type>. This includes types
>> > > such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
>> > > <type>character varying</type>, and <type>bytea</type> (with
>> > > <code>ENCODING UTF8</code>), as well as any domains over these types.
>> >
>> > Reading this message in conjunction with [0] makes me think that we are
>> > really talking about a function that takes a first parameter of type jsonb,
>> > and behaves exactly that way (so any cast required is applied by the system
>> > ahead of the call). Under those conditions, this seems like an unusual
>> > sentence to add in the docs, at least until we have also documented that
>> > tan's argument can be of any type that can be cast to double precision.
>> >
>>
>> I guess it would be fine to add an unusual sentence to the docs.
>>
>> imagine a function: array_avg(anyarray) returns anyelement.
>> array_avg calculate an array's elements's avg. like
>> array('{1,2,3}'::int[]) returns 2.
>> but array_avg won't make sense if the input argument is a date array.
>> so mentioning in the doc: array_avg can accept anyarray, but anyarray
>> cannot date array.
>> seems ok.
>
>
> There is existing wording for this:
>
> "The expression can be of any JSON type, any character string type, or bytea in UTF8 encoding."
>
> If you add this sentence to the paragraph the link that already exists, which simply points the reader to this
sentence,becomes redundant and should be removed. 

I've just posted a patch in the other thread [1] to restrict
context_item to be of jsonb type, which users would need to ensure by
adding an explicit cast if needed.  I think that makes this
clarification unnecessary.

> As for table 9.16.3 - it is unwieldy already.  Lets try and make the core syntax shorter, not longer.  We already
haveprecedence in the subsequent json_table section - give each major clause item a name then below the table define
thesyntax and meaning for those names.  Unlike in that section - which probably should be modified too - context_item
shouldhave its own description line. 

I had posted a patch a little while ago at [1] to render the syntax a
bit differently with each function getting its own syntax synopsis.
Resending it here; have addressed Jian He's comments.

--
Thanks, Amit Langote
[1] https://www.postgresql.org/message-id/CA%2BHiwqF2Z6FATWQV6bG9NeKYf%3D%2B%2BfOgmdbYc9gWSNJ81jfqCuA%40mail.gmail.com

On Thu, Jun 20, 2024 at 5:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
> > On Wed, Jun 19, 2024 at 8:29 AM jian he <jian.universality@gmail.com> wrote:
> >>
> >> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack <jcflack@acm.org> wrote:
> >> >
> >> > Hi,
> >> >
> >> > On 06/17/24 02:43, Amit Langote wrote:
> >> > > <replaceable>context_item</replaceable> expression can be a value of
> >> > > any type that can be cast to <type>jsonb</type>. This includes types
> >> > > such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
> >> > > <type>character varying</type>, and <type>bytea</type> (with
> >> > > <code>ENCODING UTF8</code>), as well as any domains over these types.
> >> >
> >> > Reading this message in conjunction with [0] makes me think that we are
> >> > really talking about a function that takes a first parameter of type jsonb,
> >> > and behaves exactly that way (so any cast required is applied by the system
> >> > ahead of the call). Under those conditions, this seems like an unusual
> >> > sentence to add in the docs, at least until we have also documented that
> >> > tan's argument can be of any type that can be cast to double precision.
> >> >
> >>
> >> I guess it would be fine to add an unusual sentence to the docs.
> >>
> >> imagine a function: array_avg(anyarray) returns anyelement.
> >> array_avg calculate an array's elements's avg. like
> >> array('{1,2,3}'::int[]) returns 2.
> >> but array_avg won't make sense if the input argument is a date array.
> >> so mentioning in the doc: array_avg can accept anyarray, but anyarray
> >> cannot date array.
> >> seems ok.
> >
> >
> > There is existing wording for this:
> >
> > "The expression can be of any JSON type, any character string type, or bytea in UTF8 encoding."
> >
> > If you add this sentence to the paragraph the link that already exists, which simply points the reader to this
sentence,becomes redundant and should be removed. 
>
> I've just posted a patch in the other thread [1] to restrict
> context_item to be of jsonb type, which users would need to ensure by
> adding an explicit cast if needed.  I think that makes this
> clarification unnecessary.
>
> > As for table 9.16.3 - it is unwieldy already.  Lets try and make the core syntax shorter, not longer.  We already
haveprecedence in the subsequent json_table section - give each major clause item a name then below the table define
thesyntax and meaning for those names.  Unlike in that section - which probably should be modified too - context_item
shouldhave its own description line. 
>
> I had posted a patch a little while ago at [1] to render the syntax a
> bit differently with each function getting its own syntax synopsis.
> Resending it here; have addressed Jian He's comments.
>
> --

@@ -18746,6 +18752,7 @@ ERROR:  jsonpath array subscript is out of bounds
         <literal>PASSING</literal> <replaceable>value</replaceable>s.
        </para>
        <para>
+        Returns the result of applying the SQL/JSON
         If the path expression returns multiple SQL/JSON items, it might be
         necessary to wrap the result using the <literal>WITH WRAPPER</literal>
         clause to make it a valid JSON string.  If the wrapper is


+        Returns the result of applying the SQL/JSON
 is redundant?


playing around with it.
found some minor issues:

json_exists allow:  DEFAULT expression ON ERROR, which is not
mentioned in the doc.
for example:
select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default
true ON ERROR);
select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 0 ON ERROR);
select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 11 ON ERROR);



JSON_VALUE on error, on empty semantics should be the same as json_query.
like:
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])

examples:
select JSON_value(jsonb '[]' , '$'  empty array on error);
select JSON_value(jsonb '[]' , '$'  empty object on error);

On Fri, Jun 21, 2024 at 9:47 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> On Thu, Jun 20, 2024 at 9:01 AM jian he <jian.universality@gmail.com> wrote:
>>
>> On Thu, Jun 20, 2024 at 5:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> >
>> > On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
>> > <david.g.johnston@gmail.com> wrote:
>> > > On Wed, Jun 19, 2024 at 8:29 AM jian he <jian.universality@gmail.com> wrote:
>> > >>
>> > >> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack <jcflack@acm.org> wrote:
>> > >> >
>> > >> > Hi,
>> > >> >
>> > >> > On 06/17/24 02:43, Amit Langote wrote:
>> > >> > > <replaceable>context_item</replaceable> expression can be a value of
>> > >> > > any type that can be cast to <type>jsonb</type>. This includes types
>> > >> > > such as <type>char</type>,  <type>text</type>, <type>bpchar</type>,
>> > >> > > <type>character varying</type>, and <type>bytea</type> (with
>> > >> > > <code>ENCODING UTF8</code>), as well as any domains over these types.
>> > >> >
>> > >> > Reading this message in conjunction with [0] makes me think that we are
>> > >> > really talking about a function that takes a first parameter of type jsonb,
>> > >> > and behaves exactly that way (so any cast required is applied by the system
>> > >> > ahead of the call). Under those conditions, this seems like an unusual
>> > >> > sentence to add in the docs, at least until we have also documented that
>> > >> > tan's argument can be of any type that can be cast to double precision.
>> > >> >
>> > >>
>> > >> I guess it would be fine to add an unusual sentence to the docs.
>> > >>
>> > >> imagine a function: array_avg(anyarray) returns anyelement.
>> > >> array_avg calculate an array's elements's avg. like
>> > >> array('{1,2,3}'::int[]) returns 2.
>> > >> but array_avg won't make sense if the input argument is a date array.
>> > >> so mentioning in the doc: array_avg can accept anyarray, but anyarray
>> > >> cannot date array.
>> > >> seems ok.
>> > >
>> > >
>> > > There is existing wording for this:
>> > >
>> > > "The expression can be of any JSON type, any character string type, or bytea in UTF8 encoding."
>> > >
>> > > If you add this sentence to the paragraph the link that already exists, which simply points the reader to this
sentence,becomes redundant and should be removed. 
>> >
>> > I've just posted a patch in the other thread [1] to restrict
>> > context_item to be of jsonb type, which users would need to ensure by
>> > adding an explicit cast if needed.  I think that makes this
>> > clarification unnecessary.
>> >
>> > > As for table 9.16.3 - it is unwieldy already.  Lets try and make the core syntax shorter, not longer.  We
alreadyhave precedence in the subsequent json_table section - give each major clause item a name then below the table
definethe syntax and meaning for those names.  Unlike in that section - which probably should be modified too -
context_itemshould have its own description line. 
>> >
>> > I had posted a patch a little while ago at [1] to render the syntax a
>> > bit differently with each function getting its own syntax synopsis.
>> > Resending it here; have addressed Jian He's comments.
>> >
>> > --
>
>
> I was thinking more like:
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index c324906b22..b9d157663a 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -18692,8 +18692,10 @@ $.* ? (@ like_regex "^\\d+$")
>        <entry role="func_table_entry"><para role="func_signature">
>          <indexterm><primary>json_exists</primary></indexterm>
>          <function>json_exists</function> (
> -        <replaceable>context_item</replaceable>, <replaceable>path_expression</replaceable> <optional>
<literal>PASSING</literal>{ <replaceable>value</replaceable> <literal>AS</literal> <replaceable>varname</replaceable> }
<optional>,...</optional></optional> 
> -        <optional> { <literal>TRUE</literal> | <literal>FALSE</literal> |<literal> UNKNOWN</literal> |
<literal>ERROR</literal>} <literal>ON ERROR</literal> </optional>) 
> +        <replaceable>context_item</replaceable>,
> +        <replaceable>path_expression</replaceable>
> +        <optional>variable_definitions</optional>
> +        <optional>on_error_boolean</optional>)
>         </para>
>         <para>
>          Returns true if the SQL/JSON <replaceable>path_expression</replaceable>
> @@ -18732,12 +18734,14 @@ ERROR:  jsonpath array subscript is out of bounds
>        <entry role="func_table_entry"><para role="func_signature">
>          <indexterm><primary>json_query</primary></indexterm>
>          <function>json_query</function> (
> -        <replaceable>context_item</replaceable>, <replaceable>path_expression</replaceable> <optional>
<literal>PASSING</literal>{ <replaceable>value</replaceable> <literal>AS</literal> <replaceable>varname</replaceable> }
<optional>,...</optional></optional> 
> -        <optional> <literal>RETURNING</literal> <replaceable>data_type</replaceable> <optional> <literal>FORMAT
JSON</literal><optional> <literal>ENCODING UTF8</literal> </optional> </optional> </optional> 
> -        <optional> { <literal>WITHOUT</literal> | <literal>WITH</literal> { <literal>CONDITIONAL</literal> |
<optional><literal>UNCONDITIONAL</literal></optional>} } <optional> <literal>ARRAY</literal> </optional>
<literal>WRAPPER</literal></optional> 
> -        <optional> { <literal>KEEP</literal> | <literal>OMIT</literal> } <literal>QUOTES</literal> <optional>
<literal>ONSCALAR STRING</literal> </optional> </optional> 
> -        <optional> { <literal>ERROR</literal> | <literal>NULL</literal> | <literal>EMPTY</literal> { <optional>
<literal>ARRAY</literal></optional> | <literal>OBJECT</literal> } | <literal>DEFAULT</literal>
<replaceable>expression</replaceable>} <literal>ON EMPTY</literal> </optional> 
> -        <optional> { <literal>ERROR</literal> | <literal>NULL</literal> | <literal>EMPTY</literal> { <optional>
<literal>ARRAY</literal></optional> | <literal>OBJECT</literal> } | <literal>DEFAULT</literal>
<replaceable>expression</replaceable>} <literal>ON ERROR</literal> </optional>) 
> +        <replaceable>context_item</replaceable>,
> +        <replaceable>path_expression</replaceable>
> +        <optional>variable_definitions</optional>
> +        <optional>return_clause</optional>
> +        <optional>wrapping_clause</optional>
> +        <optional>quoting_clause</optional>
> +        <optional>on_empty_set</optional>
> +        <optional>on_error_set</optional>)
>        </para>
>         <para>
>          Returns the result of applying the SQL/JSON
> @@ -18809,11 +18813,12 @@ DETAIL:  Missing "]" after array dimensions.
>        <entry role="func_table_entry"><para role="func_signature">
>          <indexterm><primary>json_value</primary></indexterm>
>          <function>json_value</function> (
> -        <replaceable>context_item</replaceable>, <replaceable>path_expression</replaceable>
> -        <optional> <literal>PASSING</literal> { <replaceable>value</replaceable> <literal>AS</literal>
<replaceable>varname</replaceable>} <optional>, ...</optional></optional> 
> -        <optional> <literal>RETURNING</literal> <replaceable>data_type</replaceable> </optional>
> -        <optional> { <literal>ERROR</literal> | <literal>NULL</literal> | <literal>DEFAULT</literal>
<replaceable>expression</replaceable>} <literal>ON EMPTY</literal> </optional> 
> -        <optional> { <literal>ERROR</literal> | <literal>NULL</literal> | <literal>DEFAULT</literal>
<replaceable>expression</replaceable>} <literal>ON ERROR</literal> </optional>) 
> +        <replaceable>context_item</replaceable>,
> +        <replaceable>path_expression</replaceable>
> +        <optional>variable_definitions</optional>
> +        <optional>return_type</optional>
> +        <optional>on_empty_value</optional>
> +        <optional>on_error_value</optional>)
>         </para>
>         <para>
>          Returns the result of applying the SQL/JSON
>
> Then defining each of those below the table - keeping the on_error variants together.

That sounds appealing. I'll try to come up with a patch unless you or
anyone else wants to take a stab at it.

>> playing around with it.
>> found some minor issues:
>>
>> json_exists allow:  DEFAULT expression ON ERROR, which is not
>> mentioned in the doc.
>> for example:
>> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default
>> true ON ERROR);
>> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 0 ON ERROR);
>> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 11 ON ERROR);
>
>
> Yeah, surprised it works, the documented behavior seems logical.  Being able to return a non-boolean here seems odd.
Especiallysince it is cast to boolean on output. 
>
>>
>> JSON_VALUE on error, on empty semantics should be the same as json_query.
>> like:
>> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
>> ON EMPTY ]
>> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
>> ON ERROR ])
>>
>> examples:
>> select JSON_value(jsonb '[]' , '$'  empty array on error);
>> select JSON_value(jsonb '[]' , '$'  empty object on error);
>
>
> Again the documented behavior seems to make sense though and the ability to specify empty in the value function seems
likea bug.  If you really want an empty array or object you do have access to default.  The reason json_query provides
foran empty array/object is that it is already expecting to produce an array (object seems a little odd). 
>
> I agree our docs and code do not match which needs to be fixed, ideally in the direction of the standard which I'm
guessingour documentation is based off of.  But let's not go off of my guess. 

Oops, that is indeed not great and, yes, the problem is code not
matching the documentation, the latter of which is "correct".

Basically, the grammar allows specifying any of the all possible ON
ERROR/EMPTY behavior values irrespective of the function, so parse
analysis should be catching and flagging an attempt to specify
incompatible value for a given function, which it does not.

I've attached a patch to fix that, which I'd like to push before
anything else we've been discussing.

--
Thanks, Amit Langote

On Fri, Jun 21, 2024 at 9:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jun 21, 2024 at 9:47 AM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
> > On Thu, Jun 20, 2024 at 9:01 AM jian he <jian.universality@gmail.com> wrote:
> >> playing around with it.
> >> found some minor issues:
> >>
> >> json_exists allow:  DEFAULT expression ON ERROR, which is not
> >> mentioned in the doc.
> >> for example:
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default
> >> true ON ERROR);
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 0 ON ERROR);
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 11 ON ERROR);
> >
> >
> > Yeah, surprised it works, the documented behavior seems logical.  Being able to return a non-boolean here seems
odd. Especially since it is cast to boolean on output. 
> >
> >>
> >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> >> like:
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON EMPTY ]
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON ERROR ])
> >>
> >> examples:
> >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> >
> >
> > Again the documented behavior seems to make sense though and the ability to specify empty in the value function
seemslike a bug.  If you really want an empty array or object you do have access to default.  The reason json_query
providesfor an empty array/object is that it is already expecting to produce an array (object seems a little odd). 
> >
> > I agree our docs and code do not match which needs to be fixed, ideally in the direction of the standard which I'm
guessingour documentation is based off of.  But let's not go off of my guess. 
>
> Oops, that is indeed not great and, yes, the problem is code not
> matching the documentation, the latter of which is "correct".
>
> Basically, the grammar allows specifying any of the all possible ON
> ERROR/EMPTY behavior values irrespective of the function, so parse
> analysis should be catching and flagging an attempt to specify
> incompatible value for a given function, which it does not.
>
> I've attached a patch to fix that, which I'd like to push before
> anything else we've been discussing.

While there are still a few hours to go before Saturday noon UTC when
beta2 freeze goes into effect, I'm thinking to just push this after
beta2 is stamped.  Adding an open item for now.

--
Thanks, Amit Langote

On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> >> like:
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON EMPTY ]
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON ERROR ])
> >>
> >> examples:
> >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> >
> > Again the documented behavior seems to make sense though and the ability to specify empty in the value function
seemslike a bug.  If you really want an empty array or object you do have access to default.  The reason json_query
providesfor an empty array/object is that it is already expecting to produce an array (object seems a little odd). 
> >
> > I agree our docs and code do not match which needs to be fixed, ideally in the direction of the standard which I'm
guessingour documentation is based off of.  But let's not go off of my guess. 
>
> Oops, that is indeed not great and, yes, the problem is code not
> matching the documentation, the latter of which is "correct".
>
> Basically, the grammar allows specifying any of the all possible ON
> ERROR/EMPTY behavior values irrespective of the function, so parse
> analysis should be catching and flagging an attempt to specify
> incompatible value for a given function, which it does not.
>
> I've attached a patch to fix that, which I'd like to push before
> anything else we've been discussing.
>

+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
<value> is allowed in ON ERROR for JSON_QUERY()."),
+ parser_errposition(pstate, func->on_error->location));

`EMPTY [ ARRAY | OBJECT }` seems not correct,
maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
(apply to other places)


`DEFAULT <value>`
`DEFAULT <expression>` or just `DEFAULT expression` would be more correct?
(apply to other places)

I think we should make json_query, json_value on empty, on error
behave the same way.
otherwise, it will have consistency issues for scalar jsonb.
for example, we should expect the following two queries to  return the
same result?
SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);

Also the json_table function will call json_value or json_query,
make these two functions on error, on empty behavior the same can
reduce unintended complexity.

So based on your
patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
and the above points, I have made some changes, attached.
it will make json_value, json_query not allow {true | false | unknown
} on error, {true | false | unknown } on empty.
json_table error message deal the same way as
b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af


BTW,
i found one JSON_TABLE document deficiency
        [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
expression } ON EMPTY ]
        [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
expression } ON ERROR ]

it should be

        [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
expression } ON EMPTY ]
        [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
expression } ON ERROR ]

Hi,

On Sat, Jun 22, 2024 at 6:39 PM jian he <jian.universality@gmail.com> wrote:
> On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> > >> like:
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON EMPTY ]
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON ERROR ])
> > >>
> > >> examples:
> > >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> > >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> > >
> > > Again the documented behavior seems to make sense though and the ability to specify empty in the value function
seemslike a bug.  If you really want an empty array or object you do have access to default.  The reason json_query
providesfor an empty array/object is that it is already expecting to produce an array (object seems a little odd). 
> > >
> > > I agree our docs and code do not match which needs to be fixed, ideally in the direction of the standard which
I'mguessing our documentation is based off of.  But let's not go off of my guess. 
> >
> > Oops, that is indeed not great and, yes, the problem is code not
> > matching the documentation, the latter of which is "correct".
> >
> > Basically, the grammar allows specifying any of the all possible ON
> > ERROR/EMPTY behavior values irrespective of the function, so parse
> > analysis should be catching and flagging an attempt to specify
> > incompatible value for a given function, which it does not.
> >
> > I've attached a patch to fix that, which I'd like to push before
> > anything else we've been discussing.
> >
>
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid ON ERROR behavior"),
> + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> <value> is allowed in ON ERROR for JSON_QUERY()."),
> + parser_errposition(pstate, func->on_error->location));
>
> `EMPTY [ ARRAY | OBJECT }` seems not correct,
> maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> (apply to other places)

Or EMPTY [ ARRAY ], EMPTY OBJECT

> `DEFAULT <value>`
> `DEFAULT <expression>` or just `DEFAULT expression` would be more correct?
> (apply to other places)

"DEFAULT expression" sounds good.

> I think we should make json_query, json_value on empty, on error
> behave the same way.
> otherwise, it will have consistency issues for scalar jsonb.
> for example, we should expect the following two queries to  return the
> same result?
> SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
>
> Also the json_table function will call json_value or json_query,
> make these two functions on error, on empty behavior the same can
> reduce unintended complexity.
>
> So based on your
> patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> and the above points, I have made some changes, attached.
> it will make json_value, json_query not allow {true | false | unknown
> } on error, {true | false | unknown } on empty.
> json_table error message deal the same way as
> b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af

Here is an updated patch that I think takes care of these points.

> BTW,
> i found one JSON_TABLE document deficiency
>         [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON EMPTY ]
>         [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON ERROR ]
>
> it should be
>
>         [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON EMPTY ]
>         [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON ERROR ]

You're right.  Fixed.

Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
columns which is meaningless because JSON_EXISTS() doesn't have a
corresponding ON EMPTY clause.  Fixed grammar to prevent that in the
attached 0002.

--
Thanks, Amit Langote

On Thu, Jun 27, 2024 at 9:01 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Sat, Jun 22, 2024 at 6:39 PM jian he <jian.universality@gmail.com> wrote:
> > On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > > >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> > > >> like:
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON EMPTY ]
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON ERROR ])
> > > >>
> > > >> examples:
> > > >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> > > >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> > > >
> > > > Again the documented behavior seems to make sense though and the ability to specify empty in the value function
seemslike a bug.  If you really want an empty array or object you do have access to default.  The reason json_query
providesfor an empty array/object is that it is already expecting to produce an array (object seems a little odd). 
> > > >
> > > > I agree our docs and code do not match which needs to be fixed, ideally in the direction of the standard which
I'mguessing our documentation is based off of.  But let's not go off of my guess. 
> > >
> > > Oops, that is indeed not great and, yes, the problem is code not
> > > matching the documentation, the latter of which is "correct".
> > >
> > > Basically, the grammar allows specifying any of the all possible ON
> > > ERROR/EMPTY behavior values irrespective of the function, so parse
> > > analysis should be catching and flagging an attempt to specify
> > > incompatible value for a given function, which it does not.
> > >
> > > I've attached a patch to fix that, which I'd like to push before
> > > anything else we've been discussing.
> > >
> >
> > + errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("invalid ON ERROR behavior"),
> > + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> > <value> is allowed in ON ERROR for JSON_QUERY()."),
> > + parser_errposition(pstate, func->on_error->location));
> >
> > `EMPTY [ ARRAY | OBJECT }` seems not correct,
> > maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> > (apply to other places)
>
> Or EMPTY [ ARRAY ], EMPTY OBJECT
>
> > `DEFAULT <value>`
> > `DEFAULT <expression>` or just `DEFAULT expression` would be more correct?
> > (apply to other places)
>
> "DEFAULT expression" sounds good.
>
> > I think we should make json_query, json_value on empty, on error
> > behave the same way.
> > otherwise, it will have consistency issues for scalar jsonb.
> > for example, we should expect the following two queries to  return the
> > same result?
> > SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> > SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
> >
> > Also the json_table function will call json_value or json_query,
> > make these two functions on error, on empty behavior the same can
> > reduce unintended complexity.
> >
> > So based on your
> > patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> > and the above points, I have made some changes, attached.
> > it will make json_value, json_query not allow {true | false | unknown
> > } on error, {true | false | unknown } on empty.
> > json_table error message deal the same way as
> > b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
>
> Here is an updated patch that I think takes care of these points.
>
> > BTW,
> > i found one JSON_TABLE document deficiency
> >         [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> >         [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON ERROR ]
> >
> > it should be
> >
> >         [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> >         [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON ERROR ]
>
> You're right.  Fixed.
>
> Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
> columns which is meaningless because JSON_EXISTS() doesn't have a
> corresponding ON EMPTY clause.  Fixed grammar to prevent that in the
> attached 0002.

I've pushed this for now to close out the open item.

I know there's some documentation improvement work left to do [1],
which I'll try to find some time for next week.

--
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com

>
> > As for table 9.16.3 - it is unwieldy already.  Lets try and make the core syntax shorter, not longer.  We already have precedence in the subsequent json_table section - give each major clause item a name then below the table define the syntax and meaning for those names.  Unlike in that section - which probably should be modified too - context_item should have its own description line.
>
> I had posted a patch a little while ago at [1] to render the syntax a
> bit differently with each function getting its own syntax synopsis.
> Resending it here; have addressed Jian He's comments.
>
> --

I was thinking more like:

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c324906b22..b9d157663a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18692,8 +18692,10 @@ $.* ? (@ like_regex "^\\d+$")
       <entry role="func_table_entry"><para role="func_signature">
         <indexterm><primary>json_exists</primary></indexterm>
         <function>json_exists</function> (
-        <replaceable>context_item</replaceable>, <replaceable>path_expression</replaceable> <optional> <literal>PASSING</literal> { <replaceable>value</replaceable> <literal>AS</literal> <replaceable>varname</replaceable> } <optional>, ...</optional></optional>
-        <optional> { <literal>TRUE</literal> | <literal>FALSE</literal> |<literal> UNKNOWN</literal> | <literal>ERROR</literal> } <literal>ON ERROR</literal> </optional>)
+        <replaceable>context_item</replaceable>,
+        <replaceable>path_expression</replaceable>
+        <optional>variable_definitions</optional>
+        <optional>on_error_boolean</optional>)
        </para> empty semantics should be the same as json_query.