Re: SQL/JSON revisited

[Amit Langote]

Hi,

On Thu, May 4, 2023 at 3:58 AM Matthias Kurz  wrote:
> On Wed, 3 May 2023 at 20:17, Alvaro Herrera  wrote:
>> I would suggest to start a new thread with updated patches, and then a new 
>> commitfest entry can be created with those.
>
> Whoever starts that new thread, please link link it here, I am keen to follow 
> it ;) Thanks a lot!
> Thanks a lot for all your hard work btw, it's highly appreciated!

Just created a new thread:

https://www.postgresql.org/message-id/CA%2BHiwqE4XTdfb1nW%3DOjoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg%40mail.gmail.com

CF entry: https://commitfest.postgresql.org/43/4377/

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

Re: SQL/JSON revisited

[Matthias Kurz]

On Wed, 3 May 2023 at 20:17, Alvaro Herrera  wrote:

>
> I would suggest to start a new thread with updated patches, and then a new
> commitfest entry can be created with those.
>

Whoever starts that new thread, please link link it here, I am keen to
follow it ;) Thanks a lot!
Thanks a lot for all your hard work btw, it's highly appreciated!

Best,
Matthias

Re: SQL/JSON revisited

[Alvaro Herrera]

On 2023-May-03, Matthias Kurz wrote:

> On Wed, 5 Apr 2023 at 09:53, Alvaro Herrera  wrote:
> 
> > Okay, I've marked the CF entry as committed then.
> 
> This was marked as commited in the 2023-03 commitfest, however there are
> still patches missing (for example the JSON_TABLE one).
> However, I can not see an entry in the current 2023-07 Commitfest.
> I think it would be a good idea for a new entry in the current commitfest,
> just to not forget about the not-yet-commited features.

Yeah ... These remaining patches have to be rebased, and a few things
fixed (I left a few review comments somewhere).  I would suggest to
start a new thread with updated patches, and then a new commitfest entry
can be created with those.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Las mujeres son como hondas:  mientras más resistencia tienen,
 más lejos puedes llegar con ellas"  (Jonas Nightingale, Leap of Faith)

Re: SQL/JSON revisited

[Matthias Kurz]

On Wed, 5 Apr 2023 at 09:53, Alvaro Herrera  wrote:

>
> Okay, I've marked the CF entry as committed then.
>

This was marked as commited in the 2023-03 commitfest, however there are
still patches missing (for example the JSON_TABLE one).
However, I can not see an entry in the current 2023-07 Commitfest.
I think it would be a good idea for a new entry in the current commitfest,
just to not forget about the not-yet-commited features.

Thanks!
Matthias

Re: SQL/JSON revisited (documentation)

[Erik Rijkers]

Hi,

IS JSON is documented as:

expression IS [ NOT ] JSON
  [ { VALUE | SCALAR | ARRAY | OBJECT } ]
  [ { WITH | WITHOUT } UNIQUE [ KEYS ] ]

which is fine but 'VALUE' is nowhere mentioned
(except in the commit-message as: IS JSON [VALUE] )

Unless I'm mistaken 'VALUE' does indeed not change an IS JSON statement, 
so to document we could simply insert this line (as in the attached):


"The VALUE key word is optional noise."

Somewhere in its text in func.sgml, which is now:

"This predicate tests whether expression can be parsed as JSON, possibly 
of a specified type.  If SCALAR or ARRAY or OBJECT is specified, the 
test is whether or not the JSON is of that particular type. If WITH 
UNIQUE KEYS is specified, then any object in the expression is also 
tested to see if it has duplicate keys."



Erik Rijkers
--- doc/src/sgml/func.sgml.orig 2023-04-12 06:16:40.517722315 +0200
+++ doc/src/sgml/func.sgml  2023-04-12 06:30:56.410837805 +0200
@@ -16037,6 +16037,7 @@

 This predicate tests whether expression can 
be
 parsed as JSON, possibly of a specified type.
+The VALUE key word is optional noise.
 If SCALAR or ARRAY or
 OBJECT is specified, the
 test is whether or not the JSON is of that particular type. If

Re: SQL/JSON revisited

[Alvaro Herrera]

On 2023-Apr-04, Andrew Dunstan wrote:

> On 2023-04-04 Tu 08:36, Alvaro Herrera wrote:
> > 
> > Surely this can be made cleaner.
> > 
> > By the way -- that comment about clauses being non-standard, can you
> > spot exactly *which* clauses that comment applies to?
> 
> Sadly, I don't think we're going to be able to make further progress before
> feature freeze. Thanks to Alvaro for advancing us a way down the field. I
> hope we can get the remainder committed in the July CF.

Okay, I've marked the CF entry as committed then.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

Re: SQL/JSON revisited

[Andrew Dunstan]

On 2023-04-04 Tu 08:36, Alvaro Herrera wrote:


Surely this can be made cleaner.

By the way -- that comment about clauses being non-standard, can you
spot exactly *which* clauses that comment applies to?



Sadly, I don't think we're going to be able to make further progress 
before feature freeze. Thanks to Alvaro for advancing us a way down the 
field. I hope we can get the remainder committed in the July CF.



cheers


andrew

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

Re: SQL/JSON revisited

[Jonathan S. Katz]

On 4/4/23 3:40 PM, Nikita Malakhov wrote:

Hi hackers!

The latest SQL standard contains dot notation for JSON. Are there any 
plans to include it into Pg 16?

Or maybe we should start a separate thread for it?


I would recommend starting a new thread to discuss the dot notation.

Thanks,

Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: SQL/JSON revisited

[Nikita Malakhov]

Hi hackers!

The latest SQL standard contains dot notation for JSON. Are there any plans
to include it into Pg 16?
Or maybe we should start a separate thread for it?

Thanks!


On Tue, Apr 4, 2023 at 3:36 PM Alvaro Herrera 
wrote:

> On 2023-Apr-04, Amit Langote wrote:
>
> > On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera 
> wrote:
>
> > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
> > >   I think we could make that stuff use something similar to
> > >   ConstraintAttributeSpec with an accompanying post-processing
> function.
> > >   That would reduce the number of ad-hoc hacks, which seem excessive.
> >
> > Do you mean the solution involving the JsonBehavior node?
>
> Right.  It has spilled as the separate on_behavior struct in the core
> parser %union in addition to the raw jsbehavior, which is something
> we've gone 30 years without having, and I don't see why we should start
> now.
>
> This stuff is terrible:
>
> json_exists_error_clause_opt:
> json_exists_error_behavior ON ERROR_P   { $$ = $1; }
> | /* EMPTY */   { $$ = NULL; }
> ;
>
> json_exists_error_behavior:
> ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | TRUE_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE,
> NULL); }
> | FALSE_P   { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE,
> NULL); }
> | UNKNOWN   { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN,
> NULL); }
> ;
>
> json_value_behavior:
> NULL_P  { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL);
> }
> | ERROR_P   { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | DEFAULT a_expr{ $$ =
> makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); }
> ;
>
> json_value_on_behavior_clause_opt:
> json_value_behavior ON EMPTY_P
> { $$.on_empty = $1; $$.on_error =
> NULL; }
> | json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P
> { $$.on_empty = $1; $$.on_error = $4; }
> | json_value_behavior ON ERROR_P
> { $$.on_empty = NULL; $$.on_error =
> $1; }
> |  /* EMPTY */
> { $$.on_empty = NULL; $$.on_error =
> NULL; }
> ;
>
> json_query_behavior:
> ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | NULL_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL,
> NULL); }
> | EMPTY_P ARRAY { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> /* non-standard, for Oracle compatibility only */
> | EMPTY_P   { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> | EMPTY_P OBJECT_P  { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); }
> | DEFAULT a_expr{ $$ =
> makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); }
> ;
>
> json_query_on_behavior_clause_opt:
> json_query_behavior ON EMPTY_P
> { $$.on_empty = $1; $$.on_error =
> NULL; }
> | json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P
> { $$.on_empty = $1; $$.on_error = $4; }
> | json_query_behavior ON ERROR_P
> { $$.on_empty = NULL; $$.on_error =
> $1; }
> |  /* EMPTY */
> { $$.on_empty = NULL; $$.on_error =
> NULL; }
> ;
>
> Surely this can be made cleaner.
>
> By the way -- that comment about clauses being non-standard, can you
> spot exactly *which* clauses that comment applies to?
>
> --
> Álvaro Herrera PostgreSQL Developer  —
> https://www.EnterpriseDB.com/
> "El número de instalaciones de UNIX se ha elevado a 10,
> y se espera que este número aumente" (UPM, 1972)
>
>
>

