Re: SQL/JSON: JSON_TABLE

Attached patches implementing JSON_TABLE.

This patchset depends on the 8th version of SQL/JSON functions patchset
that was posted in
/message-id/cd0bb935-0158-78a7-08b5-904886deac4b%40postgrespro.ru

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 9th version of JSON_TABLE patches rebased onto the latest master.

Documentation drafts written by Oleg Bartunov:
https://github.com/obartunov/sqljsondoc

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 10th version of JSON_TABLE patches rebased onto the latest master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 11th version of JSON_TABLE patches rebased onto the latest master.

Fixed PLAN DEFAULT flags assignment in gram.y.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 12th version of JSON_TABLE patches rebased onto the latest master.

Fixed JSON_TABLE plan validation.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 13th version of JSON_TABLE patches rebased onto the latest master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 15th version of JSON_TABLE patches.

Implicit root path name assignment was disabled (it is unclear from standard).
Now all JSON path names are required if the explicit PLAN clause is used.

The documentation for JSON_TABLE can be found now in a separate patch:
/message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 16th version of JSON_TABLE patches.

Changed only results of regression tests after the implicit coercion via I/O
was removed from JSON_VALUE.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> On Tue, Jul 3, 2018 at 4:50 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>
> Attached 16th version of JSON_TABLE patches.
>
> Changed only results of regression tests after the implicit coercion via I/O
> was removed from JSON_VALUE.

Thank you for working on this patch! Unfortunately, the current version of
patch 0015-json_table doesn't not apply anymore without conflicts, could you
please rebase it? In the meantime I'll try to provide some review.

On 26.11.2018 15:55, Dmitry Dolgov wrote:

>> On Tue, Jul 3, 2018 at 4:50 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>>
>> Attached 16th version of JSON_TABLE patches.
>>
>> Changed only results of regression tests after the implicit coercion via I/O
>> was removed from JSON_VALUE.
> Thank you for working on this patch! Unfortunately, the current version of
> patch 0015-json_table doesn't not apply anymore without conflicts, could you
> please rebase it? In the meantime I'll try to provide some review.

Attached 20th version of the patches rebased onto the current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 21st version of the patches rebased onto the current master.

You can also see all SQL/JSON v21 patches successfully applied in our GitHub
repository on the following branches:
https://github.com/postgrespro/sqljson/tree/sqljson_v21 (one commit per patch)
https://github.com/postgrespro/sqljson/tree/sqljson (original commit history)

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 34th version of the patches.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Feb 28, 2019 at 8:19 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> Attached 34th version of the patches.

Kinda strange version numbering -- the last post on this thread is v21.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 01.03.2019 19:17, Robert Haas wrote:

> On Thu, Feb 28, 2019 at 8:19 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>> Attached 34th version of the patches.
> Kinda strange version numbering -- the last post on this thread is v21.

For simplicity of dependence tracking, version numbering of JSON_TABLE patches
matches the version numbering of the patches on which it depends -- jsonpath
and SQL/JSON. The corresponding jsonpath patch has version v34 now.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 36th version of patches rebased onto jsonpath v36.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

so 29. 6. 2019 v 7:26 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> Attached 36th version of patches rebased onto jsonpath v36.
>

I cannot to apply these patches on master. Please, can you check these
patches?

Regards

Pavel

>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

On 29.06.2019 8:40, Pavel Stehule wrote:

> Hi
>
> so 29. 6. 2019 v 7:26 odesílatel Nikita Glukhov
> <n(dot)gluhov(at)postgrespro(dot)ru <mailto:n(dot)gluhov(at)postgrespro(dot)ru>> napsal:
>
> Attached 36th version of patches rebased onto jsonpath v36.
>
>
> I cannot to apply these patches on master. Please, can you check these
> patches?
>

Attached 37th version of patches rebased onto current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

út 16. 7. 2019 v 16:06 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> On 29.06.2019 8:40, Pavel Stehule wrote:
>
> Hi
>
> so 29. 6. 2019 v 7:26 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
> napsal:
>
>> Attached 36th version of patches rebased onto jsonpath v36.
>>
>
> I cannot to apply these patches on master. Please, can you check these
> patches?
>
>
> Attached 37th version of patches rebased onto current master.
>

I got warning

ar crs libpgcommon.a base64.o config_info.o controldata_utils.o d2s.o
exec.o f2s.o file_perm.o ip.o keywords.o kwlookup.o link-canary.o md5.o
pg_lzcompress.o pgfnames.o psprintf.o relpath.o rmtree.o saslprep.o
scram-common.o string.o unicode_norm.o username.o wait_error.>
...skipping...
clauses.c:1076:3: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
1076 | if (ExecEvalJsonNeedsSubTransaction(jsexpr, NULL))
| ^~
clauses.c:1078:4: note: ...this statement, but the latter is misleadingly
indented as if it were guarded by the ‘if’
1078 | return true;
| ^~~~~~
gcc -Wall -Wmissing-protot

Regress tests diff is not empty - see attached file

some strange fragments from code:

deparseExpr(node->arg, context);
- if (node->relabelformat != COERCE_IMPLICIT_CAST)
+ if (node->relabelformat != COERCE_IMPLICIT_CAST &&
+ node->relabelformat == COERCE_INTERNAL_CAST)

Now, "format" is type_func_name_keyword, so when you use it, then nobody
can use "format" as column name. It can break lot of application. "format"
is common name. It is relatively unhappy, and it can touch lot of users.

This patch set (JSON functions & JSON_TABLE) has more tha 20K rows. More,
there are more than few features are implemented.

Is possible to better (deeper) isolate these features, please? I have
nothing against any implemented feature, but it is hard to work intensively
(hard test) on this large patch. JSON_TABLE has only 184kB, can we start
with this patch?

SQLJSON_FUNCTIONS has 760kB - it is maybe too much for one feature, one
patch.

Pavel

> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Now this is one giant patchset ... and at least the first patch seems to
have more than one thing within -- even the commit message says so. It
seems clear that this is going to take a long time to digest; maybe if
we can get it in smaller pieces we can try to have a little at a time?
In other words, may I suggest to split it up in pieces that can be
reviewed and committed independently?

v37 no longer applies so it requires a rebase, and also typedefs.list
was wrongly merged.

Please update.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03.09.2019 20:29, Alvaro Herrera wrote:

> Now this is one giant patchset ... and at least the first patch seems to
> have more than one thing within -- even the commit message says so. It
> seems clear that this is going to take a long time to digest; maybe if
> we can get it in smaller pieces we can try to have a little at a time?
> In other words, may I suggest to split it up in pieces that can be
> reviewed and committed independently?

Patch 0001 is simply a squash of all 7 v38 patches from the thread
"SQL/JSON: functions". These patches are preliminary for JSON_TABLE.

Patch 0002 only needs to be review in this thread.

> v37 no longer applies so it requires a rebase, and also typedefs.list
> was wrongly merged.

typedefs.list was fixed.

> Please update.

Attached 38th version of the patches.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com The Russian Postgres Company

On 23.07.2019 16:58, Pavel Stehule wrote:
>
> I got warning
>
> ar crs libpgcommon.a base64.o config_info.o controldata_utils.o d2s.o
> exec.o f2s.o file_perm.o ip.o keywords.o kwlookup.o link-canary.o
> md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o rmtree.o
> saslprep.o scram-common.o string.o unicode_norm.o username.o wait_error.>
> ...skipping...
> clauses.c:1076:3: warning: this ‘if’ clause does not guard...
> [-Wmisleading-indentation]
>  1076 |   if (ExecEvalJsonNeedsSubTransaction(jsexpr, NULL))
>       |   ^~
> clauses.c:1078:4: note: ...this statement, but the latter is
> misleadingly indented as if it were guarded by the ‘if’
>  1078 |    return true;
>       |    ^~~~~~
> gcc -Wall -Wmissing-protot

Fixed in 38th version. Thanks.

> Regress tests diff is not empty - see attached file

Unfortunately, this is not reproducible on my machine, but really seems to be
a bug.

> some strange fragments from code:
>
>     deparseExpr(node->arg, context);
> -   if (node->relabelformat != COERCE_IMPLICIT_CAST)
> +   if (node->relabelformat != COERCE_IMPLICIT_CAST &&
> +       node->relabelformat == COERCE_INTERNAL_CAST)
>
There obviously should be

node->relabelformat != COERCE_INTERNAL_CAST

Fixed in 38th version. Thanks.

> Now, "format"  is type_func_name_keyword, so when you use it, then nobody
> can use "format" as column name. It can break lot of application. "format"
> is common name. It is relatively unhappy, and it can touch lot of users.

FORMAT was moved to unreserved_keywords in the 38th version. I remember that
there was conflicts with FORMAT, but now it works as unreserved_keyword.

>
> This patch set (JSON functions & JSON_TABLE) has more tha 20K rows.
> More, there are more than few features are implemented.
>
> Is possible to better (deeper) isolate these features, please? I have
> nothing against any implemented feature, but it is hard to work
> intensively (hard test) on this large patch. JSON_TABLE has only
> 184kB, can we start with this patch?
>
> SQLJSON_FUNCTIONS has 760kB - it is maybe too much for one feature,
> one patch.
>
Patch 0001 is simply a squash of all 7 patches from the thread
"SQL/JSON: functions". These patches are preliminary for JSON_TABLE.

Patch 0002 only needs to be review in this thread.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 39th version of the patches rebased onto current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

so 28. 9. 2019 v 3:53 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> Attached 39th version of the patches rebased onto current master.
>
>
Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there
will not be new format like XML or JSON

* there are new 600 lines to parse_clause.c, maybe this code can be placed
in new file parse_jsontable.c ? parse_clause.c is pretty long already
(json_table has very complex syntax)

*
+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell *exprlc;
+<-><-->ListCell *namelc;
+

It's uncommon usage of list_length function. More common is just "if
(ci->passing.values) {}". Is there any reason for list_length?

* I tested some examples that I found on net. It works very well. Minor
issues are white chars for json type. Probably json_table should to trim
returned values, because after cutting from document, original white chars
lost sense. It is not a problem jsonb type, that reduce white chars on
input.

I did only simple tests and I didn't find any other issues than white chars
problems for json type. I'll continue in some deeper tests. Please, prepare
documentation. Without documentation there is not clean what features are
supported. I have to do blind testing.

Regards

Pavel

> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Hi

po 30. 9. 2019 v 18:09 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi
>
> so 28. 9. 2019 v 3:53 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
> napsal:
>
>> Attached 39th version of the patches rebased onto current master.
>>
>>
This patch is still pretty big - it is about 6000 lines (without any
documentation). I checked the standard - and this patch try to implement

JSON_TABLE as part of T821
Plan clause T824
Plan default clause T838.

Unfortunately for last two features there are few documentation other than
standard, and probably other databases doesn't implement these features (I
didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided
by these features? I hope so separate review and commit can increase a
chance to merge this code (or the most significant part) in this release.

It is pretty hard to do any deeper review without documentation and without
other information sources.

What do you think?

Regards

Pavel

