Re: remaining sql/json patches

Hi,

On Fri, Jan 19, 2024 at 7:46 PM jian he <jian.universality@gmail.com> wrote:
> play with domain types.
> in ExecEvalJsonCoercion, seems func json_populate_type cannot cope
> with domain type.
>
> tests:
> drop domain test;
> create domain test as int[] check ( array_length(value,1) =2 and
> (value[1] = 1 or value[2] = 2));
> SELECT * from JSON_QUERY(jsonb'{"rec": "{1,2,3}"}', '$.rec' returning
> test omit quotes);
> SELECT * from JSON_QUERY(jsonb'{"rec": "{1,11}"}', '$.rec' returning
> test keep quotes);
> SELECT * from JSON_QUERY(jsonb'{"rec": "{2,11}"}', '$.rec' returning
> test omit quotes error on error);
> SELECT * from JSON_QUERY(jsonb'{"rec": "{2,2}"}', '$.rec' returning
> test keep quotes error on error);
>
> SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
> test omit quotes );
> SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
> test omit quotes null on error);
> SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
> test null on error);
> SELECT * from JSON_QUERY(jsonb'{"rec": [1,11]}', '$.rec' returning
> test omit quotes);
> SELECT * from JSON_QUERY(jsonb'{"rec": [2,2]}', '$.rec' returning test
> omit quotes);
>
> Many domain related tests seem not right.
> like the following, i think it should just return null.
> +SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING sqljsonb_int_not_null);
> +ERROR:  domain sqljsonb_int_not_null does not allow null values
>
> --another example
> SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING
> sqljsonb_int_not_null null on error);

Hmm, yes, I've thought the same thing, but the patch since it has
existed appears to have made an exception for the case when the
RETURNING type is a domain for some reason; I couldn't find any
mention of why in the old discussions.  I suspect it might be because
a domain's constraints should always be enforced, irrespective of what
the SQL/JSON's ON ERROR says.

Though, I'm inclined to classify the domain constraint failure errors
into the same class as any other error as far as the ON ERROR clause
is concerned, so have adjusted the code to do so.

Please check if the attached looks fine.

> Maybe in node JsonCoercion, we don't need both via_io and
> via_populate, but we can have one bool to indicate either call
> InputFunctionCallSafe or json_populate_type in ExecEvalJsonCoercion.

I'm not sure if there's a way to set such a bool statically, because
the decision between calling input function or json_populate_type()
must be made at run-time based on whether the input jsonb datum is a
scalar or not.  That said, I think we should ideally be able to always
use json_populate_type(), but it can't handle OMIT QUOTES for scalars
and I haven't been able to refactor it to do so

On Mon, Jan 22, 2024 at 9:00 AM jian he <jian.universality@gmail.com> wrote:
>
> based on v35.
> Now I only applied from 0001 to 0007.
> For {DEFAULT expression  ON EMPTY}  | {DEFAULT expression ON ERROR}
> restrict DEFAULT expression be either Const node or FuncExpr node.
> so these 3 SQL/JSON functions can be used in the btree expression index.

I'm not really excited about adding these restrictions into the
transformJsonFuncExpr() path.  Index or any other code that wants to
put restrictions already have those in place, no need to add them
here.  Moreover, by adding these restrictions, we might end up
preventing users from doing useful things with this like specify
column references.  If there are semantic issues with allowing that,
we should discuss them.

> I made some big changes on the doc. (see attachment)
> list (json_query, json_exists, json_value) as a new <section2> may be
> a good idea.
>
> follow these two links, we can see the difference.
> only apply v35, 0001 to 0007: https://v35-functions-json-html.vercel.app
> apply v35, 0001 to 0007 plus my changes:
> https://html-starter-seven-pied.vercel.app

Thanks for your patch.  I've adapted some of your proposed changes.

> minor issues:
> +        Note that if the <replaceable>path_expression</replaceable>
> +        is <literal>strict</literal>, an error is generated if it yields no
> +        items, provided the specified <literal>ON ERROR</literal> behavior is
> +        <literal>ERROR</literal>.
>
> how about something like this:
> +        Note that if the <replaceable>path_expression</replaceable>
> +        is <literal>strict</literal> and <literal>ON ERROR</literal>
> behavior is specified
> +        <literal>ERROR</literal>, an error is generated if it yields no
> +        items