-- 
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Re: SQL/JSON revisited

[Alvaro Herrera]

On 2023-Apr-04, Amit Langote wrote:

> On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera  wrote:

> > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
> >   I think we could make that stuff use something similar to
> >   ConstraintAttributeSpec with an accompanying post-processing function.
> >   That would reduce the number of ad-hoc hacks, which seem excessive.
> 
> Do you mean the solution involving the JsonBehavior node?

Right.  It has spilled as the separate on_behavior struct in the core
parser %union in addition to the raw jsbehavior, which is something
we've gone 30 years without having, and I don't see why we should start
now.

This stuff is terrible:

json_exists_error_clause_opt:
json_exists_error_behavior ON ERROR_P   { $$ = $1; } 
| /* EMPTY */   { $$ = NULL; }
;

json_exists_error_behavior:
ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); }
| TRUE_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE, NULL); }
| FALSE_P   { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE, NULL); 
}
| UNKNOWN   { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN, 
NULL); }
;

json_value_behavior:
NULL_P  { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); }
| ERROR_P   { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); 
}
| DEFAULT a_expr{ $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, 
$2); }
;

json_value_on_behavior_clause_opt:
json_value_behavior ON EMPTY_P
{ $$.on_empty = $1; $$.on_error = NULL; }
| json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P
{ $$.on_empty = $1; $$.on_error = $4; }
| json_value_behavior ON ERROR_P
{ $$.on_empty = NULL; $$.on_error = $1; }
|  /* EMPTY */
{ $$.on_empty = NULL; $$.on_error = NULL; }
;

json_query_behavior:
ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); }
| NULL_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); }
| EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, 
NULL); }
/* non-standard, for Oracle compatibility only */
| EMPTY_P   { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, 
NULL); }
| EMPTY_P OBJECT_P  { $$ = 
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); }
| DEFAULT a_expr{ $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, 
$2); }
;

json_query_on_behavior_clause_opt:
json_query_behavior ON EMPTY_P
{ $$.on_empty = $1; $$.on_error = NULL; }
| json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P
{ $$.on_empty = $1; $$.on_error = $4; }
| json_query_behavior ON ERROR_P
{ $$.on_empty = NULL; $$.on_error = $1; }
|  /* EMPTY */
{ $$.on_empty = NULL; $$.on_error = NULL; }
;

Surely this can be made cleaner.

By the way -- that comment about clauses being non-standard, can you
spot exactly *which* clauses that comment applies to?

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)

Re: SQL/JSON revisited

[Amit Langote]

Hi Alvaro,

On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera  wrote:
> On 2023-Mar-29, Alvaro Herrera wrote:
> > In the meantime, here's the next two patches of the series: IS JSON and
> > the "query" functions.  I think this is as much as I can get done for
> > this release, so the last two pieces of functionality would have to wait
> > for 17.  I still need to clean these up some more.  These are not
> > thoroughly tested either; 0001 compiles and passes regression tests, but
> > I didn't verify 0003 other than there being no Git conflicts and bison
> > doesn't complain.
> >
> > Also, notable here is that I realized that I need to backtrack on my
> > change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT
> > TIME ZONE), and I changed to be for UNIQUE.  But now that I've done
> > "JSON query functions" I realize that it needed to be the other way for
> > the WITHOUT ARRAY WRAPPER clause too.  So 0002 reverts that choice.
>
> So I pushed 0001 on Friday, and here are 0002 (which I intend to push
> shortly, since it shouldn't be controversial) and the "JSON query
> functions" patch as 0003.  After looking at it some more, I think there
> are some things that need to be addressed by one of the authors:
>
> - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
>   I think we could make that stuff use something similar to
>   ConstraintAttributeSpec with an accompanying post-processing function.
>   That would reduce the number of ad-hoc hacks, which seem excessive.

Do you mean the solution involving the JsonBehavior node?

> - the changes in formatting.h have no explanation whatsoever.  At the
>   very least, the new function should have a comment in the .c file.
>   (And why is it at end of file?  I bet there's a better location)
>
> - some nasty hacks are being used in the ECPG grammar with no tests at
>   all.  It's easy to add a few lines to the .pgc file I added in prior
>   commits.
>
> - Some functions in jsonfuncs.c have changed from throwing hard errors
>   into soft ones.  I think this deserves more commentary.
>
> - func.sgml: The new functions are documented in a separate table for no
>   reason that I can see.  Needs to be merged into one of the existing
>   tables.  I didn't actually review the docs.

I made the jsonfuncs.c changes to use soft error handling when needed,
so I took a stab at that; attached a delta patch, which also fixes the
problematic comments mentioned by Alexander in his comments 1 and 3.

I'll need to spend some time to address other points.

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


v15-0002-delta.patch
Description: Binary data

Re: SQL/JSON revisited

[Alexander Lakhin]

Hi Alvaro,

03.04.2023 20:16, Alvaro Herrera wrote:


So I pushed 0001 on Friday, and here are 0002 (which I intend to push
shortly, since it shouldn't be controversial) and the "JSON query
functions" patch as 0003.  After looking at it some more, I think there
are some things that need to be addressed by one of the authors:

- the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
   I think we could make that stuff use something similar to
   ConstraintAttributeSpec with an accompanying post-processing function.
   That would reduce the number of ad-hoc hacks, which seem excessive.

- the changes in formatting.h have no explanation whatsoever.  At the
   very least, the new function should have a comment in the .c file.
   (And why is it at end of file?  I bet there's a better location)

- some nasty hacks are being used in the ECPG grammar with no tests at
   all.  It's easy to add a few lines to the .pgc file I added in prior
   commits.

- Some functions in jsonfuncs.c have changed from throwing hard errors
   into soft ones.  I think this deserves more commentary.

- func.sgml: The new functions are documented in a separate table for no
   reason that I can see.  Needs to be merged into one of the existing
   tables.  I didn't actually review the docs.


Please take a look at the following minor issues in
v15-0002-SQL-JSON-query-functions.patch:
1)
s/addreess/address/

2)
ECPGColLabelCommon gone with 83f1c7b74, but is still mentioned in ecpg.trailer.

3)
s/ExecEvalJsonCoercion/ExecEvalJsonExprCoercion/ ?
(there is no ExecEvalJsonCoercion() function)

4)
json_table mentioned in func.sgml:
    details the SQL/JSON
   functions that can be used to query JSON data, except
   for json_table.

