Re: SQL/JSON: functions

From: Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Cc: Erik Rijkers <er(at)xs4all(dot)nl>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andrew Alsup <bluesbreaker(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Oleg Bartunov <obartunov(at)postgrespro(dot)ru>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Michael Paquier <michael(at)paquier(dot)xyz>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>
Subject: Re: SQL/JSON: functions
Date: 2020-03-19 22:57:21
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Attached 45th version of the patches.

Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.

On 17.03.2020 21:35, Pavel Stehule wrote:
> út 17. 3. 2020 v 1:55 odesílatel Nikita Glukhov
> <n(dot)gluhov(at)postgrespro(dot)ru <mailto:n(dot)gluhov(at)postgrespro(dot)ru>> napsal:
> 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)

> 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:
The Russian Postgres Company

Attachment Content-Type Size
0001-Jsonpath-support-for-json-v45.patch.gz application/gzip 43.6 KB
0002-Add-common-SQL-JSON-clauses-v45.patch.gz application/gzip 8.0 KB
0003-SQL-JSON-constructors-v45.patch.gz application/gzip 30.4 KB
0004-SQL-JSON-IS-JSON-predicate-v45.patch.gz application/gzip 12.6 KB
0005-SQL-JSON-query-functions-v45.patch.gz application/gzip 39.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2020-03-19 23:04:46 Re: Missing errcode() in ereport
Previous Message Andres Freund 2020-03-19 22:44:49 Why does [auto-]vacuum delay not report a wait event?