>
> Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1
>
> Comments:
>
> * +<->/* Only XMLTABLE and JSON_TABLE are supported currently */
>
> this comment has not sense more. Can be removed. Probably long time there
> will not be new format like XML or JSON
>
> * there are new 600 lines to parse_clause.c, maybe this code can be placed
> in new file parse_jsontable.c ? parse_clause.c is pretty long already
> (json_table has very complex syntax)
>
> *
> +<->if (list_length(ci->passing.values) > 0)
> +<->{
> +<-><-->ListCell *exprlc;
> +<-><-->ListCell *namelc;
> +
>
> It's uncommon usage of list_length function. More common is just "if
> (ci->passing.values) {}". Is there any reason for list_length?
>
> * I tested some examples that I found on net. It works very well. Minor
> issues are white chars for json type. Probably json_table should to trim
> returned values, because after cutting from document, original white chars
> lost sense. It is not a problem jsonb type, that reduce white chars on
> input.
>
> I did only simple tests and I didn't find any other issues than white
> chars problems for json type. I'll continue in some deeper tests. Please,
> prepare documentation. Without documentation there is not clean what
> features are supported. I have to do blind testing.
>
> Regards
>
> Pavel
>
>
>
>
>
>
>
>
>
>> --
>> Nikita Glukhov
>> Postgres Professional: http://www.postgrespro.com
>> The Russian Postgres Company
>>
>

Attached 40th version of the patches.

On 19.10.2019 18:31, Pavel Stehule wrote:
> This patch is still pretty big - it is about 6000 lines (without any
> documentation). I checked the standard - and this patch try to implement
>
> JSON_TABLE as part of T821
> Plan clause T824
> Plan default clause T838.
>
> Unfortunately for last two features there are few documentation other
> than standard, and probably other databases doesn't implement these
> features (I didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be
> this patch divided by these features? I hope so separate review and
> commit can increase a chance to merge this code (or the most
> significant part) in this release.
>
> It is pretty hard to do any deeper review without documentation and
> without other information sources.
>
> What do you think?

I think it is a good idea. So I have split JSON_TABLE patch into three
patches, each SQL feature. This really helped to reduce the size of the
main patch by about 40%.

On 30.09.2019 19:09, Pavel Stehule wrote:
> Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1
>
Unfortunately, this is still not reproducible on my computer with 64bit
Linux and gcc 9.2.1.

> Comments:
>
> * +<->/* Only XMLTABLE and JSON_TABLE are supported currently */
>
> this comment has not sense more. Can be removed. Probably long time
> there will not be new format like XML or JSON
Fixed.

> * there are new 600 lines to parse_clause.c, maybe this code can be
> placed in new file parse_jsontable.c ? parse_clause.c is pretty long
> already (json_table has very complex syntax)
Ok, the code was moved to parse_jsontable.c.

> *
> +<->if (list_length(ci->passing.values) > 0)
> +<->{
> +<-><-->ListCell   *exprlc;
> +<-><-->ListCell   *namelc;
> +
>
> It's uncommon usage of list_length function. More common is just "if
> (ci->passing.values) {}". Is there any reason for list_length?
>
Fixed.

> * I tested some examples that I found on net. It works very well.
> Minor issues are white chars for json type. Probably json_table should
> to trim returned values, because after cutting from document, original
> white chars lost sense. It is not a problem jsonb type, that reduce
> white chars on input.
>
> I did only simple tests and I didn't find any other issues than white
> chars problems for json type. I'll continue in some deeper tests.
> Please, prepare documentation. Without documentation there is not
> clean what features are supported. I have to do blind testing.
>
I have added some documentation to the patches which has simply been
copied from [1], but It still needs some work.

[1]
/message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

út 12. 11. 2019 v 1:13 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> Attached 40th version of the patches.
>
>
> On 19.10.2019 18:31, Pavel Stehule wrote:
>
> This patch is still pretty big - it is about 6000 lines (without any
> documentation). I checked the standard - and this patch try to implement
>
> JSON_TABLE as part of T821
> Plan clause T824
> Plan default clause T838.
>
> Unfortunately for last two features there are few documentation other than
> standard, and probably other databases doesn't implement these features (I
> didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided
> by these features? I hope so separate review and commit can increase a
> chance to merge this code (or the most significant part) in this release.
>
> It is pretty hard to do any deeper review without documentation and
> without other information sources.
>
> What do you think?
>
>
> I think it is a good idea. So I have split JSON_TABLE patch into three
> patches, each SQL feature. This really helped to reduce the size of the
> main patch by about 40%.
>

super, I'lll check main patch only now - to time when it will be merged to
core

>
> On 30.09.2019 19:09, Pavel Stehule wrote:
>
> Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1
>
> Unfortunately, this is still not reproducible on my computer with 64bit
> Linux and gcc 9.2.1.
>

Maybe it is locale depending issue. My LANG is LANG=cs_CZ.utf-8

>
> Comments:
>
> * +<->/* Only XMLTABLE and JSON_TABLE are supported currently */
>
> this comment has not sense more. Can be removed. Probably long time there
> will not be new format like XML or JSON
>
> Fixed.
>
> * there are new 600 lines to parse_clause.c, maybe this code can be placed
> in new file parse_jsontable.c ? parse_clause.c is pretty long already
> (json_table has very complex syntax)
>
> Ok, the code was moved to parse_jsontable.c.
>
> *
> +<->if (list_length(ci->passing.values) > 0)
> +<->{
> +<-><-->ListCell *exprlc;
> +<-><-->ListCell *namelc;
> +
>
> It's uncommon usage of list_length function. More common is just "if
> (ci->passing.values) {}". Is there any reason for list_length?
>
> Fixed.
>
> * I tested some examples that I found on net. It works very well. Minor
> issues are white chars for json type. Probably json_table should to trim
> returned values, because after cutting from document, original white chars
> lost sense. It is not a problem jsonb type, that reduce white chars on
> input.
>
> I did only simple tests and I didn't find any other issues than white
> chars problems for json type. I'll continue in some deeper tests. Please,
> prepare documentation. Without documentation there is not clean what
> features are supported. I have to do blind testing.
>
> I have added some documentation to the patches which has simply been
> copied from [1], but It still needs some work.
>

ok

Pavel

>
> [1]
> /message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Hi

please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a problem
with patching

Pavel

On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> On 12.11.2019 20:54, Pavel Stehule wrote:
>
> > Hi
> >
> > please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> > problem with patching
> >
> > Pavel
>
> Attached 41th version of the patches rebased onto current master.
>

I can say so broken regress tests has related to locales

with czech locale LANG=cs_CZ.UTF8 following two tests fails

json_sqljson ... FAILED 148 ms
jsonb_sqljson ... FAILED 3791 ms

The problem is in comparison digits and chars. the result is locale depend.

postgres=# select '10' > 'a' collate "C";
┌──────────┐
│ ?column? │
╞══════════╡
│ f │
└──────────┘
(1 row)

postgres=# select '10' > 'a' collate "cs_CZ";
┌──────────┐
│ ?column? │
╞══════════╡
│ t │
└──────────┘
(1 row)
postgres=# select '10' > 'a' collate "en_US";
┌──────────┐
│ ?column? │
╞══════════╡
│ f │
└──────────┘
(1 row)

Regards

Pavel

>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> On 12.11.2019 20:54, Pavel Stehule wrote:
>
> > Hi
> >
> > please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> > problem with patching
> >
> > Pavel
>
> Attached 41th version of the patches rebased onto current master.
>

I testing functionality - randomly testing some examples that I found on
internet.

I found:

a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not.
I think should be useful support this clause too.

SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

There is a question how to map boolean result to other data types.

b) When searched value is not scalar, then it returns null. This behave can
be suppressed by clause FORMAT Json. I found a different behave, and maybe
I found a bug. On MySQL this clause is by default for JSON values (what has
sense).

SELECT *
FROM
JSON_TABLE(
'[{"a":[1,2]}]',
'$[*]'
COLUMNS(
aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
)
) AS tt;

It returns null, although it should to return [1,2].

There is another bug maybe. Although there is DEFAULT clause. It returns
NULL.

I got correct result when I used FORMAT JSON clause. I think it should be
default behave for json and jsonb columns.

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? Why I cannot to use together FORMAT
JSON and DEFAULT clauses?

Note - this behave is not described in documentation.

Regards

Pavel

>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

On 17.11.2019 13:35, Pavel Stehule wrote:

> Hi
>
> út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov
> <n(dot)gluhov(at)postgrespro(dot)ru <mailto:n(dot)gluhov(at)postgrespro(dot)ru>> napsal:
>
> On 12.11.2019 20:54, Pavel Stehule wrote:
>
> > Hi
> >
> > please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> > problem with patching
> >
> > Pavel
>
> Attached 41th version of the patches rebased onto current master.
>
>
> I testing functionality - randomly testing some examples that I found
> on internet.
>
Thank you for testing JSON_TABLE.

> I found:
> a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not.
> I think should be useful support this clause too.
> SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...
>
EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:

=# SELECT *
FROM JSON_TABLE('{"a": 1}', '$'
COLUMNS (
a bool PATH 'exists($.a)',
b bool PATH 'exists($.b)'
));
a | b
---+---
t | f
(1 row)

But this works as expected only in lax mode. In strict mode EXISTS() returns
Unknown that transformed into SQL NULL:

=# SELECT *
FROM JSON_TABLE('{"a": 1}', '$'
COLUMNS (
a bool PATH 'strict exists($.a)',
b bool PATH 'strict exists($.b)'
));
a | b
---+---
t |
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.

So, I think it's worth to add EXISTS PATH clause to our implementation.

> There is a question how to map boolean result to other data types.

Now, boolean result can be used in JSON_TABLE columns of bool, int4, text,
json[b], and other types which have CAST from bool:

SELECT *
FROM JSON_TABLE('{"a": 1}', '$'
COLUMNS (
a int PATH 'exists($.a)',
b text PATH 'exists($.b)'
));
a | b
---+-------
1 | false
(1 row)

> b) When searched value is not scalar, then it returns null. This behave can be
> suppressed by clause FORMAT Json. I found a different behave, and maybe I found
> a bug. On MySQL this clause is by default for JSON values (what has sense).
> SELECT *
> FROM
>       JSON_TABLE(
>         '[{"a":[1,2]}]',
>         '$[*]'
>         COLUMNS(
>          aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
>         )
>       ) AS tt;
> It returns null, although it should to return [1,2].

Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values.
Otherwise an error is thrown, which can be caught by ON ERROR clause. This
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit
FORMAT JSON for json[b] typed-columns. But I'm a bit afraid that different
behavior can be standardized after introduction of json data types in SQL.

> There is another bug maybe. Although there is DEFAULT clause. It returns NULL.

ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM
JSON_TABLE(
'[{"a":[1,2]}]',
'$[*]'
COLUMNS(
aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
)
) AS tt;

aj
------------
{"x": 333}
(1 row)

ON EMPTY catches only empty-result case (for example, non-existent path in
lax mode):

SELECT *
FROM
JSON_TABLE(
'[{"a":[1,2]}]',
'$[*]'
COLUMNS(
aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY
)
) AS tt;
aj
------------
{"x": 333}
(1 row)

> I got correct result when I used FORMAT JSON clause.
> I think it should be default behave for json and jsonb columns.

I agree that FORMAT JSON could be implicit for json[b] columns. But I think
there could be one minor problem if we want to verify that returned value is
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM
JSON_TABLE(
'[{"a":[1,2]}]',
'$[*]'
COLUMNS (
aj JSON PATH 'lax $.a' ERROR ON ERROR
)
) AS tt;
ERROR: JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)

But with FORMAT JSON we need to construct complex jsonpath with a filter and
override ON EMPTY behavior:

SELECT *
FROM
JSON_TABLE(
'[{"a":[1,2]}]',
'$[*]'
COLUMNS (
aj JSON FORMAT JSON
-- strict mode is mandatory to prevent array unwrapping
PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
ERROR ON EMPTY ERROR ON ERROR
)
) AS tt;
ERROR: no SQL/JSON item

> Another question - when I used FORMAT JSON clause, then I got syntax error
> on DEFAULT keyword .. . Is it correct?
>
> Why I cannot to use together FORMAT JSON and DEFAULT clauses?

JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::=
<column name> <data type> FORMAT <JSON representation>
[ PATH <JSON table column path specification> ]
[ <JSON table formatted column wrapper behavior> WRAPPER ]
[ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
[ <JSON table formatted column empty behavior> ON EMPTY ]
[ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::=
ERROR
| NULL
| EMPTY ARRAY
| EMPTY OBJECT

<JSON table formatted column error behavior> ::=
ERROR
| NULL
| EMPTY ARRAY
| EMPTY OBJECT

But I also think that DEFAULT clause could be very useful in JSON_QUERY and
formatted JSON_TABLE columns.

> Note - this behave is not described in documentation.

There are references to JSON_QUERY and JSON_VALUE behavior in the definitions
of JSON_TABLE columns, but their behavior still seems to be unclear. This
needs to be fixed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> On 17.11.2019 13:35, Pavel Stehule wrote:
>
> Hi
>
> út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
> napsal:
>
>> On 12.11.2019 20:54, Pavel Stehule wrote:
>>
>> > Hi
>> >
>> > please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
>> > problem with patching
>> >
>> > Pavel
>>
>> Attached 41th version of the patches rebased onto current master.
>>
>
> I testing functionality - randomly testing some examples that I found on
> internet.
>
> Thank you for testing JSON_TABLE.
>
> I found:
>
> a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not.
> I think should be useful support this clause too.
>
> SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...
>
>
> EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
>
> =# SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a bool PATH 'exists($.a)',
> b bool PATH 'exists($.b)'
> ));
> a | b
> ---+---
> t | f
> (1 row)
>
> But this works as expected only in lax mode. In strict mode EXISTS() returns
> Unknown that transformed into SQL NULL:
>
> =# SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a bool PATH 'strict exists($.a)',
> b bool PATH 'strict exists($.b)'
> ));
> a | b
> ---+---
> t |
> (1 row)
>
> There is no easy way to return false without external COALESCE(),
> DEFAULT false ON ERROR also does not help.
>
> So, I think it's worth to add EXISTS PATH clause to our implementation.
>
>
>
> There is a question how to map boolean result to other data types.
>
> Now, boolean result can be used in JSON_TABLE columns of bool, int4, text,
> json[b], and other types which have CAST from bool:
>
> SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a int PATH 'exists($.a)',
> b text PATH 'exists($.b)'
> ));
> a | b
> ---+-------
> 1 | false
> (1 row)
>
>
> b) When searched value is not scalar, then it returns null. This behave can be
> suppressed by clause FORMAT Json. I found a different behave, and maybe I found
> a bug. On MySQL this clause is by default for JSON values (what has sense).
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS(
> aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
> )
> ) AS tt;
>
> It returns null, although it should to return [1,2].
>
> Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values.
> Otherwise an error is thrown, which can be caught by ON ERROR clause. This
> behavior is specified by the standard.
>
> FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
> standard does not have any json data types, so I think we can add implicit
> FORMAT JSON for json[b] typed-columns. But I'm a bit afraid that different
> behavior can be standardized after introduction of json data types in SQL.
>
>
> There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
>
> ON ERROR should be used if "not a scalar" error needs to be caught:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS(
> aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
> )
> ) AS tt;
>
> aj
> ------------
> {"x": 333}
> (1 row)
>
>
> ON EMPTY catches only empty-result case (for example, non-existent path in
> lax mode):
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS(
> aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY
> )
> ) AS tt;
> aj
> ------------
> {"x": 333}
> (1 row)
>
>
>
> I got correct result when I used FORMAT JSON clause.
> I think it should be default behave for json and jsonb columns.
>
> I agree that FORMAT JSON could be implicit for json[b] columns. But I think
> there could be one minor problem if we want to verify that returned value is
> scalar.
>
> Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS (
> aj JSON PATH 'lax $.a' ERROR ON ERROR
> )
> ) AS tt;
> ERROR: JSON path expression in JSON_VALUE should return singleton scalar item
>
> (This error message with the reference to implicit JSON_VALUE needs to be fixed.)
>
>
> But with FORMAT JSON we need to construct complex jsonpath with a filter and
> override ON EMPTY behavior:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS (
> aj JSON FORMAT JSON
> -- strict mode is mandatory to prevent array unwrapping
> PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
> ERROR ON EMPTY ERROR ON ERROR
> )
> ) AS tt;
> ERROR: no SQL/JSON item
>
> please, check the behave of other databases. I think so good conformance
with other RDBMS is important. More this method for checking if value is
object or not looks little bit scary.