but if JSON_TABLE not going to be committed in v16, maybe remove that reference
to it.

There is also a reference to JSON_TABLE in src/backend/parser/README:
parse_jsontable.c handle JSON_TABLE
(It was added with 9853bf6ab and survived the revert of SQL JSON last
year somehow.)

Best regards,
Alexander

Re: SQL/JSON revisited

[Alvaro Herrera]

Here's v14 of the IS JSON predicate.  I find no further problems here
and intend to get it pushed later today.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
>From 998ec6bd170e83cdb916ab40bec5cac56ecd1d81 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 31 Mar 2023 18:47:51 +0200
Subject: [PATCH v14] SQL/JSON: support the IS JSON predicate

This patch introduces the SQL standard IS JSON predicate. It operates
on text and bytea values representing JSON, as well as on the json and
jsonb types. Each test has IS and IS NOT variants and supports a WITH
UNIQUE KEYS flag. The tests are:

IS JSON [VALUE]
IS JSON ARRAY
IS JSON OBJECT
IS JSON SCALAR

These should be self-explanatory.

The WITH UNIQUE KEYS flag makes these return false when duplicate keys
exist in any object within the value, not necessarily directly contained
in the outermost object.

Author: Nikita Glukhov 
Author: Teodor Sigaev 
Author: Oleg Bartunov 
Author: Alexander Korotkov 
Author: Amit Langote 
Author: Andrew Dunstan 

Reviewers have included (in no particular order) Andres Freund, Alexander
Korotkov, Pavel Stehule, Andrew Alsup, Erik Rijkers, Zihong Yu,
Himanshu Upadhyaya, Daniel Gustafsson, Justin Pryzby.

Discussion: https://postgr.es/m/CAF4Au4w2x-5LTnN_bxky-mq4=woqsgsxspenczhrazsned8...@mail.gmail.com
Discussion: https://postgr.es/m/cd0bb935-0158-78a7-08b5-904886dea...@postgrespro.ru
Discussion: https://postgr.es/m/20220616233130.rparivafipt6d...@alap3.anarazel.de
Discussion: https://postgr.es/m/abd9b83b-aa66-f230-3d6d-734817f0995d%40postgresql.org
---
 doc/src/sgml/func.sgml|  80 +++
 src/backend/executor/execExpr.c   |  13 ++
 src/backend/executor/execExprInterp.c |  99 +
 src/backend/jit/llvm/llvmjit_expr.c   |   6 +
 src/backend/jit/llvm/llvmjit_types.c  |   1 +
 src/backend/nodes/makefuncs.c |  19 ++
 src/backend/nodes/nodeFuncs.c |  26 +++
 src/backend/parser/gram.y |  70 ++-
 src/backend/parser/parse_expr.c   |  76 +++
 src/backend/parser/parser.c   |   3 +
 src/backend/utils/adt/json.c  | 132 +++-
 src/backend/utils/adt/jsonfuncs.c |  20 ++
 src/backend/utils/adt/ruleutils.c |  37 
 src/include/catalog/catversion.h  |   2 +-
 src/include/executor/execExpr.h   |   8 +
 src/include/nodes/makefuncs.h |   3 +
 src/include/nodes/primnodes.h |  26 +++
 src/include/parser/kwlist.h   |   1 +
 src/include/utils/json.h  |   1 +
 src/include/utils/jsonfuncs.h |   3 +
 src/interfaces/ecpg/preproc/parse.pl  |   1 +
 src/interfaces/ecpg/preproc/parser.c  |   3 +
 .../ecpg/test/expected/sql-sqljson.c  |  73 +--
 .../ecpg/test/expected/sql-sqljson.stderr |  86 +---
 .../ecpg/test/expected/sql-sqljson.stdout |   8 +
 src/interfaces/ecpg/test/sql/sqljson.pgc  |  17 ++
 src/test/regress/expected/sqljson.out | 198 ++
 src/test/regress/sql/sqljson.sql  |  96 +
 28 files changed, 1039 insertions(+), 69 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 38e7f46760..918a492234 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -16005,6 +16005,86 @@ table2-mapping
 

 
+  
+details SQL/JSON
+   facilities for testing JSON.
+  
+
+  
+   SQL/JSON Testing Functions
+   
+
+ 
+  
+Function signature
+   
+   
+Description
+   
+   
+Example(s)
+  
+ 
+
+
+ 
+  
+IS JSON
+expression IS  NOT  JSON
+ { VALUE | SCALAR | ARRAY | OBJECT } 
+ { WITH | WITHOUT } UNIQUE  KEYS  
+   
+   
+This predicate tests whether expression can be
+parsed as JSON, possibly of a specified type.
+If SCALAR or ARRAY or
+OBJECT is specified, the
+test is whether or not the JSON is of that particular type. If
+WITH UNIQUE KEYS is specified, then any object in the
+expression is also tested to see if it
+has duplicate keys.
+   
+   
+
+SELECT js,
+  js IS JSON "json?",
+  js IS JSON SCALAR "scalar?",
+  js IS JSON OBJECT "object?",
+  js IS JSON ARRAY "array?"
+FROM (VALUES
+  ('123'), ('"abc"'), ('{"a": "b"}'), ('[1,2]'),('abc')) foo(js);
+ js | json? | scalar? | object? | array? 
++---+-+-+
+ 123| t | t   | f   | f
+ "abc"  | t | t   | f   | f
+ {"a": "b"} | t | f   | t   | f
+ [1,2]  | t | f   | f   | t
+ abc| f | f   | f   | f
+
+   
+   
+
+SELECT js,
+  js IS JSON OBJECT 

Re: SQL/JSON revisited

[Alexander Lakhin]

Hi,

29.03.2023 13:27, Alvaro Herrera wrote:

... and pushed it now, after some more meddling.

I'll rebase the rest of the series now.


Please look at the several minor issues/inconsistencies,
I've spotted in the commit:

1) s/JSON_ARRRAYAGG/JSON_ARRAYAGG/

2)
check_key_uniqueness vs check_unique
IIUC, these are different names of the same entity.

3)
elog(ERROR, "invalid JsonConstructorExprType %d", ctor->type);
vs
elog(ERROR, "invalid JsonConstructorExpr type %d", ctor->type);
I'd choose the latter spelling as the JsonConstructorExprType entity does not 
exist.

4)
In the block:
    else
    {
    res = (Datum) 0;
    elog(ERROR, "invalid JsonConstructorExpr type %d", ctor->type);
    }
res is assigned but never used.

5)
(expr [FORMAT json_format]) ->? (expr [FORMAT JsonFormat])
(json_format not found anywhere else)

Best regards,
Alexander

Re: SQL/JSON revisited

[Erik Rijkers]

Op 3/29/23 om 12:27 schreef Alvaro Herrera:

On 2023-Mar-28, Erik Rijkers wrote:


In the json_arrayagg() description, it says:
'If ABSENT ON NULL is specified, any NULL values are omitted.'
That's true, but as omitting NULL values is the default (i.e., also without
that clause) maybe it's better to say:
'Any NULL values are omitted unless NULL ON NULL is specified'


Doh, somehow I misread your report and modified the json_object()
documentation instead after experimenting with it (so now the
ABSENT/NULL ON NULL clause is inconsistenly described everywhere).
Would you mind submitting a patch fixing this mistake?


I think the json_object text was OK.  Attached are some changes where 
they were needed IMHO.


Erik



... and pushed it now, after some more meddling.

