Attached 45th version of the patches.
Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.
On 17.03.2020 21:35, Pavel Stehule wrote:
Attached 44th version of the patches.
On 12.03.2020 16:41, Pavel Stehule wrote:
On 12.03.2020 00:09 Nikita Glukhov wrote:
Attached 43rd version of the patches.
The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).
Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.
It looks very well - all tests passed, code looks well.
Now, when there are special nodes, then the introduction of COERCE_INTERNAL_CAST is not necessary, and it is only my one and last objection again this patch's set.
I have removed patch #3 with COERCE_INTERNAL_CAST too.
Coercions in JsonValueExpr in JsonCtorExpr, which were previously hidden with
COERCE_INTERNAL_CAST and which were outputted as RETURNING or FORMAT JSON
clauses, now moved into separate expressions.
I am looking on the code, and although the code is correct it doesn't look well (consistently with other node types).
I think so JsonFormat and JsonReturning should be node types, not just structs. If these types will be nodes, then you can simplify _readJsonExpr and all node operations on this node.
JsonFormat and JsonReturning was transformed into nodes, and not included
directly into other nodes now.
User functions json[b]_build_object_ext() and json[b]_build_array_ext() also
can be easily removed. But it seems harder to remove new aggregate functions
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called
directly from JsonCtorExpr node.
I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.
+<->READ_ENUM_FIELD(on_error.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_error.default_expr);
+<->READ_ENUM_FIELD(on_empty.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_empty.default_expr);
JsonBehavior is node type. Then why you don't write just
READ_NODE_FIELD(on_error);
READ_NODE_FIELD(on_empty)
??
JsonBehavior now used in JsonExpr as a pointer to node.
And maybe the code can be better if you don't use JsonPassing struct (or use it only inside gram.y) and pass just List *names, List *values.
Nodes should to contains another nodes or scalar types. Using structs (that are not nodes) inside doesn't look consistently.
JsonPassing was replaced with two Lists.
I found some not finished code in 0003 patch
+
+json_name_and_value:
+/* TODO
+<-><--><-->KEY c_expr VALUE_P json_value_expr %prec POSTFIXOP
+<-><--><--><-->{ $$ = makeJsonKeyValue($2, $4); }
+<-><--><-->|
+*/
This is unsupported variant of standard syntax, because it seems to lead
to unresolvable conflicts. The only supports syntax is:
JSON_OBJECT(key : value)
JSON_OBJECT(key VALUE value)
The support for json type in jsonpath also seems to be immature, so I will try
to remove it in the next iteration.
What do you think? This patch is little bit off topic, so if you don't need it, then can be removed. Is there some dependency for "jsontable" ?
There is a dependency in SQL/JSON query functions executor, because executor
uses new structure JsonItem instead of plain JsonbValue. I will try to
preserve refactoring with JsonItem introduction, but remove json support.
If it will be still unacceptable, I will try to completely remove patch #1.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