Sure.

> +  <note>
> +   <para>
> +    SQL/JSON path expression can currently only accept values of the
> +    <type>jsonb</type> type, so it might be necessary to cast the
> +    <replaceable>context_item</replaceable> argument of these functions to
> +    <type>jsonb</type>.
> +   </para>
> +  </note>
> here should it be "SQL/JSON query functions"?

"path expressions" is not wrong but I agree that "query functions"
might be better, so changed.  I've also mentioned that the restriction
arises from the fact that SQL/JSON path langage expects the input
document to be passed as jsonb.

On Mon, Jan 22, 2024 at 3:14 PM jian he <jian.universality@gmail.com> wrote:
> I found two main issues regarding cocece SQL/JSON function output to
> other data types.
> * returning typmod influence the returning result of JSON_VALUE | JSON_QUERY.
> * JSON_VALUE | JSON_QUERY handles returning type domains allowing null
> and not allowing null inconsistencies.
>
> in ExecInitJsonExprCoercion, there is IsA(coercion,JsonCoercion) or
> not difference.
> for the returning of (JSON_VALUE | JSON_QUERY),
> "coercion" is a JsonCoercion or not is set in coerceJsonFuncExprOutput.
>
> this influence returning type with typmod is not -1.
> if set "coercion" as JsonCoercion Node then it may call the
> InputFunctionCallSafe to do the coercion.
> If not, it may call ExecInitFunc related code which is wrapped in
> ExecEvalCoerceViaIOSafe.
>
> for example:
> SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3));
> will ExecInitFunc, will init function bpchar(character, integer,
> boolean). it will set the third argument to true.
> so it will initiate related instructions like: `select
> bpchar('[,2]',7,true); ` which in the end will make the result be
> `[,2`
> However, InputFunctionCallSafe cannot handle that.
> simple demo:
> create table t(a char(3));
> --fail.
> INSERT INTO t values ('test');
> --ok
> select 'test'::char(3);
>
> however current ExecEvalCoerceViaIOSafe cannot handle omit quotes.
>
> even if I made the changes, still not bullet-proof.
> for example:
> create domain char3_domain_not_null as char(3) NOT NULL;
> create domain hello as text NOT NULL check (value = 'hello');
> create domain int42 as int check (value = 42);
> CREATE TYPE comp_domain_with_typmod AS (a char3_domain_not_null, b int42);
>
> SELECT JSON_VALUE(jsonb'{"rec": "(abcd,42)"}', '$.rec' returning
> comp_domain_with_typmod);
> will return NULL
>
> however
> SELECT JSON_VALUE(jsonb'{"rec": "abcd"}', '$.rec' returning
> char3_domain_not_null);
> will return `abc`.
>
> I made the modification, you can see the difference.
> attached is test_coerce.sql is the test file.
> test_coerce_only_v35.out  is the test output of only applying v35 0001
> to 0007 plus my previous changes[0].
> test_coerce_v35_plus_change.out is  the test output of applying to v35
> 0001 to 0007 plus changes (attachment) and previous changes[0].
>
> [0] https://www.postgresql.org/message-id/CACJufxHo1VVk_0th3AsFxqdMgjaUDz6s0F7%2Bj9rYA3d%3DURw97A%40mail.gmail.com

I'll think about this tomorrow.

In the meantime, here are the updated/reorganized patches with the
following changes:

* I started having second thoughts about introducing
json_populate_type(), jspIsMutable, and JsonbUnquote() in commits
separate from the commit introducing the SQL/JSON query functions
patch where they are needed, so I moved them back into that patch.  So
there are 2 fewer patches -- 0005, 0006 squashed into 0007.

* Boke the test file jsonb_sqljson into 2 files named
sqljson_queryfuncs and sqljson_jsontable.  Also, the test files under
ECPG to sql_jsontable

* Some cosmetic improvements in the JSON_TABLE() patch

I'll push 0001-0004 tomorrow, barring objections.

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