I'll rebase the rest of the series now.
--- doc/src/sgml/func.sgml.orig	2023-03-29 12:45:45.013598284 +0200
+++ doc/src/sgml/func.sgml	2023-03-29 14:24:41.966456134 +0200
@@ -15830,10 +15830,10 @@
  Constructs a JSON array from either a series of
  value_expression parameters or from the results
  of query_expression,
- which must be a SELECT query returning a single column. If
- ABSENT ON NULL is specified, NULL values are ignored.
- This is always the case if a
- query_expression is used.
+ which must be a SELECT query returning a single column. 
+ If the input is a series of value_expressions, NULL values are omitted
+ unless NULL ON NULL is specified.  If a query_expression is used NULLs
+ are always ignored.
 
 
  json_array(1,true,json '{"a":null}')
@@ -20310,13 +20310,14 @@
  ORDER BY sort_expression 
  { NULL | ABSENT } ON NULL 
  RETURNING data_type  FORMAT JSON  ENCODING UTF8   )
+json


 Behaves in the same way as json_array
 but as an aggregate function so it only takes one
 value_expression parameter.
-If ABSENT ON NULL is specified, any NULL
-values are omitted.
+NULL values are omitted unless NULL ON NULL
+is specified.
 If ORDER BY is specified, the elements will
 appear in the array in that order rather than in the input order.

Re: SQL/JSON revisited

[Alvaro Herrera]

On 2023-Mar-28, Erik Rijkers wrote:

> In the json_arrayagg() description, it says:
> 'If ABSENT ON NULL is specified, any NULL values are omitted.'
> That's true, but as omitting NULL values is the default (i.e., also without
> that clause) maybe it's better to say:
> 'Any NULL values are omitted unless NULL ON NULL is specified'

Doh, somehow I misread your report and modified the json_object()
documentation instead after experimenting with it (so now the
ABSENT/NULL ON NULL clause is inconsistenly described everywhere).
Would you mind submitting a patch fixing this mistake?

... and pushed it now, after some more meddling.

I'll rebase the rest of the series now.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

Re: SQL/JSON revisited

[Erik Rijkers]

Op 3/27/23 om 20:54 schreef Alvaro Herrera:

Docs amended as I threatened.  Other than that, this has required more


> [v12-0001-SQL-JSON-constructors.patch]
> [v12-0001-delta-uniqueifyJsonbObject-bugfix.patch]

In doc/src/sgml/func.sgml, some minor stuff:

'which specify the data type returned'  should be
'which specifies the data type returned'

In the json_arrayagg() description, it says:
'If ABSENT ON NULL is specified, any NULL values are omitted.'
That's true, but as omitting NULL values is the default (i.e., also 
without that clause) maybe it's better to say:

'Any NULL values are omitted unless NULL ON NULL is specified'


I've found no bugs in functionality.

Thanks,

Erik Rijkers

Re: SQL/JSON revisited

[Peter Eisentraut]

On 27.03.23 20:54, Alvaro Herrera wrote:

Even so, I was unable to get bison
to accept the 'KEY name VALUES blah' syntax; it might be a
fun/challenging project to change the productions that we use for JSON
names and values:

+json_name_and_value:
+/* Supporting this syntax seems to require major surgery
+   KEY c_expr VALUE_P json_value_expr
+   { $$ = makeJsonKeyValue($2, $4); }
+   |
+*/
+   c_expr VALUE_P json_value_expr
+   { $$ = makeJsonKeyValue($1, $3); }
+   |
+   a_expr ':' json_value_expr
+   { $$ = makeJsonKeyValue($1, $3); }
+   ;

If we uncomment the KEY bit there, a bunch of conflicts emerge.


This is a known bug in the SQL standard.  Because KEY is a non-reserved 
keyword, writing


KEY (x) VALUE y

is ambiguous because KEY could be the keyword for this clause or a 
function call key(x).


It's ok to leave it like this for now.

Re: SQL/JSON revisited

[Amit Langote]

On Tue, Mar 28, 2023 at 6:18 AM Justin Pryzby  wrote:
> I ran sqlsmith on this patch for a short while, and reduced one of its
> appalling queries to this:
>
> postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8);
> ERROR:  unexpected jsonb type as object key

I think this may have to do with the following changes to
uniqueifyJsonbObject() that the patch makes:

@@ -1936,7 +1942,7 @@ lengthCompareJsonbPair(const void *a, const void
*b, void *binequal)
  * Sort and unique-ify pairs in JsonbValue object
  */
 static void
-uniqueifyJsonbObject(JsonbValue *object)
+uniqueifyJsonbObject(JsonbValue *object, bool unique_keys, bool skip_nulls)
 {
boolhasNonUniq = false;

@@ -1946,15 +1952,32 @@ uniqueifyJsonbObject(JsonbValue *object)
qsort_arg(object->val.object.pairs, object->val.object.nPairs,
sizeof(JsonbPair),
  lengthCompareJsonbPair, );

-   if (hasNonUniq)
+   if (hasNonUniq && unique_keys)
+   ereport(ERROR,
+   errcode(ERRCODE_DUPLICATE_JSON_OBJECT_KEY_VALUE),
+   errmsg("duplicate JSON object key value"));
+
+   if (hasNonUniq || skip_nulls)
{
-   JsonbPair  *ptr = object->val.object.pairs + 1,
-  *res = object->val.object.pairs;
+   JsonbPair  *ptr,
+  *res;
+
+   while (skip_nulls && object->val.object.nPairs > 0 &&
+  object->val.object.pairs->value.type == jbvNull)
+   {
+   /* If skip_nulls is true, remove leading items with null */
+   object->val.object.pairs++;
+   object->val.object.nPairs--;
+   }
+
+   ptr = object->val.object.pairs + 1;
+   res = object->val.object.pairs;

The code below the while loop does not take into account the
possibility that object->val.object.pairs would be pointing to garbage
when object->val.object.nPairs is 0.

Attached delta patch that applies on top of Alvaro's v12-0001 fixes
the case for me:

postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8);
 jsonb_object_agg_unique_strict

 {}
(1 row)

postgres=# SELECT jsonb_object_agg_unique_strict('1', null::xid8);
 jsonb_object_agg_unique_strict

 {}
(1 row)

SELECT jsonb_object_agg_unique_strict('1', '1'::xid8);
 jsonb_object_agg_unique_strict

 {"1": "1"}
(1 row)

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


v12-0001-delta-uniqueifyJsonbObject-bugfix.patch
Description: Binary data

Re: SQL/JSON revisited

[Justin Pryzby]

I ran sqlsmith on this patch for a short while, and reduced one of its
appalling queries to this:

postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8);
ERROR:  unexpected jsonb type as object key

postgres=# \errverbose 
ERROR:  XX000: unexpected jsonb type as object key
UBICACIÓN:  JsonbIteratorNext, jsonb_util.c:958

As you know, it's considered bad if elog()s are reachable, user-facing errors.