maybe we can implement some functions like JSON_IS_OBJECT(),
JSON_IS_ARRAY(), JSON_IS_VALUE()?

More - we have this functionality already

ostgres=# select json_typeof('[10,20]');
┌─────────────┐
│ json_typeof │
╞═════════════╡
│ array │
└─────────────┘
(1 row)

Another question - when I used FORMAT JSON clause, then I got syntax error
> on DEFAULT keyword .. . Is it correct?
>
> Why I cannot to use together FORMAT JSON and DEFAULT clauses?
>
> JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only
> ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.
>
> This syntax is specified in the SQL standard:
>
> <JSON table formatted column definition> ::=
> <column name> <data type> FORMAT <JSON representation>
> [ PATH <JSON table column path specification> ]
> [ <JSON table formatted column wrapper behavior> WRAPPER ]
> [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
> [ <JSON table formatted column empty behavior> ON EMPTY ]
> [ <JSON table formatted column error behavior> ON ERROR ]
>
> <JSON table formatted column empty behavior> ::=
> ERROR
> | NULL
> | EMPTY ARRAY
> | EMPTY OBJECT
>
> <JSON table formatted column error behavior> ::=
> ERROR
> | NULL
> | EMPTY ARRAY
> | EMPTY OBJECT
>
>
> But I also think that DEFAULT clause could be very useful in JSON_QUERY and
> formatted JSON_TABLE columns.
>
>
> Note - this behave is not described in documentation.
>
> There are references to JSON_QUERY and JSON_VALUE behavior in the definitions
> of JSON_TABLE columns, but their behavior still seems to be unclear. This
> needs to be fixed.
>
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Attached 42th version of the patches rebased onto current master.

Changes from the previous version:
* added EXISTS PATH columns
* added DEFAULT clause for FORMAT JSON columns
* added implicit FORMAT JSON for columns of json[b], array and composite types

On 21.11.2019 19:51, Pavel Stehule wrote:
>
> čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov
> <n(dot)gluhov(at)postgrespro(dot)ru <mailto:n(dot)gluhov(at)postgrespro(dot)ru>> napsal:
>
> On 17.11.2019 13:35, Pavel Stehule wrote:
>
> I found:
>> a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not.
>> I think should be useful support this clause too.
>> SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...
>>
> EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
>
> =# SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a bool PATH 'exists($.a)',
> b bool PATH 'exists($.b)'
> ));
> a | b
> ---+---
> t | f
> (1 row)
>
> But this works as expected only in lax mode. In strict mode EXISTS() returns
> Unknown that transformed into SQL NULL:
>
> =# SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a bool PATH 'strict exists($.a)',
> b bool PATH 'strict exists($.b)'
> ));
> a | b
> ---+---
> t |
> (1 row)
>
> There is no easy way to return false without external COALESCE(),
> DEFAULT false ON ERROR also does not help.
>
> So, I think it's worth to add EXISTS PATH clause to our implementation.
>
>> There is a question how to map boolean result to other data types.
>
> Now, boolean result can be used in JSON_TABLE columns of bool, int4, text,
> json[b], and other types which have CAST from bool:
>
> SELECT *
> FROM JSON_TABLE('{"a": 1}', '$'
> COLUMNS (
> a int PATH 'exists($.a)',
> b text PATH 'exists($.b)'
> ));
> a | b
> ---+-------
> 1 | false
> (1 row)
>
EXISTS PATH columns were added. Only column types having CASTS
from boolean type are accepted.

Example:

SELECT *
FROM JSON_TABLE(
'{"foo": "bar"}', '$'
COLUMNS (
foo_exists boolean EXISTS PATH '$.foo',
foo int EXISTS,
err text EXISTS PATH '$ / 0' TRUE ON ERROR
)
);

foo_exists | foo | err
------------+-----+------
t | 1 | true
(1 row)

>> b) When searched value is not scalar, then it returns null. This behave can be
>> suppressed by clause FORMAT Json. I found a different behave, and maybe I found
>> a bug. On MySQL this clause is by default for JSON values (what has sense).
>> SELECT *
>> FROM
>>       JSON_TABLE(
>>         '[{"a":[1,2]}]',
>>         '$[*]'
>>         COLUMNS(
>>          aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
>>         )
>>       ) AS tt;
>> It returns null, although it should to return [1,2].
>
> Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values.
> Otherwise an error is thrown, which can be caught by ON ERROR clause. This
> behavior is specified by the standard.
>
> FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
> standard does not have any json data types, so I think we can add implicit
> FORMAT JSON for json[b] typed-columns. But I'm a bit afraid that different
> behavior can be standardized after introduction of json data types in SQL.
>
>> There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
>
> ON ERROR should be used if "not a scalar" error needs to be caught:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS(
> aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
> )
> ) AS tt;
>
> aj
> ------------
> {"x": 333}
> (1 row)
>
>
> ON EMPTY catches only empty-result case (for example, non-existent path in
> lax mode):
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS(
> aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY
> )
> ) AS tt;
> aj
> ------------
> {"x": 333}
> (1 row)
>
>> I got correct result when I used FORMAT JSON clause.
>> I think it should be default behave for json and jsonb columns.
>
> I agree that FORMAT JSON could be implicit for json[b] columns. But I think
> there could be one minor problem if we want to verify that returned value is
> scalar.
>
> Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS (
> aj JSON PATH 'lax $.a' ERROR ON ERROR
> )
> ) AS tt;
> ERROR: JSON path expression in JSON_VALUE should return singleton scalar item
>
> (This error message with the reference to implicit JSON_VALUE needs to be fixed.)
>
>
> But with FORMAT JSON we need to construct complex jsonpath with a filter and
> override ON EMPTY behavior:
>
> SELECT *
> FROM
> JSON_TABLE(
> '[{"a":[1,2]}]',
> '$[*]'
> COLUMNS (
> aj JSON FORMAT JSON
> -- strict mode is mandatory to prevent array unwrapping
> PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
> ERROR ON EMPTY ERROR ON ERROR
> )
> ) AS tt;
> ERROR: no SQL/JSON item
>
> please, check the behave of other databases. I think so good
> conformance with other RDBMS is important. More this method for
> checking if value is object or not looks little bit scary.
>
> maybe we can implement some functions like JSON_IS_OBJECT(),
> JSON_IS_ARRAY(), JSON_IS_VALUE()?
> More - we have this functionality already
>
> ostgres=# select json_typeof('[10,20]');
> ┌─────────────┐
> │ json_typeof │
> ╞═════════════╡
> │ array       │
> └─────────────┘
> (1 row)

Implicit FORMAT JSON is used for columns of json[b], array and composite types now.
The behavior is similar to behavior of json_populate_record().

Example:

CREATE TYPE test_record AS (foo text[], bar int);

SELECT *
FROM JSON_TABLE(
'{"foo": ["bar", 123, null]}', '$'
COLUMNS (
js json PATH '$',
jsonb_arr jsonb[] PATH '$.foo',
text_arr text[] PATH '$.foo',
int_arr int[] PATH '$.foo' DEFAULT '{}' ON ERROR,
rec test_record PATH '$'
)
);
js | jsonb_arr | text_arr | int_arr | rec
-----------------------------+----------------------+----------------+---------+---------------------
{"foo": ["bar", 123, null]} | {"\"bar\"",123,NULL} | {bar,123,NULL} | {} | ("{bar,123,NULL}",)
(1 row)

