Re: remaining sql/json patches
От | Amit Langote |
---|---|
Тема | Re: remaining sql/json patches |
Дата | |
Msg-id | CA+HiwqGsYmyHy8EZA42JoW3ywGRFDGYQWz6X6-c_MG9y9+AkEA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: remaining sql/json patches (jian he <jian.universality@gmail.com>) |
Ответы |
Re: remaining sql/json patches
(jian he <jian.universality@gmail.com>)
|
Список | pgsql-hackers |
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
Вложения
- v36-0004-Add-jsonpath_exec-APIs-to-use-in-SQL-JSON-query-.patch
- v36-0002-Adjust-populate_record_field-to-handle-errors-so.patch
- v36-0001-Add-soft-error-handling-to-some-expression-nodes.patch
- v36-0003-Refactor-code-used-by-jsonpath-executor-to-fetch.patch
- v36-0005-SQL-JSON-query-functions.patch
- v36-0006-JSON_TABLE.patch
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Aleksander AlekseevДата:
Сообщение: Re: Remove unused fields in ReorderBufferTupleBuf