2023-03-27 15:46:47.351 CDT client backend[13361] psql ERROR:  unexpected jsonb 
type as object key
2023-03-27 15:46:47.351 CDT client backend[13361] psql BACKTRACE:  
postgres: pryzbyj postgres [local] SELECT(JsonbIteratorNext+0x1e5) 
[0x5638fa11ba82]
postgres: pryzbyj postgres [local] SELECT(+0x4ff951) [0x5638fa114951]
postgres: pryzbyj postgres [local] SELECT(JsonbToCString+0x12) 
[0x5638fa116584]
postgres: pryzbyj postgres [local] SELECT(jsonb_out+0x24) 
[0x5638fa1165ad]
postgres: pryzbyj postgres [local] SELECT(FunctionCall1Coll+0x51) 
[0x5638fa1ef585]
postgres: pryzbyj postgres [local] SELECT(OutputFunctionCall+0x15) 
[0x5638fa1f067d]
postgres: pryzbyj postgres [local] SELECT(+0xe7ef7) [0x5638f9cfcef7]
postgres: pryzbyj postgres [local] SELECT(+0x2b4271) [0x5638f9ec9271]
postgres: pryzbyj postgres [local] SELECT(standard_ExecutorRun+0x146) 
[0x5638f9ec9402]

What might indicate a worse problem is that with debug_discard_caches=1, it
does something different:

postgres=# \errverbose 
ERROR:  XX000: invalid jsonb scalar type
UBICACIÓN:  convertJsonbScalar, jsonb_util.c:1865

2023-03-27 15:51:21.788 CDT client backend[15939] psql ERROR:  invalid jsonb 
scalar type
2023-03-27 15:51:21.788 CDT client backend[15939] psql CONTEXT:  parallel worker
2023-03-27 15:51:21.788 CDT client backend[15939] psql BACKTRACE:  
postgres: pryzbyj postgres [local] SELECT(ThrowErrorData+0x2a6) 
[0x5638fa1ec8f3]
postgres: pryzbyj postgres [local] SELECT(+0x194820) [0x5638f9da9820]
postgres: pryzbyj postgres [local] SELECT(HandleParallelMessages+0x15d) 
[0x5638f9daac95]
postgres: pryzbyj postgres [local] SELECT(ProcessInterrupts+0x906) 
[0x5638fa094873]
postgres: pryzbyj postgres [local] SELECT(+0x2d202b) [0x5638f9ee702b]
postgres: pryzbyj postgres [local] SELECT(+0x2d2206) [0x5638f9ee7206]
postgres: pryzbyj postgres [local] SELECT(+0x2d245a) [0x5638f9ee745a]
postgres: pryzbyj postgres [local] SELECT(+0x2bbcec) [0x5638f9ed0cec]
postgres: pryzbyj postgres [local] SELECT(+0x2b4240) [0x5638f9ec9240]
postgres: pryzbyj postgres [local] SELECT(standard_ExecutorRun+0x146) 
[0x5638f9ec9402]

+valgrind indicates this:

==14095== Use of uninitialised value of size 8
==14095==at 0x60D1C9: convertJsonbScalar (jsonb_util.c:1822)
==14095==by 0x60D44F: convertJsonbObject (jsonb_util.c:1741)
==14095==by 0x60D630: convertJsonbValue (jsonb_util.c:1611)
==14095==by 0x60D903: convertToJsonb (jsonb_util.c:1565)
==14095==by 0x60F272: JsonbValueToJsonb (jsonb_util.c:117)
==14095==by 0x60A504: jsonb_object_agg_finalfn (jsonb.c:2057)
==14095==by 0x3D0806: finalize_aggregate (nodeAgg.c:1119)
==14095==by 0x3D2210: finalize_aggregates (nodeAgg.c:1353)
==14095==by 0x3D2E7F: agg_retrieve_direct (nodeAgg.c:2512)
==14095==by 0x3D32DC: ExecAgg (nodeAgg.c:2172)
==14095==by 0x3C3CEB: ExecProcNodeFirst (execProcnode.c:464)
==14095==by 0x3BC23F: ExecProcNode (executor.h:272)
==14095==by 0x3BC23F: ExecutePlan (execMain.c:1633)

And then it shows a different error:
2023-03-27 16:00:10.072 CDT standalone backend[14095] ERROR:  unknown type of 
jsonb container to convert

In the docs:

+The key can not be null. If the
+value is null then the entry is skipped,

s/can not/cannot/
The "," is dangling.

-- 
Justin

Re: SQL/JSON revisited

[Peter Eisentraut]

On 08.03.23 22:40, Andrew Dunstan wrote:
There are probably some fairly easy opportunities to reduce the number 
of non-terminals introduced here (e.g. I think json_aggregate_func could 
possibly be expanded in place without introducing 
json_object_aggregate_constructor and json_array_aggregate_constructor). 
I'm going to make an attempt at that, at least to pick some low hanging 
fruit. But in the end I think we are going to be left with a significant 
expansion of the grammar rules, more or less forced on us by the way the 
SQL Standards Committee rather profligately invents new ways of 
contorting the grammar.


I browsed these patches, and I agree that the grammar is the thing that 
sticks out as something that could be tightened up a bit.  Try to reduce 
the number of different symbols, and check that the keywords are all in 
alphabetical order.


There are also various bits of code that are commented out, in some 
cases because they can't be implemented, in some cases without 
explanation.  I think these should all be removed.  Otherwise, whoever 
needs to touch this code next would be under some sort of obligation to 
keep the commented-out code up to date with surrounding changes, which 
would be awkward.  We can find better ways to explain missing 
functionality and room for improvement.


Also, perhaps we can find better names for the new test files.  Like, 
what does "sqljson.sql" mean, as opposed to, say, "json.sql"?  Maybe 
something like "json_functions", "json_expressions", etc. would be 
clearer.  (Starting it with "json" would also group the files better.)


These both seem like things not worth holding up progress for, and I 
think it would be good to get these patches committed as soon as 
possible. My intention is to commit them (after some grammar 
adjustments) plus their documentation in the next few days.


If possible, the documentation for each incremental part should be part 
of that patch, not a separate all-in-one patch.

Re: SQL/JSON revisited

[Andrew Dunstan]

On 2023-03-05 Su 22:18, Amit Langote wrote:

On Tue, Feb 28, 2023 at 8:36 PM Amit Langote  wrote:

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote  wrote:

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund  wrote:

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates?  That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is.  I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

Hmm, the idea to turn JSON_TABLE into a single expression turned out
to be a non-starter after all, because, unlike JsonExpr, it can
produce multiple values.  So there must be an ExprState for computing
each column of its output rows.  As I mentioned in my other reply,
TableFuncScanState has a list of ExprStates anyway for
TableFunc.colexprs.  What we could do is move the ExprStates of
TableFunc.colvalexprs into TableFuncScanState instead of making that
part of the JSON_TABLE opaque state, as I've done in the attached
updated patch.

Here's another version in which I've also moved the ExprStates of
PASSING args into TableFuncScanState instead of keeping them in
JSON_TABLE opaque state.  That means all the subsidiary ExprStates of
TableFuncScanState are now initialized only once during
ExecInitTableFuncScan().  Previously, JSON_TABLE related ones would be
initialized on every call of JsonTableInitOpaque().

I've also done some cosmetic changes such as renaming the
JsonTableContext to JsonTableParseContext in parse_jsontable.c and to
JsonTableExecContext in jsonpath_exec.c.




Hi, I have just spent some time going through the first five patches 
(i.e. those that precede the JSONTABLE patches) and Andres's comments in





AFAICT there are only two possible matters of concern that remain, both 
regarding the grammar.



First is this general complaint:



This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?


I mentioned that more than a year ago, I think, without anybody taking 
the matter up, so I didn't pursue it. I guess I should have.


There are probably some fairly easy opportunities to reduce the number 
of non-terminals introduced here (e.g. I think json_aggregate_func could 
possibly be expanded in place without introducing 
json_object_aggregate_constructor and json_array_aggregate_constructor). 
I'm going to make an attempt at that, at least to pick some low hanging 
fruit. But in the end I think we are going to be left with a significant 
expansion of the grammar rules, more or less forced on us by the way the 
SQL Standards Committee rather profligately invents new ways of 
contorting the grammar.