>> Another question - when I used FORMAT JSON clause, then I got syntax error
>> on DEFAULT keyword .. . Is it correct?
>>
>> Why I cannot to use together FORMAT JSON and DEFAULT clauses?
>
> JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only
> ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.
>
> This syntax is specified in the SQL standard:
>
> <JSON table formatted column definition> ::=
> <column name> <data type> FORMAT <JSON representation>
> [ PATH <JSON table column path specification> ]
> [ <JSON table formatted column wrapper behavior> WRAPPER ]
> [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
> [ <JSON table formatted column empty behavior> ON EMPTY ]
> [ <JSON table formatted column error behavior> ON ERROR ]
>
> <JSON table formatted column empty behavior> ::=
> ERROR
> | NULL
> | EMPTY ARRAY
> | EMPTY OBJECT
>
> <JSON table formatted column error behavior> ::=
> ERROR
> | NULL
> | EMPTY ARRAY
> | EMPTY OBJECT
>
>
> But I also think that DEFAULT clause could be very useful in JSON_QUERY and
> formatted JSON_TABLE columns.
>

DEFAULT clause was enabled in JSON_QUERY() and formatted JSON_TABLE columns:

SELECT *
FROM JSON_TABLE(
'{"foo": "bar"}', '$'
COLUMNS (
baz json FORMAT JSON DEFAULT '"empty"' ON EMPTY
)
);
baz
---------
"empty"
(1 row)

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

I read this patch

There are some typo in doc

*name* *type* EXISTS [ PATH *json_path_specification* ]

*Gerenates* a column and inserts a boolean item into each row of this
column.

Is good to allow repeat examples from documentation - so documentation
should to contains a INSERT with JSON, query and result.

JSON_TABLE is pretty complex function, probably the most complex function
what I know, so I propose to divide documentation to two parts - basic
advanced. The basic should to coverage the work with missing or error
values (with examples), and explain what are wrappers. Advanced part should
to describe work with plans. I afraid so lot of smaller examples has to be
necessary. Personally I propose postpone 0003 and 0004 patches to some next
releases. This is extra functionality and not well used and documented in
other RDBMS (depends on your capacity) - there is problem only in well
documentation - because this feature is not almost used in projects, the
small differences from standard or other RDBMS can be fixed later (like we
fixed XMLTABLE last year).

The documentation is good enough for initial commit - but should be
significantly enhanced before release.

I did some small performance tests - and parsing json with result cca 25000
rows needs 150 ms. It is great time.

My previous objections was solved.

The patches was applied cleanly. The compilation is without any issues and
warnings.
There are enough regress tests, and check-world was passed without problem.
Source code is readable, and well formatted.

I checked standard and checked conformance with other RDBMS.

I will mark this patch - JSON_TABLE implementation as ready for commiter.
The documentation should be enhanced - more examples, more simple examples
are necessary.

Regards

Thank you for your great, complex and hard work

It will be great feature

Pavel

út 14. 1. 2020 v 16:26 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
napsal:

> Attached 42th version of the patches rebased onto current master.
>
>
> Changes from the previous version:
> * added EXISTS PATH columns
> * added DEFAULT clause for FORMAT JSON columns
> * added implicit FORMAT JSON for columns of json[b], array and composite types
>
>
> On 21.11.2019 19:51, Pavel Stehule wrote:
>
>
> čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
> napsal:
>
>> On 17.11.2019 13:35, Pavel Stehule wrote:
>> I found:
>>
>> a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not.
>> I think should be useful support this clause too.
>>
>> SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...
>>
>>
>> EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
>>
>> =# SELECT *
>> FROM JSON_TABLE('{"a": 1}', '$'
>> COLUMNS (
>> a bool PATH 'exists($.a)',
>> b bool PATH 'exists($.b)'
>> ));
>> a | b
>> ---+---
>> t | f
>> (1 row)
>>
>> But this works as expected only in lax mode. In strict mode EXISTS() returns
>> Unknown that transformed into SQL NULL:
>>
>> =# SELECT *
>> FROM JSON_TABLE('{"a": 1}', '$'
>> COLUMNS (
>> a bool PATH 'strict exists($.a)',
>> b bool PATH 'strict exists($.b)'
>> ));
>> a | b
>> ---+---
>> t |
>> (1 row)
>>
>> There is no easy way to return false without external COALESCE(),
>> DEFAULT false ON ERROR also does not help.
>>
>> So, I think it's worth to add EXISTS PATH clause to our implementation.
>>
>>
>> There is a question how to map boolean result to other data types.
>>
>> Now, boolean result can be used in JSON_TABLE columns of bool, int4, text,
>> json[b], and other types which have CAST from bool:
>>
>> SELECT *
>> FROM JSON_TABLE('{"a": 1}', '$'
>> COLUMNS (
>> a int PATH 'exists($.a)',
>> b text PATH 'exists($.b)'
>> ));
>> a | b
>> ---+-------
>> 1 | false
>> (1 row)
>>
>> EXISTS PATH columns were added. Only column types having CASTS
> from boolean type are accepted.
>
> Example:
>
> SELECT *
> FROM JSON_TABLE(
> '{"foo": "bar"}', '$'
> COLUMNS (
> foo_exists boolean EXISTS PATH '$.foo',
> foo int EXISTS,
> err text EXISTS PATH '$ / 0' TRUE ON ERROR
> )
> );
>
> foo_exists | foo | err
> ------------+-----+------
> t | 1 | true
> (1 row)
>
>
>
> b) When searched value is not scalar, then it returns null. This behave can be
>> suppressed by clause FORMAT Json. I found a different behave, and maybe I found
>> a bug. On MySQL this clause is by default for JSON values (what has sense).
>>
>> SELECT *
>> FROM
>> JSON_TABLE(
>> '[{"a":[1,2]}]',
>> '$[*]'
>> COLUMNS(
>> aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
>> )
>> ) AS tt;
>>
>> It returns null, although it should to return [1,2].
>>
>> Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values.
>> Otherwise an error is thrown, which can be caught by ON ERROR clause. This
>> behavior is specified by the standard.
>>
>> FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
>> standard does not have any json data types, so I think we can add implicit
>> FORMAT JSON for json[b] typed-columns. But I'm a bit afraid that different
>> behavior can be standardized after introduction of json data types in SQL.
>>
>>
>> There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
>>
>> ON ERROR should be used if "not a scalar" error needs to be caught:
>>
>> SELECT *
>> FROM
>> JSON_TABLE(
>> '[{"a":[1,2]}]',
>> '$[*]'
>> COLUMNS(
>> aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
>> )
>> ) AS tt;
>>
>> aj
>> ------------
>> {"x": 333}
>> (1 row)
>>
>>
>> ON EMPTY catches only empty-result case (for example, non-existent path in
>> lax mode):
>>
>> SELECT *
>> FROM
>> JSON_TABLE(
>> '[{"a":[1,2]}]',
>> '$[*]'
>> COLUMNS(
>> aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY
>> )
>> ) AS tt;
>> aj
>> ------------
>> {"x": 333}
>> (1 row)
>>
>>
>> I got correct result when I used FORMAT JSON clause.
>> I think it should be default behave for json and jsonb columns.
>>
>> I agree that FORMAT JSON could be implicit for json[b] columns. But I think
>> there could be one minor problem if we want to verify that returned value is
>> scalar.
>>
>> Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:
>>
>> SELECT *
>> FROM
>> JSON_TABLE(
>> '[{"a":[1,2]}]',
>> '$[*]'
>> COLUMNS (
>> aj JSON PATH 'lax $.a' ERROR ON ERROR
>> )
>> ) AS tt;
>> ERROR: JSON path expression in JSON_VALUE should return singleton scalar item
>>
>> (This error message with the reference to implicit JSON_VALUE needs to be fixed.)
>>
>>
>> But with FORMAT JSON we need to construct complex jsonpath with a filter and
>> override ON EMPTY behavior:
>>
>> SELECT *
>> FROM
>> JSON_TABLE(
>> '[{"a":[1,2]}]',
>> '$[*]'
>> COLUMNS (
>> aj JSON FORMAT JSON
>> -- strict mode is mandatory to prevent array unwrapping
>> PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
>> ERROR ON EMPTY ERROR ON ERROR
>> )
>> ) AS tt;
>> ERROR: no SQL/JSON item
>>
>> please, check the behave of other databases. I think so good conformance
> with other RDBMS is important. More this method for checking if value is
> object or not looks little bit scary.
>
> maybe we can implement some functions like JSON_IS_OBJECT(),
> JSON_IS_ARRAY(), JSON_IS_VALUE()?
>
> More - we have this functionality already
>
> ostgres=# select json_typeof('[10,20]');
> ┌─────────────┐
> │ json_typeof │
> ╞═════════════╡
> │ array │
> └─────────────┘
> (1 row)
>
> Implicit FORMAT JSON is used for columns of json[b], array and composite types now.
> The behavior is similar to behavior of json_populate_record().
>
> Example:
>
> CREATE TYPE test_record AS (foo text[], bar int);
>
> SELECT *
> FROM JSON_TABLE(
> '{"foo": ["bar", 123, null]}', '$'
> COLUMNS (
> js json PATH '$',
> jsonb_arr jsonb[] PATH '$.foo',
> text_arr text[] PATH '$.foo',
> int_arr int[] PATH '$.foo' DEFAULT '{}' ON ERROR,
> rec test_record PATH '$'
> )
> );
> js | jsonb_arr | text_arr | int_arr | rec
> -----------------------------+----------------------+----------------+---------+---------------------
> {"foo": ["bar", 123, null]} | {"\"bar\"",123,NULL} | {bar,123,NULL} | {} | ("{bar,123,NULL}",)
> (1 row)
>
>
> Another question - when I used FORMAT JSON clause, then I got syntax error
>> on DEFAULT keyword .. . Is it correct?
>>
>> Why I cannot to use together FORMAT JSON and DEFAULT clauses?
>>
>> JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only
>> ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.
>>
>> This syntax is specified in the SQL standard:
>>
>> <JSON table formatted column definition> ::=
>> <column name> <data type> FORMAT <JSON representation>
>> [ PATH <JSON table column path specification> ]
>> [ <JSON table formatted column wrapper behavior> WRAPPER ]
>> [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
>> [ <JSON table formatted column empty behavior> ON EMPTY ]
>> [ <JSON table formatted column error behavior> ON ERROR ]
>>
>> <JSON table formatted column empty behavior> ::=
>> ERROR
>> | NULL
>> | EMPTY ARRAY
>> | EMPTY OBJECT
>>
>> <JSON table formatted column error behavior> ::=
>> ERROR
>> | NULL
>> | EMPTY ARRAY
>> | EMPTY OBJECT
>>
>>
>> But I also think that DEFAULT clause could be very useful in JSON_QUERY and
>> formatted JSON_TABLE columns.
>>
>> DEFAULT clause was enabled in JSON_QUERY() and formatted JSON_TABLE columns:
>
> SELECT *
> FROM JSON_TABLE(
> '{"foo": "bar"}', '$'
> COLUMNS (
> baz json FORMAT JSON DEFAULT '"empty"' ON EMPTY
> )
> );
> baz
> ---------
> "empty"
> (1 row)
>
>
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Hi

This patch needs rebase

Regards

Pavel

On 23.03.2020 19:24, Pavel Stehule wrote:

> This patch needs rebase

Attached 43rd version of the patches based on the latest (v47) SQL/JSON
functions patches.

Nothing significant has changed from the previous version, excluding
removed support for json type.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Mar 23, 2020 at 08:33:34PM +0300, Nikita Glukhov wrote:
> On 23.03.2020 19:24, Pavel Stehule wrote:
> > This patch needs rebase
>
> Attached 43rd version of the patches based on the latest (v47) SQL/JSON
> functions patches.

It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".

--
Justin

On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
> It looks like this needs to be additionally rebased - I will set cfbot to
> "Waiting".

... Something that has not happened in four weeks, so this is marked
as returned with feedback. Please feel free to resubmit once a rebase
is done.
--
Michael

On 03.08.2020 10:55, Michael Paquier wrote:
> On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
>> It looks like this needs to be additionally rebased - I will set cfbot to
>> "Waiting".
> ... Something that has not happened in four weeks, so this is marked
> as returned with feedback. Please feel free to resubmit once a rebase
> is done.
> --
> Michael

Atatched 44th version of the pacthes rebased onto current master
(#0001 corresponds to v51 of SQL/JSON patches).

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

For new files introduced in the patches:

+ * Portions Copyright (c) 1996-2019, 토토 사이트

2021 is a few days ahead. It would be good to update the year range.

For transformJsonTableColumn:

+ jfexpr->op =
+ jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+ jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;

Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?

For JsonTableDestroyOpaque():

+ state->opaque = NULL;

Should the memory pointed to by opaque be freed ?

+ l2 = list_head(tf->coltypes);
+ l3 = list_head(tf->coltypmods);
+ l4 = list_head(tf->colvalexprs);

Maybe the ListCell pointer variables can be named corresponding to the
lists they iterate so that the code is easier to understand.

+typedef enum JsonTablePlanJoinType
+{
+ JSTP_INNER = 0x01,
+ JSTP_OUTER = 0x02,
+ JSTP_CROSS = 0x04,

Since plan type enum starts with JSTP_, I think the plan join type should
start with JSTPJ_ or JSTJ_. Otherwise the following would be a bit hard to
read:

+ else if (plan->plan_type == JSTP_JOINED)
+ {
+ if (plan->join_type == JSTP_INNER ||
+ plan->join_type == JSTP_OUTER)

since different fields are checked in the two if statements but the
prefixes don't convey that.

+ Even though the path names are not incuded into the <literal>PLAN
DEFAULT</literal>

Typo: incuded -> included

+ int nchilds = 0;

nchilds -> nchildren

+#if 0 /* XXX it' unclear from the standard whether root path name is
mandatory or not */
+ if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)

Do you plan to drop the if block ?

Cheers

On Fri, Dec 25, 2020 at 12:32 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
wrote:

>
> On 03.08.2020 10:55, Michael Paquier wrote:
>
> On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
>
> It looks like this needs to be additionally rebased - I will set cfbot to
> "Waiting".
>
> ... Something that has not happened in four weeks, so this is marked
> as returned with feedback. Please feel free to resubmit once a rebase
> is done.
> --
> Michael
>
>
> Atatched 44th version of the pacthes rebased onto current master
> (#0001 corresponds to v51 of SQL/JSON patches).
>
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Thank you for review.

Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to
v52 patch set posted in the separate thread.

On 27.12.2020 01:16, Zhihong Yu wrote:
> For new files introduced in the patches:
>
> + * Portions Copyright (c) 1996-2019, 토토 사이트
>
> 2021 is a few days ahead. It would be good to update the year range.

Fixed.

> For transformJsonTableColumn:
>
> +   jfexpr->op =
> +       jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
> +       jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;
>
> Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?

Added mention of EXISTS PATH column to the comment.

> For JsonTableDestroyOpaque():
>
> +   state->opaque = NULL;
>
> Should the memory pointed to by opaque be freed ?

I think it's not necessary, because FunctionScan, caller of
JsonTableDestroyOpaque(), will free it and also all opaque's fields using
MemoryContextReset().

>
> +   l2 = list_head(tf->coltypes);
> +   l3 = list_head(tf->coltypmods);
> +   l4 = list_head(tf->colvalexprs);
>
> Maybe the ListCell pointer variables can be named corresponding to the
> lists they iterate so that the code is easier to understand.

Variable were renamed, also foreach() loop was refactored using forfour() macro.

>
> +typedef enum JsonTablePlanJoinType
> +{
> +   JSTP_INNER = 0x01,
> +   JSTP_OUTER = 0x02,
> +   JSTP_CROSS = 0x04,
>
> Since plan type enum starts with JSTP_, I think the plan join type
> should start with JSTPJ_ or JSTJ_. Otherwise the following would be a
> bit hard to read:
>
> +   else if (plan->plan_type == JSTP_JOINED)
> +   {
> +       if (plan->join_type == JSTP_INNER ||
> +           plan->join_type == JSTP_OUTER)
>
> since different fields are checked in the two if statements but the
> prefixes don't convey that.

Enumeration members were renames using prefix JSTPJ_.

> +      Even though the path names are not incuded into the
> <literal>PLAN DEFAULT</literal>
>
> Typo: incuded -> included

Fixed.

> +   int         nchilds = 0;
>
> nchilds -> nchildren

Fixed.

>
> +#if 0 /* XXX it' unclear from the standard whether root path name is
> mandatory or not */
> +   if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)
>
> Do you plan to drop the if block ?

If it becomes clear, I will drop it.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> Thank you for review.
>
> Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to
> v52 patch set posted in the separate thread.

Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log)
marked Waiting on Author.

I can see that Álvaro suggested that the patch be split up so it can be
reviewed and committed in pieces. It looks like you've done that to some
extent, but I wonder if more can be done. In particular, it looks like
that first patch could be broken up -- at lot.

Regards,
--
-David
david(at)pgmasters(dot)net

On 3/25/21 8:10 AM, David Steele wrote:
> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>> Thank you for review.
>>
>> Attached 45th version of the patches. "SQL/JSON functions" patch
>> corresponds to
>> v52 patch set posted in the separate thread.
>
> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log)
> marked Waiting on Author.
>
> I can see that Álvaro suggested that the patch be split up so it can
> be reviewed and committed in pieces. It looks like you've done that to
> some extent, but I wonder if more can be done. In particular, it looks
> like that first patch could be broken up -- at lot.
>
>

I've rebased this. Note that the large first patch is just the
accumulated patches from the 'SQL/JSON functions' thread, and should be
reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
there are no changes at all in those patches from the previous set other
than a little patch fuzz. The only substantial changes are in patch 1,
which had bitrotted. However, I'm posting a new series to keep the
numbering in sync.

If the cfbot is happy I will set back to 'Needs review'

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

> On 2021.03.26. 21:28 Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On 3/25/21 8:10 AM, David Steele wrote:
> > On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> >> Thank you for review.
> >>
> >> Attached 45th version of the patches. "SQL/JSON functions" patch
> >> corresponds to
> >> v52 patch set posted in the separate thread.
> >
> > Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log)
> > marked Waiting on Author.
> >
> > I can see that Álvaro suggested that the patch be split up so it can
> > be reviewed and committed in pieces. It looks like you've done that to
> > some extent, but I wonder if more can be done. In particular, it looks
> > like that first patch could be broken up -- at lot.
> >
> >
>
> I've rebased this. Note that the large first patch is just the
> accumulated patches from the 'SQL/JSON functions' thread, and should be
> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
> there are no changes at all in those patches from the previous set other
> than a little patch fuzz. The only substantial changes are in patch 1,
> which had bitrotted. However, I'm posting a new series to keep the
> numbering in sync.
>
> If the cfbot is happy I will set back to 'Needs review'

> 0001-SQL-JSON-functions-v46.patch
> 0002-JSON_TABLE-v46.patch
> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
> 0004-JSON_TABLE-PLAN-clause-v46.patch

Hi,

The four v46 patches apply fine, but on compile I get (debian/gcc):

make --quiet -j 4
make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'. Stop.
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed

Erik

On 3/26/21 4:48 PM, Erik Rijkers wrote:
>> On 2021.03.26. 21:28 Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 3/25/21 8:10 AM, David Steele wrote:
>>> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>>>> Thank you for review.
>>>>
>>>> Attached 45th version of the patches. "SQL/JSON functions" patch
>>>> corresponds to
>>>> v52 patch set posted in the separate thread.
>>> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log)
>>> marked Waiting on Author.
>>>
>>> I can see that Álvaro suggested that the patch be split up so it can
>>> be reviewed and committed in pieces. It looks like you've done that to
>>> some extent, but I wonder if more can be done. In particular, it looks
>>> like that first patch could be broken up -- at lot.
>>>
>>>
>> I've rebased this. Note that the large first patch is just the
>> accumulated patches from the 'SQL/JSON functions' thread, and should be
>> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
>> there are no changes at all in those patches from the previous set other
>> than a little patch fuzz. The only substantial changes are in patch 1,
>> which had bitrotted. However, I'm posting a new series to keep the
>> numbering in sync.
>>
>> If the cfbot is happy I will set back to 'Needs review'
>> 0001-SQL-JSON-functions-v46.patch
>> 0002-JSON_TABLE-v46.patch
>> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
>> 0004-JSON_TABLE-PLAN-clause-v46.patch
>
> Hi,
>
> The four v46 patches apply fine, but on compile I get (debian/gcc):
>
> make --quiet -j 4
> make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'. Stop.
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [parser-recursive] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [all-backend-recurse] Error 2
> make: *** [all-src-recurse] Error 2
> common.mk:39: recipe for target 'parser-recursive' failed
> Makefile:42: recipe for target 'all-backend-recurse' failed
> GNUmakefile:11: recipe for target 'all-src-recurse' failed
>

Yeah, I messed up :-( Forgot to git-add some files.

will repost soon.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Attached 47th version of the patches.

On 26.03.2021 23:58, Andrew Dunstan wrote:
> On 3/26/21 4:48 PM, Erik Rijkers wrote:
>
>> Hi,
>>
>> The four v46 patches apply fine, but on compile I get (debian/gcc):
>>
>> make --quiet -j 4
>> make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'. Stop.
>> make[3]: *** Waiting for unfinished jobs....
>> make[2]: *** [parser-recursive] Error 2
>> make[2]: *** Waiting for unfinished jobs....
>> make[1]: *** [all-backend-recurse] Error 2
>> make: *** [all-src-recurse] Error 2
>> common.mk:39: recipe for target 'parser-recursive' failed
>> Makefile:42: recipe for target 'all-backend-recurse' failed
>> GNUmakefile:11: recipe for target 'all-src-recurse' failed
> Yeah, I messed up :-( Forgot to git-add some files.
>
>
> will repost soon.

I have added forgotten files and fixed the first patch.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>
> Attached 47th version of the patches.
>
[..]
>
> I have added forgotten files and fixed the first patch.
>
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]

Hi,

Apply, build all fine. It also works quite well, and according to specification, as far as I can tell.

But today I ran into:

ERROR: function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).

This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):