Second is this complaint:



+json_behavior_empty_array:
+   EMPTY_P ARRAY   { $$ = 
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+   /* non-standard, for Oracle compatibility only */
+   | EMPTY_P   { $$ = 
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+   ;
Do we really want to add random oracle compat crud here?



I think this case is pretty harmless, and it literally involves one line 
of code, so I'm inclined to leave it.


These both seem like things not worth holding up progress for, and I 
think it would be good to get these patches committed as soon as 
possible. My intention is to commit them (after some grammar 
adjustments) plus their documentation in the next few days. That would 
leave the JSONTABLE patches still to go. They are substantial, but a far 
more manageable chunk of work for some committer (not me) once we get 
this foundational piece in.



cheers


andrew

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

Re: SQL/JSON revisited

[Amit Langote]

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote  wrote:
> On Tue, Feb 21, 2023 at 2:25 AM Andres Freund  wrote:
> > Evaluating N expressions for a json table isn't a good approach, both memory
> > and CPU efficiency wise.
>
> Are you referring to JsonTableInitOpaque() initializing all these
> sub-expressions of JsonTableParent, especially colvalexprs, using N
> *independent* ExprStates?  That could perhaps be made to work by
> making JsonTableParent be an expression recognized by execExpr.c, so
> that a single ExprState can store the steps for all its
> sub-expressions, much like JsonExpr is.  I'll give that a try, though
> I wonder if the semantics of making this work in a single
> ExecEvalExpr() call will mismatch that of the current way, because
> different sub-expressions are currently evaluated under different APIs
> of TableFuncRoutine.

I was looking at this and realized that using N ExprStates for various
subsidiary expressions is not something specific to JSON_TABLE
implementation.  I mean we already have bunch of ExprStates being
created in ExecInitTableFuncScan():

scanstate->ns_uris =
ExecInitExprList(tf->ns_uris, (PlanState *) scanstate);
scanstate->docexpr =
ExecInitExpr((Expr *) tf->docexpr, (PlanState *) scanstate);
scanstate->rowexpr =
ExecInitExpr((Expr *) tf->rowexpr, (PlanState *) scanstate);
scanstate->colexprs =
ExecInitExprList(tf->colexprs, (PlanState *) scanstate);
scanstate->coldefexprs =
ExecInitExprList(tf->coldefexprs, (PlanState *) scanstate);

Or maybe you're worried about jsonpath_exec.c using so many ExprStates
*privately* to put into TableFuncScanState.opaque?

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

Re: SQL/JSON revisited

[Amit Langote]

On Tue, Feb 21, 2023 at 12:09 PM Amit Langote  wrote:
> On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers  wrote:
> > Op 20-02-2023 om 08:35 schreef Amit Langote:
> > > Rebased again over queryjumble overhaul.
> > But the following statement is a problem. It does not crash but it goes
> > off, half-freezing the machine, and only comes back after fanatic
> > Ctrl-C'ing.
> >
> > select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object
> > on error);
> >
> > Can you have a look?
>
> Thanks for the test case.  It caused ExecInterpExpr() to enter an
> infinite loop, which I've fixed in the attached updated version.  I've
> also merged Elena's documentation changes; I can see that
>  is more correct.

Oops, I hadn't actually done the latter.  Will do when posting the next version.


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

Re: SQL/JSON revisited

[Andres Freund]

Hi,

On 2023-02-20 16:35:52 +0900, Amit Langote wrote:
> Subject: [PATCH v4 03/10] SQL/JSON query functions
> +/*
> + * Evaluate a JSON error/empty behavior result.
> + */
> +static Datum
> +ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null)
> +{
> + *is_null = false;
> +
> + switch (behavior->btype)
> + {
> + case JSON_BEHAVIOR_EMPTY_ARRAY:
> + return JsonbPGetDatum(JsonbMakeEmptyArray());
> +
> + case JSON_BEHAVIOR_EMPTY_OBJECT:
> + return JsonbPGetDatum(JsonbMakeEmptyObject());
> +
> + case JSON_BEHAVIOR_TRUE:
> + return BoolGetDatum(true);
> +
> + case JSON_BEHAVIOR_FALSE:
> + return BoolGetDatum(false);
> +
> + case JSON_BEHAVIOR_NULL:
> + case JSON_BEHAVIOR_UNKNOWN:
> + *is_null = true;
> + return (Datum) 0;
> +
> + case JSON_BEHAVIOR_DEFAULT:
> + /* Always handled in the caller. */
> + Assert(false);
> + return (Datum) 0;
> +
> + default:
> + elog(ERROR, "unrecognized SQL/JSON behavior %d", 
> behavior->btype);
> + return (Datum) 0;
> + }
> +}

Does this actually need to be evaluated at expression eavluation time?
Couldn't we just emit the proper constants in execExpr.c?

> +/* 
> + *   ExecEvalJson
> + * 
> + */
> +void
> +ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)

Pointless comment.


> +{
> + JsonExprState *jsestate = op->d.jsonexpr.jsestate;
> + JsonExprPreEvalState *pre_eval = >pre_eval;
> + JsonExprPostEvalState *post_eval = >post_eval;
> + JsonExpr   *jexpr = jsestate->jsexpr;
> + Datum   item;
> + Datum   res = (Datum) 0;
> + JsonPath   *path;
> + boolthrowErrors = jexpr->on_error->btype == 
> JSON_BEHAVIOR_ERROR;
> +
> + *op->resnull = true;/* until we get a result */
> + *op->resvalue = (Datum) 0;
> +
> + item = pre_eval->formatted_expr.value;
> + path = DatumGetJsonPathP(pre_eval->pathspec.value);
> +
> + /* Reset JsonExprPostEvalState for this evaluation. */
> + memset(post_eval, 0, sizeof(*post_eval));
> +
> + res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull,
> +!throwErrors ? 
> _eval->error : NULL);
> +
> + *op->resvalue = res;
> +}

I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's
really no way to know what which version does, just based on the name.


> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y

This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?


> +json_behavior_empty_array:
> + EMPTY_P ARRAY   { $$ = 
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> + /* non-standard, for Oracle compatibility only */
> + | EMPTY_P   { $$ = 
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> + ;

Do we really want to add random oracle compat crud here?


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

Missing static?

> +{
> + JsonPathVariableEvalContext *var = NULL;
> + List   *vars = cxt;
> + ListCell   *lc;
> + int id = 1;
> +
> + if (!varName)
> + return list_length(vars);
> +
> + foreach(lc, vars)
> + {
> + var = lfirst(lc);
> +
> + if (!strncmp(var->name, varName, varNameLen))
> + break;
> +
> + var = NULL;
> + id++;
> + }
> +
> + if (!var)
> + return -1;
> +
> + /*
> +  * When belonging to a JsonExpr, path variables are computed with the
> +  * JsonExpr's ExprState (var->estate is NULL), so don't need to be 
> computed
> +  * here.  In some other cases, such as when the path variables belonging
> +  * to a JsonTable instead, those variables must be evaluated on their 
> own,
> +  * without the enclosing JsonExpr itself needing to be evaluated, so 
> must
> +  * be handled here.
> +  */
> + if (var->estate && !var->evaluated)
> + {
> + Assert(var->econtext != NULL);
> + var->value = ExecEvalExpr(var->estate, var->econtext, 
> >isnull);
> + var->evaluated = true;

Uh, so this continues to do recursive expression evaluation, as
ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar)

I'm getting grumpy here. This is wrong, has been 

Re: SQL/JSON revisited

[Erik Rijkers]

Op 20-02-2023 om 08:35 schreef Amit Langote:




Rebased again over queryjumble overhaul.



Hi,


But the following statement is a problem. It does not crash but it goes 
off, half-freezing the machine, and only comes back after fanatic 
Ctrl-C'ing.


select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object 
on error);


Can you have a look?

Thanks,

Erik Rijkers



PS
Log doesn't really have anything interesting:

2023-02-20 14:57:06.073 CET 1336 LOG:  server process (PID 1493) was 
terminated by signal 9: Killed
2023-02-20 14:57:06.073 CET 1336 DETAIL:  Failed process was running: 
select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object 
on error);
2023-02-20 14:57:06.359 CET 1336 LOG:  terminating any other active 
server processes
2023-02-20 14:57:06.667 CET 1336 LOG:  all server processes terminated; 
reinitializing
2023-02-20 14:57:11.870 CET 1556 LOG:  database system was interrupted; 
last known up at 2023-02-20 14:44:43 CET

Re: SQL/JSON revisited

[e . indrupskaya]

Hi Amit and Andrew,

Regarding not squashing [PATCH v3 11/11] Proposed reworking of
SQL/JSON documentaion, here is exactly what Tom Lane wrote in the comment to 
commit 47046763c3:

Use 
consistently for things that are in fact names of parameters (including
OUT parameters), reserving  for things that aren't.

Following this,  tags should be replaced with  because
the SQL/JSON functions' code does not explicitly specify those tagged variables
as function parameters. Doesn't it convince you to look at the patch again? 
Thank you.

On 20.02.2023 10:35, Amit Langote wrote:

no parameter names in the functions' code either

Hi,

On Mon, Jan 30, 2023 at 3:39 PM Amit Langote  wrote:

On Fri, Jan 27, 2023 at 11:27 PM vignesh C  wrote:

On Tue, 17 Jan 2023 at 19:01, Amit Langote  wrote:

And I've just finished doing that.  In the attached updated 0004,
which adds the JsonExpr node, its evaluation code is now broken into
ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
expression nodes that previously used ExprState for recursive
evaluation.  Andres didn't like the latter as previously discussed at
[1].

I've also attached the patch that Elena has proposed as the patch
0011.  I haven't managed to review it yet, though once I do, I'll
merge it into the main documentation patch 0009.  Thanks Elena.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

Thanks for the heads up.  Here's a rebased version.

Rebased again over queryjumble overhaul.

I decided to squash what was "[PATCH v3 01/11] Common SQL/JSON
clauses" into "[PATCH v3 02/11] SQL/JSON constructors", because I
noticed "useless productions" warnings against its gram.y additions
when building just 0001.

I also looked at squashing "[PATCH v3 11/11] Proposed reworking of
SQL/JSON documentaion" into "[PATCH v3 09/11] Documentation for
SQL/JSON features", but didn't, again, because I am still not sure
which one of  and  is correct for the SQL/JSON
function constructs.  Maybe it's the latter looking at the markup for
some text on [1], such as exists ( path_expression ) → boolean, but
Andrew sounded doubtful about that upthread.

Re: SQL/JSON revisited

[vignesh C]

On Tue, 17 Jan 2023 at 19:01, Amit Langote  wrote:
>
> On Wed, Dec 28, 2022 at 4:28 PM Amit Langote  wrote:
> >
> > Hi,
> >
> > Rebased the SQL/JSON patches over the latest HEAD.  I've decided to
> > keep the same division of code into individual commits as that
> > mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
> > that list into the appropriate feature commits.
> >
> > The main difference from the patches as they were committed into v15
> > is that JsonExpr evaluation no longer needs to use sub-transactions,
> > thanks to the work done recently to handle type errors softly.  I've
> > made the new code pass an ErrorSaveContext into the type-conversion
> > related functions as needed and also added an ExecEvalExprSafe() to
> > evaluate sub-expressions of JsonExpr that might contain expressions
> > that call type-conversion functions, such as CoerceViaIO contained in
> > JsonCoercion nodes.  ExecExprEvalSafe() is based on one of the patches
> > that Nikita Glukhov had submitted in a previous discussion about
> > redesigning SQL/JSON expression evaluation [1].  Though, I think that
> > new interface will become unnecessary after I have finished rebasing
> > my patches to remove subsidiary ExprStates of JsonExprState that we
> > had also discussed back in [2].
>
> And I've just finished doing that.  In the attached updated 0004,
> which adds the JsonExpr node, its evaluation code is now broken into
> ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
> expression nodes that previously used ExprState for recursive
> evaluation.  Andres didn't like the latter as previously discussed at
> [1].
>
> I've also attached the patch that Elena has proposed as the patch
> 0011.  I haven't managed to review it yet, though once I do, I'll
> merge it into the main documentation patch 0009.  Thanks Elena.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
37e267335068059ac9bd4ec5d06b493afb4b73e8 ===
=== applying patch ./v2-0001-Common-SQL-JSON-clauses.patch

can't find file to patch at input line 717
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--
|diff --git a/src/backend/utils/misc/queryjumble.c
b/src/backend/utils/misc/queryjumble.c
|index 328995a7dc..2361845a62 100644
|--- a/src/backend/utils/misc/queryjumble.c
|+++ b/src/backend/utils/misc/queryjumble.c
--
No file to patch.  Skipping patch.
1 out of 1 hunk ignored

[1] - http://cfbot.cputube.org/patch_41_4086.log

Regards,
Vignesh

Re: SQL/JSON revisited

[John Naylor]

On Wed, Jan 11, 2023 at 2:02 PM Elena Indrupskaya <
e.indrupsk...@postgrespro.ru> wrote:
>
> Sorry for upsetting your bot. :(

What I do in these cases is save the incremental patch as a .txt file --
that way people can read it, but the cf bot doesn't try to launch a CI run.
And if I forget that detail, well it's not a big deal, it happens sometimes.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: SQL/JSON revisited

[Elena Indrupskaya]

Tags in the patch follow the markup of the XMLTABLE function:

XMLTABLE (
     XMLNAMESPACES ( 
namespace_uri AS 
namespace_name , ... ), 

    row_expression 
PASSING BY 
{REF|VALUE} 
document_expression 
BY 
{REF|VALUE}
    COLUMNS name { 
type PATH 
column_expression 
DEFAULT 
default_expression 
NOT NULL | NULL

  | FOR ORDINALITY }
    , ...
) setof record

In the above, as well as in the signatures of SQL/JSON functions, there 
are no exact parameter names; otherwise, they should have been followed 
by the  tag, which is not the case. There are no parameter names 
in the functions' code either. Therefore,  tags seem more 
appropriate, according to the comment to commit 47046763c3.


Sorry for upsetting your bot. :(

--
Elena Indrupskaya
Lead Technical Writer
Postgres Professional http://www.postgrespro.com

On 2023-01-10 Tu 07:51, Elena Indrupskaya wrote:

Hi,

The Postgres Pro documentation team prepared another SQL/JSON
documentation patch (attached), to apply on top of
v1-0009-Documentation-for-SQL-JSON-features.patch.
The new patch:
- Fixes minor typos
- Does some rewording agreed with Nikita Glukhov
- Updates Docbook markup to make tags consistent across SQL/JSON
documentation and across func.sgml, and in particular, consistent with
the XMLTABLE function, which resembles SQL/JSON functions pretty much.


That's nice, but please don't post incremental patches like this. It
upsets the cfbot. (I wish there were a way to tell the cfbot to ignore
patches)

Also, I'm fairly certain that a good many of your changes are not
according to project style. The rule as I understand it is that
 is used for things that are parameters and  is
only used for things that are not parameters. (I'm not sure where that's
documented other than the comment on commit 47046763c3, but it's what I
attempted to do with the earlier doc tidy up.)


cheers


andrew

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

Re: SQL/JSON revisited

[Andrew Dunstan]

On 2023-01-10 Tu 07:51, Elena Indrupskaya wrote:
> Hi,
>
> The Postgres Pro documentation team prepared another SQL/JSON
> documentation patch (attached), to apply on top of
> v1-0009-Documentation-for-SQL-JSON-features.patch.
> The new patch:
> - Fixes minor typos
> - Does some rewording agreed with Nikita Glukhov
> - Updates Docbook markup to make tags consistent across SQL/JSON
> documentation and across func.sgml, and in particular, consistent with
> the XMLTABLE function, which resembles SQL/JSON functions pretty much.
>

That's nice, but please don't post incremental patches like this. It
upsets the cfbot. (I wish there were a way to tell the cfbot to ignore
patches)

Also, I'm fairly certain that a good many of your changes are not
according to project style. The rule as I understand it is that
 is used for things that are parameters and  is
only used for things that are not parameters. (I'm not sure where that's
documented other than the comment on commit 47046763c3, but it's what I
attempted to do with the earlier doc tidy up.)


cheers


andrew

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

Re: SQL/JSON revisited

[Elena Indrupskaya]

Hi,

The Postgres Pro documentation team prepared another SQL/JSON 
documentation patch (attached), to apply on top of 
v1-0009-Documentation-for-SQL-JSON-features.patch.

The new patch:
- Fixes minor typos
- Does some rewording agreed with Nikita Glukhov
- Updates Docbook markup to make tags consistent across SQL/JSON 
documentation and across func.sgml, and in particular, consistent with 
the XMLTABLE function, which resembles SQL/JSON functions pretty much.


--
Elena Indrupskaya
Lead Technical Writer
Postgres Professional http://www.postgrespro.com

On 28.12.2022 10:28, Amit Langote wrote:

Hi,

Rebased the SQL/JSON patches over the latest HEAD.  I've decided to
keep the same division of code into individual commits as that
mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
that list into the appropriate feature commits.

The main difference from the patches as they were committed into v15
is that JsonExpr evaluation no longer needs to use sub-transactions,
thanks to the work done recently to handle type errors softly.  I've
made the new code pass an ErrorSaveContext into the type-conversion
related functions as needed and also added an ExecEvalExprSafe() to
evaluate sub-expressions of JsonExpr that might contain expressions
that call type-conversion functions, such as CoerceViaIO contained in
JsonCoercion nodes.  ExecExprEvalSafe() is based on one of the patches
that Nikita Glukhov had submitted in a previous discussion about
redesigning SQL/JSON expression evaluation [1].  Though, I think that
new interface will become unnecessary after I have finished rebasing
my patches to remove subsidiary ExprStates of JsonExprState that we
had also discussed back in [2].

Adding this to January CF.
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cc72b9c2f6d..8614a26fe95 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17675,18 +17675,18 @@ $.* ? (@ like_regex "^\\d+$")
   
 json constructor
   json (
-  expression
+  expression
FORMAT JSON  ENCODING UTF8 
{ WITH | WITHOUT } UNIQUE  KEYS 
RETURNING json_data_type )


-The expression can be any text type or a
+The expression can be any text type or a
 bytea in UTF8 encoding. If the
-expression is NULL, an
+expression is NULL, an
 SQL null value is returned.
 If WITH UNIQUE is specified, the
-expression must not contain any duplicate
+expression must not contain any duplicate
 object keys.


@@ -17701,12 +17701,12 @@ $.* ? (@ like_regex "^\\d+$")
  
   
 json_scalar
-json_scalar (expression
+json_scalar (expression
  RETURNING json_data_type )


 Returns a JSON scalar value representing
-expression.
+expression.
 If the input is NULL, an SQL NULL is returned. If the input is a number
 or a boolean value, a corresponding JSON number or boolean value is
 returned. For any other value a JSON string is returned.
@@ -17724,8 +17724,8 @@ $.* ? (@ like_regex "^\\d+$")
   
 json_object
 json_object (
- { key_expression { VALUE | ':' }
- value_expression  FORMAT JSON  ENCODING UTF8   }, ... 
+ { key_expression { VALUE | ':' }
+ value_expression  FORMAT JSON  ENCODING UTF8   }, ... 
  { NULL | ABSENT } ON NULL 
  { WITH | WITHOUT } UNIQUE  KEYS  
  RETURNING data_type  FORMAT JSON  ENCODING UTF8   )
@@ -17733,15 +17733,15 @@ $.* ? (@ like_regex "^\\d+$")

 Constructs a JSON object of all the key value pairs given,
 or an empty object if none are given.
-key_expression is a scalar expression
+key_expression is a scalar expression
 defining the JSON key, which is
 converted to the text type.
 It cannot be NULL nor can it
 belong to a type that has a cast to the json.
 If WITH UNIQUE is specified, there must not
-be any duplicate key_expression.
+be any duplicate key_expression.
 If ABSENT ON NULL is specified, the entire
-pair is omitted if the value_expression
+pair is omitted if the value_expression
 is NULL.


@@ -17753,7 +17753,7 @@ $.* ? (@ like_regex "^\\d+$")
   
 json_objectagg
 json_objectagg (
- { key_expression { VALUE | ':' } value_expression } 
+ { key_expression { VALUE | ':' } value_expression } 
  { NULL | ABSENT } ON NULL 
  { WITH | WITHOUT } UNIQUE  KEYS  
  RETURNING data_type  FORMAT JSON  ENCODING UTF8   )
@@ -17761,8 +17761,8 @@ $.* ? (@ like_regex "^\\d+$")

 Behaves like json_object above, but as an
 aggregate function, so it only takes one
-key_expression and one
-value_expression parameter.
+

Re: SQL/JSON revisited

[Amit Langote]

On Wed, Dec 28, 2022 at 4:28 PM Amit Langote  wrote:
>
> Hi,
>
> Rebased the SQL/JSON patches over the latest HEAD.  I've decided to
> keep the same division of code into individual commits as that
> mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
> that list into the appropriate feature commits.
>
> The main difference from the patches as they were committed into v15
> is that JsonExpr evaluation no longer needs to use sub-transactions,
> thanks to the work done recently to handle type errors softly.  I've
> made the new code pass an ErrorSaveContext into the type-conversion
> related functions as needed and also added an ExecEvalExprSafe() to
> evaluate sub-expressions of JsonExpr that might contain expressions
> that call type-conversion functions, such as CoerceViaIO contained in
> JsonCoercion nodes.  ExecExprEvalSafe() is based on one of the patches
> that Nikita Glukhov had submitted in a previous discussion about
> redesigning SQL/JSON expression evaluation [1].  Though, I think that
> new interface will become unnecessary after I have finished rebasing
> my patches to remove subsidiary ExprStates of JsonExprState that we
> had also discussed back in [2].
>
> Adding this to January CF.

Done.

https://commitfest.postgresql.org/41/4086/

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