select jt1.*
from myjsonfile100k as t(js, id)
, json_table(
t.js
, '$' columns (
"lastname" text path '$. "lastname" '
, "firstname" text path '$. "firstname" '
, "date" text path '$. "date" '
, "city" text path '$. "city" '
, "country" text path '$. "country" '
, "name 0(1)" text path '$. "array"[0] '
, "name 4(5)" text path '$. "array"[4] '
, "names" text[] path '$. "array" '
, "randfloat" float path '$. "random float" '
)
) as jt1
where js @> ('[ { "city": "Santiago de Cuba" } ]')
and js[0]->>'firstname' = 'Gilda'
;
ERROR: function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).

Erik Rijkers

On 30.03.2021 19:56, Erik Rijkers wrote:

>> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>>
>> Attached 47th version of the patches.
> Hi,
>
> Apply, build all fine. It also works quite well, and according to specification, as far as I can tell.
>
> But today I ran into:
>
> ERROR: function ExecEvalJson not in llvmjit_types.c
>
> I think that it is caused by:
>
> set enable_bitmapscan = off;
>
> (I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).
>
>
> This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):
>
> select jt1.*
> from myjsonfile100k as t(js, id)
> , json_table(
> t.js
> , '$' columns (
> "lastname" text path '$. "lastname" '
> , "firstname" text path '$. "firstname" '
> , "date" text path '$. "date" '
> , "city" text path '$. "city" '
> , "country" text path '$. "country" '
> , "name 0(1)" text path '$. "array"[0] '
> , "name 4(5)" text path '$. "array"[4] '
> , "names" text[] path '$. "array" '
> , "randfloat" float path '$. "random float" '
> )
> ) as jt1
> where js @> ('[ { "city": "Santiago de Cuba" } ]')
> and js[0]->>'firstname' = 'Gilda'
> ;
> ERROR: function ExecEvalJson not in llvmjit_types.c
>
> That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).

Thank you for testing.

I think you can try to add 3 missing functions references to the end of
src/backend/jit/llvm/llvmjit_types.c:

 void       *referenced_functions[] =
{
     ...
     ExecEvalXmlExpr,
+    ExecEvalJsonConstructor,
+    ExecEvalIsJsonPredicate,
+    ExecEvalJson,
     MakeExpandedObjectReadOnlyInternal,
     ...
};

If this fixes problem, I will add this to the new version of the patches.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.co <http://www.postgrespro.com>The Russian Postgres Company

> On 2021.03.30. 22:25 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>
>
> On 30.03.2021 19:56, Erik Rijkers wrote:
>
> >> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> >>
> >> Attached 47th version of the patches.
> > Hi,
> >
> > Apply, build all fine. It also works quite well, and according to specification, as far as I can tell.
> >
> > But today I ran into:
> >
> > ERROR: function ExecEvalJson not in llvmjit_types.c
> >
> > I think that it is caused by:
> >
> > set enable_bitmapscan = off;
> >
> > (I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).
> >
> >
> > This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):
> >
> > select jt1.*
> > from myjsonfile100k as t(js, id)
> > , json_table(
> > t.js
> > , '$' columns (
> > "lastname" text path '$. "lastname" '
> > , "firstname" text path '$. "firstname" '
> > , "date" text path '$. "date" '
> > , "city" text path '$. "city" '
> > , "country" text path '$. "country" '
> > , "name 0(1)" text path '$. "array"[0] '
> > , "name 4(5)" text path '$. "array"[4] '
> > , "names" text[] path '$. "array" '
> > , "randfloat" float path '$. "random float" '
> > )
> > ) as jt1
> > where js @> ('[ { "city": "Santiago de Cuba" } ]')
> > and js[0]->>'firstname' = 'Gilda'
> > ;
> > ERROR: function ExecEvalJson not in llvmjit_types.c
> >
> > That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).
>
> Thank you for testing.
>
>
> I think you can try to add 3 missing functions references to the end of
> src/backend/jit/llvm/llvmjit_types.c:
>
>  void       *referenced_functions[] =
> {
>      ...
>      ExecEvalXmlExpr,
> +    ExecEvalJsonConstructor,
> +    ExecEvalIsJsonPredicate,
> +    ExecEvalJson,
>      MakeExpandedObjectReadOnlyInternal,
>      ...
> };
>
>
> If this fixes problem, I will add this to the new version of the patches.

It does almost fix it, but in the above is a typo:
+ ExecEvalIsJsonPredicate should to be changed to:
+ ExecEvalJsonIsPredicate.

With that change the problem vanishes.

Thanks!

Erik Rijkers

>
> --
> Nikita Glukhov
> Postgres Professional:http://www.postgrespro.co <http://www.postgrespro.com>The Russian Postgres Company

> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> Attached 47th version of the patches.

We're past feature freeze for 14 and alas, JSON_TABLE has not made it.

I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at least mention that here once.

I looked at v47, these files
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]
> [manual_addition_fixed.patch] # for this see [1], [2]

(v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march 2021)

I hope it will fare better next round, version 15.

Thanks,

Erik Rijkers

[1] /message-id/69eefc5a-cabc-8dd3-c689-93da038c0d6a%40postgrespro.ru
[2] /message-id/19181987.22943.1617141503618%40webmailclassic.xs4all.nl

> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

On 4/12/21 11:34 AM, Erik Rijkers wrote:
>> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>> Attached 47th version of the patches.
> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>
> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at least mention that here once.
>
> I looked at v47, these files
>> [0001-SQL-JSON-functions-v47.patch]
>> [0002-JSON_TABLE-v47.patch]
>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>> [manual_addition_fixed.patch] # for this see [1], [2]
> (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march 2021)
>
> I hope it will fare better next round, version 15.
>

Me too. Here's a set that should remove the bitrot.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 5/8/21 2:23 PM, Andrew Dunstan wrote:
> On 4/12/21 11:34 AM, Erik Rijkers wrote:
>>> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>>> Attached 47th version of the patches.
>> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>>
>> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at least mention that here once.
>>
>> I looked at v47, these files
>>> [0001-SQL-JSON-functions-v47.patch]
>>> [0002-JSON_TABLE-v47.patch]
>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>>> [manual_addition_fixed.patch] # for this see [1], [2]
>> (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march 2021)
>>
>> I hope it will fare better next round, version 15.
>>
>
> Me too. Here's a set that should remove the bitrot.
>
>

Rebased for removal of serial schedule

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 5/18/21 9:23 PM, Andrew Dunstan wrote:
>
> On 5/8/21 2:23 PM, Andrew Dunstan wrote:
>> On 4/12/21 11:34 AM, Erik Rijkers wrote:
>>>> On 2021.03.27. 02:12 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>>>> Attached 47th version of the patches.
>>> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>>>
>>> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at least mention that here once.
>>>
>>> I looked at v47, these files
>>>> [0001-SQL-JSON-functions-v47.patch]
>>>> [0002-JSON_TABLE-v47.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>>>> [manual_addition_fixed.patch] # for this see [1], [2]
>>> (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march 2021)
>>>
>>> I hope it will fare better next round, version 15.
>>
>> Me too. Here's a set that should remove the bitrot.
>
> Rebased for removal of serial schedule

Can one of you please add or integrate this patch to the JSON_TABLE changes?

It contains the fix for a bug that I reported earlier (on 2021-03-30 see
[1]). Nikita did diagnose this fix but today I noticed it was still not
included in the latest version, v49.

Thanks,

Erik Rijkers

[1]
/message-id/2101814418.20240.1617123418368%40webmailclassic.xs4all.nl

>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>

Hi

Here are the 4 unchanged patches from v49, to which I added 2 patches,
which are small changes wrt usage of 'JsonIs' versus 'IsJson'.

That should make the cfbot green again.

Erik Rijkers

Below are a few small comments from a casual read-through. I noticed that
there was a new version posted after I had finished perusing, but it seems to
address other aspects.

+ Gerenates a column and inserts a composite SQL/JSON
s/Gerenates/Generates/

+ into both child and parrent columns for all missing values.
s/parrent/parent/

- objectname = "xmltable";
+ objectname = rte->tablefunc ?
+ rte->tablefunc->functype == TFT_XMLTABLE ?
+ "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan? Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find json_table isn't all that convenient.
That also removes the need for comments stating why a ternary operator is Ok in
the first place.

+ errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+ switch (get_typtype(typid))
+ {
+ case TYPTYPE_COMPOSITE:
+ return true;
+
+ case TYPTYPE_DOMAIN:
+ return typeIsComposite(getBaseType(typid));
+ }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry. This one can
easily be rewritten with two if-statements on a cached get_typtype()
returnvalue.

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+ /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix
something?

--
Daniel Gustafsson https://vmware.com/

On 7/22/21 3:49 AM, Erik Rijkers wrote:
> Hi
>
> Here are the 4 unchanged patches from v49, to which I added 2 patches,
> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>
> That should make the cfbot green again.

Apparently not, but I have rebased this and the sql/json function patch
set and incorporated your changes in both.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>
> On 7/22/21 3:49 AM, Erik Rijkers wrote:
>> Hi
>>
>> Here are the 4 unchanged patches from v49, to which I added 2 patches,
>> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>>
>> That should make the cfbot green again.
>
>
> Apparently not, but I have rebased this and the sql/json function patch
> set and incorporated your changes in both.

> [0001-SQL-JSON-functions-v50.patch]
> [0002-JSON_TABLE-v50.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v50.patch]
> [0004-JSON_TABLE-PLAN-clause-v50.patch]

These don't apply any more, could you have a look?

Thanks,

Erik Rijkers

(output from gcc 11.2:)

parse_jsontable.c: In function ‘makeStringConst’:
parse_jsontable.c:57:15: error: ‘union ValUnion’ has no member named ‘type’
57 | n->val.type = T_String;
| ^
parse_jsontable.c:58:16: error: ‘union ValUnion’ has no member named
‘val’; did you mean ‘ival’?
58 | n->val.val.str = str;
| ^~~
| ival
parse_jsontable.c: In function ‘transformJsonTable’:
parse_jsontable.c:714:61: error: ‘union ValUnion’ has no member named ‘type’
714 | castNode(A_Const,
jt->common->pathspec)->val.type != T_String)
| ^
parse_jsontable.c:721:65: error: ‘union ValUnion’ has no member named
‘val’; did you mean ‘ival’?
721 | rootPath = castNode(A_Const,
jt->common->pathspec)->val.val.str;
| ^~~
|
ival
make[3]: *** [parse_jsontable.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
../../../src/Makefile.global:938: recipe for target 'parse_jsontable.o'
failed
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed

On 9/13/21 5:41 AM, Erik Rijkers wrote:
> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>
>> On 7/22/21 3:49 AM, Erik Rijkers wrote:
>>> Hi
>>>
>>> Here are the 4 unchanged patches from v49, to which I added 2 patches,
>>> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>>>
>>> That should make the cfbot green again.
>>
>>
>> Apparently not, but I have rebased this and the sql/json function patch
>> set and incorporated your changes in both.
>
>
> > [0001-SQL-JSON-functions-v50.patch]
> > [0002-JSON_TABLE-v50.patch]
> > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v50.patch]
> > [0004-JSON_TABLE-PLAN-clause-v50.patch]
>
>
> These don't apply any more, could you have a look?
>
>
>

Yeah, we ran foul of the removal of the Value node type. Here's a rebase.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 9/14/21 2:53 PM, Andrew Dunstan wrote:
> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>

>> [0001-SQL-JSON-functions-v51.patch]
>> [0002-JSON_TABLE-v51.patch]
>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
>> [0004-JSON_TABLE-PLAN-clause-v51.patch]

Thanks, builds fine now.

But I found that the server crashes on certain forms of SQL when
postgresql.conf has a 'shared_preload_libraries' that contains module
'pg_stat_statements' (my value was:
'pg_stat_statements,auth_delay,auto_explain,passwordcheck'). Only
pg_stat_statements seems to cause the problem.

The offending SQL (I took it from the jsonb_sqljson.sql test file):

testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x &&
@ < $y)' PASSING 0 AS x, 2 AS y);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
Time: 2.551 ms
!?>

(Of course, that SQL running during regression testing has no problems
as there is then no pg_stat_statements.)

The statement sometimes succeeds but never very often.

The same crash was there before but I only now saw the connection with
the 'shared_preload_libraries/pg_stat_statements'.

I seem to remember some things changed in pg_stat_statements but I
didn't follow and don't know who to CC for it.

Thanks,

Erik Rijkers

út 14. 9. 2021 v 20:04 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
> > On 9/13/21 5:41 AM, Erik Rijkers wrote:
> >> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
> >>
>
> >> [0001-SQL-JSON-functions-v51.patch]
> >> [0002-JSON_TABLE-v51.patch]
> >> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
> >> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>
> Thanks, builds fine now.
>
> But I found that the server crashes on certain forms of SQL when
> postgresql.conf has a 'shared_preload_libraries' that contains module
> 'pg_stat_statements' (my value was:
> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck'). Only
> pg_stat_statements seems to cause the problem.
>
> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>
> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x &&
> @ < $y)' PASSING 0 AS x, 2 AS y);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> Time: 2.551 ms
> !?>
>
> (Of course, that SQL running during regression testing has no problems
> as there is then no pg_stat_statements.)
>
> The statement sometimes succeeds but never very often.
>
> The same crash was there before but I only now saw the connection with
> the 'shared_preload_libraries/pg_stat_statements'.
>
> I seem to remember some things changed in pg_stat_statements but I
> didn't follow and don't know who to CC for it.
>

These issues are easily debugged - you can run gdb in the outer terminal,
and attach it to the psql session. Then you can run the query.

Probably it will be a problem in pg_stat_statements callbacks - maybe query
processing there doesn't know some new nodes that this patch introduces.

Regards

Pavel

> Thanks,
>
> Erik Rijkers
>

On 9/14/21 2:04 PM, Erik Rijkers wrote:
> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>
>
> >> [0001-SQL-JSON-functions-v51.patch]
> >> [0002-JSON_TABLE-v51.patch]
> >> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
> >> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>
> Thanks, builds fine now.
>
> But I found that the server crashes on certain forms of SQL when
> postgresql.conf has a 'shared_preload_libraries' that contains module
> 'pg_stat_statements' (my value was:
> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
> pg_stat_statements seems to cause the problem.
>
> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>
> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
> && @ < $y)' PASSING 0 AS x, 2 AS y);
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> Time: 2.551 ms
> !?>
>
> (Of course, that SQL running during regression testing has no problems
> as there is then no pg_stat_statements.)
>
> The statement sometimes succeeds but never very often.
>
> The same crash was there before but I only now saw the connection with
> the 'shared_preload_libraries/pg_stat_statements'.
>
> I seem to remember some things changed in pg_stat_statements but I
> didn't follow and don't know who to CC for it.
>
>

Yeah, I had to make a change in that area, looks like I got it wrong.
I'll follow up. Thanks for the report.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 9/14/21 3:18 PM, Andrew Dunstan wrote:
> On 9/14/21 2:04 PM, Erik Rijkers wrote:
>> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>>
>>>> [0001-SQL-JSON-functions-v51.patch]
>>>> [0002-JSON_TABLE-v51.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>> Thanks, builds fine now.
>>
>> But I found that the server crashes on certain forms of SQL when
>> postgresql.conf has a 'shared_preload_libraries' that contains module
>> 'pg_stat_statements' (my value was:
>> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
>> pg_stat_statements seems to cause the problem.
>>
>> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>>
>> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
>> && @ < $y)' PASSING 0 AS x, 2 AS y);
>> server closed the connection unexpectedly
>>         This probably means the server terminated abnormally
>>         before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>> Time: 2.551 ms
>> !?>
>>
>> (Of course, that SQL running during regression testing has no problems
>> as there is then no pg_stat_statements.)
>>
>> The statement sometimes succeeds but never very often.
>>
>> The same crash was there before but I only now saw the connection with
>> the 'shared_preload_libraries/pg_stat_statements'.
>>
>> I seem to remember some things changed in pg_stat_statements but I
>> didn't follow and don't know who to CC for it.
>>
>>
>
> Yeah, I had to make a change in that area, looks like I got it wrong.
> I'll follow up. Thanks for the report.
>

Rebased and fixed. It's actually an old bug, I reproduced it with a
previous patch set.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 9/16/21 10:55, Andrew Dunstan wrote:
> On 9/14/21 3:18 PM, Andrew Dunstan wrote:
>> On 9/14/21 2:04 PM, Erik Rijkers wrote:
>>> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>>>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>>>
>>>>> [0001-SQL-JSON-functions-v51.patch]
>>>>> [0002-JSON_TABLE-v51.patch]
>>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
>>>>> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>>> Thanks, builds fine now.
>>>
>>> But I found that the server crashes on certain forms of SQL when
>>> postgresql.conf has a 'shared_preload_libraries' that contains module
>>> 'pg_stat_statements' (my value was:
>>> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
>>> pg_stat_statements seems to cause the problem.
>>>
>>> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>>>
>>> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
>>> && @ < $y)' PASSING 0 AS x, 2 AS y);
>>> server closed the connection unexpectedly
>>>         This probably means the server terminated abnormally
>>>         before or while processing the request.
>>> The connection to the server was lost. Attempting reset: Failed.
>>> Time: 2.551 ms
>>> !?>
>>>
>>> (Of course, that SQL running during regression testing has no problems
>>> as there is then no pg_stat_statements.)
>>>
>>> The statement sometimes succeeds but never very often.
>>>
>>> The same crash was there before but I only now saw the connection with
>>> the 'shared_preload_libraries/pg_stat_statements'.
>>>
>>> I seem to remember some things changed in pg_stat_statements but I
>>> didn't follow and don't know who to CC for it.
>>>
>>>
>> Yeah, I had to make a change in that area, looks like I got it wrong.
>> I'll follow up. Thanks for the report.
>>
> Rebased and fixed. It's actually an old bug, I reproduced it with a
> previous patch set.
>
>

rebased again.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
>
> rebased again.

This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
Can you send a rebased version?

On 1/18/22 01:32, Julien Rouhaud wrote:
> Hi,
>
> On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
>> rebased again.
> This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
> Can you send a rebased version?

attached

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 1/18/22 16:23, Andrew Dunstan wrote:
> On 1/18/22 01:32, Julien Rouhaud wrote:
>> Hi,
>>
>> On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
>>> rebased again.
>> This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
>> Can you send a rebased version?
>
> attached
>

rebased with some review comments attended to.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> rebased with some review comments attended to.

I am in process of reviewing these patches, initially, have started
with 0002-JSON_TABLE-v55.patch.
Tested many different scenarios with various JSON messages and these
all are working as expected. Just one question on the below output.

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON EMPTY)) jt;
a
---

(1 row)

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON ERROR)) jt;
a
---

(1 row)

is not "ERROR ON ERROR" is expected to give error?

There are a few minor comments on the patch:
1)
Few Typo
+ Sibling columns are joined using
+ <literal>FULL OUTER JOIN ON FALSE</literal>, so that both
parent and child
+ rows are included into the output, with NULL values inserted
+ into both child and parrent columns for all missing values.

Parrent should be parent.

+ <para>
+ Gerenates a column and inserts a boolean item into each row of
this column.
+ </para>
Gerenates should be Generates.

+ <para>
+ Extracts SQL/JSON items from nested levels of the row pattern,
+ gerenates one or more columns as defined by the <literal>COLUMNS</literal>
+ subclause, and inserts the extracted SQL/JSON items into each
row of these columns.
+ The <replaceable>json_table_column</replaceable> expression in the
+ <literal>COLUMNS</literal> subclause uses the same syntax as in the
+ parent <literal>COLUMNS</literal> clause.
+ </para>

Gerenates should be Generates.

+/*-------------------------------------------------------------------------
+ *
+ * parse_jsontable.c
+ * pasring of JSON_TABLE

pasring should be parsing.

2) Albhabatic include.
+
+#include "postgres.h"
+
+#include "miscadmin.h"
+
include files are not in alphabetic order.

3)
+-- JSON_TABLE: nested paths and plans
+-- Should fail (column names anf path names shall be distinct)
+SELECT * FROM JSON_TABLE(
+ jsonb '[]', '$'
+ COLUMNS (
+ a int,
+ b text,
+ a jsonb
+ )
+) jt;
+ERROR: duplicate JSON_TABLE column name: a
+HINT: JSON_TABLE path names and column names shall be distinct from
one another

HINT is not much relevant, can't we simply say "JSON_TABLE column
names should be distinct from one another"?

4)
@@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
/* Want to execute expressions inside function's memory context */
MemoryContextSwitchTo(oldcontext);

+

we can remove this empty line.

5)
/*
* The production for a qualified relation name has to exactly match the
* production for a qualified func_name, because in a FROM clause we cannot
* tell which we are parsing until we see what comes after it ('(' for a
* func_name, something else for a relation). Therefore we allow 'indirection'
* which may contain subscripts, and reject that case in the C code.
*/

I think the sentence "because in a FROM clause we cannot
* tell which we are parsing..." should be changed to "because in a
FROM clause we cannot
* tell what we are parsing "

6)
@@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
RangeTableFunc *rtf)
char **names;
int colno;

- /* Currently only XMLTABLE is supported */

can we change(and not remove) the comment to "/* Currently only
XMLTABLE and JSON_TABLE is supported */"

7)
/*
* TableFunc - node for a table function, such as XMLTABLE.
*
* Entries in the ns_names list are either String nodes containing
* literal namespace names, or NULL pointers to represent DEFAULT.
*/
typedef struct TableFunc

can we change the comment to "...such as XMLTABLE or JSON_TABLE."?

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
> a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
> a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?
>
> There are a few minor comments on the patch:
> 1)
> Few Typo
> + Sibling columns are joined using
> + <literal>FULL OUTER JOIN ON FALSE</literal>, so that both
> parent and child
> + rows are included into the output, with NULL values inserted
> + into both child and parrent columns for all missing values.
>
> Parrent should be parent.
>
> + <para>
> + Gerenates a column and inserts a boolean item into each row of
> this column.
> + </para>
> Gerenates should be Generates.
>
> + <para>
> + Extracts SQL/JSON items from nested levels of the row pattern,
> + gerenates one or more columns as defined by the <literal>COLUMNS</literal>
> + subclause, and inserts the extracted SQL/JSON items into each
> row of these columns.
> + The <replaceable>json_table_column</replaceable> expression in the
> + <literal>COLUMNS</literal> subclause uses the same syntax as in the
> + parent <literal>COLUMNS</literal> clause.
> + </para>
>
> Gerenates should be Generates.
>
> +/*-------------------------------------------------------------------------
> + *
> + * parse_jsontable.c
> + * pasring of JSON_TABLE
>
> pasring should be parsing.
>
> 2) Albhabatic include.
> +
> +#include "postgres.h"
> +
> +#include "miscadmin.h"
> +
> include files are not in alphabetic order.
>
> 3)
> +-- JSON_TABLE: nested paths and plans
> +-- Should fail (column names anf path names shall be distinct)
> +SELECT * FROM JSON_TABLE(
> + jsonb '[]', '$'
> + COLUMNS (
> + a int,
> + b text,
> + a jsonb
> + )
> +) jt;
> +ERROR: duplicate JSON_TABLE column name: a
> +HINT: JSON_TABLE path names and column names shall be distinct from
> one another
>
> HINT is not much relevant, can't we simply say "JSON_TABLE column
> names should be distinct from one another"?
>
> 4)
> @@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
> /* Want to execute expressions inside function's memory context */
> MemoryContextSwitchTo(oldcontext);
>
> +
>
> we can remove this empty line.
>
> 5)
> /*
> * The production for a qualified relation name has to exactly match the
> * production for a qualified func_name, because in a FROM clause we cannot
> * tell which we are parsing until we see what comes after it ('(' for a
> * func_name, something else for a relation). Therefore we allow 'indirection'
> * which may contain subscripts, and reject that case in the C code.
> */
>
> I think the sentence "because in a FROM clause we cannot
> * tell which we are parsing..." should be changed to "because in a
> FROM clause we cannot
> * tell what we are parsing "
>
> 6)
> @@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
> RangeTableFunc *rtf)
> char **names;
> int colno;
>
> - /* Currently only XMLTABLE is supported */
>
> can we change(and not remove) the comment to "/* Currently only
> XMLTABLE and JSON_TABLE is supported */"
>
> 7)
> /*
> * TableFunc - node for a table function, such as XMLTABLE.
> *
> * Entries in the ns_names list are either String nodes containing
> * literal namespace names, or NULL pointers to represent DEFAULT.
> */
> typedef struct TableFunc
>
> can we change the comment to "...such as XMLTABLE or JSON_TABLE."?
>

This set of patches deals with items 1..7 above, but not yet the ERROR
ON ERROR issue. It also makes some message cleanups, but there is more
to come in that area.

It is based on the latest SQL/JSON Functions patch set, which does not
include the sql_json GUC patch.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>

> This set of patches deals with items 1..7 above, but not yet the ERROR
> ON ERROR issue. It also makes some message cleanups, but there is more
> to come in that area.
>
> It is based on the latest SQL/JSON Functions patch set, which does not
> include the sql_json GUC patch.
>
> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
> [0002-JSON_TABLE-v56.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
> [0004-JSON_TABLE-PLAN-clause-v56.patch]

Hi,

I quickly tried the tests I already had and there are two statements
that stopped working:

testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
RETURNING jsonb);
ERROR: syntax error at or near "RETURNING"
LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING ...

testdb=# select JSON_SCALAR(123.45 returning jsonb);
ERROR: syntax error at or near "returning"
LINE 1: select JSON_SCALAR(123.45 returning jsonb)

(the '^' pointer in both cases underneath 'RETURNING'

thanks,

Erik Rijkers

>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

On 3/4/22 13:13, Erikjan Rijkers wrote:
> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>
>
>> This set of patches deals with items 1..7 above, but not yet the ERROR
>> ON ERROR issue. It also makes some message cleanups, but there is more
>> to come in that area.
>>
>> It is based on the latest SQL/JSON Functions patch set, which does not
>> include the sql_json GUC patch.
>>
> > [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
> > [0002-JSON_TABLE-v56.patch]
> > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
> > [0004-JSON_TABLE-PLAN-clause-v56.patch]
>
> Hi,
>
> I quickly tried the tests I already had and there are two statements
> that stopped working:
>
> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
> RETURNING jsonb);
> ERROR:  syntax error at or near "RETURNING"
> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
> ...
>
> testdb=# select JSON_SCALAR(123.45 returning jsonb);
> ERROR:  syntax error at or near "returning"
> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>
>   (the '^' pointer in both cases underneath 'RETURNING'
>
>
>

Yes, you're right, that was implemented as part of the GUC patch. I'll
try to split that out and send new patchsets with the RETURNING clause
but without the GUC (see upthread for reasons)

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/4/22 15:05, Andrew Dunstan wrote:
> On 3/4/22 13:13, Erikjan Rijkers wrote:
>> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>> This set of patches deals with items 1..7 above, but not yet the ERROR
>>> ON ERROR issue. It also makes some message cleanups, but there is more
>>> to come in that area.
>>>
>>> It is based on the latest SQL/JSON Functions patch set, which does not
>>> include the sql_json GUC patch.
>>>
>>> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
>>> [0002-JSON_TABLE-v56.patch]
>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
>>> [0004-JSON_TABLE-PLAN-clause-v56.patch]
>> Hi,
>>
>> I quickly tried the tests I already had and there are two statements
>> that stopped working:
>>
>> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
>> RETURNING jsonb);
>> ERROR:  syntax error at or near "RETURNING"
>> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
>> ...
>>
>> testdb=# select JSON_SCALAR(123.45 returning jsonb);
>> ERROR:  syntax error at or near "returning"
>> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>>
>>   (the '^' pointer in both cases underneath 'RETURNING'
>>
>>
>>
>
> Yes, you're right, that was implemented as part of the GUC patch. I'll
> try to split that out and send new patchsets with the RETURNING clause
> but without the GUC (see upthread for reasons)
>
>

Here's a patchset with RETURNING for JSON() and JSON_SCALAR() but
without the GUC

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
> a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
> a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?

I think I understand what's going on here. In the first example 'ERROR
ON EMPTY' causes an error condition, but as the default action for an
error condition is to return null that's what happens. To get an error
raised you would need to say 'ERROR ON EMPTY ERROR ON ERROR'. I don't
know if that's according to spec. It seems kinda screwy, arguably a POLA
violation, although that would hardly be a first for the SQL Standards
body.  But I'm speculating here, I'm not a standards lawyer.

In the second case it looks like there isn't really an error. There
would be if you used 'strict' in the path expression.

This whole area needs more documentation.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi everyone!

I am watching this thread since quite a while and I am waiting eagerly a
long time already that this feature finally lands in PostgreSQL.
Given that in around 2 weeks PostgreSQL 15 will go into feature freeze (in
the last years that usually happened around the 8th of April AFAIK), is
there any chance this will be committed? As far as I understand the patches
are almost ready.

Sorry for the noise, I just wanted to draw attention that there are people
out there looking forward to JSON_TABLE ;)

Thanks everyone for your fantastic work!
Matthias

On Sun, 13 Mar 2022 at 22:22, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> > On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net>
> wrote:
> >>
> >> rebased with some review comments attended to.
> > I am in process of reviewing these patches, initially, have started
> > with 0002-JSON_TABLE-v55.patch.
> > Tested many different scenarios with various JSON messages and these
> > all are working as expected. Just one question on the below output.
> >
> > ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> > (a int PATH '$.a' ERROR ON EMPTY)) jt;
> > a
> > ---
> >
> > (1 row)
> >
> > ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> > (a int PATH '$.a' ERROR ON ERROR)) jt;
> > a
> > ---
> >
> > (1 row)
> >
> > is not "ERROR ON ERROR" is expected to give error?
>
>
> I think I understand what's going on here. In the first example 'ERROR
> ON EMPTY' causes an error condition, but as the default action for an
> error condition is to return null that's what happens. To get an error
> raised you would need to say 'ERROR ON EMPTY ERROR ON ERROR'. I don't
> know if that's according to spec. It seems kinda screwy, arguably a POLA
> violation, although that would hardly be a first for the SQL Standards
> body. But I'm speculating here, I'm not a standards lawyer.
>
> In the second case it looks like there isn't really an error. There
> would be if you used 'strict' in the path expression.
>
>
> This whole area needs more documentation.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>
>
>

On Tue, Mar 22, 2022 at 12:53 PM Matthias Kurz <m(dot)kurz(at)irregular(dot)at> wrote:

> Hi everyone!
>
> I am watching this thread since quite a while and I am waiting eagerly a
> long time already that this feature finally lands in PostgreSQL.
> Given that in around 2 weeks PostgreSQL 15 will go into feature freeze (in
> the last years that usually happened around the 8th of April AFAIK), is
> there any chance this will be committed? As far as I understand the patches
> are almost ready.
>

We are waiting too :)

>
> Sorry for the noise, I just wanted to draw attention that there are people
> out there looking forward to JSON_TABLE ;)
>

IS JSON is also cool in light of the work on JSON Schema
https://github.com/json-schema-org/vocab-database/blob/main/database.md,
which opens a lot of useful features and optimizations like json
dictionary compression.

>
> Thanks everyone for your fantastic work!
> Matthias
>
>
> On Sun, 13 Mar 2022 at 22:22, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>>
>> On 2/9/22 08:22, Himanshu Upadhyaya wrote:
>> > On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>> >>
>> >> rebased with some review comments attended to.
>> > I am in process of reviewing these patches, initially, have started
>> > with 0002-JSON_TABLE-v55.patch.
>> > Tested many different scenarios with various JSON messages and these
>> > all are working as expected. Just one question on the below output.
>> >
>> > ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
>> > (a int PATH '$.a' ERROR ON EMPTY)) jt;
>> > a
>> > ---
>> >
>> > (1 row)
>> >
>> > ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
>> > (a int PATH '$.a' ERROR ON ERROR)) jt;
>> > a
>> > ---
>> >
>> > (1 row)
>> >
>> > is not "ERROR ON ERROR" is expected to give error?
>>
>>
>> I think I understand what's going on here. In the first example 'ERROR
>> ON EMPTY' causes an error condition, but as the default action for an
>> error condition is to return null that's what happens. To get an error
>> raised you would need to say 'ERROR ON EMPTY ERROR ON ERROR'. I don't
>> know if that's according to spec. It seems kinda screwy, arguably a POLA
>> violation, although that would hardly be a first for the SQL Standards
>> body. But I'm speculating here, I'm not a standards lawyer.
>>
>> In the second case it looks like there isn't really an error. There
>> would be if you used 'strict' in the path expression.
>>
>>
>> This whole area needs more documentation.
>>
>>
>> cheers
>>
>>
>> andrew
>>
>> --
>> Andrew Dunstan
>> EDB: https://www.enterprisedb.com
>>
>>
>>
>>

--
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 3/22/22 09:28, Oleg Bartunov wrote:
>
>
> On Tue, Mar 22, 2022 at 12:53 PM Matthias Kurz <m(dot)kurz(at)irregular(dot)at>
> wrote:
>
> Hi everyone!
>
> I am watching this thread since quite a while and I am waiting
> eagerly a long time already that this feature finally lands in
> PostgreSQL.
> Given that in around 2 weeks PostgreSQL 15 will go into feature
> freeze (in the last years that usually happened around the 8th of
> April AFAIK), is there any chance this will be committed? As far
> as I understand the patches are almost ready.
>
>
> We are waiting too :)

I'm planning on pushing the functions patch set this week and json-table
next week.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, 22 Mar 2022 at 15:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> I'm planning on pushing the functions patch set this week and json-table
> next week.
>

Great! Thank you very much!

On 2022-Mar-22, Andrew Dunstan wrote:

> I'm planning on pushing the functions patch set this week and json-table
> next week.

I think it'd be a good idea to push the patches one by one and let a day
between each. That would make it easier to chase buildfarm issues
individually, and make sure they are fixed before the next step.
Each patch in each series is already big enough.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

> I'm planning on pushing the functions patch set this week and json-table
> next week.

My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are yet to be
addressed (or at all responded to) in this patchset. I'll paste the ones which
still apply to make it easier:

+ into both child and parrent columns for all missing values.
s/parrent/parent/

- objectname = "xmltable";
+ objectname = rte->tablefunc ?
+ rte->tablefunc->functype == TFT_XMLTABLE ?
+ "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan? Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find where TFT_JSON_TABLE could be used
isn't all that convenient.

+ errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is IMO misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+ switch (get_typtype(typid))
+ {
+ case TYPTYPE_COMPOSITE:
+ return true;
+
+ case TYPTYPE_DOMAIN:
+ return typeIsComposite(getBaseType(typid));
+ }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry. This one can
easily be rewritten with two if-statements on a cached get_typtype()
returnvalue.

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+ /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix
something?

--
Daniel Gustafsson https://vmware.com/

On Tue, Mar 22, 2022 at 5:32 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 3/22/22 09:28, Oleg Bartunov wrote:
> >
> >
> > On Tue, Mar 22, 2022 at 12:53 PM Matthias Kurz <m(dot)kurz(at)irregular(dot)at>
> > wrote:
> >
> > Hi everyone!
> >
> > I am watching this thread since quite a while and I am waiting
> > eagerly a long time already that this feature finally lands in
> > PostgreSQL.
> > Given that in around 2 weeks PostgreSQL 15 will go into feature
> > freeze (in the last years that usually happened around the 8th of
> > April AFAIK), is there any chance this will be committed? As far
> > as I understand the patches are almost ready.
> >
> >
> > We are waiting too :)
>
>
> I'm planning on pushing the functions patch set this week and json-table
> next week.
>
>
Great, Andrew !

>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>
>
>

--
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 3/22/22 10:53, Alvaro Herrera wrote:
> On 2022-Mar-22, Andrew Dunstan wrote:
>
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> I think it'd be a good idea to push the patches one by one and let a day
> between each. That would make it easier to chase buildfarm issues
> individually, and make sure they are fixed before the next step.
> Each patch in each series is already big enough.

OK, can do it that way. First one will be later today then.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/22/22 10:55, Daniel Gustafsson wrote:
>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are yet to be
> addressed (or at all responded to) in this patchset. I'll paste the ones which
> still apply to make it easier:

Thanks for reminding me. I will make sure these are attended to.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/22/22 11:27, Andrew Dunstan wrote:
> On 3/22/22 10:53, Alvaro Herrera wrote:
>> On 2022-Mar-22, Andrew Dunstan wrote:
>>
>>> I'm planning on pushing the functions patch set this week and json-table
>>> next week.
>> I think it'd be a good idea to push the patches one by one and let a day
>> between each. That would make it easier to chase buildfarm issues
>> individually, and make sure they are fixed before the next step.
>> Each patch in each series is already big enough.
>
>
> OK, can do it that way. First one will be later today then.
>
>

OK, pushed the first of the functions patches. That means the cfbot will
break on these two CF items, but it's way too much trouble to have to
remake the patch sets every day, so we can just live without the cfbot
on these for a bit. I will of course test before committing and fix any
bitrot.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/5/22 09:35, Andrew Dunstan wrote:
> On 3/4/22 15:05, Andrew Dunstan wrote:
>> On 3/4/22 13:13, Erikjan Rijkers wrote:
>>> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>>> This set of patches deals with items 1..7 above, but not yet the ERROR
>>>> ON ERROR issue. It also makes some message cleanups, but there is more
>>>> to come in that area.
>>>>
>>>> It is based on the latest SQL/JSON Functions patch set, which does not
>>>> include the sql_json GUC patch.
>>>>
>>>> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
>>>> [0002-JSON_TABLE-v56.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v56.patch]
>>> Hi,
>>>
>>> I quickly tried the tests I already had and there are two statements
>>> that stopped working:
>>>
>>> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
>>> RETURNING jsonb);
>>> ERROR:  syntax error at or near "RETURNING"
>>> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
>>> ...
>>>
>>> testdb=# select JSON_SCALAR(123.45 returning jsonb);
>>> ERROR:  syntax error at or near "returning"
>>> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>>>
>>>   (the '^' pointer in both cases underneath 'RETURNING'
>>>
>>>
>>>
>> Yes, you're right, that was implemented as part of the GUC patch. I'll
>> try to split that out and send new patchsets with the RETURNING clause
>> but without the GUC (see upthread for reasons)
>>
>>
> Here's a patchset with RETURNING for JSON() and JSON_SCALAR() but
> without the GUC
>

Here's a new set with the latest sql/json functions patch and fixes for
some further node handling  inadequacies.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/22/22 10:55, Daniel Gustafsson wrote:
>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are yet to be
> addressed (or at all responded to) in this patchset. I'll paste the ones which
> still apply to make it easier:
>

I think I have fixed all those. See attached. I haven't prepared a new
patch set for SQL/JSON functions because there's just one typo to fix,
but I won't forget it. Please let me know if there's anything else you see.

At this stage I think I have finished with the actual code, and I'm
concentrating on improving the docs a bit.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Fri, Mar 25, 2022 at 1:30 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 3/22/22 10:55, Daniel Gustafsson wrote:
> >> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> >> I'm planning on pushing the functions patch set this week and json-table
> >> next week.
> > My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are yet
> to be
> > addressed (or at all responded to) in this patchset. I'll paste the
> ones which
> > still apply to make it easier:
> >
>
>
> I think I have fixed all those. See attached. I haven't prepared a new
> patch set for SQL/JSON functions because there's just one typo to fix,
> but I won't forget it. Please let me know if there's anything else you see.
>
>
> At this stage I think I have finished with the actual code, and I'm
> concentrating on improving the docs a bit.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Hi,
w.r.t. 0001-SQL-JSON-functions-without-sql_json-GUC-v59.patch :

+ Datum *innermost_caseval =
state->innermost_caseval;
+ bool *innermost_isnull =
state->innermost_casenull;
+
+ state->innermost_caseval = resv;
+ state->innermost_casenull = resnull;
+
+ ExecInitExprRec(jve->formatted_expr, state, resv,
resnull);
+
+ state->innermost_caseval = innermost_caseval;
+ state->innermost_casenull = innermost_isnull;

Code similar to the above block appears at least twice. Maybe extract into
a helper func to reuse code.

+ * Evaluate a JSON path variable caching computed value.
+ */
+int
+EvalJsonPathVar(void *cxt, char *varName, int varNameLen,

Please add description for return value in the comment.

+ if (formatted && IsA(formatted, Const))
+ return formatted;

If formatted is NULL, it cannot be Const. So the if can be simplified:

+ if (IsA(formatted, Const))

For transformJsonConstructorOutput(), it seems the variable have_json is
not used - you can drop the variable.

+ * Coerce a expression in JSON DEFAULT behavior to the target output type.

a expression -> an expression

Cheers

Op 25-03-2022 om 21:30 schreef Andrew Dunstan:
>
> On 3/22/22 10:55, Daniel Gustafsson wrote:
>>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>> I'm planning on pushing the functions patch set this week and json-table
>>> next week.
>> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are yet to be
>> addressed (or at all responded to) in this patchset. I'll paste the ones which
>> still apply to make it easier:
>>
>
>
> I think I have fixed all those. See attached. I haven't prepared a new
> patch set for SQL/JSON functions because there's just one typo to fix,
> but I won't forget it. Please let me know if there's anything else you see.
>
>
> At this stage I think I have finished with the actual code, and I'm
> concentrating on improving the docs a bit.

> [ v59 ]

FWIW, I went through func.sgml (of v59) once.

Erik Rijkers

On 3/26/22 07:29, Erik Rijkers wrote:
> Op 25-03-2022 om 21:30 schreef Andrew Dunstan:
>>
>> On 3/22/22 10:55, Daniel Gustafsson wrote:
>>>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>>> I'm planning on pushing the functions patch set this week and
>>>> json-table
>>>> next week.
>>> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8(at)yesql(dot)se are
>>> yet to be
>>> addressed (or at all responded to) in this patchset.  I'll paste the
>>> ones which
>>> still apply to make it easier:
>>>
>>
>>
>> I think I have fixed all those. See attached. I haven't prepared a new
>> patch set for SQL/JSON functions because there's just one typo to fix,
>> but I won't forget it. Please let me know if there's anything else
>> you see.
>>
>>
>> At this stage I think I have finished with the actual code, and I'm
>> concentrating on improving the docs a bit.
>
> > [ v59 ]
>
>
> FWIW, I went through func.sgml (of v59) once.
>
>
>